Leif,
2016-10-26 18:12 GMT+02:00 Leif Lindholm leif.lindholm@linaro.org:
On Wed, Oct 26, 2016 at 05:59:00PM +0200, Marcin Wojtas wrote:
Hi Leif,
Thank you for review, please see inline.
+/* Update parser Tcam and Sram hw entries */ +STATIC +INT32 +Mvpp2PrsHwWrite (
- IN MVPP2_SHARED *Priv,
- IN MVPP2_PRS_ENTRY *Pe
OUT (5.7.1.11). If the location is modified.
IN OUT if it is both read and modified.
OK.
Sorry I was just running away to a meeting and didn't want to postpone sending the feedback. I had meant to add: Can you go through these throughout the file? If insure, missing IN/OUT indicators are better than incorrect ones.
Yes, I even wrote it in last email.
- Mvpp2PrsHwRead (Priv, Pe);
- match = Mvpp2PrsTcamDataCmp (Pe, 0, Mvpp2Swab16 (tpid));
So, I was going to ask again to replace these calls with SwapBytes16, and searched for the #define, but can't find it in this patch. Does this currently compile?
Not only compiles, but also works. However for that are needed patches 2/3 and 3/3. Mvpp2Lib relies on the glue header and I'll update define there according to your request.
Right, but that breaks bisect. Each commit in the repository should be buildable. Macros that are needed by one patch should not be added in a later patch. So could you either change these to use SwapBytes16 or move the macros to this patch?
But this patch itself also contains tons of stuff which is comprised by glue header Pp2Dxe (e.g. structures, read/write routines, etc.). On the branch this commit builds anyway, because whole Drivers/Net/Pp2Dxe is added to compilation in 3/3. I can of course add the Pp2Dxe header to this patch, but it won't be such logical split (Lib, Driver, Compilation enable + Dsc), as we have now. In terms of building each commit it's also transparent and I mention in the commit log, that it's a preparation for upcoming full driver support.
Anyway, I'll do what you prefer. Please let know your opinion.
- /* Go through the all entires with MVPP2_PRS_LU_MAC */
- for (Tid = MVPP2_PE_FIRST_FREE_TID; Tid <= MVPP2_PE_LAST_FREE_TID; Tid++) {
- UINT32 EntryPmap;
- if (!Priv->PrsShadow[Tid].Valid
|| (Priv->PrsShadow[Tid].Lu != MVPP2_PRS_LU_MAC)|| (Priv->PrsShadow[Tid].Udf != UdfType))OK, so in general, this patch is now substantially improved and I won't bother pointing out the minor remaining coding style issues, although if we can get PatchCheck.py to do so we may revisit in future.
However, since I think it would really improve readability, could you reformat the above construct (and other instances following the same pattern) as:
if (!Priv->PrsShadow[Tid].Valid || (Priv->PrsShadow[Tid].Lu != MVPP2_PRS_LU_MAC) || (Priv->PrsShadow[Tid].Udf != UdfType))?
(5.2.2.11)
Sure.
Thanks.
- Mvpp2Write (
Port->Priv,MVPP2_CLS_OVERSIZE_RXQ_LOW_REG(Port->Id),Port->FirstRxq & MVPP2_CLS_OVERSIZE_RXQ_LOW_MASK);+#ifdef MVPP2_V1
I don't actually see this set anywhere. Is it a pre-production version?
No, this is just different silicone revision used e.g. in Armada375 SoC.
EDK2-wise, this would generally be better as a Pcd, although I realise this would be a semi-major rewrite. Could these conditional blocks simply be dropped?
Do you think that following may work on preprocessor level: In glue header: #define MVPP2_V1 PcdGetBool(PcdPp2V1Version)
In Lib: #if MVPP2_V1 ... #endif ?
This would be least intrusive, however not sure if that may even work.
If it works, I'm entirely happy with that solution.
Ok, will check it.
If not, I agree it would be cleaner to remove sections.
Thanks.
- /* Close bandwidth for all Queues */
- for (Queue = 0; Queue < MVPP2_MAX_TXQ; Queue++) {
- pTxq = Mvpp2TxqPhys (Port->Id, Queue);
- Mvpp2Write (Port->Priv, MVPP2_TXQ_SCHED_TOKEN_CNTR_REG(pTxq), 0);
- }
Spurious extra blank line.
OK.
+/* Clear counter of sent packets */ +VOID +Mvpp2TxqSentCounterClear (
- IN VOID *arg
IN OUT
OK.
+/*
- Set the number of packets that will be received before Rx interrupt
- will be generated by HW.
- */
+VOID +Mvpp2RxPktsCoalSet (
- IN PP2DXE_PORT *Port,
- IN MVPP2_RX_QUEUE *Rxq,
IN OUT
(No more individual comments below, but do please look at the general ones.)
Ok, I'll got through it again. I'll try to prepare complete v4 for all patches, so that they will be available for you tomorrow morning.
Actually, they haven't changed, apart from updating structures' fields naming update, so I think it's better to wait until you have v4 in hand tomorrow morning.
Best regards, Marcin