Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 23 Mar 2022 11:52:40 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Yangbo Lu <yangbo.lu@nxp.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel@pengutronix.de
Subject: Re: sja1105q: proper way to solve PHY clk dependecy
Message-ID: <20220323095240.y4xnp6ivz57obyvv@skbuf>
References: <20220323060331.GA4519@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220323060331.GA4519@pengutronix.de>
Precedence: bulk

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.