Hi Leif,
Thanks for your remarks. Please find comments inline.
Best Regards, Jan
- EFI_STATUS Status = EFI_SUCCESS;
- EFI_PHYSICAL_ADDRESS HpipeAddr = HPIPE_ADDR(HpipeBase, Lane);
- EFI_PHYSICAL_ADDRESS ComPhyAddr = COMPHY_ADDR(ComPhyBase, Lane);
- DEBUG((DEBUG_INFO, "ComPhy: stage: RFU configurations - hard reset "
- "ComPhy\n"));
Actually, for output strings, I much prefer the source line to go > 80 characters to splitting the string up. Otherwise it can be tricky to find the message in the source by grepping for a substring. There are a few such instances in this patch - I would appreciate if they could all be merged into single strings. (Although in this instance one could also argue that dropping the last 'ComPhy' would not reduce the amount of information provided.)
Ok, I want to be strict, but I see your point.
- /* Set PIPE soft reset */
- Mask = HPIPE_RST_CLK_CTRL_PIPE_RST_MASK;
- Data = 0x1 << HPIPE_RST_CLK_CTRL_PIPE_RST_OFFSET;
Could we have blank lines preceding each comment line please? (Throughout.)
Ok.
- EFI_STATUS Status;
- CHIP_COMPHY_CONFIG ChipConfig[MAX_CHIPS], *PtrChipCfg;
- PCD_LANE_MAP LaneData[MAX_CHIPS];
- UINT32 Lane, ChipCount, i, MaxComphyCount;
- ChipCount = PcdGet32 (PcdComPhyChipCount);
- GetComPhyPcd(ChipConfig, 0);
- GetComPhyPcd(ChipConfig, 1);
- GetComPhyPcd(ChipConfig, 2);
- GetComPhyPcd(ChipConfig, 3);
Is there any non-obvious side effect to this that means the above four lines could not be replaced by[1]?
Since GetComPhyPcd is preprocessor macro, its arguments cannot be incremented using for loop, but must be stated during preprocessing. In our case, this can be fulfilled only by invoking macro four times, each time with different argument.
+#undef FillLaneMap
This wasn't what I meant - see comments by macro definition below.
- if (ChipCount <= 0)
|| ChipCount > MAX_CHIPS ?
Ok.
- return EFI_INVALID_PARAMETER;
- for (i = 0; i < ChipCount ; i++) {
[1] GetComPhyPcd(ChipConfig, i);
Please find one of above comments about this.
+/***** Parsing PCD *****/ +#define GET_TYPE_STRING(id) PcdGetPtr(PcdChip##id##Compatible) +#define GET_LANE_TYPE(id) PcdGetPtr(PcdChip##id##ComPhyTypes) +#define GET_LANE_SPEED(id) PcdGetPtr(PcdChip##id##ComPhySpeeds) +#define GET_LANE_INV(id) PcdGetPtr(PcdChip##id##ComPhyInvFlags) +#define GET_COMPHY_BASE_ADDR(id) PcdGet64(PcdChip##id##ComPhyBaseAddress) +#define GET_HPIPE3_BASE_ADDR(id) PcdGet64(PcdChip##id##Hpipe3BaseAddress) +#define GET_MUX_BIT_COUNT(id) PcdGet32(PcdChip##id##ComPhyMuxBitCount) +#define GET_MAX_LANES(id) PcdGet32(PcdChip##id##ComPhyMaxLanes)
+#define FillLaneMap(id) { \
- ParsePcdString((CHAR16 *) GET_LANE_TYPE(id), ChipConfig[id].LanesCount, NULL, LaneData[id].TypeStr); \
- ParsePcdString((CHAR16 *) GET_LANE_SPEED(id), ChipConfig[id].LanesCount, LaneData[id].SpeedValue, NULL); \
- ParsePcdString((CHAR16 *) GET_LANE_INV(id), ChipConfig[id].LanesCount, LaneData[id].InvFlag, NULL); \
+}
This seems to not have addressed one of my comments from previous review:
This macro hides LaneData also - so can that be added as well, turning the below into a 3-input macro? If the above is _never_ called externally, the above macro can stay as is, but if so I would like it
#undef'd below GetComPhyPcd.
Was this an oversight, or was I unclear last time around?
My point is that these macros modify variables in the local scope holding a particular name. This makes for very difficult understanding of the code in ComPhyInit() from reading it, and makes it very tedious to maintain these macros if they ever need to change. Naming also the "LaneData" variable as an input to GetComPhyPcd, and adding it also to the FillLaneMap macro, improves the situation.
It was my mistake - I didn't understand you properly last time. I definitely agree with you, that we should try to make these macros as clear as possible (especially due to length). I will add new parameter to FillLaneMap.
+#define GetComPhyPcd(struct_name,id) { \
- struct_name[id].ChipType = (CHAR16 *) GET_TYPE_STRING(id); \
- struct_name[id].ComPhyBaseAddr = GET_COMPHY_BASE_ADDR(id); \
- struct_name[id].Hpipe3BaseAddr = GET_HPIPE3_BASE_ADDR(id); \
- struct_name[id].MuxBitCount = GET_MUX_BIT_COUNT(id); \
- struct_name[id].LanesCount = GET_MAX_LANES(id); \
- FillLaneMap(id); \
+}
(This was where I meant for the #undef to go, but actually I'd much rather see the 3-parameter GetComPhyPcd.)
It's no longer necessary with new approach.