Move MT7530 phy muxing from DSA to PHY driver

Move MT7530 phy muxing from DSA to PHY driver

[Arınç ÜNAL]

Hello folks.

MediaTek MT7530 switch has got 5 phys and 7 gmacs. gmac5 and gmac6 are 
treated as CPU ports.

This switch has got a feature which phy0 or phy4 can be muxed to gmac5 
of the switch. This allows an ethernet mac connected to gmac5 to 
directly connect to the phy.

PHY muxing works by looking for the compatible string "mediatek,eth-mac" 
then the mac address to find the gmac1 node. Then, it checks the mdio 
address on the node which "phy-handle" on the gmac1 node points to. If 
the mdio address is 0, phy0 is muxed to gmac5 of the switch. If it's 4, 
phy4 is muxed.

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n2238

Because that DSA probes the switch before muxing the phy, this won't 
work on devices which only use a single switch phy because probing will 
fail.

I'd like this operation to be done from the MediaTek Gigabit PHY driver 
instead. The motives for this change are that we solve the behaviour 
above, liberate the need to use DSA for this operation and get rid of 
the DSA overhead.

Would a change like this make sense and be accepted into netdev?

Arınç

Move MT7530 phy muxing from DSA to PHY driver

[Arınç ÜNAL]

Hello folks.

MediaTek MT7530 switch has got 5 phys and 7 gmacs. gmac5 and gmac6 are 
treated as CPU ports.

This switch has got a feature which phy0 or phy4 can be muxed to gmac5 
of the switch. This allows an ethernet mac connected to gmac5 to 
directly connect to the phy.

PHY muxing works by looking for the compatible string "mediatek,eth-mac" 
then the mac address to find the gmac1 node. Then, it checks the mdio 
address on the node which "phy-handle" on the gmac1 node points to. If 
the mdio address is 0, phy0 is muxed to gmac5 of the switch. If it's 4, 
phy4 is muxed.

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n2238

Because that DSA probes the switch before muxing the phy, this won't 
work on devices which only use a single switch phy because probing will 
fail.

I'd like this operation to be done from the MediaTek Gigabit PHY driver 
instead. The motives for this change are that we solve the behaviour 
above, liberate the need to use DSA for this operation and get rid of 
the DSA overhead.

Would a change like this make sense and be accepted into netdev?

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Move MT7530 phy muxing from DSA to PHY driver

[Andrew Lunn]

On Thu, Sep 15, 2022 at 01:07:13AM +0300, Arınç ÜNAL wrote:
> Hello folks.
> 
> MediaTek MT7530 switch has got 5 phys and 7 gmacs. gmac5 and gmac6 are
> treated as CPU ports.
> 
> This switch has got a feature which phy0 or phy4 can be muxed to gmac5 of
> the switch. This allows an ethernet mac connected to gmac5 to directly
> connect to the phy.
> 
> PHY muxing works by looking for the compatible string "mediatek,eth-mac"
> then the mac address to find the gmac1 node. Then, it checks the mdio
> address on the node which "phy-handle" on the gmac1 node points to. If the
> mdio address is 0, phy0 is muxed to gmac5 of the switch. If it's 4, phy4 is
> muxed.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n2238
> 
> Because that DSA probes the switch before muxing the phy, this won't work on
> devices which only use a single switch phy because probing will fail.
> 
> I'd like this operation to be done from the MediaTek Gigabit PHY driver
> instead. The motives for this change are that we solve the behaviour above,
> liberate the need to use DSA for this operation and get rid of the DSA
> overhead.
> 
> Would a change like this make sense and be accepted into netdev?

Where in the address range is the mux register? Officially, PHY
drivers only have access to PHY registers, via MDIO. If the mux
register is in the switch address space, it would be better if the
switch did the mux configuration. An alternative might be to represent
the mux in DT somewhere, and have a mux driver.

    Andrew

Re: Move MT7530 phy muxing from DSA to PHY driver

[Andrew Lunn]

On Thu, Sep 15, 2022 at 01:07:13AM +0300, Arınç ÜNAL wrote:
> Hello folks.
> 
> MediaTek MT7530 switch has got 5 phys and 7 gmacs. gmac5 and gmac6 are
> treated as CPU ports.
> 
> This switch has got a feature which phy0 or phy4 can be muxed to gmac5 of
> the switch. This allows an ethernet mac connected to gmac5 to directly
> connect to the phy.
> 
> PHY muxing works by looking for the compatible string "mediatek,eth-mac"
> then the mac address to find the gmac1 node. Then, it checks the mdio
> address on the node which "phy-handle" on the gmac1 node points to. If the
> mdio address is 0, phy0 is muxed to gmac5 of the switch. If it's 4, phy4 is
> muxed.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n2238
> 
> Because that DSA probes the switch before muxing the phy, this won't work on
> devices which only use a single switch phy because probing will fail.
> 
> I'd like this operation to be done from the MediaTek Gigabit PHY driver
> instead. The motives for this change are that we solve the behaviour above,
> liberate the need to use DSA for this operation and get rid of the DSA
> overhead.
> 
> Would a change like this make sense and be accepted into netdev?

Where in the address range is the mux register? Officially, PHY
drivers only have access to PHY registers, via MDIO. If the mux
register is in the switch address space, it would be better if the
switch did the mux configuration. An alternative might be to represent
the mux in DT somewhere, and have a mux driver.

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Move MT7530 phy muxing from DSA to PHY driver

[Arınç ÜNAL]

Hey Andrew,

On 15.09.2022 05:38, Andrew Lunn wrote:
> On Thu, Sep 15, 2022 at 01:07:13AM +0300, Arınç ÜNAL wrote:
>> Hello folks.
>>
>> MediaTek MT7530 switch has got 5 phys and 7 gmacs. gmac5 and gmac6 are
>> treated as CPU ports.
>>
>> This switch has got a feature which phy0 or phy4 can be muxed to gmac5 of
>> the switch. This allows an ethernet mac connected to gmac5 to directly
>> connect to the phy.
>>
>> PHY muxing works by looking for the compatible string "mediatek,eth-mac"
>> then the mac address to find the gmac1 node. Then, it checks the mdio
>> address on the node which "phy-handle" on the gmac1 node points to. If the
>> mdio address is 0, phy0 is muxed to gmac5 of the switch. If it's 4, phy4 is
>> muxed.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n2238
>>
>> Because that DSA probes the switch before muxing the phy, this won't work on
>> devices which only use a single switch phy because probing will fail.
>>
>> I'd like this operation to be done from the MediaTek Gigabit PHY driver
>> instead. The motives for this change are that we solve the behaviour above,
>> liberate the need to use DSA for this operation and get rid of the DSA
>> overhead.
>>
>> Would a change like this make sense and be accepted into netdev?
> 
> Where in the address range is the mux register? Officially, PHY
> drivers only have access to PHY registers, via MDIO. If the mux
> register is in the switch address space, it would be better if the
> switch did the mux configuration. An alternative might be to represent
> the mux in DT somewhere, and have a mux driver.

I don't know this part very well but it's in the register for hw trap 
modification which, I think, is in the switch address space.

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n941

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.h?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n500

Like you said, I don't think we can move away from the DSA driver, and 
would rather keep the driver do the mux configuration.

We could change the check for phy muxing to define the phy muxing 
bindings in the DSA node instead. If I understand correctly, the mdio 
address for PHYs is fake, it's for the sole purpose of making the driver 
check if there's request for phy muxing and which phy to mux. I'm saying 
this because the MT7530 switch works fine at address 0 while also using 
phy0 as a slave interface.

A property could be introduced on the DSA node for the MT7530 DSA driver:

     mdio {
         #address-cells = <1>;
         #size-cells = <0>;

         switch@0 {
             compatible = "mediatek,mt7530";
             reg = <0>;

             reset-gpios = <&pio 33 0>;

             core-supply = <&mt6323_vpa_reg>;
             io-supply = <&mt6323_vemc3v3_reg>;

             mt7530,mux-phy = <&sw0_p0>;

             ethernet-ports {
                 #address-cells = <1>;
                 #size-cells = <0>;

                 sw0_p0: port@0 {
                     reg = <0>;
                 };
             };
         };
     };

This would also allow using the phy muxing feature with any ethernet 
mac. Currently, phy muxing check wants the ethernet mac to be gmac1 of a 
MediaTek SoC. However, on a standalone MT7530, the switch can be wired 
to any SoC's ethernet mac.

For the port which is set for PHY muxing, do not bring it as a slave 
interface, just do the phy muxing operation.

Do not fail because there's no CPU port (ethernet property) defined when 
there's only one port defined and it's set for PHY muxing.

I don't know if the ethernet mac needs phy-handle defined in this case.

Arınç

Re: Move MT7530 phy muxing from DSA to PHY driver

[Arınç ÜNAL]

Hey Andrew,

On 15.09.2022 05:38, Andrew Lunn wrote:
> On Thu, Sep 15, 2022 at 01:07:13AM +0300, Arınç ÜNAL wrote:
>> Hello folks.
>>
>> MediaTek MT7530 switch has got 5 phys and 7 gmacs. gmac5 and gmac6 are
>> treated as CPU ports.
>>
>> This switch has got a feature which phy0 or phy4 can be muxed to gmac5 of
>> the switch. This allows an ethernet mac connected to gmac5 to directly
>> connect to the phy.
>>
>> PHY muxing works by looking for the compatible string "mediatek,eth-mac"
>> then the mac address to find the gmac1 node. Then, it checks the mdio
>> address on the node which "phy-handle" on the gmac1 node points to. If the
>> mdio address is 0, phy0 is muxed to gmac5 of the switch. If it's 4, phy4 is
>> muxed.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n2238
>>
>> Because that DSA probes the switch before muxing the phy, this won't work on
>> devices which only use a single switch phy because probing will fail.
>>
>> I'd like this operation to be done from the MediaTek Gigabit PHY driver
>> instead. The motives for this change are that we solve the behaviour above,
>> liberate the need to use DSA for this operation and get rid of the DSA
>> overhead.
>>
>> Would a change like this make sense and be accepted into netdev?
> 
> Where in the address range is the mux register? Officially, PHY
> drivers only have access to PHY registers, via MDIO. If the mux
> register is in the switch address space, it would be better if the
> switch did the mux configuration. An alternative might be to represent
> the mux in DT somewhere, and have a mux driver.

I don't know this part very well but it's in the register for hw trap 
modification which, I think, is in the switch address space.

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n941

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.h?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n500

Like you said, I don't think we can move away from the DSA driver, and 
would rather keep the driver do the mux configuration.

We could change the check for phy muxing to define the phy muxing 
bindings in the DSA node instead. If I understand correctly, the mdio 
address for PHYs is fake, it's for the sole purpose of making the driver 
check if there's request for phy muxing and which phy to mux. I'm saying 
this because the MT7530 switch works fine at address 0 while also using 
phy0 as a slave interface.

A property could be introduced on the DSA node for the MT7530 DSA driver:

     mdio {
         #address-cells = <1>;
         #size-cells = <0>;

         switch@0 {
             compatible = "mediatek,mt7530";
             reg = <0>;

             reset-gpios = <&pio 33 0>;

             core-supply = <&mt6323_vpa_reg>;
             io-supply = <&mt6323_vemc3v3_reg>;

             mt7530,mux-phy = <&sw0_p0>;

             ethernet-ports {
                 #address-cells = <1>;
                 #size-cells = <0>;

                 sw0_p0: port@0 {
                     reg = <0>;
                 };
             };
         };
     };

This would also allow using the phy muxing feature with any ethernet 
mac. Currently, phy muxing check wants the ethernet mac to be gmac1 of a 
MediaTek SoC. However, on a standalone MT7530, the switch can be wired 
to any SoC's ethernet mac.

For the port which is set for PHY muxing, do not bring it as a slave 
interface, just do the phy muxing operation.

Do not fail because there's no CPU port (ethernet property) defined when 
there's only one port defined and it's set for PHY muxing.

I don't know if the ethernet mac needs phy-handle defined in this case.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Move MT7530 phy muxing from DSA to PHY driver

[Andrew Lunn]

> > Where in the address range is the mux register? Officially, PHY
> > drivers only have access to PHY registers, via MDIO. If the mux
> > register is in the switch address space, it would be better if the
> > switch did the mux configuration. An alternative might be to represent
> > the mux in DT somewhere, and have a mux driver.
> 
> I don't know this part very well but it's in the register for hw trap
> modification which, I think, is in the switch address space.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n941
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.h?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n500
> 
> Like you said, I don't think we can move away from the DSA driver, and would
> rather keep the driver do the mux configuration.
> 
> We could change the check for phy muxing to define the phy muxing bindings
> in the DSA node instead. If I understand correctly, the mdio address for
> PHYs is fake, it's for the sole purpose of making the driver check if
> there's request for phy muxing and which phy to mux. I'm saying this because
> the MT7530 switch works fine at address 0 while also using phy0 as a slave
> interface.
> 
> A property could be introduced on the DSA node for the MT7530 DSA driver:
> 
>     mdio {
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         switch@0 {
>             compatible = "mediatek,mt7530";
>             reg = <0>;
> 
>             reset-gpios = <&pio 33 0>;
> 
>             core-supply = <&mt6323_vpa_reg>;
>             io-supply = <&mt6323_vemc3v3_reg>;
> 
>             mt7530,mux-phy = <&sw0_p0>;
> 
>             ethernet-ports {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> 
>                 sw0_p0: port@0 {
>                     reg = <0>;
>                 };
>             };
>         };
>     };
> 
> This would also allow using the phy muxing feature with any ethernet mac.
> Currently, phy muxing check wants the ethernet mac to be gmac1 of a MediaTek
> SoC. However, on a standalone MT7530, the switch can be wired to any SoC's
> ethernet mac.
> 
> For the port which is set for PHY muxing, do not bring it as a slave
> interface, just do the phy muxing operation.
> 
> Do not fail because there's no CPU port (ethernet property) defined when
> there's only one port defined and it's set for PHY muxing.
> 
> I don't know if the ethernet mac needs phy-handle defined in this case.

From mediatek,mt7530.yaml:

  Port 5 modes/configurations:
  1. Port 5 is disabled and isolated: An external phy can interface to the 2nd
     GMAC of the SOC.
     In the case of a build-in MT7530 switch, port 5 shares the RGMII bus with 2nd
     GMAC and an optional external phy. Mind the GPIO/pinctl settings of the SOC!
  2. Port 5 is muxed to PHY of port 0/4: Port 0/4 interfaces with 2nd GMAC.
     It is a simple MAC to PHY interface, port 5 needs to be setup for xMII mode
     and RGMII delay.
  3. Port 5 is muxed to GMAC5 and can interface to an external phy.
     Port 5 becomes an extra switch port.
     Only works on platform where external phy TX<->RX lines are swapped.
     Like in the Ubiquiti ER-X-SFP.
  4. Port 5 is muxed to GMAC5 and interfaces with the 2nd GAMC as 2nd CPU port.
     Currently a 2nd CPU port is not supported by DSA code.

So this mux has a scope bigger than the switch, it also affects one of
the SoCs MACs.

The phy-handle should have all the information you need, but it is
scattered over multiple locations. It could be in switch port 5, or it
could be in the SoC GMAC node.

Although the mux is in the switches address range, could you have a
tiny driver using that address range. Have this tiny driver export a
function to set the mux. Both the GMAC and the DSA driver make use of
the function, which should be enough to force the tiny driver to load
first. The GMAC and the DSA driver can then look at there phy-handle,
and determine how the mux should be set. The GMAC should probably do
that before register_netdev. The DSA driver before it registers the
switch with the DSA core.

Does that solve all your ordering issues?

By using the phy-handle, you don't need any additional properties, so
backwards compatibility should not be a problem. You can change driver
code as much as you want, but ABI like DT is fixed.

     Andrew

Re: Move MT7530 phy muxing from DSA to PHY driver

[Andrew Lunn]

> > Where in the address range is the mux register? Officially, PHY
> > drivers only have access to PHY registers, via MDIO. If the mux
> > register is in the switch address space, it would be better if the
> > switch did the mux configuration. An alternative might be to represent
> > the mux in DT somewhere, and have a mux driver.
> 
> I don't know this part very well but it's in the register for hw trap
> modification which, I think, is in the switch address space.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n941
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.h?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n500
> 
> Like you said, I don't think we can move away from the DSA driver, and would
> rather keep the driver do the mux configuration.
> 
> We could change the check for phy muxing to define the phy muxing bindings
> in the DSA node instead. If I understand correctly, the mdio address for
> PHYs is fake, it's for the sole purpose of making the driver check if
> there's request for phy muxing and which phy to mux. I'm saying this because
> the MT7530 switch works fine at address 0 while also using phy0 as a slave
> interface.
> 
> A property could be introduced on the DSA node for the MT7530 DSA driver:
> 
>     mdio {
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         switch@0 {
>             compatible = "mediatek,mt7530";
>             reg = <0>;
> 
>             reset-gpios = <&pio 33 0>;
> 
>             core-supply = <&mt6323_vpa_reg>;
>             io-supply = <&mt6323_vemc3v3_reg>;
> 
>             mt7530,mux-phy = <&sw0_p0>;
> 
>             ethernet-ports {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> 
>                 sw0_p0: port@0 {
>                     reg = <0>;
>                 };
>             };
>         };
>     };
> 
> This would also allow using the phy muxing feature with any ethernet mac.
> Currently, phy muxing check wants the ethernet mac to be gmac1 of a MediaTek
> SoC. However, on a standalone MT7530, the switch can be wired to any SoC's
> ethernet mac.
> 
> For the port which is set for PHY muxing, do not bring it as a slave
> interface, just do the phy muxing operation.
> 
> Do not fail because there's no CPU port (ethernet property) defined when
> there's only one port defined and it's set for PHY muxing.
> 
> I don't know if the ethernet mac needs phy-handle defined in this case.

From mediatek,mt7530.yaml:

  Port 5 modes/configurations:
  1. Port 5 is disabled and isolated: An external phy can interface to the 2nd
     GMAC of the SOC.
     In the case of a build-in MT7530 switch, port 5 shares the RGMII bus with 2nd
     GMAC and an optional external phy. Mind the GPIO/pinctl settings of the SOC!
  2. Port 5 is muxed to PHY of port 0/4: Port 0/4 interfaces with 2nd GMAC.
     It is a simple MAC to PHY interface, port 5 needs to be setup for xMII mode
     and RGMII delay.
  3. Port 5 is muxed to GMAC5 and can interface to an external phy.
     Port 5 becomes an extra switch port.
     Only works on platform where external phy TX<->RX lines are swapped.
     Like in the Ubiquiti ER-X-SFP.
  4. Port 5 is muxed to GMAC5 and interfaces with the 2nd GAMC as 2nd CPU port.
     Currently a 2nd CPU port is not supported by DSA code.

So this mux has a scope bigger than the switch, it also affects one of
the SoCs MACs.

The phy-handle should have all the information you need, but it is
scattered over multiple locations. It could be in switch port 5, or it
could be in the SoC GMAC node.

Although the mux is in the switches address range, could you have a
tiny driver using that address range. Have this tiny driver export a
function to set the mux. Both the GMAC and the DSA driver make use of
the function, which should be enough to force the tiny driver to load
first. The GMAC and the DSA driver can then look at there phy-handle,
and determine how the mux should be set. The GMAC should probably do
that before register_netdev. The DSA driver before it registers the
switch with the DSA core.

Does that solve all your ordering issues?

By using the phy-handle, you don't need any additional properties, so
backwards compatibility should not be a problem. You can change driver
code as much as you want, but ABI like DT is fixed.

     Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Move MT7530 phy muxing from DSA to PHY driver

[Arınç ÜNAL]

On 17.09.2022 18:07, Andrew Lunn wrote:
>>> Where in the address range is the mux register? Officially, PHY
>>> drivers only have access to PHY registers, via MDIO. If the mux
>>> register is in the switch address space, it would be better if the
>>> switch did the mux configuration. An alternative might be to represent
>>> the mux in DT somewhere, and have a mux driver.
>>
>> I don't know this part very well but it's in the register for hw trap
>> modification which, I think, is in the switch address space.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n941
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.h?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n500
>>
>> Like you said, I don't think we can move away from the DSA driver, and would
>> rather keep the driver do the mux configuration.
>>
>> We could change the check for phy muxing to define the phy muxing bindings
>> in the DSA node instead. If I understand correctly, the mdio address for
>> PHYs is fake, it's for the sole purpose of making the driver check if
>> there's request for phy muxing and which phy to mux. I'm saying this because
>> the MT7530 switch works fine at address 0 while also using phy0 as a slave
>> interface.
>>
>> A property could be introduced on the DSA node for the MT7530 DSA driver:
>>
>>      mdio {
>>          #address-cells = <1>;
>>          #size-cells = <0>;
>>
>>          switch@0 {
>>              compatible = "mediatek,mt7530";
>>              reg = <0>;
>>
>>              reset-gpios = <&pio 33 0>;
>>
>>              core-supply = <&mt6323_vpa_reg>;
>>              io-supply = <&mt6323_vemc3v3_reg>;
>>
>>              mt7530,mux-phy = <&sw0_p0>;
>>
>>              ethernet-ports {
>>                  #address-cells = <1>;
>>                  #size-cells = <0>;
>>
>>                  sw0_p0: port@0 {
>>                      reg = <0>;
>>                  };
>>              };
>>          };
>>      };
>>
>> This would also allow using the phy muxing feature with any ethernet mac.
>> Currently, phy muxing check wants the ethernet mac to be gmac1 of a MediaTek
>> SoC. However, on a standalone MT7530, the switch can be wired to any SoC's
>> ethernet mac.
>>
>> For the port which is set for PHY muxing, do not bring it as a slave
>> interface, just do the phy muxing operation.
>>
>> Do not fail because there's no CPU port (ethernet property) defined when
>> there's only one port defined and it's set for PHY muxing.
>>
>> I don't know if the ethernet mac needs phy-handle defined in this case.
> 
>  From mediatek,mt7530.yaml:
> 
>    Port 5 modes/configurations:
>    1. Port 5 is disabled and isolated: An external phy can interface to the 2nd
>       GMAC of the SOC.
>       In the case of a build-in MT7530 switch, port 5 shares the RGMII bus with 2nd
>       GMAC and an optional external phy. Mind the GPIO/pinctl settings of the SOC!
>    2. Port 5 is muxed to PHY of port 0/4: Port 0/4 interfaces with 2nd GMAC.
>       It is a simple MAC to PHY interface, port 5 needs to be setup for xMII mode
>       and RGMII delay.
>    3. Port 5 is muxed to GMAC5 and can interface to an external phy.
>       Port 5 becomes an extra switch port.
>       Only works on platform where external phy TX<->RX lines are swapped.
>       Like in the Ubiquiti ER-X-SFP.
>    4. Port 5 is muxed to GMAC5 and interfaces with the 2nd GAMC as 2nd CPU port.
>       Currently a 2nd CPU port is not supported by DSA code.
> 
> So this mux has a scope bigger than the switch, it also affects one of
> the SoCs MACs.
> 
> The phy-handle should have all the information you need, but it is
> scattered over multiple locations. It could be in switch port 5, or it
> could be in the SoC GMAC node.
> 
> Although the mux is in the switches address range, could you have a
> tiny driver using that address range. Have this tiny driver export a
> function to set the mux. Both the GMAC and the DSA driver make use of
> the function, which should be enough to force the tiny driver to load
> first. The GMAC and the DSA driver can then look at there phy-handle,
> and determine how the mux should be set. The GMAC should probably do
> that before register_netdev. The DSA driver before it registers the
> switch with the DSA core.
> 
> Does that solve all your ordering issues?

I believe it does.

> 
> By using the phy-handle, you don't need any additional properties, so
> backwards compatibility should not be a problem. You can change driver
> code as much as you want, but ABI like DT is fixed.

Understood, thanks Andrew!

Arınç

Re: Move MT7530 phy muxing from DSA to PHY driver

[Arınç ÜNAL]

On 17.09.2022 18:07, Andrew Lunn wrote:
>>> Where in the address range is the mux register? Officially, PHY
>>> drivers only have access to PHY registers, via MDIO. If the mux
>>> register is in the switch address space, it would be better if the
>>> switch did the mux configuration. An alternative might be to represent
>>> the mux in DT somewhere, and have a mux driver.
>>
>> I don't know this part very well but it's in the register for hw trap
>> modification which, I think, is in the switch address space.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n941
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.h?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n500
>>
>> Like you said, I don't think we can move away from the DSA driver, and would
>> rather keep the driver do the mux configuration.
>>
>> We could change the check for phy muxing to define the phy muxing bindings
>> in the DSA node instead. If I understand correctly, the mdio address for
>> PHYs is fake, it's for the sole purpose of making the driver check if
>> there's request for phy muxing and which phy to mux. I'm saying this because
>> the MT7530 switch works fine at address 0 while also using phy0 as a slave
>> interface.
>>
>> A property could be introduced on the DSA node for the MT7530 DSA driver:
>>
>>      mdio {
>>          #address-cells = <1>;
>>          #size-cells = <0>;
>>
>>          switch@0 {
>>              compatible = "mediatek,mt7530";
>>              reg = <0>;
>>
>>              reset-gpios = <&pio 33 0>;
>>
>>              core-supply = <&mt6323_vpa_reg>;
>>              io-supply = <&mt6323_vemc3v3_reg>;
>>
>>              mt7530,mux-phy = <&sw0_p0>;
>>
>>              ethernet-ports {
>>                  #address-cells = <1>;
>>                  #size-cells = <0>;
>>
>>                  sw0_p0: port@0 {
>>                      reg = <0>;
>>                  };
>>              };
>>          };
>>      };
>>
>> This would also allow using the phy muxing feature with any ethernet mac.
>> Currently, phy muxing check wants the ethernet mac to be gmac1 of a MediaTek
>> SoC. However, on a standalone MT7530, the switch can be wired to any SoC's
>> ethernet mac.
>>
>> For the port which is set for PHY muxing, do not bring it as a slave
>> interface, just do the phy muxing operation.
>>
>> Do not fail because there's no CPU port (ethernet property) defined when
>> there's only one port defined and it's set for PHY muxing.
>>
>> I don't know if the ethernet mac needs phy-handle defined in this case.
> 
>  From mediatek,mt7530.yaml:
> 
>    Port 5 modes/configurations:
>    1. Port 5 is disabled and isolated: An external phy can interface to the 2nd
>       GMAC of the SOC.
>       In the case of a build-in MT7530 switch, port 5 shares the RGMII bus with 2nd
>       GMAC and an optional external phy. Mind the GPIO/pinctl settings of the SOC!
>    2. Port 5 is muxed to PHY of port 0/4: Port 0/4 interfaces with 2nd GMAC.
>       It is a simple MAC to PHY interface, port 5 needs to be setup for xMII mode
>       and RGMII delay.
>    3. Port 5 is muxed to GMAC5 and can interface to an external phy.
>       Port 5 becomes an extra switch port.
>       Only works on platform where external phy TX<->RX lines are swapped.
>       Like in the Ubiquiti ER-X-SFP.
>    4. Port 5 is muxed to GMAC5 and interfaces with the 2nd GAMC as 2nd CPU port.
>       Currently a 2nd CPU port is not supported by DSA code.
> 
> So this mux has a scope bigger than the switch, it also affects one of
> the SoCs MACs.
> 
> The phy-handle should have all the information you need, but it is
> scattered over multiple locations. It could be in switch port 5, or it
> could be in the SoC GMAC node.
> 
> Although the mux is in the switches address range, could you have a
> tiny driver using that address range. Have this tiny driver export a
> function to set the mux. Both the GMAC and the DSA driver make use of
> the function, which should be enough to force the tiny driver to load
> first. The GMAC and the DSA driver can then look at there phy-handle,
> and determine how the mux should be set. The GMAC should probably do
> that before register_netdev. The DSA driver before it registers the
> switch with the DSA core.
> 
> Does that solve all your ordering issues?

I believe it does.

> 
> By using the phy-handle, you don't need any additional properties, so
> backwards compatibility should not be a problem. You can change driver
> code as much as you want, but ABI like DT is fixed.

Understood, thanks Andrew!

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Move MT7530 phy muxing from DSA to PHY driver

[Florian Fainelli]

On 9/18/2022 4:28 AM, Arınç ÜNAL wrote:
> On 17.09.2022 18:07, Andrew Lunn wrote:
>>>> Where in the address range is the mux register? Officially, PHY
>>>> drivers only have access to PHY registers, via MDIO. If the mux
>>>> register is in the switch address space, it would be better if the
>>>> switch did the mux configuration. An alternative might be to represent
>>>> the mux in DT somewhere, and have a mux driver.
>>>
>>> I don't know this part very well but it's in the register for hw trap
>>> modification which, I think, is in the switch address space.
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n941
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.h?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n500
>>>
>>> Like you said, I don't think we can move away from the DSA driver, 
>>> and would
>>> rather keep the driver do the mux configuration.
>>>
>>> We could change the check for phy muxing to define the phy muxing 
>>> bindings
>>> in the DSA node instead. If I understand correctly, the mdio address for
>>> PHYs is fake, it's for the sole purpose of making the driver check if
>>> there's request for phy muxing and which phy to mux. I'm saying this 
>>> because
>>> the MT7530 switch works fine at address 0 while also using phy0 as a 
>>> slave
>>> interface.
>>>
>>> A property could be introduced on the DSA node for the MT7530 DSA 
>>> driver:
>>>
>>>      mdio {
>>>          #address-cells = <1>;
>>>          #size-cells = <0>;
>>>
>>>          switch@0 {
>>>              compatible = "mediatek,mt7530";
>>>              reg = <0>;
>>>
>>>              reset-gpios = <&pio 33 0>;
>>>
>>>              core-supply = <&mt6323_vpa_reg>;
>>>              io-supply = <&mt6323_vemc3v3_reg>;
>>>
>>>              mt7530,mux-phy = <&sw0_p0>;
>>>
>>>              ethernet-ports {
>>>                  #address-cells = <1>;
>>>                  #size-cells = <0>;
>>>
>>>                  sw0_p0: port@0 {
>>>                      reg = <0>;
>>>                  };
>>>              };
>>>          };
>>>      };
>>>
>>> This would also allow using the phy muxing feature with any ethernet 
>>> mac.
>>> Currently, phy muxing check wants the ethernet mac to be gmac1 of a 
>>> MediaTek
>>> SoC. However, on a standalone MT7530, the switch can be wired to any 
>>> SoC's
>>> ethernet mac.
>>>
>>> For the port which is set for PHY muxing, do not bring it as a slave
>>> interface, just do the phy muxing operation.
>>>
>>> Do not fail because there's no CPU port (ethernet property) defined when
>>> there's only one port defined and it's set for PHY muxing.
>>>
>>> I don't know if the ethernet mac needs phy-handle defined in this case.
>>
>>  From mediatek,mt7530.yaml:
>>
>>    Port 5 modes/configurations:
>>    1. Port 5 is disabled and isolated: An external phy can interface 
>> to the 2nd
>>       GMAC of the SOC.
>>       In the case of a build-in MT7530 switch, port 5 shares the RGMII 
>> bus with 2nd
>>       GMAC and an optional external phy. Mind the GPIO/pinctl settings 
>> of the SOC!
>>    2. Port 5 is muxed to PHY of port 0/4: Port 0/4 interfaces with 2nd 
>> GMAC.
>>       It is a simple MAC to PHY interface, port 5 needs to be setup 
>> for xMII mode
>>       and RGMII delay.
>>    3. Port 5 is muxed to GMAC5 and can interface to an external phy.
>>       Port 5 becomes an extra switch port.
>>       Only works on platform where external phy TX<->RX lines are 
>> swapped.
>>       Like in the Ubiquiti ER-X-SFP.
>>    4. Port 5 is muxed to GMAC5 and interfaces with the 2nd GAMC as 2nd 
>> CPU port.
>>       Currently a 2nd CPU port is not supported by DSA code.
>>
>> So this mux has a scope bigger than the switch, it also affects one of
>> the SoCs MACs.
>>
>> The phy-handle should have all the information you need, but it is
>> scattered over multiple locations. It could be in switch port 5, or it
>> could be in the SoC GMAC node.
>>
>> Although the mux is in the switches address range, could you have a
>> tiny driver using that address range. Have this tiny driver export a
>> function to set the mux. Both the GMAC and the DSA driver make use of
>> the function, which should be enough to force the tiny driver to load
>> first. The GMAC and the DSA driver can then look at there phy-handle,
>> and determine how the mux should be set. The GMAC should probably do
>> that before register_netdev. The DSA driver before it registers the
>> switch with the DSA core.
>>
>> Does that solve all your ordering issues?
> 
> I believe it does.
> 
>>
>> By using the phy-handle, you don't need any additional properties, so
>> backwards compatibility should not be a problem. You can change driver
>> code as much as you want, but ABI like DT is fixed.
> 
> Understood, thanks Andrew!

Yes this seems like a reasonably good idea, I would be a bit concerned 
about possibly running into issues with fw_devlink=on and whichever 
driver is managing the PHY device not being an actual PHY device driver 
provider and thus preventing the PHY device consumers from probing. This 
is not necessarily an issue right now though because 'phy-handle' is not 
(yet again) part of of_supplier_bindings.

Maybe what you can do is just describe that mux register space using a 
dedicated DT node, and use a syscon phandle for both the switch and/or 
the MAC and have them use an exported symbol routine that is responsible 
for configuring the mux in an atomic and consistent way.
-- 
Florian

Re: Move MT7530 phy muxing from DSA to PHY driver

[Florian Fainelli]

On 9/18/2022 4:28 AM, Arınç ÜNAL wrote:
> On 17.09.2022 18:07, Andrew Lunn wrote:
>>>> Where in the address range is the mux register? Officially, PHY
>>>> drivers only have access to PHY registers, via MDIO. If the mux
>>>> register is in the switch address space, it would be better if the
>>>> switch did the mux configuration. An alternative might be to represent
>>>> the mux in DT somewhere, and have a mux driver.
>>>
>>> I don't know this part very well but it's in the register for hw trap
>>> modification which, I think, is in the switch address space.
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n941
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.h?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n500
>>>
>>> Like you said, I don't think we can move away from the DSA driver, 
>>> and would
>>> rather keep the driver do the mux configuration.
>>>
>>> We could change the check for phy muxing to define the phy muxing 
>>> bindings
>>> in the DSA node instead. If I understand correctly, the mdio address for
>>> PHYs is fake, it's for the sole purpose of making the driver check if
>>> there's request for phy muxing and which phy to mux. I'm saying this 
>>> because
>>> the MT7530 switch works fine at address 0 while also using phy0 as a 
>>> slave
>>> interface.
>>>
>>> A property could be introduced on the DSA node for the MT7530 DSA 
>>> driver:
>>>
>>>      mdio {
>>>          #address-cells = <1>;
>>>          #size-cells = <0>;
>>>
>>>          switch@0 {
>>>              compatible = "mediatek,mt7530";
>>>              reg = <0>;
>>>
>>>              reset-gpios = <&pio 33 0>;
>>>
>>>              core-supply = <&mt6323_vpa_reg>;
>>>              io-supply = <&mt6323_vemc3v3_reg>;
>>>
>>>              mt7530,mux-phy = <&sw0_p0>;
>>>
>>>              ethernet-ports {
>>>                  #address-cells = <1>;
>>>                  #size-cells = <0>;
>>>
>>>                  sw0_p0: port@0 {
>>>                      reg = <0>;
>>>                  };
>>>              };
>>>          };
>>>      };
>>>
>>> This would also allow using the phy muxing feature with any ethernet 
>>> mac.
>>> Currently, phy muxing check wants the ethernet mac to be gmac1 of a 
>>> MediaTek
>>> SoC. However, on a standalone MT7530, the switch can be wired to any 
>>> SoC's
>>> ethernet mac.
>>>
>>> For the port which is set for PHY muxing, do not bring it as a slave
>>> interface, just do the phy muxing operation.
>>>
>>> Do not fail because there's no CPU port (ethernet property) defined when
>>> there's only one port defined and it's set for PHY muxing.
>>>
>>> I don't know if the ethernet mac needs phy-handle defined in this case.
>>
>>  From mediatek,mt7530.yaml:
>>
>>    Port 5 modes/configurations:
>>    1. Port 5 is disabled and isolated: An external phy can interface 
>> to the 2nd
>>       GMAC of the SOC.
>>       In the case of a build-in MT7530 switch, port 5 shares the RGMII 
>> bus with 2nd
>>       GMAC and an optional external phy. Mind the GPIO/pinctl settings 
>> of the SOC!
>>    2. Port 5 is muxed to PHY of port 0/4: Port 0/4 interfaces with 2nd 
>> GMAC.
>>       It is a simple MAC to PHY interface, port 5 needs to be setup 
>> for xMII mode
>>       and RGMII delay.
>>    3. Port 5 is muxed to GMAC5 and can interface to an external phy.
>>       Port 5 becomes an extra switch port.
>>       Only works on platform where external phy TX<->RX lines are 
>> swapped.
>>       Like in the Ubiquiti ER-X-SFP.
>>    4. Port 5 is muxed to GMAC5 and interfaces with the 2nd GAMC as 2nd 
>> CPU port.
>>       Currently a 2nd CPU port is not supported by DSA code.
>>
>> So this mux has a scope bigger than the switch, it also affects one of
>> the SoCs MACs.
>>
>> The phy-handle should have all the information you need, but it is
>> scattered over multiple locations. It could be in switch port 5, or it
>> could be in the SoC GMAC node.
>>
>> Although the mux is in the switches address range, could you have a
>> tiny driver using that address range. Have this tiny driver export a
>> function to set the mux. Both the GMAC and the DSA driver make use of
>> the function, which should be enough to force the tiny driver to load
>> first. The GMAC and the DSA driver can then look at there phy-handle,
>> and determine how the mux should be set. The GMAC should probably do
>> that before register_netdev. The DSA driver before it registers the
>> switch with the DSA core.
>>
>> Does that solve all your ordering issues?
> 
> I believe it does.
> 
>>
>> By using the phy-handle, you don't need any additional properties, so
>> backwards compatibility should not be a problem. You can change driver
>> code as much as you want, but ABI like DT is fixed.
> 
> Understood, thanks Andrew!

Yes this seems like a reasonably good idea, I would be a bit concerned 
about possibly running into issues with fw_devlink=on and whichever 
driver is managing the PHY device not being an actual PHY device driver 
provider and thus preventing the PHY device consumers from probing. This 
is not necessarily an issue right now though because 'phy-handle' is not 
(yet again) part of of_supplier_bindings.

Maybe what you can do is just describe that mux register space using a 
dedicated DT node, and use a syscon phandle for both the switch and/or 
the MAC and have them use an exported symbol routine that is responsible 
for configuring the mux in an atomic and consistent way.
-- 
Florian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Move MT7530 phy muxing from DSA to PHY driver

[Arınç ÜNAL]

On 19.09.2022 01:58, Florian Fainelli wrote:
> 
> 
> On 9/18/2022 4:28 AM, Arınç ÜNAL wrote:
>> On 17.09.2022 18:07, Andrew Lunn wrote:
>>>>> Where in the address range is the mux register? Officially, PHY
>>>>> drivers only have access to PHY registers, via MDIO. If the mux
>>>>> register is in the switch address space, it would be better if the
>>>>> switch did the mux configuration. An alternative might be to represent
>>>>> the mux in DT somewhere, and have a mux driver.
>>>>
>>>> I don't know this part very well but it's in the register for hw trap
>>>> modification which, I think, is in the switch address space.
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n941
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.h?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n500
>>>>
>>>> Like you said, I don't think we can move away from the DSA driver, 
>>>> and would
>>>> rather keep the driver do the mux configuration.
>>>>
>>>> We could change the check for phy muxing to define the phy muxing 
>>>> bindings
>>>> in the DSA node instead. If I understand correctly, the mdio address 
>>>> for
>>>> PHYs is fake, it's for the sole purpose of making the driver check if
>>>> there's request for phy muxing and which phy to mux. I'm saying this 
>>>> because
>>>> the MT7530 switch works fine at address 0 while also using phy0 as a 
>>>> slave
>>>> interface.
>>>>
>>>> A property could be introduced on the DSA node for the MT7530 DSA 
>>>> driver:
>>>>
>>>>      mdio {
>>>>          #address-cells = <1>;
>>>>          #size-cells = <0>;
>>>>
>>>>          switch@0 {
>>>>              compatible = "mediatek,mt7530";
>>>>              reg = <0>;
>>>>
>>>>              reset-gpios = <&pio 33 0>;
>>>>
>>>>              core-supply = <&mt6323_vpa_reg>;
>>>>              io-supply = <&mt6323_vemc3v3_reg>;
>>>>
>>>>              mt7530,mux-phy = <&sw0_p0>;
>>>>
>>>>              ethernet-ports {
>>>>                  #address-cells = <1>;
>>>>                  #size-cells = <0>;
>>>>
>>>>                  sw0_p0: port@0 {
>>>>                      reg = <0>;
>>>>                  };
>>>>              };
>>>>          };
>>>>      };
>>>>
>>>> This would also allow using the phy muxing feature with any ethernet 
>>>> mac.
>>>> Currently, phy muxing check wants the ethernet mac to be gmac1 of a 
>>>> MediaTek
>>>> SoC. However, on a standalone MT7530, the switch can be wired to any 
>>>> SoC's
>>>> ethernet mac.
>>>>
>>>> For the port which is set for PHY muxing, do not bring it as a slave
>>>> interface, just do the phy muxing operation.
>>>>
>>>> Do not fail because there's no CPU port (ethernet property) defined 
>>>> when
>>>> there's only one port defined and it's set for PHY muxing.
>>>>
>>>> I don't know if the ethernet mac needs phy-handle defined in this case.
>>>
>>>  From mediatek,mt7530.yaml:
>>>
>>>    Port 5 modes/configurations:
>>>    1. Port 5 is disabled and isolated: An external phy can interface 
>>> to the 2nd
>>>       GMAC of the SOC.
>>>       In the case of a build-in MT7530 switch, port 5 shares the 
>>> RGMII bus with 2nd
>>>       GMAC and an optional external phy. Mind the GPIO/pinctl 
>>> settings of the SOC!
>>>    2. Port 5 is muxed to PHY of port 0/4: Port 0/4 interfaces with 
>>> 2nd GMAC.
>>>       It is a simple MAC to PHY interface, port 5 needs to be setup 
>>> for xMII mode
>>>       and RGMII delay.
>>>    3. Port 5 is muxed to GMAC5 and can interface to an external phy.
>>>       Port 5 becomes an extra switch port.
>>>       Only works on platform where external phy TX<->RX lines are 
>>> swapped.
>>>       Like in the Ubiquiti ER-X-SFP.
>>>    4. Port 5 is muxed to GMAC5 and interfaces with the 2nd GAMC as 
>>> 2nd CPU port.
>>>       Currently a 2nd CPU port is not supported by DSA code.
>>>
>>> So this mux has a scope bigger than the switch, it also affects one of
>>> the SoCs MACs.
>>>
>>> The phy-handle should have all the information you need, but it is
>>> scattered over multiple locations. It could be in switch port 5, or it
>>> could be in the SoC GMAC node.
>>>
>>> Although the mux is in the switches address range, could you have a
>>> tiny driver using that address range. Have this tiny driver export a
>>> function to set the mux. Both the GMAC and the DSA driver make use of
>>> the function, which should be enough to force the tiny driver to load
>>> first. The GMAC and the DSA driver can then look at there phy-handle,
>>> and determine how the mux should be set. The GMAC should probably do
>>> that before register_netdev. The DSA driver before it registers the
>>> switch with the DSA core.
>>>
>>> Does that solve all your ordering issues?
>>
>> I believe it does.
>>
>>>
>>> By using the phy-handle, you don't need any additional properties, so
>>> backwards compatibility should not be a problem. You can change driver
>>> code as much as you want, but ABI like DT is fixed.
>>
>> Understood, thanks Andrew!
> 
> Yes this seems like a reasonably good idea, I would be a bit concerned 
> about possibly running into issues with fw_devlink=on and whichever 
> driver is managing the PHY device not being an actual PHY device driver 
> provider and thus preventing the PHY device consumers from probing. This 
> is not necessarily an issue right now though because 'phy-handle' is not 
> (yet again) part of of_supplier_bindings.
> 
> Maybe what you can do is just describe that mux register space using a 
> dedicated DT node, and use a syscon phandle for both the switch and/or 
> the MAC and have them use an exported symbol routine that is responsible 
> for configuring the mux in an atomic and consistent way.

I'm currently working on the mt7530 driver and I think I found a way
that takes the least effort, won't break the ABI, and most importantly,
will work.

As we acknowledged, the registers are in the switch address space. This
must also mean that the switch must be properly probed before doing
anything with the registers.

I'm not sure how exactly making a tiny driver would work in this case.

I figured we can just run the phy muxing code before the DSA driver
exits because there are no (CPU) ports defined on the devicetree. Right
after probing is done on mt7530_probe, before dsa_register_switch() is run.

For proof of concept, I've moved some necessary switch probing code from
mt7530_setup() to mt7530_probe(). After the switch is properly reset,
phy4 is muxed, before dsa_register_switch() is run.

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index b0d9ea7b8e6d..0c1d9f83e73b 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2085,7 +2085,6 @@ mt7530_setup(struct dsa_switch *ds)
  	struct device_node *dn = NULL;
  	struct device_node *phy_node;
  	struct device_node *mac_np;
-	struct mt7530_dummy_poll p;
  	phy_interface_t interface;
  	struct dsa_port *cpu_dp;
  	u32 id, val;
@@ -2111,58 +2110,6 @@ mt7530_setup(struct dsa_switch *ds)
  	ds->assisted_learning_on_cpu_port = true;
  	ds->mtu_enforcement_ingress = true;
  
-	if (priv->id == ID_MT7530) {
-		regulator_set_voltage(priv->core_pwr, 1000000, 1000000);
-		ret = regulator_enable(priv->core_pwr);
-		if (ret < 0) {
-			dev_err(priv->dev,
-				"Failed to enable core power: %d\n", ret);
-			return ret;
-		}
-
-		regulator_set_voltage(priv->io_pwr, 3300000, 3300000);
-		ret = regulator_enable(priv->io_pwr);
-		if (ret < 0) {
-			dev_err(priv->dev, "Failed to enable io pwr: %d\n",
-				ret);
-			return ret;
-		}
-	}
-
-	/* Reset whole chip through gpio pin or memory-mapped registers for
-	 * different type of hardware
-	 */
-	if (priv->mcm) {
-		reset_control_assert(priv->rstc);
-		usleep_range(1000, 1100);
-		reset_control_deassert(priv->rstc);
-	} else {
-		gpiod_set_value_cansleep(priv->reset, 0);
-		usleep_range(1000, 1100);
-		gpiod_set_value_cansleep(priv->reset, 1);
-	}
-
-	/* Waiting for MT7530 got to stable */
-	INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_HWTRAP);
-	ret = readx_poll_timeout(_mt7530_read, &p, val, val != 0,
-				 20, 1000000);
-	if (ret < 0) {
-		dev_err(priv->dev, "reset timeout\n");
-		return ret;
-	}
-
-	id = mt7530_read(priv, MT7530_CREV);
-	id >>= CHIP_NAME_SHIFT;
-	if (id != MT7530_ID) {
-		dev_err(priv->dev, "chip %x can't be supported\n", id);
-		return -ENODEV;
-	}
-
-	/* Reset the switch through internal reset */
-	mt7530_write(priv, MT7530_SYS_CTRL,
-		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
-		     SYS_CTRL_REG_RST);
-
  	mt7530_pll_setup(priv);
  
  	/* Lower Tx driving for TRGMII path */
@@ -3079,6 +3026,9 @@ mt7530_probe(struct mdio_device *mdiodev)
  {
  	struct mt7530_priv *priv;
  	struct device_node *dn;
+	struct mt7530_dummy_poll p;
+	u32 id, val;
+	int ret;
  
  	dn = mdiodev->dev.of_node;
  
@@ -3155,6 +3105,91 @@ mt7530_probe(struct mdio_device *mdiodev)
  	mutex_init(&priv->reg_mutex);
  	dev_set_drvdata(&mdiodev->dev, priv);
  
+	if (priv->id == ID_MT7530) {
+		regulator_set_voltage(priv->core_pwr, 1000000, 1000000);
+		ret = regulator_enable(priv->core_pwr);
+		if (ret < 0) {thought a bit of what exactly
+			dev_err(priv->dev,
+				"Failed to enable core power: %d\n", ret);
+			return ret;
+		}
+
+		regulator_set_voltage(priv->io_pwr, 3300000, 3300000);
+		ret = regulator_enable(priv->io_pwr);
+		if (ret < 0) {
+			dev_err(priv->dev, "Failed to enable io pwr: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	/* Reset whole chip through gpio pin or memory-mapped registers for
+	 * different type of hardware
+	 */
+	if (priv->mcm) {
+		reset_control_assert(priv->rstc);
+		usleep_range(1000, 1100);
+		reset_control_deassert(priv->rstc);
+	} else {
+		gpiod_set_value_cansleep(priv->reset, 0);
+		usleep_range(1000, 1100);
+		gpiod_set_value_cansleep(priv->reset, 1);
+	}
+
+	/* Waiting for MT7530 got to stable */
+	INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_HWTRAP);
+	ret = readx_poll_timeout(_mt7530_read, &p, val, val != 0,
+				 20, 1000000);
+	if (ret < 0) {
+		dev_err(priv->dev, "reset timeout\n");
+		return ret;
+	}
+
+	id = mt7530_read(priv, MT7530_CREV);
+	id >>= CHIP_NAME_SHIFT;
+	if (id != MT7530_ID) {
+		dev_err(priv->dev, "chip %x can't be supported\n", id);
+		return -ENODEV;
+	}
+
+	/* Reset the switch through internal reset */
+	mt7530_write(priv, MT7530_SYS_CTRL,
+		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
+		     SYS_CTRL_REG_RST);
+
+	/* Setup phy muxing */
+	mutex_lock(&priv->reg_mutex);
+
+	val = mt7530_read(priv, MT7530_MHWTRAP);
+
+	val |= MHWTRAP_MANUAL | MHWTRAP_P5_MAC_SEL | MHWTRAP_P5_DIS;
+	val &= ~MHWTRAP_P5_RGMII_MODE & ~MHWTRAP_PHY0_SEL;
+
+	/* MT7530_P5_MODE_GPHY_P4: 2nd GMAC -> P5 -> P4 */
+	val &= ~MHWTRAP_P5_MAC_SEL & ~MHWTRAP_P5_DIS;
+
+	/* Setup the MAC by default for the cpu port */
+	mt7530_write(priv, MT7530_PMCR_P(5), 0x56300);
+
+	val |= MHWTRAP_P5_RGMII_MODE;
+
+	/* P5 RGMII RX Clock Control: delay setting for 1000M */
+	mt7530_write(priv, MT7530_P5RGMIIRXCR, CSR_RGMII_EDGE_ALIGN);
+
+	/* P5 RGMII TX Clock Control: delay x */
+	mt7530_write(priv, MT7530_P5RGMIITXCR,
+		     CSR_RGMII_TXC_CFG(0x10 + 4));
+
+	/* reduce P5 RGMII Tx driving, 8mA */
+	mt7530_write(priv, MT7530_IO_DRV_CR,
+		     P5_IO_CLK_DRV(1) | P5_IO_DATA_DRV(1));
+
+	mt7530_write(priv, MT7530_MHWTRAP, val);
+
+	dev_info(priv->dev, "muxing phy4 to gmac5\n");
+
+	mutex_unlock(&priv->reg_mutex);
+
  	return dsa_register_switch(priv->ds);
  }
  

[    0.650721] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
[    0.660285] mt7530 mdio-bus:1f: muxing phy4 to gmac5
[    0.665284] mt7530 mdio-bus:1f: no ports child node found
[    0.670688] mt7530: probe of mdio-bus:1f failed with error -22
[    0.679118] mtk_soc_eth 1e100000.ethernet: generated random MAC address b6:9c:4d:eb:1f:8e
[    0.688922] mtk_soc_eth 1e100000.ethernet eth0: mediatek frame engine at 0xbe100000, irq 15

---

# ifup eth0
[   30.674595] mtk_soc_eth 1e100000.ethernet eth0: configuring for fixed/rgmii link mode
[   30.683451] mtk_soc_eth 1e100000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
# ping 192.168.2.2
PING 192.168.2.2 (192.168.2.2): 56 data bytes
64 bytes from 192.168.2.2: seq=0 ttl=64 time=0.688 ms
64 bytes from 192.168.2.2: seq=1 ttl=64 time=0.375 ms
64 bytes from 192.168.2.2: seq=2 ttl=64 time=0.357 ms
64 bytes from 192.168.2.2: seq=3 ttl=64 time=0.323 ms

---

# ip a
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
     link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
     inet 127.0.0.1/8 scope host lo
        valid_lft forever preferred_lft forever
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
     link/ether b6:9c:4d:eb:1f:8e brd ff:ff:ff:ff:ff:ff
     inet 192.168.2.1/24 scope global eth0
        valid_lft forever preferred_lft forever

There is a lot to do, such as fixing the method to read from the
devicetree as it relies on the mac node the CPU port is connected to but
when this is finalised, we should be able to use it like this:

mac@1 {
	compatible = "mediatek,eth-mac";
	reg = <1>;
	phy-mode = "rgmii";
	phy-handle = <&ethphy0>;
};

mdio-bus {
	#address-cells = <1>;
	#size-cells = <0>;

	ethphy0: ethernet-phy@0 {
		reg = <0>;
	};

	switch@1f {
		compatible = "mediatek,mt7530";
		reg = <0x1f>;
		reset-gpios = <&pio 33 0>;
		core-supply = <&mt6323_vpa_reg>;
		io-supply = <&mt6323_vemc3v3_reg>;
	};
};

Arınç

Re: Move MT7530 phy muxing from DSA to PHY driver

[Arınç ÜNAL]

On 19.09.2022 01:58, Florian Fainelli wrote:
> 
> 
> On 9/18/2022 4:28 AM, Arınç ÜNAL wrote:
>> On 17.09.2022 18:07, Andrew Lunn wrote:
>>>>> Where in the address range is the mux register? Officially, PHY
>>>>> drivers only have access to PHY registers, via MDIO. If the mux
>>>>> register is in the switch address space, it would be better if the
>>>>> switch did the mux configuration. An alternative might be to represent
>>>>> the mux in DT somewhere, and have a mux driver.
>>>>
>>>> I don't know this part very well but it's in the register for hw trap
>>>> modification which, I think, is in the switch address space.
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n941
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.h?id=1f9a6abecf538cc73635f6082677a2f4dc9c89a4#n500
>>>>
>>>> Like you said, I don't think we can move away from the DSA driver, 
>>>> and would
>>>> rather keep the driver do the mux configuration.
>>>>
>>>> We could change the check for phy muxing to define the phy muxing 
>>>> bindings
>>>> in the DSA node instead. If I understand correctly, the mdio address 
>>>> for
>>>> PHYs is fake, it's for the sole purpose of making the driver check if
>>>> there's request for phy muxing and which phy to mux. I'm saying this 
>>>> because
>>>> the MT7530 switch works fine at address 0 while also using phy0 as a 
>>>> slave
>>>> interface.
>>>>
>>>> A property could be introduced on the DSA node for the MT7530 DSA 
>>>> driver:
>>>>
>>>>      mdio {
>>>>          #address-cells = <1>;
>>>>          #size-cells = <0>;
>>>>
>>>>          switch@0 {
>>>>              compatible = "mediatek,mt7530";
>>>>              reg = <0>;
>>>>
>>>>              reset-gpios = <&pio 33 0>;
>>>>
>>>>              core-supply = <&mt6323_vpa_reg>;
>>>>              io-supply = <&mt6323_vemc3v3_reg>;
>>>>
>>>>              mt7530,mux-phy = <&sw0_p0>;
>>>>
>>>>              ethernet-ports {
>>>>                  #address-cells = <1>;
>>>>                  #size-cells = <0>;
>>>>
>>>>                  sw0_p0: port@0 {
>>>>                      reg = <0>;
>>>>                  };
>>>>              };
>>>>          };
>>>>      };
>>>>
>>>> This would also allow using the phy muxing feature with any ethernet 
>>>> mac.
>>>> Currently, phy muxing check wants the ethernet mac to be gmac1 of a 
>>>> MediaTek
>>>> SoC. However, on a standalone MT7530, the switch can be wired to any 
>>>> SoC's
>>>> ethernet mac.
>>>>
>>>> For the port which is set for PHY muxing, do not bring it as a slave
>>>> interface, just do the phy muxing operation.
>>>>
>>>> Do not fail because there's no CPU port (ethernet property) defined 
>>>> when
>>>> there's only one port defined and it's set for PHY muxing.
>>>>
>>>> I don't know if the ethernet mac needs phy-handle defined in this case.
>>>
>>>  From mediatek,mt7530.yaml:
>>>
>>>    Port 5 modes/configurations:
>>>    1. Port 5 is disabled and isolated: An external phy can interface 
>>> to the 2nd
>>>       GMAC of the SOC.
>>>       In the case of a build-in MT7530 switch, port 5 shares the 
>>> RGMII bus with 2nd
>>>       GMAC and an optional external phy. Mind the GPIO/pinctl 
>>> settings of the SOC!
>>>    2. Port 5 is muxed to PHY of port 0/4: Port 0/4 interfaces with 
>>> 2nd GMAC.
>>>       It is a simple MAC to PHY interface, port 5 needs to be setup 
>>> for xMII mode
>>>       and RGMII delay.
>>>    3. Port 5 is muxed to GMAC5 and can interface to an external phy.
>>>       Port 5 becomes an extra switch port.
>>>       Only works on platform where external phy TX<->RX lines are 
>>> swapped.
>>>       Like in the Ubiquiti ER-X-SFP.
>>>    4. Port 5 is muxed to GMAC5 and interfaces with the 2nd GAMC as 
>>> 2nd CPU port.
>>>       Currently a 2nd CPU port is not supported by DSA code.
>>>
>>> So this mux has a scope bigger than the switch, it also affects one of
>>> the SoCs MACs.
>>>
>>> The phy-handle should have all the information you need, but it is
>>> scattered over multiple locations. It could be in switch port 5, or it
>>> could be in the SoC GMAC node.
>>>
>>> Although the mux is in the switches address range, could you have a
>>> tiny driver using that address range. Have this tiny driver export a
>>> function to set the mux. Both the GMAC and the DSA driver make use of
>>> the function, which should be enough to force the tiny driver to load
>>> first. The GMAC and the DSA driver can then look at there phy-handle,
>>> and determine how the mux should be set. The GMAC should probably do
>>> that before register_netdev. The DSA driver before it registers the
>>> switch with the DSA core.
>>>
>>> Does that solve all your ordering issues?
>>
>> I believe it does.
>>
>>>
>>> By using the phy-handle, you don't need any additional properties, so
>>> backwards compatibility should not be a problem. You can change driver
>>> code as much as you want, but ABI like DT is fixed.
>>
>> Understood, thanks Andrew!
> 
> Yes this seems like a reasonably good idea, I would be a bit concerned 
> about possibly running into issues with fw_devlink=on and whichever 
> driver is managing the PHY device not being an actual PHY device driver 
> provider and thus preventing the PHY device consumers from probing. This 
> is not necessarily an issue right now though because 'phy-handle' is not 
> (yet again) part of of_supplier_bindings.
> 
> Maybe what you can do is just describe that mux register space using a 
> dedicated DT node, and use a syscon phandle for both the switch and/or 
> the MAC and have them use an exported symbol routine that is responsible 
> for configuring the mux in an atomic and consistent way.

I'm currently working on the mt7530 driver and I think I found a way
that takes the least effort, won't break the ABI, and most importantly,
will work.

As we acknowledged, the registers are in the switch address space. This
must also mean that the switch must be properly probed before doing
anything with the registers.

I'm not sure how exactly making a tiny driver would work in this case.

I figured we can just run the phy muxing code before the DSA driver
exits because there are no (CPU) ports defined on the devicetree. Right
after probing is done on mt7530_probe, before dsa_register_switch() is run.

For proof of concept, I've moved some necessary switch probing code from
mt7530_setup() to mt7530_probe(). After the switch is properly reset,
phy4 is muxed, before dsa_register_switch() is run.

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index b0d9ea7b8e6d..0c1d9f83e73b 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2085,7 +2085,6 @@ mt7530_setup(struct dsa_switch *ds)
  	struct device_node *dn = NULL;
  	struct device_node *phy_node;
  	struct device_node *mac_np;
-	struct mt7530_dummy_poll p;
  	phy_interface_t interface;
  	struct dsa_port *cpu_dp;
  	u32 id, val;
@@ -2111,58 +2110,6 @@ mt7530_setup(struct dsa_switch *ds)
  	ds->assisted_learning_on_cpu_port = true;
  	ds->mtu_enforcement_ingress = true;
  
-	if (priv->id == ID_MT7530) {
-		regulator_set_voltage(priv->core_pwr, 1000000, 1000000);
-		ret = regulator_enable(priv->core_pwr);
-		if (ret < 0) {
-			dev_err(priv->dev,
-				"Failed to enable core power: %d\n", ret);
-			return ret;
-		}
-
-		regulator_set_voltage(priv->io_pwr, 3300000, 3300000);
-		ret = regulator_enable(priv->io_pwr);
-		if (ret < 0) {
-			dev_err(priv->dev, "Failed to enable io pwr: %d\n",
-				ret);
-			return ret;
-		}
-	}
-
-	/* Reset whole chip through gpio pin or memory-mapped registers for
-	 * different type of hardware
-	 */
-	if (priv->mcm) {
-		reset_control_assert(priv->rstc);
-		usleep_range(1000, 1100);
-		reset_control_deassert(priv->rstc);
-	} else {
-		gpiod_set_value_cansleep(priv->reset, 0);
-		usleep_range(1000, 1100);
-		gpiod_set_value_cansleep(priv->reset, 1);
-	}
-
-	/* Waiting for MT7530 got to stable */
-	INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_HWTRAP);
-	ret = readx_poll_timeout(_mt7530_read, &p, val, val != 0,
-				 20, 1000000);
-	if (ret < 0) {
-		dev_err(priv->dev, "reset timeout\n");
-		return ret;
-	}
-
-	id = mt7530_read(priv, MT7530_CREV);
-	id >>= CHIP_NAME_SHIFT;
-	if (id != MT7530_ID) {
-		dev_err(priv->dev, "chip %x can't be supported\n", id);
-		return -ENODEV;
-	}
-
-	/* Reset the switch through internal reset */
-	mt7530_write(priv, MT7530_SYS_CTRL,
-		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
-		     SYS_CTRL_REG_RST);
-
  	mt7530_pll_setup(priv);
  
  	/* Lower Tx driving for TRGMII path */
@@ -3079,6 +3026,9 @@ mt7530_probe(struct mdio_device *mdiodev)
  {
  	struct mt7530_priv *priv;
  	struct device_node *dn;
+	struct mt7530_dummy_poll p;
+	u32 id, val;
+	int ret;
  
  	dn = mdiodev->dev.of_node;
  
@@ -3155,6 +3105,91 @@ mt7530_probe(struct mdio_device *mdiodev)
  	mutex_init(&priv->reg_mutex);
  	dev_set_drvdata(&mdiodev->dev, priv);
  
+	if (priv->id == ID_MT7530) {
+		regulator_set_voltage(priv->core_pwr, 1000000, 1000000);
+		ret = regulator_enable(priv->core_pwr);
+		if (ret < 0) {thought a bit of what exactly
+			dev_err(priv->dev,
+				"Failed to enable core power: %d\n", ret);
+			return ret;
+		}
+
+		regulator_set_voltage(priv->io_pwr, 3300000, 3300000);
+		ret = regulator_enable(priv->io_pwr);
+		if (ret < 0) {
+			dev_err(priv->dev, "Failed to enable io pwr: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	/* Reset whole chip through gpio pin or memory-mapped registers for
+	 * different type of hardware
+	 */
+	if (priv->mcm) {
+		reset_control_assert(priv->rstc);
+		usleep_range(1000, 1100);
+		reset_control_deassert(priv->rstc);
+	} else {
+		gpiod_set_value_cansleep(priv->reset, 0);
+		usleep_range(1000, 1100);
+		gpiod_set_value_cansleep(priv->reset, 1);
+	}
+
+	/* Waiting for MT7530 got to stable */
+	INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_HWTRAP);
+	ret = readx_poll_timeout(_mt7530_read, &p, val, val != 0,
+				 20, 1000000);
+	if (ret < 0) {
+		dev_err(priv->dev, "reset timeout\n");
+		return ret;
+	}
+
+	id = mt7530_read(priv, MT7530_CREV);
+	id >>= CHIP_NAME_SHIFT;
+	if (id != MT7530_ID) {
+		dev_err(priv->dev, "chip %x can't be supported\n", id);
+		return -ENODEV;
+	}
+
+	/* Reset the switch through internal reset */
+	mt7530_write(priv, MT7530_SYS_CTRL,
+		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
+		     SYS_CTRL_REG_RST);
+
+	/* Setup phy muxing */
+	mutex_lock(&priv->reg_mutex);
+
+	val = mt7530_read(priv, MT7530_MHWTRAP);
+
+	val |= MHWTRAP_MANUAL | MHWTRAP_P5_MAC_SEL | MHWTRAP_P5_DIS;
+	val &= ~MHWTRAP_P5_RGMII_MODE & ~MHWTRAP_PHY0_SEL;
+
+	/* MT7530_P5_MODE_GPHY_P4: 2nd GMAC -> P5 -> P4 */
+	val &= ~MHWTRAP_P5_MAC_SEL & ~MHWTRAP_P5_DIS;
+
+	/* Setup the MAC by default for the cpu port */
+	mt7530_write(priv, MT7530_PMCR_P(5), 0x56300);
+
+	val |= MHWTRAP_P5_RGMII_MODE;
+
+	/* P5 RGMII RX Clock Control: delay setting for 1000M */
+	mt7530_write(priv, MT7530_P5RGMIIRXCR, CSR_RGMII_EDGE_ALIGN);
+
+	/* P5 RGMII TX Clock Control: delay x */
+	mt7530_write(priv, MT7530_P5RGMIITXCR,
+		     CSR_RGMII_TXC_CFG(0x10 + 4));
+
+	/* reduce P5 RGMII Tx driving, 8mA */
+	mt7530_write(priv, MT7530_IO_DRV_CR,
+		     P5_IO_CLK_DRV(1) | P5_IO_DATA_DRV(1));
+
+	mt7530_write(priv, MT7530_MHWTRAP, val);
+
+	dev_info(priv->dev, "muxing phy4 to gmac5\n");
+
+	mutex_unlock(&priv->reg_mutex);
+
  	return dsa_register_switch(priv->ds);
  }
  

[    0.650721] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
[    0.660285] mt7530 mdio-bus:1f: muxing phy4 to gmac5
[    0.665284] mt7530 mdio-bus:1f: no ports child node found
[    0.670688] mt7530: probe of mdio-bus:1f failed with error -22
[    0.679118] mtk_soc_eth 1e100000.ethernet: generated random MAC address b6:9c:4d:eb:1f:8e
[    0.688922] mtk_soc_eth 1e100000.ethernet eth0: mediatek frame engine at 0xbe100000, irq 15

---

# ifup eth0
[   30.674595] mtk_soc_eth 1e100000.ethernet eth0: configuring for fixed/rgmii link mode
[   30.683451] mtk_soc_eth 1e100000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
# ping 192.168.2.2
PING 192.168.2.2 (192.168.2.2): 56 data bytes
64 bytes from 192.168.2.2: seq=0 ttl=64 time=0.688 ms
64 bytes from 192.168.2.2: seq=1 ttl=64 time=0.375 ms
64 bytes from 192.168.2.2: seq=2 ttl=64 time=0.357 ms
64 bytes from 192.168.2.2: seq=3 ttl=64 time=0.323 ms

---

# ip a
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
     link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
     inet 127.0.0.1/8 scope host lo
        valid_lft forever preferred_lft forever
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
     link/ether b6:9c:4d:eb:1f:8e brd ff:ff:ff:ff:ff:ff
     inet 192.168.2.1/24 scope global eth0
        valid_lft forever preferred_lft forever

There is a lot to do, such as fixing the method to read from the
devicetree as it relies on the mac node the CPU port is connected to but
when this is finalised, we should be able to use it like this:

mac@1 {
	compatible = "mediatek,eth-mac";
	reg = <1>;
	phy-mode = "rgmii";
	phy-handle = <&ethphy0>;
};

mdio-bus {
	#address-cells = <1>;
	#size-cells = <0>;

	ethphy0: ethernet-phy@0 {
		reg = <0>;
	};

	switch@1f {
		compatible = "mediatek,mt7530";
		reg = <0x1f>;
		reset-gpios = <&pio 33 0>;
		core-supply = <&mt6323_vpa_reg>;
		io-supply = <&mt6323_vemc3v3_reg>;
	};
};

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Move MT7530 phy muxing from DSA to PHY driver

[Vladimir Oltean]

On Sun, Mar 26, 2023 at 07:52:12PM +0300, Arınç ÜNAL wrote:
> I'm currently working on the mt7530 driver and I think I found a way
> that takes the least effort, won't break the ABI, and most importantly,
> will work.

This sounds promising....

> As we acknowledged, the registers are in the switch address space. This
> must also mean that the switch must be properly probed before doing
> anything with the registers.
> 
> I'm not sure how exactly making a tiny driver would work in this case.

I'm not sure how it would work, either. It sounds like the driver for
the mdio-bus address @1f should have been a parent driver (MFD or not)
with 2 (platform device) children, one for the switch and another for
the HWTRAP registers and whatever else might be needed for the PHY
multiplexing. The parent (mdio_device) driver deals with the chip-wide
reset, resources, and manages the registers, giving them regmaps.
The driver with the mux probably just exports a symbol representing a
function that gets called by the "mediatek,eth-mac" driver and/or the
switch driver.

BTW, I have something vaguely similar to this in a stalled WIP branch
for sja1105, but things like this get really complicated really quickly
if the DSA driver's DT bindings weren't designed from day one to not
cover the entire switch's register map.

> I figured we can just run the phy muxing code before the DSA driver
> exits because there are no (CPU) ports defined on the devicetree. Right
> after probing is done on mt7530_probe, before dsa_register_switch() is run.

Aren't there timing issues, though? When is the earliest moment that the
"mediatek,eth-mac" driver needs the HWTRAP muxing to be changed?
The operation of changing that from the "mediatek,mt7530" driver is
completely asynchronous to the probing of "mediatek,eth-mac".
What's the worst that will happen with incorrect (not yet updated) GMII
signal muxing? "Just" some lost packets?

> 
> For proof of concept, I've moved some necessary switch probing code from
> mt7530_setup() to mt7530_probe(). After the switch is properly reset,
> phy4 is muxed, before dsa_register_switch() is run.

This is fragile because someone eager for some optimizations could move
the code back the way it was, and say: "the switch initialization costs
X ms and is done X times, because dsa_register_switch() -> ... ->
of_find_net_device_by_node() returns -EPROBE_DEFER the first X-1 times.
If we move the switch initialization to ds->ops->setup(), it will run
only once, after the handle to the DSA master has been obtained, and
this gives us a boost in kernel startup time."

It's even more fragile because currently (neither before nor after your change),
mt7530_remove() does not do the mirror opposite of mt7530_probe(), and somebody
eager from the future will notice this, and add an error handling path for
dsa_register_switch(), which calls the opposite of regulator_enable(),
regulator_disable(), saying "hey, there's no reason to let the regulators
on if the switch failed to probe, it consumes power for nothing!".

It's an open question whether that regulator is needed for anything after
the HWMUX registers has been changed, or if it can indeed be turned off.
Not knowing this, it's hard to say if the change is okay or not.
It seems that there's a high probability it will work for a while,
by coincidence.

> 
> [    0.650721] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
> [    0.660285] mt7530 mdio-bus:1f: muxing phy4 to gmac5
> [    0.665284] mt7530 mdio-bus:1f: no ports child node found
> [    0.670688] mt7530: probe of mdio-bus:1f failed with error -22
> [    0.679118] mtk_soc_eth 1e100000.ethernet: generated random MAC address b6:9c:4d:eb:1f:8e
> [    0.688922] mtk_soc_eth 1e100000.ethernet eth0: mediatek frame engine at 0xbe100000, irq 15
> 
> ---
> 
> # ifup eth0
> [   30.674595] mtk_soc_eth 1e100000.ethernet eth0: configuring for fixed/rgmii link mode
> [   30.683451] mtk_soc_eth 1e100000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
> # ping 192.168.2.2
> PING 192.168.2.2 (192.168.2.2): 56 data bytes
> 64 bytes from 192.168.2.2: seq=0 ttl=64 time=0.688 ms
> 64 bytes from 192.168.2.2: seq=1 ttl=64 time=0.375 ms
> 64 bytes from 192.168.2.2: seq=2 ttl=64 time=0.357 ms
> 64 bytes from 192.168.2.2: seq=3 ttl=64 time=0.323 ms
> 
> ---
> 
> # ip a
> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
>     link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>     inet 127.0.0.1/8 scope host lo
>        valid_lft forever preferred_lft forever
> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
>     link/ether b6:9c:4d:eb:1f:8e brd ff:ff:ff:ff:ff:ff
>     inet 192.168.2.1/24 scope global eth0
>        valid_lft forever preferred_lft forever
> 
> There is a lot to do, such as fixing the method to read from the
> devicetree as it relies on the mac node the CPU port is connected to but
> when this is finalised, we should be able to use it like this:
> 
> mac@1 {
> 	compatible = "mediatek,eth-mac";
> 	reg = <1>;
> 	phy-mode = "rgmii";
> 	phy-handle = <&ethphy0>;
> };
> 
> mdio-bus {
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 
> 	ethphy0: ethernet-phy@0 {
> 		reg = <0>;
> 	};
> 
> 	switch@1f {
> 		compatible = "mediatek,mt7530";
> 		reg = <0x1f>;
> 		reset-gpios = <&pio 33 0>;
> 		core-supply = <&mt6323_vpa_reg>;
> 		io-supply = <&mt6323_vemc3v3_reg>;
> 	};
> };

And this is fragile because the "mediatek,eth-mac" driver only works
because of the side effects of a driver that began to probe, and failed.
Someone, seeing that "mediatek,mt7530" fails to probe, and knowing that
the switch ports are not needed/used on that board, could put a
status = "disabled"; property under the switch@1f node.

Re: Move MT7530 phy muxing from DSA to PHY driver

[Vladimir Oltean]

On Sun, Mar 26, 2023 at 07:52:12PM +0300, Arınç ÜNAL wrote:
> I'm currently working on the mt7530 driver and I think I found a way
> that takes the least effort, won't break the ABI, and most importantly,
> will work.

This sounds promising....

> As we acknowledged, the registers are in the switch address space. This
> must also mean that the switch must be properly probed before doing
> anything with the registers.
> 
> I'm not sure how exactly making a tiny driver would work in this case.

I'm not sure how it would work, either. It sounds like the driver for
the mdio-bus address @1f should have been a parent driver (MFD or not)
with 2 (platform device) children, one for the switch and another for
the HWTRAP registers and whatever else might be needed for the PHY
multiplexing. The parent (mdio_device) driver deals with the chip-wide
reset, resources, and manages the registers, giving them regmaps.
The driver with the mux probably just exports a symbol representing a
function that gets called by the "mediatek,eth-mac" driver and/or the
switch driver.

BTW, I have something vaguely similar to this in a stalled WIP branch
for sja1105, but things like this get really complicated really quickly
if the DSA driver's DT bindings weren't designed from day one to not
cover the entire switch's register map.

> I figured we can just run the phy muxing code before the DSA driver
> exits because there are no (CPU) ports defined on the devicetree. Right
> after probing is done on mt7530_probe, before dsa_register_switch() is run.

Aren't there timing issues, though? When is the earliest moment that the
"mediatek,eth-mac" driver needs the HWTRAP muxing to be changed?
The operation of changing that from the "mediatek,mt7530" driver is
completely asynchronous to the probing of "mediatek,eth-mac".
What's the worst that will happen with incorrect (not yet updated) GMII
signal muxing? "Just" some lost packets?

> 
> For proof of concept, I've moved some necessary switch probing code from
> mt7530_setup() to mt7530_probe(). After the switch is properly reset,
> phy4 is muxed, before dsa_register_switch() is run.

This is fragile because someone eager for some optimizations could move
the code back the way it was, and say: "the switch initialization costs
X ms and is done X times, because dsa_register_switch() -> ... ->
of_find_net_device_by_node() returns -EPROBE_DEFER the first X-1 times.
If we move the switch initialization to ds->ops->setup(), it will run
only once, after the handle to the DSA master has been obtained, and
this gives us a boost in kernel startup time."

It's even more fragile because currently (neither before nor after your change),
mt7530_remove() does not do the mirror opposite of mt7530_probe(), and somebody
eager from the future will notice this, and add an error handling path for
dsa_register_switch(), which calls the opposite of regulator_enable(),
regulator_disable(), saying "hey, there's no reason to let the regulators
on if the switch failed to probe, it consumes power for nothing!".

It's an open question whether that regulator is needed for anything after
the HWMUX registers has been changed, or if it can indeed be turned off.
Not knowing this, it's hard to say if the change is okay or not.
It seems that there's a high probability it will work for a while,
by coincidence.

> 
> [    0.650721] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
> [    0.660285] mt7530 mdio-bus:1f: muxing phy4 to gmac5
> [    0.665284] mt7530 mdio-bus:1f: no ports child node found
> [    0.670688] mt7530: probe of mdio-bus:1f failed with error -22
> [    0.679118] mtk_soc_eth 1e100000.ethernet: generated random MAC address b6:9c:4d:eb:1f:8e
> [    0.688922] mtk_soc_eth 1e100000.ethernet eth0: mediatek frame engine at 0xbe100000, irq 15
> 
> ---
> 
> # ifup eth0
> [   30.674595] mtk_soc_eth 1e100000.ethernet eth0: configuring for fixed/rgmii link mode
> [   30.683451] mtk_soc_eth 1e100000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
> # ping 192.168.2.2
> PING 192.168.2.2 (192.168.2.2): 56 data bytes
> 64 bytes from 192.168.2.2: seq=0 ttl=64 time=0.688 ms
> 64 bytes from 192.168.2.2: seq=1 ttl=64 time=0.375 ms
> 64 bytes from 192.168.2.2: seq=2 ttl=64 time=0.357 ms
> 64 bytes from 192.168.2.2: seq=3 ttl=64 time=0.323 ms
> 
> ---
> 
> # ip a
> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
>     link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>     inet 127.0.0.1/8 scope host lo
>        valid_lft forever preferred_lft forever
> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
>     link/ether b6:9c:4d:eb:1f:8e brd ff:ff:ff:ff:ff:ff
>     inet 192.168.2.1/24 scope global eth0
>        valid_lft forever preferred_lft forever
> 
> There is a lot to do, such as fixing the method to read from the
> devicetree as it relies on the mac node the CPU port is connected to but
> when this is finalised, we should be able to use it like this:
> 
> mac@1 {
> 	compatible = "mediatek,eth-mac";
> 	reg = <1>;
> 	phy-mode = "rgmii";
> 	phy-handle = <&ethphy0>;
> };
> 
> mdio-bus {
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 
> 	ethphy0: ethernet-phy@0 {
> 		reg = <0>;
> 	};
> 
> 	switch@1f {
> 		compatible = "mediatek,mt7530";
> 		reg = <0x1f>;
> 		reset-gpios = <&pio 33 0>;
> 		core-supply = <&mt6323_vpa_reg>;
> 		io-supply = <&mt6323_vemc3v3_reg>;
> 	};
> };

And this is fragile because the "mediatek,eth-mac" driver only works
because of the side effects of a driver that began to probe, and failed.
Someone, seeing that "mediatek,mt7530" fails to probe, and knowing that
the switch ports are not needed/used on that board, could put a
status = "disabled"; property under the switch@1f node.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Move MT7530 phy muxing from DSA to PHY driver

[Arınç ÜNAL]

On 27.03.2023 21:38, Vladimir Oltean wrote:
> On Sun, Mar 26, 2023 at 07:52:12PM +0300, Arınç ÜNAL wrote:
>> I'm currently working on the mt7530 driver and I think I found a way
>> that takes the least effort, won't break the ABI, and most importantly,
>> will work.
> 
> This sounds promising....
> 
>> As we acknowledged, the registers are in the switch address space. This
>> must also mean that the switch must be properly probed before doing
>> anything with the registers.
>>
>> I'm not sure how exactly making a tiny driver would work in this case.
> 
> I'm not sure how it would work, either. It sounds like the driver for
> the mdio-bus address @1f should have been a parent driver (MFD or not)
> with 2 (platform device) children, one for the switch and another for
> the HWTRAP registers and whatever else might be needed for the PHY
> multiplexing. The parent (mdio_device) driver deals with the chip-wide
> reset, resources, and manages the registers, giving them regmaps.
> The driver with the mux probably just exports a symbol representing a
> function that gets called by the "mediatek,eth-mac" driver and/or the
> switch driver.
> 
> BTW, I have something vaguely similar to this in a stalled WIP branch
> for sja1105, but things like this get really complicated really quickly
> if the DSA driver's DT bindings weren't designed from day one to not
> cover the entire switch's register map.
> 
>> I figured we can just run the phy muxing code before the DSA driver
>> exits because there are no (CPU) ports defined on the devicetree. Right
>> after probing is done on mt7530_probe, before dsa_register_switch() is run.
> 
> Aren't there timing issues, though? When is the earliest moment that the
> "mediatek,eth-mac" driver needs the HWTRAP muxing to be changed?
> The operation of changing that from the "mediatek,mt7530" driver is
> completely asynchronous to the probing of "mediatek,eth-mac".
> What's the worst that will happen with incorrect (not yet updated) GMII
> signal muxing? "Just" some lost packets?

We're not doing any changes to the MediaTek SoC's MAC if that's what 
you're asking. The phy muxing on the mt7530 DSA driver merely muxes PHY4 
of the switch to GMAC5 of the switch. Whatever MAC connected to GMAC5 
can access the muxed PHY.

It's just the current ABI that requires the MAC to be mediatek,eth-mac. 
PHY muxing can still be perfectly done with a simple property like 
mediatek,mt7530-muxphy = <0>;.

properties:
   mediatek,mt7530-muxphy:
     description:
       Set the PHY to mux to GMAC5. Only PHY0 or PHY4 can be muxed.
     enum:
       - 0
       - 4
     maxItems: 1

Whether or not PHY muxing will work with non-MediaTek MACs is still 
unknown as there is no known hardware that combines a standalone MT7530 
with a non-MediaTek SoC. Though in theory, it should work.

> 
>>
>> For proof of concept, I've moved some necessary switch probing code from
>> mt7530_setup() to mt7530_probe(). After the switch is properly reset,
>> phy4 is muxed, before dsa_register_switch() is run.
> 
> This is fragile because someone eager for some optimizations could move
> the code back the way it was, and say: "the switch initialization costs
> X ms and is done X times, because dsa_register_switch() -> ... ->
> of_find_net_device_by_node() returns -EPROBE_DEFER the first X-1 times.
> If we move the switch initialization to ds->ops->setup(), it will run
> only once, after the handle to the DSA master has been obtained, and
> this gives us a boost in kernel startup time."
> 
> It's even more fragile because currently (neither before nor after your change),
> mt7530_remove() does not do the mirror opposite of mt7530_probe(), and somebody
> eager from the future will notice this, and add an error handling path for
> dsa_register_switch(), which calls the opposite of regulator_enable(),
> regulator_disable(), saying "hey, there's no reason to let the regulators
> on if the switch failed to probe, it consumes power for nothing!".
> 
> It's an open question whether that regulator is needed for anything after
> the HWMUX registers has been changed, or if it can indeed be turned off.
> Not knowing this, it's hard to say if the change is okay or not.
> It seems that there's a high probability it will work for a while,
> by coincidence.

Let's just say that since it's needed for probing, it's best it stays 
on. This should prevent mt7530_remove() from going further if phy muxing 
is enabled.

> @@ -3167,6 +3173,9 @@ mt7530_remove(struct mdio_device *mdiodev)
>  	if (!priv)
>  		return;
>  
> +	if (priv->p5_intf_sel == (P5_INTF_SEL_PHY_P0 || P5_INTF_SEL_PHY_P4))
> +		return;
> +
>  	ret = regulator_disable(priv->core_pwr);
>  	if (ret < 0)
>  		dev_err(priv->dev,

Is mt7530_remove(), or to be more inclusive, the .remove operation of 
mdio_driver, run only when the switch fails to probe? If not, with the 
diff above, the switch won't be disabled in both of the cases (phy 
muxing only, phy muxing + normal DSA ports) if mt7530_remove() is run 
for some other reason.

> 
>>
>> [    0.650721] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
>> [    0.660285] mt7530 mdio-bus:1f: muxing phy4 to gmac5
>> [    0.665284] mt7530 mdio-bus:1f: no ports child node found
>> [    0.670688] mt7530: probe of mdio-bus:1f failed with error -22
>> [    0.679118] mtk_soc_eth 1e100000.ethernet: generated random MAC address b6:9c:4d:eb:1f:8e
>> [    0.688922] mtk_soc_eth 1e100000.ethernet eth0: mediatek frame engine at 0xbe100000, irq 15
>>
>> ---
>>
>> # ifup eth0
>> [   30.674595] mtk_soc_eth 1e100000.ethernet eth0: configuring for fixed/rgmii link mode
>> [   30.683451] mtk_soc_eth 1e100000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
>> # ping 192.168.2.2
>> PING 192.168.2.2 (192.168.2.2): 56 data bytes
>> 64 bytes from 192.168.2.2: seq=0 ttl=64 time=0.688 ms
>> 64 bytes from 192.168.2.2: seq=1 ttl=64 time=0.375 ms
>> 64 bytes from 192.168.2.2: seq=2 ttl=64 time=0.357 ms
>> 64 bytes from 192.168.2.2: seq=3 ttl=64 time=0.323 ms
>>
>> ---
>>
>> # ip a
>> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
>>      link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>>      inet 127.0.0.1/8 scope host lo
>>         valid_lft forever preferred_lft forever
>> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
>>      link/ether b6:9c:4d:eb:1f:8e brd ff:ff:ff:ff:ff:ff
>>      inet 192.168.2.1/24 scope global eth0
>>         valid_lft forever preferred_lft forever
>>
>> There is a lot to do, such as fixing the method to read from the
>> devicetree as it relies on the mac node the CPU port is connected to but
>> when this is finalised, we should be able to use it like this:
>>
>> mac@1 {
>> 	compatible = "mediatek,eth-mac";
>> 	reg = <1>;
>> 	phy-mode = "rgmii";
>> 	phy-handle = <&ethphy0>;
>> };
>>
>> mdio-bus {
>> 	#address-cells = <1>;
>> 	#size-cells = <0>;
>>
>> 	ethphy0: ethernet-phy@0 {
>> 		reg = <0>;
>> 	};
>>
>> 	switch@1f {
>> 		compatible = "mediatek,mt7530";
>> 		reg = <0x1f>;
>> 		reset-gpios = <&pio 33 0>;
>> 		core-supply = <&mt6323_vpa_reg>;
>> 		io-supply = <&mt6323_vemc3v3_reg>;
>> 	};
>> };
> 
> And this is fragile because the "mediatek,eth-mac" driver only works
> because of the side effects of a driver that began to probe, and failed.
> Someone, seeing that "mediatek,mt7530" fails to probe, and knowing that
> the switch ports are not needed/used on that board, could put a
> status = "disabled"; property under the switch@1f node.

This should help.

> 	if (priv->p5_intf_sel == (P5_INTF_SEL_PHY_P0 || P5_INTF_SEL_PHY_P4)) {
> 		dev_info(priv->dev,
> 			 "PHY muxing is enabled, gracefully exiting\n");
> 		return;
> 	}

Arınç

Re: Move MT7530 phy muxing from DSA to PHY driver

[Arınç ÜNAL]

On 27.03.2023 21:38, Vladimir Oltean wrote:
> On Sun, Mar 26, 2023 at 07:52:12PM +0300, Arınç ÜNAL wrote:
>> I'm currently working on the mt7530 driver and I think I found a way
>> that takes the least effort, won't break the ABI, and most importantly,
>> will work.
> 
> This sounds promising....
> 
>> As we acknowledged, the registers are in the switch address space. This
>> must also mean that the switch must be properly probed before doing
>> anything with the registers.
>>
>> I'm not sure how exactly making a tiny driver would work in this case.
> 
> I'm not sure how it would work, either. It sounds like the driver for
> the mdio-bus address @1f should have been a parent driver (MFD or not)
> with 2 (platform device) children, one for the switch and another for
> the HWTRAP registers and whatever else might be needed for the PHY
> multiplexing. The parent (mdio_device) driver deals with the chip-wide
> reset, resources, and manages the registers, giving them regmaps.
> The driver with the mux probably just exports a symbol representing a
> function that gets called by the "mediatek,eth-mac" driver and/or the
> switch driver.
> 
> BTW, I have something vaguely similar to this in a stalled WIP branch
> for sja1105, but things like this get really complicated really quickly
> if the DSA driver's DT bindings weren't designed from day one to not
> cover the entire switch's register map.
> 
>> I figured we can just run the phy muxing code before the DSA driver
>> exits because there are no (CPU) ports defined on the devicetree. Right
>> after probing is done on mt7530_probe, before dsa_register_switch() is run.
> 
> Aren't there timing issues, though? When is the earliest moment that the
> "mediatek,eth-mac" driver needs the HWTRAP muxing to be changed?
> The operation of changing that from the "mediatek,mt7530" driver is
> completely asynchronous to the probing of "mediatek,eth-mac".
> What's the worst that will happen with incorrect (not yet updated) GMII
> signal muxing? "Just" some lost packets?

We're not doing any changes to the MediaTek SoC's MAC if that's what 
you're asking. The phy muxing on the mt7530 DSA driver merely muxes PHY4 
of the switch to GMAC5 of the switch. Whatever MAC connected to GMAC5 
can access the muxed PHY.

It's just the current ABI that requires the MAC to be mediatek,eth-mac. 
PHY muxing can still be perfectly done with a simple property like 
mediatek,mt7530-muxphy = <0>;.

properties:
   mediatek,mt7530-muxphy:
     description:
       Set the PHY to mux to GMAC5. Only PHY0 or PHY4 can be muxed.
     enum:
       - 0
       - 4
     maxItems: 1

Whether or not PHY muxing will work with non-MediaTek MACs is still 
unknown as there is no known hardware that combines a standalone MT7530 
with a non-MediaTek SoC. Though in theory, it should work.

> 
>>
>> For proof of concept, I've moved some necessary switch probing code from
>> mt7530_setup() to mt7530_probe(). After the switch is properly reset,
>> phy4 is muxed, before dsa_register_switch() is run.
> 
> This is fragile because someone eager for some optimizations could move
> the code back the way it was, and say: "the switch initialization costs
> X ms and is done X times, because dsa_register_switch() -> ... ->
> of_find_net_device_by_node() returns -EPROBE_DEFER the first X-1 times.
> If we move the switch initialization to ds->ops->setup(), it will run
> only once, after the handle to the DSA master has been obtained, and
> this gives us a boost in kernel startup time."
> 
> It's even more fragile because currently (neither before nor after your change),
> mt7530_remove() does not do the mirror opposite of mt7530_probe(), and somebody
> eager from the future will notice this, and add an error handling path for
> dsa_register_switch(), which calls the opposite of regulator_enable(),
> regulator_disable(), saying "hey, there's no reason to let the regulators
> on if the switch failed to probe, it consumes power for nothing!".
> 
> It's an open question whether that regulator is needed for anything after
> the HWMUX registers has been changed, or if it can indeed be turned off.
> Not knowing this, it's hard to say if the change is okay or not.
> It seems that there's a high probability it will work for a while,
> by coincidence.

Let's just say that since it's needed for probing, it's best it stays 
on. This should prevent mt7530_remove() from going further if phy muxing 
is enabled.

> @@ -3167,6 +3173,9 @@ mt7530_remove(struct mdio_device *mdiodev)
>  	if (!priv)
>  		return;
>  
> +	if (priv->p5_intf_sel == (P5_INTF_SEL_PHY_P0 || P5_INTF_SEL_PHY_P4))
> +		return;
> +
>  	ret = regulator_disable(priv->core_pwr);
>  	if (ret < 0)
>  		dev_err(priv->dev,

Is mt7530_remove(), or to be more inclusive, the .remove operation of 
mdio_driver, run only when the switch fails to probe? If not, with the 
diff above, the switch won't be disabled in both of the cases (phy 
muxing only, phy muxing + normal DSA ports) if mt7530_remove() is run 
for some other reason.

> 
>>
>> [    0.650721] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
>> [    0.660285] mt7530 mdio-bus:1f: muxing phy4 to gmac5
>> [    0.665284] mt7530 mdio-bus:1f: no ports child node found
>> [    0.670688] mt7530: probe of mdio-bus:1f failed with error -22
>> [    0.679118] mtk_soc_eth 1e100000.ethernet: generated random MAC address b6:9c:4d:eb:1f:8e
>> [    0.688922] mtk_soc_eth 1e100000.ethernet eth0: mediatek frame engine at 0xbe100000, irq 15
>>
>> ---
>>
>> # ifup eth0
>> [   30.674595] mtk_soc_eth 1e100000.ethernet eth0: configuring for fixed/rgmii link mode
>> [   30.683451] mtk_soc_eth 1e100000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
>> # ping 192.168.2.2
>> PING 192.168.2.2 (192.168.2.2): 56 data bytes
>> 64 bytes from 192.168.2.2: seq=0 ttl=64 time=0.688 ms
>> 64 bytes from 192.168.2.2: seq=1 ttl=64 time=0.375 ms
>> 64 bytes from 192.168.2.2: seq=2 ttl=64 time=0.357 ms
>> 64 bytes from 192.168.2.2: seq=3 ttl=64 time=0.323 ms
>>
>> ---
>>
>> # ip a
>> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
>>      link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>>      inet 127.0.0.1/8 scope host lo
>>         valid_lft forever preferred_lft forever
>> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
>>      link/ether b6:9c:4d:eb:1f:8e brd ff:ff:ff:ff:ff:ff
>>      inet 192.168.2.1/24 scope global eth0
>>         valid_lft forever preferred_lft forever
>>
>> There is a lot to do, such as fixing the method to read from the
>> devicetree as it relies on the mac node the CPU port is connected to but
>> when this is finalised, we should be able to use it like this:
>>
>> mac@1 {
>> 	compatible = "mediatek,eth-mac";
>> 	reg = <1>;
>> 	phy-mode = "rgmii";
>> 	phy-handle = <&ethphy0>;
>> };
>>
>> mdio-bus {
>> 	#address-cells = <1>;
>> 	#size-cells = <0>;
>>
>> 	ethphy0: ethernet-phy@0 {
>> 		reg = <0>;
>> 	};
>>
>> 	switch@1f {
>> 		compatible = "mediatek,mt7530";
>> 		reg = <0x1f>;
>> 		reset-gpios = <&pio 33 0>;
>> 		core-supply = <&mt6323_vpa_reg>;
>> 		io-supply = <&mt6323_vemc3v3_reg>;
>> 	};
>> };
> 
> And this is fragile because the "mediatek,eth-mac" driver only works
> because of the side effects of a driver that began to probe, and failed.
> Someone, seeing that "mediatek,mt7530" fails to probe, and knowing that
> the switch ports are not needed/used on that board, could put a
> status = "disabled"; property under the switch@1f node.

This should help.

> 	if (priv->p5_intf_sel == (P5_INTF_SEL_PHY_P0 || P5_INTF_SEL_PHY_P4)) {
> 		dev_info(priv->dev,
> 			 "PHY muxing is enabled, gracefully exiting\n");
> 		return;
> 	}

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel