[net-next,v3] net: stmmac: don't create a MDIO bus if unnecessary

  The stmmac_dt_phy() function, which parses the devicetree node of the
MAC and ultimately causes MDIO bus allocation, misinterprets what
fixed-link means in relation to the MAC's MDIO bus. This results in
a MDIO bus being created in situations it need not be.

Currently a MDIO bus is created if the description is either:

    1. Not fixed-link
    2. fixed-link but contains a MDIO bus as well

The "1" case above isn't always accurate. If there's a phy-handle,
it could be referencing a phy on another MDIO controller's bus[1]. In
this case currently the MAC will make a MDIO bus and scan it all
anyways unnecessarily.

There's also a lot of upstream devicetrees[2] that expect a MDIO bus to
be created and scanned for a phy. This case can also be inferred from
the platform description by not having a phy-handle && not being
fixed-link. This hits case "1" in the current driver's logic.

Let's improve the logic to create a MDIO bus if either:

    - Devicetree contains a MDIO bus
    - !fixed-link && !phy-handle (legacy handling)

Below upstream devicetree snippets can be found that explain some of
the cases above more concretely.

Here's[0] a devicetree example where the MAC is both fixed-link and
driving a switch on MDIO (case "2" above). This needs a MDIO bus to
be created:

    &fec1 {
            phy-mode = "rmii";

            fixed-link {
                    speed = <100>;
                    full-duplex;
            };

            mdio1: mdio {
                    switch0: switch0@0 {
                            compatible = "marvell,mv88e6190";
                            pinctrl-0 = <&pinctrl_gpio_switch0>;
                    };
            };
    };

Here's[1] an example where there is no MDIO bus or fixed-link for
the ethernet1 MAC, so no MDIO bus should be created since ethernet0
is the MDIO master for ethernet1's phy:

    &ethernet0 {
            phy-mode = "sgmii";
            phy-handle = <&sgmii_phy0>;

            mdio {
                    compatible = "snps,dwmac-mdio";
                    sgmii_phy0: phy@8 {
                            compatible = "ethernet-phy-id0141.0dd4";
                            reg = <0x8>;
                            device_type = "ethernet-phy";
                    };

                    sgmii_phy1: phy@a {
                            compatible = "ethernet-phy-id0141.0dd4";
                            reg = <0xa>;
                            device_type = "ethernet-phy";
                    };
            };
    };

    &ethernet1 {
            phy-mode = "sgmii";
            phy-handle = <&sgmii_phy1>;
    };

Finally there's descriptions like this[2] which don't describe the
MDIO bus but expect it to be created and the whole address space
scanned for a phy since there's no phy-handle or fixed-link described:

    &gmac {
            phy-supply = <&vcc_lan>;
            phy-mode = "rmii";
            snps,reset-gpio = <&gpio3 RK_PB4 GPIO_ACTIVE_HIGH>;
            snps,reset-active-low;
            snps,reset-delays-us = <0 10000 1000000>;
    };

[0] https://elixir.bootlin.com/linux/v6.5-rc5/source/arch/arm/boot/dts/nxp/vf/vf610-zii-ssmb-dtu.dts
[1] https://elixir.bootlin.com/linux/v6.6-rc5/source/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
[2] https://elixir.bootlin.com/linux/v6.6-rc5/source/arch/arm64/boot/dts/rockchip/rk3368-r88.dts#L164

Co-developed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
Changes in v3:
    - Keep logic out of stmmac_probe_config_dt() since it's already massive (Serge)

Changes in v2:
    - Handle the fixed-link + mdio case (Andrew Lunn)
    - Reworded commit message
    - Still handle the "legacy" case mentioned in the commit
    - Bit further refactoring of the function for readability

- Link to v2: https://lore.kernel.org/r/20231206-stmmac-no-mdio-node-v2-1-333cae49b1ca@redhat.com
- Link to v1: https://lore.kernel.org/netdev/20230808120254.11653-1-brgl@bgdev.pl/
---
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  | 91 +++++++++++++---------
 1 file changed, 54 insertions(+), 37 deletions(-)


---
base-commit: fd8a79b63710acb84321be3ce74be23c876873fb
change-id: 20231127-stmmac-no-mdio-node-1db9da8a25d9

Best regards,
  

On Thu, Dec 07, 2023 at 03:12:40PM -0600, Andrew Halaney wrote:
> The stmmac_dt_phy() function, which parses the devicetree node of the
> MAC and ultimately causes MDIO bus allocation, misinterprets what
> fixed-link means in relation to the MAC's MDIO bus. This results in
> a MDIO bus being created in situations it need not be.
> 
> Currently a MDIO bus is created if the description is either:
> 
>     1. Not fixed-link
>     2. fixed-link but contains a MDIO bus as well
> 
> The "1" case above isn't always accurate. If there's a phy-handle,
> it could be referencing a phy on another MDIO controller's bus[1]. In
> this case currently the MAC will make a MDIO bus and scan it all
> anyways unnecessarily.
> 
> There's also a lot of upstream devicetrees[2] that expect a MDIO bus to
> be created and scanned for a phy. This case can also be inferred from
> the platform description by not having a phy-handle && not being
> fixed-link. This hits case "1" in the current driver's logic.
> 
> Let's improve the logic to create a MDIO bus if either:
> 

>     - Devicetree contains a MDIO bus
>     - !fixed-link && !phy-handle (legacy handling)

If what you suggest here is a free from regressions semantics change
(really hope it is) I will be with both my hands for it. This will
solve the problem we have with one of our device which doesn't have
SMA interface (hardware designers decided to save ~4K gates of the
chip area) but has a PHY externally attached to the DW XGMAC<->XPCS
interface. PHY is accessible via a GPIO-based MDIO bus. BTW having no
SMA interface available on a DW *MAC device but creating the MDIO-bus
on top of the non-existent SMA CSRs anyway causes having _32_ dummy
PHYs created with zero IDs.

> 
> Below upstream devicetree snippets can be found that explain some of
> the cases above more concretely.
> 
> Here's[0] a devicetree example where the MAC is both fixed-link and
> driving a switch on MDIO (case "2" above). This needs a MDIO bus to
> be created:
> 
>     &fec1 {
>             phy-mode = "rmii";
> 
>             fixed-link {
>                     speed = <100>;
>                     full-duplex;
>             };
> 
>             mdio1: mdio {
>                     switch0: switch0@0 {
>                             compatible = "marvell,mv88e6190";
>                             pinctrl-0 = <&pinctrl_gpio_switch0>;
>                     };
>             };
>     };
> 
> Here's[1] an example where there is no MDIO bus or fixed-link for
> the ethernet1 MAC, so no MDIO bus should be created since ethernet0
> is the MDIO master for ethernet1's phy:
> 
>     &ethernet0 {
>             phy-mode = "sgmii";
>             phy-handle = <&sgmii_phy0>;
> 
>             mdio {
>                     compatible = "snps,dwmac-mdio";
>                     sgmii_phy0: phy@8 {
>                             compatible = "ethernet-phy-id0141.0dd4";
>                             reg = <0x8>;
>                             device_type = "ethernet-phy";
>                     };
> 
>                     sgmii_phy1: phy@a {
>                             compatible = "ethernet-phy-id0141.0dd4";
>                             reg = <0xa>;
>                             device_type = "ethernet-phy";
>                     };
>             };
>     };
> 
>     &ethernet1 {
>             phy-mode = "sgmii";
>             phy-handle = <&sgmii_phy1>;
>     };
> 
> Finally there's descriptions like this[2] which don't describe the
> MDIO bus but expect it to be created and the whole address space
> scanned for a phy since there's no phy-handle or fixed-link described:
> 
>     &gmac {
>             phy-supply = <&vcc_lan>;
>             phy-mode = "rmii";
>             snps,reset-gpio = <&gpio3 RK_PB4 GPIO_ACTIVE_HIGH>;
>             snps,reset-active-low;
>             snps,reset-delays-us = <0 10000 1000000>;
>     };
> 
> [0] https://elixir.bootlin.com/linux/v6.5-rc5/source/arch/arm/boot/dts/nxp/vf/vf610-zii-ssmb-dtu.dts
> [1] https://elixir.bootlin.com/linux/v6.6-rc5/source/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> [2] https://elixir.bootlin.com/linux/v6.6-rc5/source/arch/arm64/boot/dts/rockchip/rk3368-r88.dts#L164
> 
> Co-developed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> ---
> Changes in v3:

>     - Keep logic out of stmmac_probe_config_dt() since it's already massive (Serge)

Thanks for taking my note into account. The change turned out even
better than I thought it would look like. The MDIO-bus setup procedure
is much more readable now. A small nitpick below.

> 
> Changes in v2:
>     - Handle the fixed-link + mdio case (Andrew Lunn)
>     - Reworded commit message
>     - Still handle the "legacy" case mentioned in the commit
>     - Bit further refactoring of the function for readability
> 
> - Link to v2: https://lore.kernel.org/r/20231206-stmmac-no-mdio-node-v2-1-333cae49b1ca@redhat.com
> - Link to v1: https://lore.kernel.org/netdev/20230808120254.11653-1-brgl@bgdev.pl/
> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_platform.c  | 91 +++++++++++++---------
>  1 file changed, 54 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 1ffde555da47..d6079c1432c7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -296,62 +296,80 @@ static int stmmac_mtl_setup(struct platform_device *pdev,
>  }
>  
>  /**
> - * stmmac_dt_phy - parse device-tree driver parameters to allocate PHY resources
> - * @plat: driver data platform structure
> - * @np: device tree node
> - * @dev: device pointer
> - * Description:
> - * The mdio bus will be allocated in case of a phy transceiver is on board;
> - * it will be NULL if the fixed-link is configured.
> - * If there is the "snps,dwmac-mdio" sub-node the mdio will be allocated
> - * in any case (for DSA, mdio must be registered even if fixed-link).
> - * The table below sums the supported configurations:
> - *	-------------------------------
> - *	snps,phy-addr	|     Y
> - *	-------------------------------
> - *	phy-handle	|     Y
> - *	-------------------------------
> - *	fixed-link	|     N
> - *	-------------------------------
> - *	snps,dwmac-mdio	|
> - *	  even if	|     Y
> - *	fixed-link	|
> - *	-------------------------------
> + * stmmac_of_get_mdio() - Gets the MDIO bus from the devicetree.
> + * @np: devicetree node
>   *
> - * It returns 0 in case of success otherwise -ENODEV.
> + * The MDIO bus will be searched for in the following ways:
> + * 1. The compatible is "snps,dwc-qos-ethernet-4.10" && a "mdio" named
> + *    child node exists
> + * 2. A child node with the "snps,dwmac-mdio" compatible is present
> + *
> + * Return: The MDIO node if present otherwise NULL
>   */
> -static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
> -			 struct device_node *np, struct device *dev)
> +static struct device_node *stmmac_of_get_mdio(struct device_node *np)
>  {
> -	bool mdio = !of_phy_is_fixed_link(np);
>  	static const struct of_device_id need_mdio_ids[] = {
>  		{ .compatible = "snps,dwc-qos-ethernet-4.10" },
>  		{},
>  	};
> +	struct device_node *mdio_node = NULL;
>  
>  	if (of_match_node(need_mdio_ids, np)) {
> -		plat->mdio_node = of_get_child_by_name(np, "mdio");
> +		mdio_node = of_get_child_by_name(np, "mdio");
>  	} else {
>  		/**
>  		 * If snps,dwmac-mdio is passed from DT, always register
>  		 * the MDIO
>  		 */
> -		for_each_child_of_node(np, plat->mdio_node) {
> -			if (of_device_is_compatible(plat->mdio_node,
> +		for_each_child_of_node(np, mdio_node) {
> +			if (of_device_is_compatible(mdio_node,
>  						    "snps,dwmac-mdio"))
>  				break;
>  		}
>  	}
>  
> -	if (plat->mdio_node) {
> +	return mdio_node;
> +}
> +
> +/**
> + * stmmac_mdio_setup() - Populate platform related MDIO structures.
> + * @plat: driver data platform structure
> + * @np: devicetree node
> + * @dev: device pointer
> + *
> + * This searches for MDIO information from the devicetree.
> + * If an MDIO node is found, it's assigned to plat->mdio_node and
> + * plat->mdio_bus_data is allocated.
> + * If no connection can be determined, just plat->mdio_bus_data is allocated
> + * to indicate a bus should be created and scanned for a phy.
> + * If it's determined there's no MDIO bus needed, both are left NULL.
> + *
> + * This expects that plat->phy_node has already been searched for.
> + *
> + * Return: 0 on success, errno otherwise.
> + */
> +static int stmmac_mdio_setup(struct plat_stmmacenet_data *plat,
> +			     struct device_node *np, struct device *dev)
> +{
> +	bool legacy_mdio;
> +
> +	plat->mdio_node = stmmac_of_get_mdio(np);
> +	if (plat->mdio_node)
>  		dev_dbg(dev, "Found MDIO subnode\n");
> -		mdio = true;
> -	}
>  
> -	if (mdio) {
> -		plat->mdio_bus_data =
> -			devm_kzalloc(dev, sizeof(struct stmmac_mdio_bus_data),
> -				     GFP_KERNEL);

> +	/* Legacy devicetrees allowed for no MDIO bus description and expect
> +	 * the bus to be scanned for devices. If there's no phy or fixed-link
> +	 * described assume this is the case since there must be something
> +	 * connected to the MAC.
> +	 */
> +	legacy_mdio = !of_phy_is_fixed_link(np) && !plat->phy_node;
> +	if (legacy_mdio)
> +		dev_info(dev, "Deprecated MDIO bus assumption used\n");
> +
> +	if (plat->mdio_node || legacy_mdio) {
> +		plat->mdio_bus_data = devm_kzalloc(dev,

Special thanks for adding the comment above this code. It will really
save time of figuring out why MDIO-bus needs to be created anyway.

> +						   sizeof(struct stmmac_mdio_bus_data),

Should v4 is required I would suggest to change this to
sizeof(*plat->mdio_bus_data).

Anyway feel free to add:
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

> +						   GFP_KERNEL);
>  		if (!plat->mdio_bus_data)
>  			return -ENOMEM;
>  
> @@ -471,8 +489,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
>  	if (of_property_read_u32(np, "snps,phy-addr", &plat->phy_addr) == 0)
>  		dev_warn(&pdev->dev, "snps,phy-addr property is deprecated\n");
>  
> -	/* To Configure PHY by using all device-tree supported properties */
> -	rc = stmmac_dt_phy(plat, np, &pdev->dev);
> +	rc = stmmac_mdio_setup(plat, np, &pdev->dev);
>  	if (rc)
>  		return ERR_PTR(rc);
>  
> 
> ---
> base-commit: fd8a79b63710acb84321be3ce74be23c876873fb
> change-id: 20231127-stmmac-no-mdio-node-1db9da8a25d9
> 
> Best regards,
> -- 
> Andrew Halaney <ahalaney@redhat.com>
> 
>
  

On Fri, Dec 08, 2023 at 01:16:12AM +0300, Serge Semin wrote:
> On Thu, Dec 07, 2023 at 03:12:40PM -0600, Andrew Halaney wrote:
> > The stmmac_dt_phy() function, which parses the devicetree node of the
> > MAC and ultimately causes MDIO bus allocation, misinterprets what
> > fixed-link means in relation to the MAC's MDIO bus. This results in
> > a MDIO bus being created in situations it need not be.
> > 
> > Currently a MDIO bus is created if the description is either:
> > 
> >     1. Not fixed-link
> >     2. fixed-link but contains a MDIO bus as well
> > 
> > The "1" case above isn't always accurate. If there's a phy-handle,
> > it could be referencing a phy on another MDIO controller's bus[1]. In
> > this case currently the MAC will make a MDIO bus and scan it all
> > anyways unnecessarily.
> > 
> > There's also a lot of upstream devicetrees[2] that expect a MDIO bus to
> > be created and scanned for a phy. This case can also be inferred from
> > the platform description by not having a phy-handle && not being
> > fixed-link. This hits case "1" in the current driver's logic.
> > 
> > Let's improve the logic to create a MDIO bus if either:
> > 
> 
> >     - Devicetree contains a MDIO bus
> >     - !fixed-link && !phy-handle (legacy handling)
> 
> If what you suggest here is a free from regressions semantics change
> (really hope it is) I will be with both my hands for it. This will
> solve the problem we have with one of our device which doesn't have
> SMA interface (hardware designers decided to save ~4K gates of the
> chip area) but has a PHY externally attached to the DW XGMAC<->XPCS
> interface. PHY is accessible via a GPIO-based MDIO bus. BTW having no
> SMA interface available on a DW *MAC device but creating the MDIO-bus
> on top of the non-existent SMA CSRs anyway causes having _32_ dummy
> PHYs created with zero IDs.

I hope it is regression free! I have tested both the [1] and [2] cases
(I hacked up the devicetree for [1] to make it look like [2]) without
any issue.

Sorry, I don't have any docs for stmmac hardware so this might be
answered in there (or just common net knowledge that I can't find
online)... what's SMA stand for? I assume it's the MDIO interface.

I agree though, if you have a phy-handle and no mdio node in your
devicetree this patch series should bail out without registering a bus
in stmmac_mdio_register().

> 
> > 
> > Below upstream devicetree snippets can be found that explain some of
> > the cases above more concretely.

<snip>

> > -	if (mdio) {
> > -		plat->mdio_bus_data =
> > -			devm_kzalloc(dev, sizeof(struct stmmac_mdio_bus_data),
> > -				     GFP_KERNEL);
> 
> > +	/* Legacy devicetrees allowed for no MDIO bus description and expect
> > +	 * the bus to be scanned for devices. If there's no phy or fixed-link
> > +	 * described assume this is the case since there must be something
> > +	 * connected to the MAC.
> > +	 */
> > +	legacy_mdio = !of_phy_is_fixed_link(np) && !plat->phy_node;
> > +	if (legacy_mdio)
> > +		dev_info(dev, "Deprecated MDIO bus assumption used\n");
> > +
> > +	if (plat->mdio_node || legacy_mdio) {
> > +		plat->mdio_bus_data = devm_kzalloc(dev,
> 
> Special thanks for adding the comment above this code. It will really
> save time of figuring out why MDIO-bus needs to be created anyway.
> 
> > +						   sizeof(struct stmmac_mdio_bus_data),
> 
> Should v4 is required I would suggest to change this to
> sizeof(*plat->mdio_bus_data).
> 
> Anyway feel free to add:
> Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> 
> -Serge(y)

Sure I will spin v4 to pick that up, thanks for catching it. I'll also
improve the motivation in the commit message a hair more per Andrew
Lunn's request over here on v2 (and will hold off a little bit just to
make sure reviews come in before a respin):

    https://lore.kernel.org/netdev/e64b14c3-4b80-4120-8cc4-9baa40cdcb75@lunn.ch/

Thanks,
Andrew
  

On Thu, Dec 07, 2023 at 05:07:24PM -0600, Andrew Halaney wrote:
> On Fri, Dec 08, 2023 at 01:16:12AM +0300, Serge Semin wrote:
> > On Thu, Dec 07, 2023 at 03:12:40PM -0600, Andrew Halaney wrote:
> > > The stmmac_dt_phy() function, which parses the devicetree node of the
> > > MAC and ultimately causes MDIO bus allocation, misinterprets what
> > > fixed-link means in relation to the MAC's MDIO bus. This results in
> > > a MDIO bus being created in situations it need not be.
> > > 
> > > Currently a MDIO bus is created if the description is either:
> > > 
> > >     1. Not fixed-link
> > >     2. fixed-link but contains a MDIO bus as well
> > > 
> > > The "1" case above isn't always accurate. If there's a phy-handle,
> > > it could be referencing a phy on another MDIO controller's bus[1]. In
> > > this case currently the MAC will make a MDIO bus and scan it all
> > > anyways unnecessarily.
> > > 
> > > There's also a lot of upstream devicetrees[2] that expect a MDIO bus to
> > > be created and scanned for a phy. This case can also be inferred from
> > > the platform description by not having a phy-handle && not being
> > > fixed-link. This hits case "1" in the current driver's logic.
> > > 
> > > Let's improve the logic to create a MDIO bus if either:
> > > 
> > 
> > >     - Devicetree contains a MDIO bus
> > >     - !fixed-link && !phy-handle (legacy handling)
> > 
> > If what you suggest here is a free from regressions semantics change
> > (really hope it is) I will be with both my hands for it. This will
> > solve the problem we have with one of our device which doesn't have
> > SMA interface (hardware designers decided to save ~4K gates of the
> > chip area) but has a PHY externally attached to the DW XGMAC<->XPCS
> > interface. PHY is accessible via a GPIO-based MDIO bus. BTW having no
> > SMA interface available on a DW *MAC device but creating the MDIO-bus
> > on top of the non-existent SMA CSRs anyway causes having _32_ dummy
> > PHYs created with zero IDs.
> 

> I hope it is regression free! I have tested both the [1] and [2] cases
> (I hacked up the devicetree for [1] to make it look like [2]) without
> any issue.
> 

I doubt you could have tested it on all the possible hardware the
STMMAC driver supports. The problem is that the DT-bindings thing is a
kind of contract which can't be changed that easily. It's like ABI but
for the hardware description so the kernel would bootup correctly on
the platforms with the old DT blobs. But if the change isn't that
critical, if the device-tree sources in the kernel fit to the updated
semantics, if the networking subsystem maintainers aren't against it
and I guess with the Rob, Krzysztof or Conor blessing (at least it
won't hurt to add them to the Cc-list together with the devicetree
mailing-list), then it will likely be accepted.

> Sorry, I don't have any docs for stmmac hardware so this might be
> answered in there (or just common net knowledge that I can't find
> online)... what's SMA stand for? I assume it's the MDIO interface.

Right. Synopsys names the MDIO-bus interface as Station Management
Agent MDIO module.

> 
> I agree though, if you have a phy-handle and no mdio node in your
> devicetree this patch series should bail out without registering a bus
> in stmmac_mdio_register().

On the other hand why would the MDIO-bus needed in such case? If the
phy-handle property is specified with no MDIO-bus DT-subnode, then it
will point out to a PHY residing an external bus. The only case I can
imagine though is that the DW XPCS device could be still auto-detected
on the internal SMA-MDIO-bus. But the only driver which currently has
XPCS auto-detection activated is the Intel glue layer (see
dwmac-intel.c and has_xpcs flag), but it doesn't use DT interface
since it handles a PCIe-based device.  So this case is out of
brackets.

> 
> > 
> > > 
> > > Below upstream devicetree snippets can be found that explain some of
> > > the cases above more concretely.
> 
> <snip>
> 
> > > -	if (mdio) {
> > > -		plat->mdio_bus_data =
> > > -			devm_kzalloc(dev, sizeof(struct stmmac_mdio_bus_data),
> > > -				     GFP_KERNEL);
> > 
> > > +	/* Legacy devicetrees allowed for no MDIO bus description and expect
> > > +	 * the bus to be scanned for devices. If there's no phy or fixed-link
> > > +	 * described assume this is the case since there must be something
> > > +	 * connected to the MAC.
> > > +	 */
> > > +	legacy_mdio = !of_phy_is_fixed_link(np) && !plat->phy_node;
> > > +	if (legacy_mdio)
> > > +		dev_info(dev, "Deprecated MDIO bus assumption used\n");
> > > +
> > > +	if (plat->mdio_node || legacy_mdio) {
> > > +		plat->mdio_bus_data = devm_kzalloc(dev,
> > 
> > Special thanks for adding the comment above this code. It will really
> > save time of figuring out why MDIO-bus needs to be created anyway.
> > 
> > > +						   sizeof(struct stmmac_mdio_bus_data),
> > 
> > Should v4 is required I would suggest to change this to
> > sizeof(*plat->mdio_bus_data).
> > 
> > Anyway feel free to add:
> > Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> > 
> > -Serge(y)
> 

> Sure I will spin v4 to pick that up, thanks for catching it. I'll also
> improve the motivation in the commit message a hair more per Andrew
> Lunn's request over here on v2 (and will hold off a little bit just to
> make sure reviews come in before a respin):
> 
>     https://lore.kernel.org/netdev/e64b14c3-4b80-4120-8cc4-9baa40cdcb75@lunn.ch/

Ok. Thanks.

-Serge(y)

> 
> Thanks,
> Andrew
>
  

On Fri, Dec 08, 2023 at 06:07:06PM +0300, Serge Semin wrote:
> On Thu, Dec 07, 2023 at 05:07:24PM -0600, Andrew Halaney wrote:
> > On Fri, Dec 08, 2023 at 01:16:12AM +0300, Serge Semin wrote:
> > > On Thu, Dec 07, 2023 at 03:12:40PM -0600, Andrew Halaney wrote:
> > > > The stmmac_dt_phy() function, which parses the devicetree node of the
> > > > MAC and ultimately causes MDIO bus allocation, misinterprets what
> > > > fixed-link means in relation to the MAC's MDIO bus. This results in
> > > > a MDIO bus being created in situations it need not be.
> > > > 
> > > > Currently a MDIO bus is created if the description is either:
> > > > 
> > > >     1. Not fixed-link
> > > >     2. fixed-link but contains a MDIO bus as well
> > > > 
> > > > The "1" case above isn't always accurate. If there's a phy-handle,
> > > > it could be referencing a phy on another MDIO controller's bus[1]. In
> > > > this case currently the MAC will make a MDIO bus and scan it all
> > > > anyways unnecessarily.
> > > > 
> > > > There's also a lot of upstream devicetrees[2] that expect a MDIO bus to
> > > > be created and scanned for a phy. This case can also be inferred from
> > > > the platform description by not having a phy-handle && not being
> > > > fixed-link. This hits case "1" in the current driver's logic.
> > > > 
> > > > Let's improve the logic to create a MDIO bus if either:
> > > > 
> > > 
> > > >     - Devicetree contains a MDIO bus
> > > >     - !fixed-link && !phy-handle (legacy handling)
> > > 
> > > If what you suggest here is a free from regressions semantics change
> > > (really hope it is) I will be with both my hands for it. This will
> > > solve the problem we have with one of our device which doesn't have
> > > SMA interface (hardware designers decided to save ~4K gates of the
> > > chip area) but has a PHY externally attached to the DW XGMAC<->XPCS
> > > interface. PHY is accessible via a GPIO-based MDIO bus. BTW having no
> > > SMA interface available on a DW *MAC device but creating the MDIO-bus
> > > on top of the non-existent SMA CSRs anyway causes having _32_ dummy
> > > PHYs created with zero IDs.
> > 
> 
> > I hope it is regression free! I have tested both the [1] and [2] cases
> > (I hacked up the devicetree for [1] to make it look like [2]) without
> > any issue.
> > 
> 
> I doubt you could have tested it on all the possible hardware the
> STMMAC driver supports. The problem is that the DT-bindings thing is a
> kind of contract which can't be changed that easily. It's like ABI but
> for the hardware description so the kernel would bootup correctly on
> the platforms with the old DT blobs. But if the change isn't that
> critical, if the device-tree sources in the kernel fit to the updated
> semantics, if the networking subsystem maintainers aren't against it
> and I guess with the Rob, Krzysztof or Conor blessing (at least it
> won't hurt to add them to the Cc-list together with the devicetree
> mailing-list), then it will likely be accepted.

To be clear, I don't think we're violating the dt-binding ABI contract
here. My intention is that all the existing use cases continue to work,
and this just improves one use case. I did a write up
over on v2 about the use cases I see and the current logic vs what
changes with this patch series:

    https://lore.kernel.org/netdev/plvbqgi2bwlv5quvpiwplq7cxx6m5rl3ghnfhuxfx4bpuhyihl@zmydwrtwdeg6/

Please comment if you think I have broken some backwards
compatibility. If I _could_ break compatibility, I'd make everyone
describe their busses entirely... but as you said that's against the
spirit of dt-bindings and would upset a lot of people. That's why I've
left the "!fixed-link && !phy-handle (legacy handling) logic :)

> 
> > Sorry, I don't have any docs for stmmac hardware so this might be
> > answered in there (or just common net knowledge that I can't find
> > online)... what's SMA stand for? I assume it's the MDIO interface.
> 
> Right. Synopsys names the MDIO-bus interface as Station Management
> Agent MDIO module.
> 
> > 
> > I agree though, if you have a phy-handle and no mdio node in your
> > devicetree this patch series should bail out without registering a bus
> > in stmmac_mdio_register().
> 
> On the other hand why would the MDIO-bus needed in such case? If the
> phy-handle property is specified with no MDIO-bus DT-subnode, then it
> will point out to a PHY residing an external bus. The only case I can
> imagine though is that the DW XPCS device could be still auto-detected
> on the internal SMA-MDIO-bus. But the only driver which currently has
> XPCS auto-detection activated is the Intel glue layer (see
> dwmac-intel.c and has_xpcs flag), but it doesn't use DT interface
> since it handles a PCIe-based device.  So this case is out of
> brackets.

Agreed, I think making the bus is not needed in the driver as is in that
case.

I'd like to think (but am not sure) that when a devicetree based DW XPCS
description comes around it will allow you to describe it exactly
instead of doing auto-detection (i.e. something like phy-handle), but I
am not very familiar with PCS and friends in the stack so that may be a
crude extension from my knowledge of MDIO.

Thanks,
Andrew

> 
> > 
> > > 
> > > > 
> > > > Below upstream devicetree snippets can be found that explain some of
> > > > the cases above more concretely.
> > 
> > <snip>
> > 
> > > > -	if (mdio) {
> > > > -		plat->mdio_bus_data =
> > > > -			devm_kzalloc(dev, sizeof(struct stmmac_mdio_bus_data),
> > > > -				     GFP_KERNEL);
> > > 
> > > > +	/* Legacy devicetrees allowed for no MDIO bus description and expect
> > > > +	 * the bus to be scanned for devices. If there's no phy or fixed-link
> > > > +	 * described assume this is the case since there must be something
> > > > +	 * connected to the MAC.
> > > > +	 */
> > > > +	legacy_mdio = !of_phy_is_fixed_link(np) && !plat->phy_node;
> > > > +	if (legacy_mdio)
> > > > +		dev_info(dev, "Deprecated MDIO bus assumption used\n");
> > > > +
> > > > +	if (plat->mdio_node || legacy_mdio) {
> > > > +		plat->mdio_bus_data = devm_kzalloc(dev,
> > > 
> > > Special thanks for adding the comment above this code. It will really
> > > save time of figuring out why MDIO-bus needs to be created anyway.
> > > 
> > > > +						   sizeof(struct stmmac_mdio_bus_data),
> > > 
> > > Should v4 is required I would suggest to change this to
> > > sizeof(*plat->mdio_bus_data).
> > > 
> > > Anyway feel free to add:
> > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> > > 
> > > -Serge(y)
> > 
> 
> > Sure I will spin v4 to pick that up, thanks for catching it. I'll also
> > improve the motivation in the commit message a hair more per Andrew
> > Lunn's request over here on v2 (and will hold off a little bit just to
> > make sure reviews come in before a respin):
> > 
> >     https://lore.kernel.org/netdev/e64b14c3-4b80-4120-8cc4-9baa40cdcb75@lunn.ch/
> 
> Ok. Thanks.
> 
> -Serge(y)
> 
> > 
> > Thanks,
> > Andrew
> > 
>
  

On Fri, Dec 08, 2023 at 10:50:29AM -0600, Andrew Halaney wrote:
> On Fri, Dec 08, 2023 at 06:07:06PM +0300, Serge Semin wrote:
> > On Thu, Dec 07, 2023 at 05:07:24PM -0600, Andrew Halaney wrote:
> > > On Fri, Dec 08, 2023 at 01:16:12AM +0300, Serge Semin wrote:
> > > > On Thu, Dec 07, 2023 at 03:12:40PM -0600, Andrew Halaney wrote:
> > > > > The stmmac_dt_phy() function, which parses the devicetree node of the
> > > > > MAC and ultimately causes MDIO bus allocation, misinterprets what
> > > > > fixed-link means in relation to the MAC's MDIO bus. This results in
> > > > > a MDIO bus being created in situations it need not be.
> > > > > 
> > > > > Currently a MDIO bus is created if the description is either:
> > > > > 
> > > > >     1. Not fixed-link
> > > > >     2. fixed-link but contains a MDIO bus as well
> > > > > 
> > > > > The "1" case above isn't always accurate. If there's a phy-handle,
> > > > > it could be referencing a phy on another MDIO controller's bus[1]. In
> > > > > this case currently the MAC will make a MDIO bus and scan it all
> > > > > anyways unnecessarily.
> > > > > 
> > > > > There's also a lot of upstream devicetrees[2] that expect a MDIO bus to
> > > > > be created and scanned for a phy. This case can also be inferred from
> > > > > the platform description by not having a phy-handle && not being
> > > > > fixed-link. This hits case "1" in the current driver's logic.
> > > > > 
> > > > > Let's improve the logic to create a MDIO bus if either:
> > > > > 
> > > > 
> > > > >     - Devicetree contains a MDIO bus
> > > > >     - !fixed-link && !phy-handle (legacy handling)
> > > > 
> > > > If what you suggest here is a free from regressions semantics change
> > > > (really hope it is) I will be with both my hands for it. This will
> > > > solve the problem we have with one of our device which doesn't have
> > > > SMA interface (hardware designers decided to save ~4K gates of the
> > > > chip area) but has a PHY externally attached to the DW XGMAC<->XPCS
> > > > interface. PHY is accessible via a GPIO-based MDIO bus. BTW having no
> > > > SMA interface available on a DW *MAC device but creating the MDIO-bus
> > > > on top of the non-existent SMA CSRs anyway causes having _32_ dummy
> > > > PHYs created with zero IDs.
> > > 
> > 
> > > I hope it is regression free! I have tested both the [1] and [2] cases
> > > (I hacked up the devicetree for [1] to make it look like [2]) without
> > > any issue.
> > > 
> > 
> > I doubt you could have tested it on all the possible hardware the
> > STMMAC driver supports. The problem is that the DT-bindings thing is a
> > kind of contract which can't be changed that easily. It's like ABI but
> > for the hardware description so the kernel would bootup correctly on
> > the platforms with the old DT blobs. But if the change isn't that
> > critical, if the device-tree sources in the kernel fit to the updated
> > semantics, if the networking subsystem maintainers aren't against it
> > and I guess with the Rob, Krzysztof or Conor blessing (at least it
> > won't hurt to add them to the Cc-list together with the devicetree
> > mailing-list), then it will likely be accepted.
> 

> To be clear, I don't think we're violating the dt-binding ABI contract
> here. My intention is that all the existing use cases continue to work,
> and this just improves one use case. I did a write up
> over on v2 about the use cases I see and the current logic vs what
> changes with this patch series:
> 
>     https://lore.kernel.org/netdev/plvbqgi2bwlv5quvpiwplq7cxx6m5rl3ghnfhuxfx4bpuhyihl@zmydwrtwdeg6/
> 
> Please comment if you think I have broken some backwards
> compatibility.

To shortly sum up so I didn't miss something. Current semantics of the
MDIO-bus registration is:
if !fixed-link || mdio_node_present
    create MDIO-bus
and the semantics of the PHY auto-probe (legacy) is:
if !(fixed-link || mdio_node_present || phy_node_present)
    auto-probe PHY

You are changing the MDIO-bus creation semantics to:
if !(fixed-link || phy_node_present) || mdio_node_present
    create MDIO-bus
with no change in the PHY auto-probe semantics:
if !(fixed-link || mdio_node_present || phy_node_present)
    auto-probe PHY

So the change is that if a PHY-handle is specified the MDIO-bus won't
be created with the rest conditions being the same.

The only concern I had was the so called legacy case and a possibility
to have MDIO-bus with other than PHY devices. Based on the pseudo-code
above the former case won't be affected since having PHY-node
specified didn't triggered MDIO-bus auto-probe even before your
change. The later case concerns for instance the DW XPCS devices which
on some platforms could be found on the DW MAC MDIO bus with not
having PHY living on that bus. But DW XPCS auto-probing currently is
only supported by the non-OF platforms (it's Intel). Thus your change
is supposed to be safe here too.

So to speak AFAICS from the STMMAC MDIO OF stuff your solution isn't
supposed to cause regressions and break the current DTs backward
compatibility indeed.

Regarding the ideal implementation. What could be much better is to
implement the next semantics:
if SMA-capability-detected &&
   (!mdio_node_present || (mdio_node_present && mdio_node_enabled))
    create MDIO-bus
and preserve the PHY auto-probe semantics for backwards compatibility.
Regarding the SMA-capability flag, it has been presented since DW GMAC
v3.50, so since very much early DW MAC releases. But even for the
early devices I think it could be auto-detected by checking the SMA
CSRs writability. At least DW XGMAC AFAICS has the command CSR not
writable if SMA is unavailable.

But I guess it's a matter of another patch.

> If I _could_ break compatibility, I'd make everyone
> describe their busses entirely... but as you said that's against the
> spirit of dt-bindings and would upset a lot of people. That's why I've
> left the "!fixed-link && !phy-handle (legacy handling) logic :)
> 
> > 
> > > Sorry, I don't have any docs for stmmac hardware so this might be
> > > answered in there (or just common net knowledge that I can't find
> > > online)... what's SMA stand for? I assume it's the MDIO interface.
> > 
> > Right. Synopsys names the MDIO-bus interface as Station Management
> > Agent MDIO module.
> > 
> > > 
> > > I agree though, if you have a phy-handle and no mdio node in your
> > > devicetree this patch series should bail out without registering a bus
> > > in stmmac_mdio_register().
> > 
> > On the other hand why would the MDIO-bus needed in such case? If the
> > phy-handle property is specified with no MDIO-bus DT-subnode, then it
> > will point out to a PHY residing an external bus. The only case I can
> > imagine though is that the DW XPCS device could be still auto-detected
> > on the internal SMA-MDIO-bus. But the only driver which currently has
> > XPCS auto-detection activated is the Intel glue layer (see
> > dwmac-intel.c and has_xpcs flag), but it doesn't use DT interface
> > since it handles a PCIe-based device.  So this case is out of
> > brackets.
> 
> Agreed, I think making the bus is not needed in the driver as is in that
> case.
> 

> I'd like to think (but am not sure) that when a devicetree based DW XPCS
> description comes around it will allow you to describe it exactly
> instead of doing auto-detection (i.e. something like phy-handle), but I
> am not very familiar with PCS and friends in the stack so that may be a
> crude extension from my knowledge of MDIO.

Right, there is a generic property for that - "pcs-handle". Last week
I submitted a series which makes it supported on the STMMAC and XPCS
drivers.

-Serge(y)

> 
> Thanks,
> Andrew
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > Below upstream devicetree snippets can be found that explain some of
> > > > > the cases above more concretely.
> > > 
> > > <snip>
> > > 
> > > > > -	if (mdio) {
> > > > > -		plat->mdio_bus_data =
> > > > > -			devm_kzalloc(dev, sizeof(struct stmmac_mdio_bus_data),
> > > > > -				     GFP_KERNEL);
> > > > 
> > > > > +	/* Legacy devicetrees allowed for no MDIO bus description and expect
> > > > > +	 * the bus to be scanned for devices. If there's no phy or fixed-link
> > > > > +	 * described assume this is the case since there must be something
> > > > > +	 * connected to the MAC.
> > > > > +	 */
> > > > > +	legacy_mdio = !of_phy_is_fixed_link(np) && !plat->phy_node;
> > > > > +	if (legacy_mdio)
> > > > > +		dev_info(dev, "Deprecated MDIO bus assumption used\n");
> > > > > +
> > > > > +	if (plat->mdio_node || legacy_mdio) {
> > > > > +		plat->mdio_bus_data = devm_kzalloc(dev,
> > > > 
> > > > Special thanks for adding the comment above this code. It will really
> > > > save time of figuring out why MDIO-bus needs to be created anyway.
> > > > 
> > > > > +						   sizeof(struct stmmac_mdio_bus_data),
> > > > 
> > > > Should v4 is required I would suggest to change this to
> > > > sizeof(*plat->mdio_bus_data).
> > > > 
> > > > Anyway feel free to add:
> > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> > > > 
> > > > -Serge(y)
> > > 
> > 
> > > Sure I will spin v4 to pick that up, thanks for catching it. I'll also
> > > improve the motivation in the commit message a hair more per Andrew
> > > Lunn's request over here on v2 (and will hold off a little bit just to
> > > make sure reviews come in before a respin):
> > > 
> > >     https://lore.kernel.org/netdev/e64b14c3-4b80-4120-8cc4-9baa40cdcb75@lunn.ch/
> > 
> > Ok. Thanks.
> > 
> > -Serge(y)
> > 
> > > 
> > > Thanks,
> > > Andrew
> > > 
> > 
>
  

On Mon, Dec 11, 2023 at 08:27:46PM +0300, Serge Semin wrote:
> On Fri, Dec 08, 2023 at 10:50:29AM -0600, Andrew Halaney wrote:
> > On Fri, Dec 08, 2023 at 06:07:06PM +0300, Serge Semin wrote:
> > > On Thu, Dec 07, 2023 at 05:07:24PM -0600, Andrew Halaney wrote:
> > > > On Fri, Dec 08, 2023 at 01:16:12AM +0300, Serge Semin wrote:
> > > > > On Thu, Dec 07, 2023 at 03:12:40PM -0600, Andrew Halaney wrote:
> > > > > > The stmmac_dt_phy() function, which parses the devicetree node of the
> > > > > > MAC and ultimately causes MDIO bus allocation, misinterprets what
> > > > > > fixed-link means in relation to the MAC's MDIO bus. This results in
> > > > > > a MDIO bus being created in situations it need not be.
> > > > > > 
> > > > > > Currently a MDIO bus is created if the description is either:
> > > > > > 
> > > > > >     1. Not fixed-link
> > > > > >     2. fixed-link but contains a MDIO bus as well
> > > > > > 
> > > > > > The "1" case above isn't always accurate. If there's a phy-handle,
> > > > > > it could be referencing a phy on another MDIO controller's bus[1]. In
> > > > > > this case currently the MAC will make a MDIO bus and scan it all
> > > > > > anyways unnecessarily.
> > > > > > 
> > > > > > There's also a lot of upstream devicetrees[2] that expect a MDIO bus to
> > > > > > be created and scanned for a phy. This case can also be inferred from
> > > > > > the platform description by not having a phy-handle && not being
> > > > > > fixed-link. This hits case "1" in the current driver's logic.
> > > > > > 
> > > > > > Let's improve the logic to create a MDIO bus if either:
> > > > > > 
> > > > > 
> > > > > >     - Devicetree contains a MDIO bus
> > > > > >     - !fixed-link && !phy-handle (legacy handling)
> > > > > 
> > > > > If what you suggest here is a free from regressions semantics change
> > > > > (really hope it is) I will be with both my hands for it. This will
> > > > > solve the problem we have with one of our device which doesn't have
> > > > > SMA interface (hardware designers decided to save ~4K gates of the
> > > > > chip area) but has a PHY externally attached to the DW XGMAC<->XPCS
> > > > > interface. PHY is accessible via a GPIO-based MDIO bus. BTW having no
> > > > > SMA interface available on a DW *MAC device but creating the MDIO-bus
> > > > > on top of the non-existent SMA CSRs anyway causes having _32_ dummy
> > > > > PHYs created with zero IDs.
> > > > 
> > > 
> > > > I hope it is regression free! I have tested both the [1] and [2] cases
> > > > (I hacked up the devicetree for [1] to make it look like [2]) without
> > > > any issue.
> > > > 
> > > 
> > > I doubt you could have tested it on all the possible hardware the
> > > STMMAC driver supports. The problem is that the DT-bindings thing is a
> > > kind of contract which can't be changed that easily. It's like ABI but
> > > for the hardware description so the kernel would bootup correctly on
> > > the platforms with the old DT blobs. But if the change isn't that
> > > critical, if the device-tree sources in the kernel fit to the updated
> > > semantics, if the networking subsystem maintainers aren't against it
> > > and I guess with the Rob, Krzysztof or Conor blessing (at least it
> > > won't hurt to add them to the Cc-list together with the devicetree
> > > mailing-list), then it will likely be accepted.
> > 
> 
> > To be clear, I don't think we're violating the dt-binding ABI contract
> > here. My intention is that all the existing use cases continue to work,
> > and this just improves one use case. I did a write up
> > over on v2 about the use cases I see and the current logic vs what
> > changes with this patch series:
> > 
> >     https://lore.kernel.org/netdev/plvbqgi2bwlv5quvpiwplq7cxx6m5rl3ghnfhuxfx4bpuhyihl@zmydwrtwdeg6/
> > 
> > Please comment if you think I have broken some backwards
> > compatibility.
> 
> To shortly sum up so I didn't miss something. Current semantics of the
> MDIO-bus registration is:
> if !fixed-link || mdio_node_present
>     create MDIO-bus
> and the semantics of the PHY auto-probe (legacy) is:
> if !(fixed-link || mdio_node_present || phy_node_present)
>     auto-probe PHY

I think phy_node_present doesn't belong in the current view of the
semantics for PHY auto-probe (legacy). This devicetree would trigger a
PHY auto-probe/scan on ethernet0's MAC's MDIO bus:

	random-mdio-bus {
		rgmii_phy: phy@0 {
		};
	};

	ethernet0 {
		phy-handle = <&rgmii_phy>;
	};

The assumption I make in this patch is that nothing useful could be on
ethernet0's MDIO bus, it certainly at least is not the phy the MAC uses.

> 
> You are changing the MDIO-bus creation semantics to:
> if !(fixed-link || phy_node_present) || mdio_node_present
>     create MDIO-bus
> with no change in the PHY auto-probe semantics:
> if !(fixed-link || mdio_node_present || phy_node_present)
>     auto-probe PHY

Unfortunately as I highlighted above this logic (while accurate to the
patch under review) is a change from the prior logic for the "auto-probe
PHY" case.

> 
> So the change is that if a PHY-handle is specified the MDIO-bus won't
> be created with the rest conditions being the same.
> 
> The only concern I had was the so called legacy case and a possibility
> to have MDIO-bus with other than PHY devices. Based on the pseudo-code
> above the former case won't be affected since having PHY-node
> specified didn't triggered MDIO-bus auto-probe even before your
> change. The later case concerns for instance the DW XPCS devices which

As I've realized in your response here, there is the possibility that
something is on the MDIO bus in the ethernet0 exmpale bus above, and would
probe, in the before handling. So I guess this isn't totally backwards
compatible. Gah, thanks for highlighting.

I'm not sure in practice if anyone out there is really relying on that
or not. I can get away with the "no auto-probe/scan of bus" optimization I
really desire by describing my MDIO bus as disabled in the devicetree
(need to send patches to do that in the dts and handle it gracefully in
stmmac). I'm wondering if I should keep forth with this patch as is, or
if I should keep the same logic but clean it up a bit as is done in the
current patch... I guess probably the latter.

> on some platforms could be found on the DW MAC MDIO bus with not
> having PHY living on that bus. But DW XPCS auto-probing currently is
> only supported by the non-OF platforms (it's Intel). Thus your change
> is supposed to be safe here too.
> 
> So to speak AFAICS from the STMMAC MDIO OF stuff your solution isn't
> supposed to cause regressions and break the current DTs backward
> compatibility indeed.
> 
> Regarding the ideal implementation. What could be much better is to
> implement the next semantics:
> if SMA-capability-detected &&
>    (!mdio_node_present || (mdio_node_present && mdio_node_enabled))
>     create MDIO-bus
> and preserve the PHY auto-probe semantics for backwards compatibility.
> Regarding the SMA-capability flag, it has been presented since DW GMAC
> v3.50, so since very much early DW MAC releases. But even for the
> early devices I think it could be auto-detected by checking the SMA
> CSRs writability. At least DW XGMAC AFAICS has the command CSR not
> writable if SMA is unavailable.
> 
> But I guess it's a matter of another patch.
> 

I like that logic for what it is worth, although would be unsure of
verifying the SMA-capability-detected part of it. But I wouldn't mind
seeing that patch :)
  

On Mon, Dec 11, 2023 at 01:48:47PM -0600, Andrew Halaney wrote:
> On Mon, Dec 11, 2023 at 08:27:46PM +0300, Serge Semin wrote:
> > On Fri, Dec 08, 2023 at 10:50:29AM -0600, Andrew Halaney wrote:
> > > On Fri, Dec 08, 2023 at 06:07:06PM +0300, Serge Semin wrote:
> > > > On Thu, Dec 07, 2023 at 05:07:24PM -0600, Andrew Halaney wrote:
> > > > > On Fri, Dec 08, 2023 at 01:16:12AM +0300, Serge Semin wrote:
> > > > > > On Thu, Dec 07, 2023 at 03:12:40PM -0600, Andrew Halaney wrote:
> > > > > > > The stmmac_dt_phy() function, which parses the devicetree node of the
> > > > > > > MAC and ultimately causes MDIO bus allocation, misinterprets what
> > > > > > > fixed-link means in relation to the MAC's MDIO bus. This results in
> > > > > > > a MDIO bus being created in situations it need not be.
> > > > > > > 
> > > > > > > Currently a MDIO bus is created if the description is either:
> > > > > > > 
> > > > > > >     1. Not fixed-link
> > > > > > >     2. fixed-link but contains a MDIO bus as well
> > > > > > > 
> > > > > > > The "1" case above isn't always accurate. If there's a phy-handle,
> > > > > > > it could be referencing a phy on another MDIO controller's bus[1]. In
> > > > > > > this case currently the MAC will make a MDIO bus and scan it all
> > > > > > > anyways unnecessarily.
> > > > > > > 
> > > > > > > There's also a lot of upstream devicetrees[2] that expect a MDIO bus to
> > > > > > > be created and scanned for a phy. This case can also be inferred from
> > > > > > > the platform description by not having a phy-handle && not being
> > > > > > > fixed-link. This hits case "1" in the current driver's logic.
> > > > > > > 
> > > > > > > Let's improve the logic to create a MDIO bus if either:
> > > > > > > 
> > > > > > 
> > > > > > >     - Devicetree contains a MDIO bus
> > > > > > >     - !fixed-link && !phy-handle (legacy handling)
> > > > > > 
> > > > > > If what you suggest here is a free from regressions semantics change
> > > > > > (really hope it is) I will be with both my hands for it. This will
> > > > > > solve the problem we have with one of our device which doesn't have
> > > > > > SMA interface (hardware designers decided to save ~4K gates of the
> > > > > > chip area) but has a PHY externally attached to the DW XGMAC<->XPCS
> > > > > > interface. PHY is accessible via a GPIO-based MDIO bus. BTW having no
> > > > > > SMA interface available on a DW *MAC device but creating the MDIO-bus
> > > > > > on top of the non-existent SMA CSRs anyway causes having _32_ dummy
> > > > > > PHYs created with zero IDs.
> > > > > 
> > > > 
> > > > > I hope it is regression free! I have tested both the [1] and [2] cases
> > > > > (I hacked up the devicetree for [1] to make it look like [2]) without
> > > > > any issue.
> > > > > 
> > > > 
> > > > I doubt you could have tested it on all the possible hardware the
> > > > STMMAC driver supports. The problem is that the DT-bindings thing is a
> > > > kind of contract which can't be changed that easily. It's like ABI but
> > > > for the hardware description so the kernel would bootup correctly on
> > > > the platforms with the old DT blobs. But if the change isn't that
> > > > critical, if the device-tree sources in the kernel fit to the updated
> > > > semantics, if the networking subsystem maintainers aren't against it
> > > > and I guess with the Rob, Krzysztof or Conor blessing (at least it
> > > > won't hurt to add them to the Cc-list together with the devicetree
> > > > mailing-list), then it will likely be accepted.
> > > 
> > 
> > > To be clear, I don't think we're violating the dt-binding ABI contract
> > > here. My intention is that all the existing use cases continue to work,
> > > and this just improves one use case. I did a write up
> > > over on v2 about the use cases I see and the current logic vs what
> > > changes with this patch series:
> > > 
> > >     https://lore.kernel.org/netdev/plvbqgi2bwlv5quvpiwplq7cxx6m5rl3ghnfhuxfx4bpuhyihl@zmydwrtwdeg6/
> > > 
> > > Please comment if you think I have broken some backwards
> > > compatibility.
> > 
> > To shortly sum up so I didn't miss something. Current semantics of the
> > MDIO-bus registration is:
> > if !fixed-link || mdio_node_present
> >     create MDIO-bus
> > and the semantics of the PHY auto-probe (legacy) is:
> > if !(fixed-link || mdio_node_present || phy_node_present)
> >     auto-probe PHY
> 

> I think phy_node_present doesn't belong in the current view of the
> semantics for PHY auto-probe (legacy). This devicetree would trigger a
> PHY auto-probe/scan on ethernet0's MAC's MDIO bus:
> 
> 	random-mdio-bus {
> 		rgmii_phy: phy@0 {
> 		};
> 	};
> 
> 	ethernet0 {
> 		phy-handle = <&rgmii_phy>;
> 	};

Em, unless I miss something, but on STMMAC it wont due to the next
statement: stmmac_mdio_register():
	if (priv->plat->phy_node || mdio_node)
		goto bus_register_done;

Note by the "PHY auto-probe (legacy)" semantics I meant the algo
implemented at the bottom of the stmmac_mdio_register() method. If no
PHY-node or MDIO-node specified it tries to find any PHY on the
DW MAC SMA MDIO bus and use it in the driver.

If what you meant was the PHY auto-probe/scan executed in the
__mdiobus_register() method, then I guess it's relevant only _if_ the
stmmac_mdio_register() method performs the legacy auto-probing too.
Yes, the MDIO/PHY subsystem _will_ scan the MDIO-bus for PHYs, but
they won't be utilized in the STMMAC driver due to the conditional
statement above. In other words I guess the scanning performed in
__mdiobus_register() will be pointless anyway in that case, since no
detected PHY-devices will be actually used afterwards.

> 
> The assumption I make in this patch is that nothing useful could be on
> ethernet0's MDIO bus, it certainly at least is not the phy the MAC uses.
> 
> > 
> > You are changing the MDIO-bus creation semantics to:
> > if !(fixed-link || phy_node_present) || mdio_node_present
> >     create MDIO-bus
> > with no change in the PHY auto-probe semantics:
> > if !(fixed-link || mdio_node_present || phy_node_present)
> >     auto-probe PHY
> 
> Unfortunately as I highlighted above this logic (while accurate to the
> patch under review) is a change from the prior logic for the "auto-probe
> PHY" case.
> 
> > 
> > So the change is that if a PHY-handle is specified the MDIO-bus won't
> > be created with the rest conditions being the same.
> > 
> > The only concern I had was the so called legacy case and a possibility
> > to have MDIO-bus with other than PHY devices. Based on the pseudo-code
> > above the former case won't be affected since having PHY-node
> > specified didn't triggered MDIO-bus auto-probe even before your
> > change. The later case concerns for instance the DW XPCS devices which
> 
> As I've realized in your response here, there is the possibility that
> something is on the MDIO bus in the ethernet0 exmpale bus above, and would
> probe, in the before handling. So I guess this isn't totally backwards
> compatible. Gah, thanks for highlighting.

AFAICS what you suggest won't break currently supported by the STMMAC
driver _platforms_. This is what actually matters. The legacy PHY
auto-probing will work as before. None of the OF-based platforms
currently expect having non-PHY devices on the bus.

> 
> I'm not sure in practice if anyone out there is really relying on that
> or not. I can get away with the "no auto-probe/scan of bus" optimization I
> really desire by describing my MDIO bus as disabled in the devicetree
> (need to send patches to do that in the dts and handle it gracefully in
> stmmac). I'm wondering if I should keep forth with this patch as is, or
> if I should keep the same logic but clean it up a bit as is done in the
> current patch... I guess probably the latter.

It's better to limit some complicated logic before somebody tries to
use it to bypass a harder but correct way of directly defining all
devices (including non-PHY ones) in dts. Seeing presumably no
currently supported platform will be broken I think you should keep
the patch as is.

-Serge(y)

> 
> > on some platforms could be found on the DW MAC MDIO bus with not
> > having PHY living on that bus. But DW XPCS auto-probing currently is
> > only supported by the non-OF platforms (it's Intel). Thus your change
> > is supposed to be safe here too.
> > 
> > So to speak AFAICS from the STMMAC MDIO OF stuff your solution isn't
> > supposed to cause regressions and break the current DTs backward
> > compatibility indeed.
> > 
> > Regarding the ideal implementation. What could be much better is to
> > implement the next semantics:
> > if SMA-capability-detected &&
> >    (!mdio_node_present || (mdio_node_present && mdio_node_enabled))
> >     create MDIO-bus
> > and preserve the PHY auto-probe semantics for backwards compatibility.
> > Regarding the SMA-capability flag, it has been presented since DW GMAC
> > v3.50, so since very much early DW MAC releases. But even for the
> > early devices I think it could be auto-detected by checking the SMA
> > CSRs writability. At least DW XGMAC AFAICS has the command CSR not
> > writable if SMA is unavailable.
> > 
> > But I guess it's a matter of another patch.
> > 
> 
> I like that logic for what it is worth, although would be unsure of
> verifying the SMA-capability-detected part of it. But I wouldn't mind
> seeing that patch :)
>
  

On Tue, Dec 12, 2023 at 01:42:35PM +0300, Serge Semin wrote:
> On Mon, Dec 11, 2023 at 01:48:47PM -0600, Andrew Halaney wrote:
> > On Mon, Dec 11, 2023 at 08:27:46PM +0300, Serge Semin wrote:
> > > On Fri, Dec 08, 2023 at 10:50:29AM -0600, Andrew Halaney wrote:
> > > > On Fri, Dec 08, 2023 at 06:07:06PM +0300, Serge Semin wrote:
> > > > > On Thu, Dec 07, 2023 at 05:07:24PM -0600, Andrew Halaney wrote:
> > > > > > On Fri, Dec 08, 2023 at 01:16:12AM +0300, Serge Semin wrote:
> > > > > > > On Thu, Dec 07, 2023 at 03:12:40PM -0600, Andrew Halaney wrote:
> > > > > > > > The stmmac_dt_phy() function, which parses the devicetree node of the
> > > > > > > > MAC and ultimately causes MDIO bus allocation, misinterprets what
> > > > > > > > fixed-link means in relation to the MAC's MDIO bus. This results in
> > > > > > > > a MDIO bus being created in situations it need not be.
> > > > > > > > 
> > > > > > > > Currently a MDIO bus is created if the description is either:
> > > > > > > > 
> > > > > > > >     1. Not fixed-link
> > > > > > > >     2. fixed-link but contains a MDIO bus as well
> > > > > > > > 
> > > > > > > > The "1" case above isn't always accurate. If there's a phy-handle,
> > > > > > > > it could be referencing a phy on another MDIO controller's bus[1]. In
> > > > > > > > this case currently the MAC will make a MDIO bus and scan it all
> > > > > > > > anyways unnecessarily.
> > > > > > > > 
> > > > > > > > There's also a lot of upstream devicetrees[2] that expect a MDIO bus to
> > > > > > > > be created and scanned for a phy. This case can also be inferred from
> > > > > > > > the platform description by not having a phy-handle && not being
> > > > > > > > fixed-link. This hits case "1" in the current driver's logic.
> > > > > > > > 
> > > > > > > > Let's improve the logic to create a MDIO bus if either:
> > > > > > > > 
> > > > > > > 
> > > > > > > >     - Devicetree contains a MDIO bus
> > > > > > > >     - !fixed-link && !phy-handle (legacy handling)
> > > > > > > 
> > > > > > > If what you suggest here is a free from regressions semantics change
> > > > > > > (really hope it is) I will be with both my hands for it. This will
> > > > > > > solve the problem we have with one of our device which doesn't have
> > > > > > > SMA interface (hardware designers decided to save ~4K gates of the
> > > > > > > chip area) but has a PHY externally attached to the DW XGMAC<->XPCS
> > > > > > > interface. PHY is accessible via a GPIO-based MDIO bus. BTW having no
> > > > > > > SMA interface available on a DW *MAC device but creating the MDIO-bus
> > > > > > > on top of the non-existent SMA CSRs anyway causes having _32_ dummy
> > > > > > > PHYs created with zero IDs.
> > > > > > 
> > > > > 
> > > > > > I hope it is regression free! I have tested both the [1] and [2] cases
> > > > > > (I hacked up the devicetree for [1] to make it look like [2]) without
> > > > > > any issue.
> > > > > > 
> > > > > 
> > > > > I doubt you could have tested it on all the possible hardware the
> > > > > STMMAC driver supports. The problem is that the DT-bindings thing is a
> > > > > kind of contract which can't be changed that easily. It's like ABI but
> > > > > for the hardware description so the kernel would bootup correctly on
> > > > > the platforms with the old DT blobs. But if the change isn't that
> > > > > critical, if the device-tree sources in the kernel fit to the updated
> > > > > semantics, if the networking subsystem maintainers aren't against it
> > > > > and I guess with the Rob, Krzysztof or Conor blessing (at least it
> > > > > won't hurt to add them to the Cc-list together with the devicetree
> > > > > mailing-list), then it will likely be accepted.
> > > > 
> > > 
> > > > To be clear, I don't think we're violating the dt-binding ABI contract
> > > > here. My intention is that all the existing use cases continue to work,
> > > > and this just improves one use case. I did a write up
> > > > over on v2 about the use cases I see and the current logic vs what
> > > > changes with this patch series:
> > > > 
> > > >     https://lore.kernel.org/netdev/plvbqgi2bwlv5quvpiwplq7cxx6m5rl3ghnfhuxfx4bpuhyihl@zmydwrtwdeg6/
> > > > 
> > > > Please comment if you think I have broken some backwards
> > > > compatibility.
> > > 
> > > To shortly sum up so I didn't miss something. Current semantics of the
> > > MDIO-bus registration is:
> > > if !fixed-link || mdio_node_present
> > >     create MDIO-bus
> > > and the semantics of the PHY auto-probe (legacy) is:
> > > if !(fixed-link || mdio_node_present || phy_node_present)
> > >     auto-probe PHY
> > 
> 
> > I think phy_node_present doesn't belong in the current view of the
> > semantics for PHY auto-probe (legacy). This devicetree would trigger a
> > PHY auto-probe/scan on ethernet0's MAC's MDIO bus:
> > 
> > 	random-mdio-bus {
> > 		rgmii_phy: phy@0 {
> > 		};
> > 	};
> > 
> > 	ethernet0 {
> > 		phy-handle = <&rgmii_phy>;
> > 	};
> 
> Em, unless I miss something, but on STMMAC it wont due to the next
> statement: stmmac_mdio_register():
> 	if (priv->plat->phy_node || mdio_node)
> 		goto bus_register_done;
> 
> Note by the "PHY auto-probe (legacy)" semantics I meant the algo
> implemented at the bottom of the stmmac_mdio_register() method. If no
> PHY-node or MDIO-node specified it tries to find any PHY on the
> DW MAC SMA MDIO bus and use it in the driver.
> 
> If what you meant was the PHY auto-probe/scan executed in the
> __mdiobus_register() method, then I guess it's relevant only _if_ the
> stmmac_mdio_register() method performs the legacy auto-probing too.
> Yes, the MDIO/PHY subsystem _will_ scan the MDIO-bus for PHYs, but
> they won't be utilized in the STMMAC driver due to the conditional
> statement above. In other words I guess the scanning performed in
> __mdiobus_register() will be pointless anyway in that case, since no
> detected PHY-devices will be actually used afterwards.
> 

Yes, I was referring to the __mdiobus_register() bit, and I now see what
you mean. So in the example I painted above the stmmac_mdio_register()
auto-probing would not happen since the phy-handle exists (and therefore
phy_node).

So the only change here is that the __mdiobus_register() scan wouldn't
happen, but since stmmac is the only MAC that would attach to a phy on
that bus it doesn't matter (the phy is already acquired by the
phy-handle).

> > 
> > The assumption I make in this patch is that nothing useful could be on
> > ethernet0's MDIO bus, it certainly at least is not the phy the MAC uses.
> > 
> > > 
> > > You are changing the MDIO-bus creation semantics to:
> > > if !(fixed-link || phy_node_present) || mdio_node_present
> > >     create MDIO-bus
> > > with no change in the PHY auto-probe semantics:
> > > if !(fixed-link || mdio_node_present || phy_node_present)
> > >     auto-probe PHY
> > 
> > Unfortunately as I highlighted above this logic (while accurate to the
> > patch under review) is a change from the prior logic for the "auto-probe
> > PHY" case.
> > 
> > > 
> > > So the change is that if a PHY-handle is specified the MDIO-bus won't
> > > be created with the rest conditions being the same.
> > > 
> > > The only concern I had was the so called legacy case and a possibility
> > > to have MDIO-bus with other than PHY devices. Based on the pseudo-code
> > > above the former case won't be affected since having PHY-node
> > > specified didn't triggered MDIO-bus auto-probe even before your
> > > change. The later case concerns for instance the DW XPCS devices which
> > 
> > As I've realized in your response here, there is the possibility that
> > something is on the MDIO bus in the ethernet0 exmpale bus above, and would
> > probe, in the before handling. So I guess this isn't totally backwards
> > compatible. Gah, thanks for highlighting.
> 
> AFAICS what you suggest won't break currently supported by the STMMAC
> driver _platforms_. This is what actually matters. The legacy PHY
> auto-probing will work as before. None of the OF-based platforms
> currently expect having non-PHY devices on the bus.

I agree, you've talked me off the edge of the cliff. Thanks so much for
your help in all this.

> 
> > 
> > I'm not sure in practice if anyone out there is really relying on that
> > or not. I can get away with the "no auto-probe/scan of bus" optimization I
> > really desire by describing my MDIO bus as disabled in the devicetree
> > (need to send patches to do that in the dts and handle it gracefully in
> > stmmac). I'm wondering if I should keep forth with this patch as is, or
> > if I should keep the same logic but clean it up a bit as is done in the
> > current patch... I guess probably the latter.
> 
> It's better to limit some complicated logic before somebody tries to
> use it to bypass a harder but correct way of directly defining all
> devices (including non-PHY ones) in dts. Seeing presumably no
> currently supported platform will be broken I think you should keep
> the patch as is.

Agreed. I'll send a new version this evening with your tags, Andrew
Lunn's requested motivation in commit message, etc. Thanks!

> 
> -Serge(y)
> 
> > 
> > > on some platforms could be found on the DW MAC MDIO bus with not
> > > having PHY living on that bus. But DW XPCS auto-probing currently is
> > > only supported by the non-OF platforms (it's Intel). Thus your change
> > > is supposed to be safe here too.
> > > 
> > > So to speak AFAICS from the STMMAC MDIO OF stuff your solution isn't
> > > supposed to cause regressions and break the current DTs backward
> > > compatibility indeed.
> > > 
> > > Regarding the ideal implementation. What could be much better is to
> > > implement the next semantics:
> > > if SMA-capability-detected &&
> > >    (!mdio_node_present || (mdio_node_present && mdio_node_enabled))
> > >     create MDIO-bus
> > > and preserve the PHY auto-probe semantics for backwards compatibility.
> > > Regarding the SMA-capability flag, it has been presented since DW GMAC
> > > v3.50, so since very much early DW MAC releases. But even for the
> > > early devices I think it could be auto-detected by checking the SMA
> > > CSRs writability. At least DW XGMAC AFAICS has the command CSR not
> > > writable if SMA is unavailable.
> > > 
> > > But I guess it's a matter of another patch.
> > > 
> > 
> > I like that logic for what it is worth, although would be unsure of
> > verifying the SMA-capability-detected part of it. But I wouldn't mind
> > seeing that patch :)
> > 
>