> -----Original Message-----
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Wednesday, May 24, 2017 5:26 PM
> To: Bogdan Purcareata <bogdan.purcare...@nxp.com>
> Cc: f.faine...@gmail.com; netdev@vger.kernel.org; linux-
> ker...@vger.kernel.org
> 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
