netdev - Re: [RFC PATCH 01/10] net: phy: Add driver for Loongson PHY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 21 Jul 2023 11:31:08 +0800
From: Feiyang Chen <chris.chenfeiyang@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Feiyang Chen <chenfeiyang@...ngson.cn>, hkallweit1@...il.com, peppe.cavallaro@...com, 
	alexandre.torgue@...s.st.com, joabreu@...opsys.com, chenhuacai@...ngson.cn, 
	linux@...linux.org.uk, dongbiao@...ngson.cn, 
	loongson-kernel@...ts.loongnix.cn, netdev@...r.kernel.org, 
	loongarch@...ts.linux.dev
Subject: Re: [RFC PATCH 01/10] net: phy: Add driver for Loongson PHY

On Mon, Jul 17, 2023 at 8:22 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> > > > > > +#define PHY_ID_LS7A2000              0x00061ce0
> > > > >
> > > > > What is Loongson OUI?
> > > > >
> > > >
> > > > Currently, we do not have an OUI for Loongson, but we are in the
> > > > process of applying for one.
> > >
> > > Is the value 0x00061ce0 baked into the silicon? Or can it be changed
> > > once you have an OUI?
> > >
> >
> > Hi, Andrew,
> >
> > The value is baked into the silicon.
>
> O.K. Thanks. Do you have an idea how long it will take to get an OUI?
>
> Does the PCI ID uniquely identify the MAC+PHY combination?
>

Hi, Andrew,

Sorry, I currently don't have an exact timeline for when the OUI will
be available. The next hardware version will address these bugs, so
we won't be going with this driver.

> > > > The PHY itself supports half-duplex, but there are issues with the
> > > > controller used in the 7A2000 chip. Moreover, the controller only
> > > > supports auto-negotiation for gigabit speeds.
> > >
> > > So you can force 10/100/1000, but auto neg only succeeds for 1G?
> > >
> > > Are the LP autoneg values available for genphy_read_lpa() to read? If
> > > the LP values are available, maybe the PHY driver can resolve the
> > > autoneg for 10 an 100.
> > >
> >
> > I apologize for the confusion caused by my previous description. To
> > clarify, the PHY supports auto-negotiation and non-auto-negotiation
> > for 10/100, but non-auto-negotiation for 1000 does not work correctly.
>
> So you can force 10/100, but auto-neg 10/100/1000.
>
> So i would suggest .get_features() indicates normal 10/100/1000
> operation. Have your .config_aneg function which is used for both
> auto-neg and forced configuration check for phydev->autoneg ==
> AUTONEG_DISABLE and phydev->speed == SPEED_1000 and return
> -EOPNOTSUPP. Otherwise call genphy_config_aneg().
>

Well, can I return -EINVAL in the .set_link_ksettings callback?
Considering that our next hardware version will have the OUI
allocated and these bugs fixed, I won't submit this driver in
the next patch version. I believe we can just use the generic
PHY for now.

> > > So what does genphy_read_abilities() return?
> > >
> > > Nobody else going to use PHY_LOONGSON_FEATURES, so i would prefer not
> > > to add it to the core. If your PHY is designed correctly,
> > > genphy_read_abilities() should determine what the PHY can do from its
> > > registers. If the registers are wrong, it is better to add a
> > > .get_features function.
> > >
> >
> > genphy_read_abilities() returns 0, and it seems to be working fine.
> > The registers are incorrect, so I will use the .get_features function
> > to clear some of the half-duplex bits.
>
> You said above that the PHY supports half duplex, the MAC has problems
> with half duplex. So it should be the MAC which indicates it does not
> unsupported half duplex link modes.
>
> So you need to modify phylink_config.mac_capabilities before
> phylink_create() is called. There is already
>
>         /* Half-Duplex can only work with single queue */
>         if (priv->plat->tx_queues_to_use > 1)
>                 priv->phylink_config.mac_capabilities &=
>                         ~(MAC_10HD | MAC_100HD | MAC_1000HD);
>
> so you just need to add a quirk for your broken hardware.
>

OK.

Thanks,
Feiyang

>         Andrew
>