[PATCH v1] net: sun8i-emac: Add support for fixed-link phy

[Andre Przywara]

On Fri, 19 Jan 2024 21:11:07 +0300
Maxim Kiselev <bigunclemax at gmail.com> wrote:

Hi Maxim,

> пт, 19 янв. 2024 г. в 20:35, Andre Przywara <andre.przywara at arm.com>:
> >
> > On Tue, 16 Jan 2024 19:58:56 +0300
> > Maxim Kiselev <bigunclemax at gmail.com> wrote:
> >
> > Hi Maxim,
> >  
> > > вт, 16 янв. 2024 г. в 03:18, Andre Przywara <andre.przywara at arm.com>:  
> > > >
> > > > On Thu, 15 Jun 2023 00:44:06 +0300
> > > > Maxim Kiselev <bigunclemax at gmail.com> wrote:
> > > >
> > > > Hi Maxim,
> > > >  
> > > > > From: Maksim Kiselev <bigunclemax at gmail.com>
> > > > >
> > > > > Based on dt-specs fixed-link doesn't require phy-handle to be used.  
> > > >
> > > > Do you have such a board?  
> > >
> > > Yes, I had a custom board with T113 SoC which has fixed phy eth.  
> >
> > So just to clarify: this board connected something like a switch directly
> > to the MAC, just using RMII/RGMII? And then you have a fixed-phy node in
> > the DT, detailing the speed and duplex parameters? Matching the fixed-phy
> > node description in ethernet-controller.yaml? And drivers/net/phy/fixed.c
> > takes care of parsing that?  
> 
> Yes, you are absolutely right. The T113s connected directly to the switch port
> via RMII interface. Here is the part of DT, that describes that configuration

Excellent, thanks for the confirmation!

Looking forward to v2!

Cheers,
Andre

> &emac {
>        pinctrl-names = "default";
>        pinctrl-0 = <&rmii_pg_pins>;
>        phy-mode = "rmii";
>        snps,reset-gpio = <&pio 4 12 GPIO_ACTIVE_LOW>; /* PE12 */
>        allwinner,tx-delay-ps = <700>;
>        allwinner,rx-delay-ps = <3100>;
> 
>        fixed-link {
>                speed = <100>;
>                full-duplex;
>        };
> };
> 
> >  
> > > > And where is that written down? I don't see
> > > > it explicitly mentioned as optional in ethernet-controller.yaml or in
> > > > the DT spec PDF.  
> > >
> > > Sorry for that commit description. I just found the similar commit, that
> > > fixes the same problem for zynq_gem
> > > 3888c8d1979289efe685fe29276aed4d4b685975 ("net: zynq_gem: Add support for
> > > fixed-link phy") and copied the description from there.
> > >  
> > > > The sun8i EMAC binding lists phy-handle as required,
> > > > so that would need to be relaxed there then.  
> > >
> > > Oh, indeed. So it will require to send dt-binding changes to Linux first...
> > >
> > > Therefore, I think we can live without fixed-phy support for sun8i EMAC.
> > > At least until some device with such a configuration appears on the
> > > market :)  
> >
> > Well, I am fine with that patch if it fixes a problem for you. It looks
> > like requiring the phy-handle property is too strict, and just needs to be
> > relaxed in the binding. If I see this correctly, the driver would still
> > accept every valid DT, by today's and future bindings?
> >
> > If you could just confirm my above assumptions, and maybe send a v2 with an
> > amended commit message, I would be happy to merge that patch.  
> 
> Sure, I'll fix the commit description and send v2 asap.
> 
> > Cheers,
> > Andre
> >  
> > > > > Fix driver to only read phy related setting when phy-handle is found.  
> > > >
> > > > The patch itself looks fine, we already specify -1 as the default when
> > > > the PHY DT node does not contain a reg property, so that looks like it
> > > > would work.
> > > >
> > > > Cheers,
> > > > Andre
> > > >  
> > > > > Signed-off-by: Maksim Kiselev <bigunclemax at gmail.com>
> > > > > ---
> > > > >  drivers/net/sun8i_emac.c | 7 ++-----
> > > > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
> > > > > index 04c3274fbe..0e339d69e0 100644
> > > > > --- a/drivers/net/sun8i_emac.c
> > > > > +++ b/drivers/net/sun8i_emac.c
> > > > > @@ -834,11 +834,8 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
> > > > >       priv->use_internal_phy = false;
> > > > >
> > > > >       offset = fdtdec_lookup_phandle(gd->fdt_blob, node, "phy-handle");
> > > > > -     if (offset < 0) {
> > > > > -             debug("%s: Cannot find PHY address\n", __func__);
> > > > > -             return -EINVAL;
> > > > > -     }
> > > > > -     priv->phyaddr = fdtdec_get_int(gd->fdt_blob, offset, "reg", -1);
> > > > > +     if (offset >= 0)
> > > > > +             priv->phyaddr = fdtdec_get_int(gd->fdt_blob, offset, "reg", -1);
> > > > >
> > > > >       pdata->phy_interface = dev_read_phy_mode(dev);
> > > > >       debug("phy interface %d\n", pdata->phy_interface);  
> > > >  
> > >
> > > Best wishes,
> > > Maksim  
> >  
> 
> Best wishes,
> Maksim