sja1105q: proper way to solve PHY clk dependecy

sja1105q: proper way to solve PHY clk dependecy

[Oleksij Rempel]

Hi Vladimir,

I have SJA1105Q based switch with 3 T1L PHYs connected over RMII
interface. The clk input "XI" of PHYs is connected to "MII0_TX_CLK/REF_CLK/TXC"
pins of the switch. Since this PHYs can't be configured reliably over MDIO
interface without running clk on XI input, i have a dependency dilemma:
i can't probe MDIO bus, without enabling DSA ports.

If I see it correctly, following steps should be done:
- register MDIO bus without scanning for PHYs
- define SJA1105Q switch as clock provider and PHYs as clk consumer
- detect and attach PHYs on port enable if clks can't be controlled
  without enabling the port.
- HW reset line of the PHYs should be asserted if we disable port and
  deasserted with proper reinit after port is enabled.

Other way would be to init and enable switch ports and PHYs by a bootloader and
keep it enabled.

What is the proper way to go?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: sja1105q: proper way to solve PHY clk dependecy

[Vladimir Oltean]

Hello Oleksij,

On Wed, Mar 23, 2022 at 07:03:31AM +0100, Oleksij Rempel wrote:
> Hi Vladimir,
> 
> I have SJA1105Q based switch with 3 T1L PHYs connected over RMII
> interface. The clk input "XI" of PHYs is connected to "MII0_TX_CLK/REF_CLK/TXC"
> pins of the switch. Since this PHYs can't be configured reliably over MDIO
> interface without running clk on XI input, i have a dependency dilemma:
> i can't probe MDIO bus, without enabling DSA ports.
> 
> If I see it correctly, following steps should be done:
> - register MDIO bus without scanning for PHYs
> - define SJA1105Q switch as clock provider and PHYs as clk consumer
> - detect and attach PHYs on port enable if clks can't be controlled
>   without enabling the port.
> - HW reset line of the PHYs should be asserted if we disable port and
>   deasserted with proper reinit after port is enabled.
> 
> Other way would be to init and enable switch ports and PHYs by a bootloader and
> keep it enabled.
> 
> What is the proper way to go?
> 
> Regards,
> Oleksij
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

The facts, as I see them, are as follows, feel free to debate them.

1. Scanning the bus is not the problem, but PHY probing is.

If the MDIO bus is registered with of_mdiobus_register() - which is to
be expected, since the sja1105 driver only connects to a PHY using a
phy-handle - that should set mdio->phy_mask = ~0; which should disable
PHY scanning.

But of_mdiobus_register() will still call of_mdiobus_register_phy()
which will probe the phy_device. Here, depending on the code path,
_some_ PHY reads might be performed - which will return an error if the
PHY is missing its clock. For example, if the PHY ID isn't part of the
compatible string, fwnode_mdiobus_register_phy() will attempt to read it
from the PHY via get_phy_device(). Alternatively, you could put the PHY
ID in the DT and this will end up calling phy_device_create().

Then there's the probe() method of the T1L PHY driver, which is the
reason why it would be good to know what that driver is. Since its clock
might not be available, I expect that this driver doesn't access
hardware from probe(), knowing that it is an RMII PHY driver and this is
a generic problem for RMII PHYs.

2. The sja1105 driver already does all it reasonably can to make the
   RMII PHY happy.

The clocks of a port are enabled/configured from sja1105_clocking_setup_port()
which has 3 call paths:
(a) during sja1105_setup(), aka during switch initialization, all ports
    except RGMII ports have their clocks configured and enabled, via
    priv->info->clocking_setup(). The RGMII ports have a clock that
    depends upon the link speed, and we don't know the link speed.
(b) during sja1105_static_config_reload(). The sja1105 switch needs to
    dynamically reset itself at runtime, and this cuts off the clocks
    for a while. Again there is a call to priv->info->clocking_setup()
    here.
(c) during phylink_mac_link_up -> sja1105_adjust_port_config(), a call
    is made to sja1105_clocking_setup_port() for RGMII PHYs, because the
    speed is now known.

Since DSA calls dsa_slave_phy_setup() _after_ dsa_switch_setup(), this
means that by the time the PHY is attached, its config_init() runs, etc,
the RMII clock configured by sja1105_setup() should be running.

3. Clock gating the PHY won't make it lose its settings.

I expect that during the time when the sja1105 switch needs to reset,
the PHY just sees this as a few hundreds of ms during which there are no
clock edges on the crystal input pin. Sure, the PHY won't do anything
during that time, but this is quite different from a reset, is it not?
So asserting the hardware reset line of the PHY during the momentary
loss of clock, which is what you seem to suggest, will actively do more
harm than good.

4. Making the sja1105 driver a clock provider doesn't solve the problem
   in the general sense.

If you make this PHY driver expect the MAC to be a clock provider,
are you going to expect that all RMII-capable MAC drivers be patched?
For this reason I am in principle opposed to making the sja1105 driver
a clock provider, you won't be able to generalize this solution and it
would just create a huge mess going forward.

Re: sja1105q: proper way to solve PHY clk dependecy

[Oleksij Rempel]

Hello Vladimir,

thank you for your response!

On Wed, Mar 23, 2022 at 11:52:40AM +0200, Vladimir Oltean wrote:
> Hello Oleksij,
> 
> On Wed, Mar 23, 2022 at 07:03:31AM +0100, Oleksij Rempel wrote:
> > Hi Vladimir,
> > 
> > I have SJA1105Q based switch with 3 T1L PHYs connected over RMII
> > interface. The clk input "XI" of PHYs is connected to "MII0_TX_CLK/REF_CLK/TXC"
> > pins of the switch. Since this PHYs can't be configured reliably over MDIO
> > interface without running clk on XI input, i have a dependency dilemma:
> > i can't probe MDIO bus, without enabling DSA ports.
> > 
> > If I see it correctly, following steps should be done:
> > - register MDIO bus without scanning for PHYs
> > - define SJA1105Q switch as clock provider and PHYs as clk consumer
> > - detect and attach PHYs on port enable if clks can't be controlled
> >   without enabling the port.
> > - HW reset line of the PHYs should be asserted if we disable port and
> >   deasserted with proper reinit after port is enabled.
> > 
> > Other way would be to init and enable switch ports and PHYs by a bootloader and
> > keep it enabled.
> > 
> > What is the proper way to go?

> The facts, as I see them, are as follows, feel free to debate them.
> 
> 1. Scanning the bus is not the problem, but PHY probing is.
> 
> If the MDIO bus is registered with of_mdiobus_register() - which is to
> be expected, since the sja1105 driver only connects to a PHY using a
> phy-handle - that should set mdio->phy_mask = ~0; which should disable
> PHY scanning.
> 
> But of_mdiobus_register() will still call of_mdiobus_register_phy()
> which will probe the phy_device. Here, depending on the code path,
> _some_ PHY reads might be performed - which will return an error if the
> PHY is missing its clock. For example, if the PHY ID isn't part of the
> compatible string, fwnode_mdiobus_register_phy() will attempt to read it
> from the PHY via get_phy_device(). Alternatively, you could put the PHY
> ID in the DT and this will end up calling phy_device_create().
> 
> Then there's the probe() method of the T1L PHY driver, which is the
> reason why it would be good to know what that driver is. Since its clock
> might not be available, I expect that this driver doesn't access
> hardware from probe(), knowing that it is an RMII PHY driver and this is
> a generic problem for RMII PHYs.

ack. describing DT with compatible PHYid seems to be good enough.

> 2. The sja1105 driver already does all it reasonably can to make the
>    RMII PHY happy.
> 
> The clocks of a port are enabled/configured from sja1105_clocking_setup_port()
> which has 3 call paths:
> (a) during sja1105_setup(), aka during switch initialization, all ports
>     except RGMII ports have their clocks configured and enabled, via
>     priv->info->clocking_setup(). The RGMII ports have a clock that
>     depends upon the link speed, and we don't know the link speed.
> (b) during sja1105_static_config_reload(). The sja1105 switch needs to
>     dynamically reset itself at runtime, and this cuts off the clocks
>     for a while. Again there is a call to priv->info->clocking_setup()
>     here.
> (c) during phylink_mac_link_up -> sja1105_adjust_port_config(), a call
>     is made to sja1105_clocking_setup_port() for RGMII PHYs, because the
>     speed is now known.
> 
> Since DSA calls dsa_slave_phy_setup() _after_ dsa_switch_setup(), this
> means that by the time the PHY is attached, its config_init() runs, etc,
> the RMII clock configured by sja1105_setup() should be running.

ack. it works.

> 3. Clock gating the PHY won't make it lose its settings.
> 
> I expect that during the time when the sja1105 switch needs to reset,
> the PHY just sees this as a few hundreds of ms during which there are no
> clock edges on the crystal input pin. Sure, the PHY won't do anything
> during that time, but this is quite different from a reset, is it not?
> So asserting the hardware reset line of the PHY during the momentary
> loss of clock, which is what you seem to suggest, will actively do more
> harm than good.

can i be sure that MDIO access happens in the period where PHY is
supplied with stable clk

> 4. Making the sja1105 driver a clock provider doesn't solve the problem
>    in the general sense.
> 
> If you make this PHY driver expect the MAC to be a clock provider,
> are you going to expect that all RMII-capable MAC drivers be patched?
> For this reason I am in principle opposed to making the sja1105 driver
> a clock provider, you won't be able to generalize this solution and it
> would just create a huge mess going forward.

I can imagine optional clk support, but right now i do not have any
stability issues so no need to spend time on it right now.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: sja1105q: proper way to solve PHY clk dependecy

[Vladimir Oltean]

On Thu, Mar 24, 2022 at 02:48:24PM +0100, Oleksij Rempel wrote:
> > 3. Clock gating the PHY won't make it lose its settings.
> > 
> > I expect that during the time when the sja1105 switch needs to reset,
> > the PHY just sees this as a few hundreds of ms during which there are no
> > clock edges on the crystal input pin. Sure, the PHY won't do anything
> > during that time, but this is quite different from a reset, is it not?
> > So asserting the hardware reset line of the PHY during the momentary
> > loss of clock, which is what you seem to suggest, will actively do more
> > harm than good.
> 
> can i be sure that MDIO access happens in the period where PHY is
> supplied with stable clk

This is a good question. I suppose not, but I never ran into this issue.
You can try to force this by having the PHY library use poll mode for an
RMII PHY (case in which, IIRC, 3 or 4 PHY registers will be read every 2
seconds), then from user space do something like this:

ip link add br0 type bridge && ip link set br0 up
ip link set swp0 master br0 && ip link set swp0 up
while :; do
	ip link set br0 type bridge vlan_filtering 1
	sleep 1
	ip link set br0 type bridge vlan_filtering 0
	sleep 1
done

Every VLAN awareness change triggers a reset in the switch, and this
ends up calling sja1105_static_config_reload().

If you can artificially reproduce PHY access failures, first it's
interesting to analyze their impact, does the PHY state machine
transition to a halted state, or does it ignore the errors and continue
with the next poll cycle? If it continues, it's probably not worth doing
something.

To avoid the problem, you should probably need to iterate using
dsa_switch_for_each_user_port(), and mutex_lock(&dp->slave->phydev->lock)
for each RMII PHY during the reset procedure (similar to the other
things we lock out during the switch reset). The tricky part seems to be
releasing the locks in the reverse order of the acquire...