Re: [net-next PATCH v9 3/4] net: dsa: Add Airoha AN8855 5-Port Gigabit DSA Switch driver

[Christian Marangi]

On Fri, Dec 06, 2024 at 01:57:09AM +0200, Vladimir Oltean wrote:
> On Thu, Dec 05, 2024 at 08:36:30PM +0100, Christian Marangi wrote:
> > > I guess the non-hack solution would be to permit MDIO buses to have
> > > #size-cells = 1, and MDIO devices to acquire a range of the address
> > > space, rather than just one address. Though take this with a grain of
> > > salt, I have a lot more to learn.
> > 
> > I remember this was an idea when PHY Package API were proposed and was
> > rejected as we wanted PHY to be single reg.
> 
> Would that effort have helped with MDIO devices, in the way it was proposed?
> Why did it die out?
> 
> > > If neither of those are options, in principle the hack with just
> > > selecting, randomly, one of the N internal PHY addresses as the central
> > > MDIO address should work equally fine regardless of whether we are
> > > talking about the DSA switch's MDIO address here, or the MFD device's
> > > MDIO address.
> > > 
> > > With MFD you still have the option of creating a fake MDIO controller
> > > child device, which has mdio-parent-bus = <&host_bus>, and redirecting
> > > all user port phy-handles to children of this bus. Since all regmap I/O
> > > of this fake MDIO bus goes to the MFD driver, you can implement there
> > > your hacks with page switching etc etc, and it should be equally
> > > safe.
> > 
> > I wonder if a node like this would be more consistent and descriptive?
> > 
> > mdio_bus: mdio-bus {
> >     #address-cells = <1>;
> >     #size-cells = <0>;
> > 
> >     ...
> > 
> >     mfd@1 {
> >             compatible = "airoha,an8855-mfd";
> >             reg = <1>;
> > 
> >             nvmem_node {
> >                     ...
> >             };
> > 
> >             switch_node {
> >                 ports {
> >                         port@0 {
> >                                 phy-handle = <&phy>;
> >                         };
> > 
> >                         port@1 {
> >                                 phy-handle = <&phy_2>;
> >                         }
> >                 };
> >             };
> > 
> >             phy: phy_node {
> > 
> >             };
> >     };
> > 
> >     phy_2: phy@2 {
> >         reg = <2>;
> >     }
> > 
> >     phy@3 {
> >         reg = <3>;
> >     }
> > 
> >     ..
> > };
> > 
> > No idea how to register that single phy in mfd... I guess a fake mdio is
> > needed anyway... What do you think of this node example? Or not worth it
> > and better have the fake MDIO with all the switch PHY in it?
> 
> Could you work with something like this? dtc seems to swallow it without
> any warnings...
> 
> mdio_bus: mdio {
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         soc@1 {
>                 compatible = "airoha,an8855";
>                 reg = <1>, <2>, <3>, <4>;
>                 reg-names = "phy0", "phy1", "phy2", "phy3";
> 
>                 nvmem {
>                         compatible = "airoha,an8855-nvmem";
>                 };
> 
>                 ethernet-switch {
>                         compatible = "airoha,an8855-switch";
> 
>                         ethernet-ports {
>                                 #address-cells = <1>;
>                                 #size-cells = <0>;
> 
>                                 ethernet-port@0 {
>                                         reg = <0>;
>                                         phy-handle = <&phy0>;
>                                         phy-mode = "internal";
>                                 };
> 
>                                 ethernet-port@1 {
>                                         reg = <1>;
>                                         phy-handle = <&phy1>;
>                                         phy-mode = "internal";
>                                 };
> 
>                                 ethernet-port@2 {
>                                         reg = <2>;
>                                         phy-handle = <&phy2>;
>                                         phy-mode = "internal";
>                                 };
> 
>                                 ethernet-port@3 {
>                                         reg = <3>;
>                                         phy-handle = <&phy3>;
>                                         phy-mode = "internal";
>                                 };
>                         };
>                 };
> 
>                 mdio {
>                         compatible = "airoha,an8855-mdio";
>                         mdio-parent-bus = <&host_mdio>;
>                         #address-cells = <1>;
>                         #size-cells = <0>;
> 
>                         phy0: ethernet-phy@1 {
>                                 reg = <1>;
>                         };
> 
>                         phy1: ethernet-phy@2 {
>                                 reg = <2>;
>                         };
> 
>                         phy2: ethernet-phy@3 {
>                                 reg = <3>;
>                         };
> 
>                         phy3: ethernet-phy@4 {
>                                 reg = <4>;
>                         };
>                 };
>         };
> };

I finished testing and this works, I'm not using mdio-parent-bus tho as
the mdio-mux driver seems overkill for the task and problematic for PAGE
handling. (mdio-mux doesn't provide a way to give the current addr that
is being accessed)

My big concern is dt_binding_check and how Rob might take this
implementation. We recently had another case with a MFD node and Rob
found some problems in having subnode with compatible but maybe for this
particular complex case it will be O.K.

Still have to check if it's ok to have multiple reg in the mfd root node
(for mdio schema)

-- 
	Ansuel