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.
- 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.
- /* 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.
- 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 not, I agree it would be cleaner to remove sections.
- /* 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.
Best regards, Marcin