RE: [PATCH v2] drivers: phy: Add Cortina CS4340 driver

[Bogdan Purcareata]

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@xxxxxxx]
> Sent: Wednesday, May 24, 2017 5:26 PM
> To: Bogdan Purcareata <bogdan.purcareata@xxxxxxx>
> Cc: f.fainelli@xxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2] drivers: phy: Add Cortina CS4340 driver
> 
> > Yes, I can do that. However, I don't see a "phy" folder under
> Documentation/devicetree/bindings/net/.
> > Should I change the location to
> Documentation/devicetree/bindings/net/cortina.txt instead?
> 
> Ah, sorry, my error. Yes,
> Documentation/devicetree/bindings/net/cortina.txt.
> 
> > > I think there needs to be some explanation here. What exactly are you
> > > using to indicate link up? What does CORTINA_GPIO_GPIO_INTS mean?
> >
> > I can only assume it's the location of an interrupt status register. The
> Cortina PHYs go mostly undocumented, I've found an implementation in an old
> thread [1], that also did the job of programming the Cortina PHY microcode.
> I understand that microcode programming is now part of the bootloader.
> >
> > The register seems to provide the link status properly, and based on that
> the phydev is initialized with the only (currently) supported
> configuration, 10Gbps full duplex.
> >
> > I can change the register name to something more meaningful, though -
> e.g. CORTINA_LINK_STATUS.
> 
> No, leave the name as is. The MIPS driver you gave a reference to
> seems to be using sensible names. My guess is, the boot loader is
> configuring the PHY to generate an interrupt on link up via one of its
> GPIO lines. And this code is reading that GPIO status. This is all
> very fragile, you are making a lot of assumptions.
> 
> Have you tested pulling the cable out? And plugging it back in again?
> There is a chance this interrupt is "link change", not "link up".

No, I have not. I had it planned but it somehow slipped. Will test.

> What i do like in that mips code is the probe function verifies the
> product ID.
> 
> +       /*
> +        * CS4312 keeps its ID values in non-standard registers, make
> +        * sure we are talking to what we think we are.
> +        */
> +       id_lsb = cs4321_phy_read(phydev, CS4321_GLOBAL_CHIP_ID_LSB);
> +       if (id_lsb < 0) {
> +               ret = id_lsb;
> +               goto err;
> +       }
> +
> +       id_msb = cs4321_phy_read(phydev, CS4321_GLOBAL_CHIP_ID_MSB);
> +       if (id_msb < 0) {
> +               ret = id_msb;
> +               goto err;
> +       }
> +
> +       if (id_lsb != 0x23E5 || id_msb != 0x1002) {
> +               ret = -ENODEV;
> +               goto err;
> +       }
> 
> You should do that as well.

Okay, I will include this check in a probe function in v3.

Thanks a lot!
Bogdan