Re: [PATCH phy-next 5/5] phy: lynx-28g: add support for 25GBASER

[Vladimir Oltean]

Hi Josua,

On Wed, May 13, 2026 at 11:41:06AM +0000, Josua Mayer wrote:
> Am 13.05.26 um 13:22 schrieb Vladimir Oltean:
> 
> > On Wed, May 13, 2026 at 11:00:32AM +0000, Josua Mayer wrote:
> >> Wouldn't it be more clear instead of indirect lane offset shift with
> >> lynx_28g_e25g_pcvt, to instead fix the E25G_CFG definition?:
> >>
> >> -#define E25G_CFG(id) (28 - (id) * 4) /* Offset into PCCD */
> >> +#define E25G_CFG(id) ((id) * 4)      /* Offset into PCCD */
> >>
> >> This is equivalent when inserting (7 - lane) into E25G_CFG id:
> >>
> >> (28 - (id) * 4) = (28 - (7 - lane) * 4) = (28 - 7*4 + lane*4)
> > The 'indirect' lane offset shift is actually a two-step lookup:
> > lane -> protocol converter index
> > protocol converted index -> offset into PCCD
> >
> > LX2160ARM documents PCCD fields as:
> > E25GA_CFG, aka E25G_CFG(0) in code: 30:28
> > E25GB_CFG, aka E25G_CFG(1) in code: 26:24
> > ...
> > E25GH_CFG, aka E25G_CFG(7) in code: 2:0
> >
> > The odd bit is that lane 0 uses E25G protocol converter 7, unlike, say,
> > 1G and 10G where we have a lane:pcvt identity mapping.
> > lynx_28g_e25g_pcvt() performs that translation.
> >
> > Additionally, for locating E40GA_CFG, E40GB_CFG in PCCE, I've adopted
> > the same scheme downstream, where E40G_CFG() returns bits 30:28 for
> > argument 0 (pcvt A) and bits 26:24 for argument 1 (pcvt B), and it is
> > called with this lane->pcvt translation function:
> >
> > static int lynx_28g_e40g_pcvt(int lane)
> > {
> > 	return lane < 4 ? 1 : 0;
> > }
> >
> > Are you saying that merging the two lookups would be more clear because
> > as a reader you'd get to ask yourself less questions (the code would
> > flow more naturally) despite the non-trivial lane<->pcvt mapping,
> Exactly for this reason. Fewer look-up steps.
> RM dos not document this mapping, but now you do in source-code.
> 
> Perhaps a short comment on the function will help, e.g.
> /* get protocol converter id for lane */
> 
> > or why
> > exactly? For me it is the exact opposite. I can follow the RM
> > definitions and then I have a separate function that tells me how lanes
> > are mapped to the protocol converters.
> I think you give a good reason here!

So you would like me to respin this series? What change would address
your feedback here? Keep the E25G_CFG() macros as is, just add this?

+/* get protocol converter id for lane */
 static int lynx_28g_e25g_pcvt(int lane)
 {
 	return 7 - lane;
 }