Re: [Upstream] [PATCH] arm64: dts: freescale: imx93-phycore-som: Delay the phy reset by a gpio

[Christoph Stoidner <C.Stoidner@phytec.de>]

Hi Primoz,

On Di, 2025-05-20 at 09:56 +0200, Primoz Fiser wrote:
> Hi Christoph,
> 
> On 20. 05. 25 09:34, Christoph Stoidner wrote:
> > According to the datasheet the phy needs to be held in reset until
> > the
> > reference clock got stable. Even though no issue was observed, fix
> > this
> > as the software should always comply with the specification.
> > 
> > Fix this in the bootloader, as this is the point where the
> > reference
> > clock gets initialized and stable first.
> 
> I would remove this paragraph about the "Fix this in the
> bootloader..."
> 
> Doesn't patch apply to both the kernel and the bootloader FEC driver?

You are right. I missed that the kernel driver also has implemented
that phy-reset-gpios handling. I will remove that paragraph in a v2.

> 
> > 
> > Use gpio4 23, which is connected to the phy reset pin. On the same
> > pin
> > RX_ER was used before, but this signal is optional and can be
> > dropped.
> > 
> > Note: This comes into effect with the phyCOREs SOM hardware
> > revision 4.
> > In revisions before, this gpio is not connected, and the phy reset
> > is
> > managed with the global hardware reset circuit.
> > 
> > Signed-off-by: Christoph Stoidner <c.stoidner@phytec.de>
> > ---
> >  arch/arm64/boot/dts/freescale/imx93-phycore-som.dtsi | 11
> > ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/imx93-phycore-som.dtsi
> > b/arch/arm64/boot/dts/freescale/imx93-phycore-som.dtsi
> > index 88c2657b50e6..f8e2f3f3baa8 100644
> > --- a/arch/arm64/boot/dts/freescale/imx93-phycore-som.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx93-phycore-som.dtsi
> > @@ -58,6 +58,9 @@ &fec {
> >   <&clk IMX93_CLK_SYS_PLL_PFD1_DIV2>,
> >   <&clk IMX93_CLK_SYS_PLL_PFD1_DIV2>;
> >   assigned-clock-rates = <100000000>, <50000000>, <50000000>;
> > + phy-reset-gpios = <&gpio4 23 GPIO_ACTIVE_HIGH>;
> > + phy-reset-duration = <1>;
> > + phy-reset-post-delay = <0>;
> >   status = "okay";
> >  
> >   mdio: mdio {
> > @@ -91,14 +94,16 @@ pinctrl_fec: fecgrp {
> >   fsl,pins = <
> >   MX93_PAD_ENET2_MDC__ENET1_MDC 0x50e
> >   MX93_PAD_ENET2_MDIO__ENET1_MDIO 0x502
> > - MX93_PAD_ENET2_RD0__ENET1_RGMII_RD0 0x57e
> > - MX93_PAD_ENET2_RD1__ENET1_RGMII_RD1 0x57e
> > - MX93_PAD_ENET2_RXC__ENET1_RX_ER 0x5fe
> > + /* the three pins below are connected to PHYs straps,
> > + * that is what the pull-up/down setting is for. */
> 
> I would remove this comment and maybe move it to the commit msg why
> are
> you changing the PD/PU configuration.

I think it is important to know why these PD/PU are configured, because
for the normal MDIO functionality PU/PD is not needed. And it is not
self explaining here, that changing those PD/PU might break phy config.

However I will of course fix the warning in a v2.

Thanks,
Christoph

> 
> Anyway, if you decide to keep it, you need to fix the following
> warning:
> 
> WARNING: Block comments use a trailing */ on a separate line
> #46: FILE: arch/arm64/boot/dts/freescale/imx93-phycore-som.dtsi:98:
> +                        * that is what the pull-up/down setting is
> for. */
> 
> BR,
> Primoz
> 
> > + MX93_PAD_ENET2_RD0__ENET1_RGMII_RD0 0x37e
> > + MX93_PAD_ENET2_RD1__ENET1_RGMII_RD1 0x37e
> >   MX93_PAD_ENET2_RX_CTL__ENET1_RGMII_RX_CTL 0x57e
> >   MX93_PAD_ENET2_TD0__ENET1_RGMII_TD0 0x50e
> >   MX93_PAD_ENET2_TD1__ENET1_RGMII_TD1 0x50e
> >   MX93_PAD_ENET2_TX_CTL__ENET1_RGMII_TX_CTL 0x50e
> >   MX93_PAD_ENET2_TD2__ENET1_TX_CLK 0x4000050e
> > + MX93_PAD_ENET2_RXC__GPIO4_IO23 0x51e
> >   >;
> >   };
> >  
>