Message-ID: <67f80275.df0a0220.39b09a.dd38@mx.google.com>
Date: Thu, 10 Apr 2025 19:40:02 +0200
From: Christian Marangi <ansuelsmth@...il.com>
To: Andrew Lunn <andrew@...n.ch>
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>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	"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 Thu, Apr 10, 2025 at 07:08:53PM +0200, Andrew Lunn wrote:
> On Tue, Apr 08, 2025 at 11:51:14AM +0200, Christian Marangi 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 patchset needs pulling apart, there are two many things going on.
> 
> You are adding at least two different features here. The current code
> only supports a single device on the bus, and it assumes the regmap
> provider knows what device that is. That is probably because all
> current users only have a single device. You now appear to want to
> pass that address to the regmap provider. I don't see the need for
> that, since it is still a single device on the bus. So adding this
> feature on its own, with a good commit message, will explain that.
>

Thing is that for C45 some kind of encoding/decoding is needed anyway
and with the suggested encoding (in previous patches) also C22 needs
special handling to extract the right address.

> You want to add C45 support. So that is another patch.
> 

I decided to implement C45 first as it would indirectly add support for
multiple register as for C45 you need to encode the PHY address anyway
(even if it's always the same) (making the next patch trivial as
everything will be already in place and just need to enable it by
passing a valid_addr_mask)

> C22 and C45 are different address spaces. To me, it seems logical to
> have different regmaps. That makes the regmap provider simpler. A C22
> regmap provider probably is just a straight access. A C45 regmap
> provider might need to handle the hardware having a sparse register
> map, only some of these 32 block of 65536 are implemented, etc.
> 
> So i think:
> 
> struct mdio_regmap_config {
>         struct device *parent;
>         struct regmap *regmap;
>         char name[MII_BUS_ID_SIZE];
>         u8 valid_addr;
>         bool autoscan;
> };
> 
> should be extended with a second regmap, used for C45.

So you are suggesting 2 regmap with dedicated read/write function.

The thing is that if the final target is to permit this driver to
support multiple PHY from a single regmap, and we also want to apply the
same encoding format, the regmap max_register will be the same for the 2
regmap making it redundant to have 2.

I think this is the blocking part that unlocks everything else.
Understand what is the preferable way to handle multiple PHY.

For C45 encoding is a MUST, and with encoding you get the side
effect/bonus feature that you can inject more info.

Hope you can give some guidance about this! Happy to split this once we
find a common point on how to proceed with this.

-- 
	Ansuel

Powered by blists - more mailing lists