Message-ID: <20250410084618.43edc269@fedora.home>
Date: Thu, 10 Apr 2025 08:46:18 +0200
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Andrew
 Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo
 Abeni <pabeni@...hat.com>, Vladimir Oltean <olteanv@...il.com>, Srinivas
 Kandagatla <srinivas.kandagatla@...aro.org>, Heiner Kallweit
 <hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>, "Chester A.
 Unal" <chester.a.unal@...nc9.com>, Daniel Golle <daniel@...rotopia.org>,
 DENG Qingfang <dqfext@...il.com>, Sean Wang <sean.wang@...iatek.com>, Simon
 Horman <horms@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
 netdev@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, upstream@...oha.com
Subject: Re: [net-next PATCH v14 07/16] net: mdio: regmap: add support for
 C45 read/write

On Wed, 9 Apr 2025 09:24:13 +0200
Christian Marangi <ansuelsmth@...il.com> wrote:

> On Wed, Apr 09, 2025 at 09:07:51AM +0200, Maxime Chevallier wrote:
> > Hi Christian,
> > 
> > On Tue,  8 Apr 2025 11:51:14 +0200
> > Christian Marangi <ansuelsmth@...il.com> wrote:
> >   
> > > Add support for C45 read/write for mdio regmap. This can be done
> > > by enabling the support_encoded_addr bool in mdio regmap config and by
> > > using the new API devm_mdio_regmap_init to init a regmap.
> > > 
> > > To support C45, additional info needs to be appended to the regmap
> > > address passed to regmap OPs.
> > > 
> > > The logic applied to the regmap address value:
> > > - First the regnum value (20, 16)
> > > - Second the devnum value (25, 21)
> > > - A bit to signal if it's C45 (26)
> > > 
> > > devm_mdio_regmap_init MUST be used to register a regmap for this to
> > > correctly handle internally the encode/decode of the address.
> > > 
> > > Drivers needs to define a mdio_regmap_init_config where an optional regmap
> > > name can be defined and MUST define C22 OPs (mdio_read/write).
> > > To support C45 operation also C45 OPs (mdio_read/write_c45).
> > > 
> > > The regmap from devm_mdio_regmap_init will internally decode the encoded
> > > regmap address and extract the various info (addr, devnum if C45 and
> > > regnum). It will then call the related OP and pass the extracted values to
> > > the function.
> > > 
> > > Example for a C45 read operation:
> > > - With an encoded address with C45 bit enabled, it will call the
> > >   .mdio_read_c45 and addr, devnum and regnum will be passed.
> > >   .mdio_read_c45 will then return the val and val will be stored in the
> > >   regmap_read pointer and will return 0. If .mdio_read_c45 returns
> > >   any error, then the regmap_read will return such error.
> > > 
> > > With support_encoded_addr enabled, also C22 will encode the address in
> > > the regmap address and .mdio_read/write will called accordingly similar
> > > to C45 operation.  
> > 
> > This driver's orginal goal is to address the case where we have a
> > PHY-like device that has the same register layout and behaviour as a
> > C22 PHY, but where the registers are not accesses through MDIO (MMIO
> > for example, as in altera-tse or dwmac-socfpga, or potentially SPI even
> > though  there's no example upstream).
> > 
> > What is done here is quite different, I guess it could work if we have
> > MMIO C45 phys that understand the proposed encoding, but I don't really
> > understand the dance where C45 accesses are wrapped by this mdio-regmap
> > driver into regmap accesss, but the regmap itself converts it back to
> > C45 accesses. Is it just so that it fits well with MFD ?  
> 
> The main task of this wrapping is to remove from the dev side having to
> handle the encode/decode part. regmap address is still a single value
> but if a phy is mmio mapped is difficult to support c45 since you need 3
> different values (phy id, mmd and addr)
> 
> With this implementation a c45 that is mmio mapped can implement
> whatever way he wants to configure each parameter for read/write
> operation.
> 
> Example the ecoding might be on different mask and with the additional
> function it can be reorganized following the specific mask.
> 
> > 
> > I'm not really against that, it still converts mdio access to regmap so
> > there's that, but is there a way to elaborate or document somewhere why
> > we need to do go through C45 -> regmap -> C45 instead of just
> > writing a mii_bus driver in the first place ?  
> 
> This was askek to prevent creating additional ""trivial"" mdio driver
> that would all do the same task. Since mdio-regmap was already in place
> it could have been extended with a more generic approach.

Ah yes that's the point I was missing, I've browsed more in depth and
indeed in V11 Vlad suggested to use this.

> Any hint on where to better document this?

Given the simplicity of the driver, I think this commit log is good
enough then :)

Let's go for it and hopefully this can be reused elsewhere !

Reviewed-by: Maxime Chevallier <maxime.chevallier@...tlin.com>

Maxime

Powered by blists - more mailing lists