Re: [PATCH net-next 09/16] net: mdio: Add Synopsys DW XPCS management interface support

[Serge Semin]

Hi Maxime,

On Tue, Dec 05, 2023 at 01:32:05PM +0100, Maxime Chevallier wrote:
> Hi Serge,
> 
> On Tue,  5 Dec 2023 13:35:30 +0300
> Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
> 
> > Synopsys DesignWare XPCS IP-core can be synthesized with the device CSRs
> > being accessible over MCI or APB3 interface instead of the MDIO bus (see
> > the CSR_INTERFACE HDL parameter). Thus all the PCS registers can be just
> > memory mapped and be a subject of standard MMIO operations of course
> > taking into account the way the Clause C45 CSRs mapping is defined. This
> > commit is about adding a device driver for the DW XPCS Management
> > Interface platform device and registering it in the framework of the
> > kernel MDIO subsystem.
> > 
> > DW XPCS platform device is supposed to be described by the respective
> > compatible string "snps,dw-xpcs-mi", CSRs memory space and optional
> > peripheral bus clock source. Note depending on the INDIRECT_ACCESS DW XPCS
> > IP-core synthesize parameter the memory-mapped reg-space can be
> > represented as either directly or indirectly mapped Clause 45 space. In
> > the former case the particular address is determined based on the MMD
> > device and the registers offset (5 + 16 bits all together) within the
> > device reg-space. In the later case there is only 256 lower address bits
> > are utilized for the registers mapping. The upper bits are supposed to be
> > written into the respective viewport CSR in order to reach the entire C45
> > space.
> 

> Too bad the mdio-regmap driver can't be re-used here, it would deal
> with reg width for you, for example. I guess the main reason would be
> the direct vs indirect accesses ?

Right, it's one of the reasons. I could have used the mdio-regmap
here, but that would have meant to implement an additional abstraction
layer: regspace<->regmap<->mdio-regmap<->mii_bus, instead of just
regspace<->mii_bus. This would have also required to patch the
mdio-remap module somehow in order to have the c45 ops supported. It
would have been much easier to do before the commit 99d5fe9c7f3d ("net:
mdio: Remove support for building C45 muxed addresses"). But since
then MDIO reg-address has no longer had muxed dev-address. Of course I
could have got it back to the mdio-regmap driver only, mix the C22/C45
address in the regmap 'addr' argument, then extract the MMD (for C45)
and reg addresses from the regmap IO ops argument and perform the
respective MMIO access. But as you can see it is much hardware and
would cause additional steps for the address translations, than
just directly implement the C22/C45 IO ops and register the MDIO bus
for them. I couldn't find much benefits to follow that road, sorry.

> 
> I do have a comment tough :
> 
> [...]
> 
> > +static inline ptrdiff_t dw_xpcs_mmio_addr_format(int dev, int reg)
> > +{
> > +	return FIELD_PREP(0x1f0000, dev) | FIELD_PREP(0xffff, reg);
> > +}
> > +
> > +static inline u16 dw_xpcs_mmio_addr_page(ptrdiff_t csr)
> > +{
> > +	return FIELD_GET(0x1fff00, csr);
> > +}
> > +
> > +static inline ptrdiff_t dw_xpcs_mmio_addr_offset(ptrdiff_t csr)
> > +{
> > +	return FIELD_GET(0xff, csr);
> > +}
> 

> You shouldn't use inline in C files, only in headers.

Could you please clarify why? I failed to find such requirement in the
coding style doc. Moreover there are multiple examples of using the
static-inline-ers in the C files in the kernel including the net/mdio
subsystem.

-Serge(y)

> 
> Maxime