Hi Andrew,
On Thu, Apr 13, 2023 at 05:45:13PM +0200, Andrew Lunn wrote:
> Hi Ladislav
>
> For phylink questions, it is a good idea to Cc: the phylink
> Maintainer. And for general PHY problems, Cc: the phy Maintainers.
>
> On Thu, Apr 13, 2023 at 04:11:07PM +0200, Ladislav Michl wrote:
> > The purpose of this patches is to provide support for SFP cage to
> > Octeon ethernet driver. This is tested with following DT snippet:
> >
> > smi0: mdio@1180000001800 {
> > compatible = "cavium,octeon-3860-mdio";
> > #address-cells = <1>;
> > #size-cells = <0>;
> > reg = <0x11800 0x00001800 0x0 0x40>;
> >
> > /* QSGMII PHY */
> > phy0: ethernet-phy@0 {
> > compatible = "marvell,88e154", "ethernet-phy-ieee802.3-c22";
>
> Please don't use a compatible for the specific PHY. In fact,
> compatibles are only used for things which are not PHYs, like Ethernet
> switches. phylib reads the ID registers of the PHY and uses them to
> load the correct PHY driver.
>
> Also, C22 is the default, so you don't need that either.
Thanks, it works equally well with compatible removed.
> > reg = <0>;
> > interrupt-parent = <&gpio>;
> > interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
> > marvell,reg-init =
> > <0xff 24 0 0x2800>, <0xff 23 0 0x2001>, /* errata 3.1.1 - PHY Initialization #1 */
> > <0 29 0 3>, <0 30 0 2>, <0 29 0 0>, /* errata 3.1.2 - PHY Initialization #2 */
>
> Please add C code to deal with these erratas in the marvell PHY
> driver.
I need to dig into 4.9 ventor (and custom) tree for them. Will do later.
> > <4 26 0 0x2>, /* prekrizeni RX a TX QSGMII sbernice */
> > <4 0 0x1000 0x1000>, /* Q_ANEG workaround: P4R0B12 = 1 */
> > <3 16 0 0x1117>; /* nastavení LED: G=link+act, Y=1Gbit */
> > };
>
> Comments are normally in English. The last one seems to be setting the
> LED. This is tolerated, but not ideal. It is not clear to me what the
> other two do.
I'm sorry for that. I took DT from local tree. It is not meant for upstream.
> > pip: pip@11800a0000000 {
> > compatible = "cavium,octeon-3860-pip";
> > #address-cells = <1>;
> > #size-cells = <0>;
> > reg = <0x11800 0xa0000000 0x0 0x2000>;
> >
> > /* Interface 0 goes to SFP */
> > interface@0 {
> > compatible = "cavium,octeon-3860-pip-interface";
> > #address-cells = <1>;
> > #size-cells = <0>;
> > reg = <0>; /* interface */
> >
> > ethernet@0 {
> > compatible = "cavium,octeon-3860-pip-port";
> > reg = <0>; /* Port */
> > local-mac-address = [ 00 00 00 00 00 00 ];
> > managed = "in-band-status";
> > phy-connection-type = "1000base-x";
> > sfp = <&sfp>;
> > };
> > };
>
> > /* Interface 1 goes to eth1-eth4 and is QSGMII */
> > interface@1 {
> > compatible = "cavium,octeon-3860-pip-interface";
> > #address-cells = <1>;
> > #size-cells = <0>;
> > reg = <1>; /* interface */
> >
> > ethernet@0 {
> > compatible = "cavium,octeon-3860-pip-port";
> > reg = <0>; /* Port */
> > local-mac-address = [ 00 00 00 00 00 00 ];
> > phy-handle = <&phy0>;
>
> If this is a QSGMII link, don't you need phy-mode property?
I would normally need that, but the way how this driver works makes it
optional.
Interfaces and their types are hardwired as well as various register address.
In the ideal world they should come from DT instead of from various
OCTEON_IS_MODEL(OCTEON_CNXXXX) ifdefery.
> > However testing revealed some glitches:
> > 1. driver previously returned -EPROBE_DEFER when no phy was attached.
> > Phylink stack does not seem to do so, which end up with:
> >
> > Marvell PHY driver as a module:
> > octeon_ethernet 11800a0000000.pip eth0: configuring for inband/1000base-x link mode
> > octeon_ethernet 11800a0000000.pip eth1: PHY [8001180000001800:00] driver [Generic PHY] (irq=POLL)
> > octeon_ethernet 11800a0000000.pip eth1: configuring for phy/sgmii link mode
> > octeon_ethernet 11800a0000000.pip eth2: PHY [8001180000001800:01] driver [Generic PHY] (irq=POLL)
> > octeon_ethernet 11800a0000000.pip eth2: configuring for phy/sgmii link mode
> > octeon_ethernet 11800a0000000.pip eth0: switched to inband/sgmii link mode
> > octeon_ethernet 11800a0000000.pip eth0: PHY [i2c:sfp:16] driver [Marvell 88E1111] (irq=POLL)
> > octeon_ethernet 11800a0000000.pip eth3: PHY [8001180000001800:02] driver [Marvell 88E1340S] (irq=25)
> > octeon_ethernet 11800a0000000.pip eth3: configuring for phy/sgmii link mode
> > octeon_ethernet 11800a0000000.pip eth4: PHY [8001180000001800:03] driver [Marvell 88E1340S] (irq=25)
> > octeon_ethernet 11800a0000000.pip eth4: configuring for phy/sgmii link mode
> > octeon_ethernet 11800a0000000.pip eth1: Link is Up - 100Mbps/Full - flow control off
> >
> > Marvell PHY driver built-in:
> > octeon_ethernet 11800a0000000.pip eth0: configuring for inband/1000base-x link mode
> > octeon_ethernet 11800a0000000.pip eth1: PHY [8001180000001800:00] driver [Marvell 88E1340S] (irq=25)
> > octeon_ethernet 11800a0000000.pip eth1: configuring for phy/sgmii link mode
> > Error: Driver 'Marvell 88E1101' is already registered, aborting...
> > libphy: Marvell 88E1101: Error -16 in registering driver
> > Error: Driver 'Marvell 88E1101' is already registered, aborting...
> > libphy: Marvell 88E1101: Error -16 in registering driver
>
> This is very odd. But it could be a side effect of the
> compatible. Please try with it removed.
That does not make any difference.
> > 2. It is not possible to call phylink_create from ndo_init callcack as
> > it evetually calls sfp_bus_add_upstream which calls rtnl_lock().
> > As this lock is already taken, it just deadlocks. Is this an unsupported
> > scenario?
>
> You normally call phylink_create() in _probe().
Ok, thank you.
> Andrew
