Message-ID: <CAJq09z7GDOmKqmbD5evafSFaQswgSJPU-G+44DtwfidmZMS6sA@mail.gmail.com>
Date: Fri, 20 Dec 2024 02:11:15 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Chris Packham <chris.packham@...iedtelesis.co.nz>
Cc: Andrew Lunn <andrew@...n.ch>, lee@...nel.org, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, andrew+netdev@...n.ch, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	tsbogend@...ha.franken.de, hkallweit1@...il.com, linux@...linux.org.uk, 
	markus.stockhausen@....de, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org, 
	linux-mips@...r.kernel.org
Subject: Re: [PATCH v2 4/4] net: mdio: Add RTL9300 MDIO driver

> On 19/12/2024 22:40, Andrew Lunn wrote:
> > On Thu, Dec 19, 2024 at 01:46:41AM -0300, Luiz Angelo Daros de Luca wrote:
> >> Hello Chris,
> >>
> >>> +++ b/drivers/net/mdio/mdio-realtek-rtl.c
> >> I wonder if the name might be dubious in the future with other realtek
> >> products with MDIO. Realtek is quite a large company with many
> >> products. Would a version/model/family/usage in that name help a far
> >> future reader to identify what this file is about?
> > Isnt rtl the family name? Or would you prefer mdio-realtek-rtl9300.c?
>
> Yes my intention was that "rtl" was the family name.

rtl just means realtek. It is the same prefix for a wide range of
Realtek products (audio, usb, storage, network).

> I'm happy to change to rtl9300.

Even the product number might be confusing. For example, the switch
rtl8365mb is newer than (and incompatible with) the rtl8366rb. Realtek
has a library/driver name that could indicate the family (rtl8367b,
rtl8367c, rtl8367d), but it is not tightly related to model numbers.
In the DSA case, we simply adopted the first device that was supported
as the filename, even after that file was expanded to include other
models. I hope rtl93xx model numbers will be less confusing but it is
a Realtek decision. With the realtek past cases, if you want to be
sure it will not be confused with another model in the future, just
use the model "most significant" to your driver, like
modio-realtek-rtl9301. Do not expect that a future rtl93xx model will
be compatible with rtl9300. If rtl9300 does not really mean rtl93??,
rtl9301 would be, at least, more informative.

mdio-realtek-rtl9300.c or even mdio-rtl9300.c are just fine, but I
prefer the former as the common prefix will group different future
models, even if realtek abandons the rtl prefix.

> I suspect this probably will be compatible with the rtl9310. I've just
> received a RTL9313 based board so will probably start looking at that in
> the new year.

At least for the realtek switch-only products, if you have access to
the Realtek Programming Guide or the vendor library/driver, it would
be easy to spot compatible models as they share the same driver
generation. From what I saw from the difference between vendor switch
drivers rtl8367b, rtl8367c and rtl8367d, Relatek tends to use an
incremental model, but breaking the support to older models on every
new family release. They share a lot of code but they differ on
registers (normally constants on each generation). If you intend to
also upstream a DSA driver, you'll probably need to share some code
with the existing drivers as duplicating that much code is normally
rejected in the upstream kernel.

> >>> +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
> >> All those realtek_mdio_* prefix might collide with realtek_mdio_* from
> >> drivers/net/dsa/realtek/realtek-mdio.c. This realtek_mdio_* is about a
> >> Realtek SoC MDIO interface with the switch. The other realtek_mdio_*
> >> is about the interface (MDIO or SMI) between (the other vendor) SoC
> >> and the switch. I don't know if the maintainers are OK with it but
> >> listing those symbols in alphabetic order from both sources might be
> >> confusing.
> > rtl9300_ as a prefix?
>
> I'd happily  change to rtl_ or rtl9300_

"rtl9300_", please (or "rtl9301_") a "rtl" just means realtek.

> >>> +static const struct of_device_id realtek_mdio_ids[] = {
> >>> +       { .compatible = "realtek,rtl9301-mdio" },
> >>> +       { .compatible = "realtek,rtl9302b-mdio" },
> >>> +       { .compatible = "realtek,rtl9302c-mdio" },
> >>> +       { .compatible = "realtek,rtl9303-mdio" },
> >> Do these different compatible strings really matter? AFAIK, compatible
> >> are not for listing all supported models/variants but to describe
> >> devices that have a different behavior and indicating that (with
> >> different strings) is needed to decide how the driver will work. If
> >> the driver does not use which compatible was set, it might indicate
> >> that we don't really need 4 compatible but 1.
> > It can be useful when we initially think they are compatible, but
> > later find out they are not, and we need different behaviour.
>
> The way I've written the dt-binding any board should include
> "realtek,rtl9301-mdio" and may also include one of
> "realtek,rtl9302b-mdio", "realtek,rtl9302c-mdio",
> "realtek,rtl9303-mdio". For the MDIO driver the specific chip could
> possibly tell us the maximum SMI bus number. Unfortunately I've only got
> a block diagram of the RTL9302C, I know that does have 4 SMI interfaces,
> the others may have fewer. Things would probably work fine for now with
> just "realtek,rtl9301-mdio" but is there any harm in including the others?

If "realtek,rtl9301-mdio" is mandatory, until you need to
differentiate each model, the extra compatible strings are not useful.
It would increase the compatible table a little bit. My concern was
not about the extra data but that a similar approach was rejected in
the past. If maintainers are OK with it, I have nothing else to say.

>>> +{
>>> +       struct regmap *regmap = priv->regmap;
>>> +       u32 reg_base = priv->reg_base;
>>> +       u32 val;
>>> +
>>> +       return regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
>> All regmap funcs are adding reg_base to the register address. Isn't a
>> remap job to do that sum? It just looks odd but I never worked with
>> MFD. It looks like it is missing a subregmap-like variant.
>
> I'm thinking about dropping the base and just using the full 16-bit
> address. I've already confused myself between this code and the datasheet.

That's what I thought when I saw the sum. I would definitely miss it
at some point.
If their positions are fixed related to syscon base, I would use the
full 16-bit. You could use the base+relative_reg_addr in the macro
that defines the register address without increasing the complexity.

Regards,

Luiz

Powered by blists - more mailing lists