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;
MIME-Version: 1.0
References: <20230808120254.11653-1-brgl@bgdev.pl> <82cd26a3-e63d-4251-9d43-d1d7443b9cce@lunn.ch>
In-Reply-To: <82cd26a3-e63d-4251-9d43-d1d7443b9cce@lunn.ch>
From:   Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Tue, 8 Aug 2023 20:51:01 +0200
Message-ID: <CAMRc=MfDtCj0ML-FQH8-Cm23YupOnmScBqKDrLExqSjqeHhOSg@mail.gmail.com>
Subject: Re: [PATCH net-next] net: stmmac: don't create the MDIO bus if
 there's no mdio node on DT
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Alexandre Torgue <alexandre.torgue@foss.st.com>,
        Jose Abreu <joabreu@synopsys.com>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        Alex Elder <elder@linaro.org>,
        Srini Kandagatla <srinivas.kandagatla@linaro.org>,
        Andrew Halaney <ahalaney@redhat.com>, netdev@vger.kernel.org,
        linux-stm32@st-md-mailman.stormreply.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Aug 8, 2023 at 8:46=E2=80=AFPM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Aug 08, 2023 at 02:02:54PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > The stmmac_dt_phy() function that parses the device-tree node of the MA=
C
> > and allocates the MDIO and PHY resources misses one use-case: when the
> > MAC doesn't have a fixed link but also doesn't define its own mdio bus
> > on the device tree and instead shares the MDIO lines with a different
> > MAC with its PHY phandle reaching over into a different node.
>
> It does not share the MDIO lines. The other MDIO bus master happens to
> have two PHYs and there are no PHYs on this MDIO bus, so no point
> instantiating it.

Yes, I sent it before we established that thanks to Andrew's input.

>
> >  static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
> >                        struct device_node *np, struct device *dev)
> >  {
> > -     bool mdio =3D !of_phy_is_fixed_link(np);
> >       static const struct of_device_id need_mdio_ids[] =3D {
> >               { .compatible =3D "snps,dwc-qos-ethernet-4.10" },
> >               {},
> >       };
> >
> > +     if (of_phy_is_fixed_link(np))
> > +             return 0;
> > +
>
>                 /**
>                  * 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,
>                                                     "snps,dwmac-mdio"))
>                                 break;
>                 }
>
> The comment suggests it should always be registered. This MAC might
> have a fixed-phy, but that does not mean there is not an Ethernet
> switch on the bus, or a PHY for some other MAC etc. MDIO busses
> masters should be considered fully independent devices.
>
> https://elixir.bootlin.com/linux/v6.5-rc5/source/arch/arm/boot/dts/nxp/vf=
/vf610-zii-ssmb-dtu.dts
>
> &fec1 {
>         phy-mode =3D "rmii";
>         pinctrl-names =3D "default";
>         pinctrl-0 =3D <&pinctrl_fec1>;
>         status =3D "okay";
>
>         fixed-link {
>                 speed =3D <100>;
>                 full-duplex;
>         };
>
>         mdio1: mdio {
>                 #address-cells =3D <1>;
>                 #size-cells =3D <0>;
>                 clock-frequency =3D <12500000>;
>                 suppress-preamble;
>                 status =3D "okay";
>
>                 switch0: switch0@0 {
>                         compatible =3D "marvell,mv88e6190";
>                         pinctrl-0 =3D <&pinctrl_gpio_switch0>;
>                         pinctrl-names =3D "default";
>                         reg =3D <0>;
>                         eeprom-length =3D <65536>;
>                         interrupt-parent =3D <&gpio3>;
>                         interrupts =3D <2 IRQ_TYPE_LEVEL_LOW>;
>                         interrupt-controller;
>                         #interrupt-cells =3D <2>;
>
> Both a fixed link, and something on the MDIO bus....
>
>      Andrew

Makes sense, we can drop all my stmmac patches from today, I need to
rethink it in detail.

Bart