Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1649666

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

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: [PATCH v2] drivers: phy: Add Cortina CS4340 driver Andrew Lunn <andrew@lunn.ch> - 2017-05-24 15:10 +0200
  RE: [PATCH v2] drivers: phy: Add Cortina CS4340 driver Bogdan Purcareata <bogdan.purcareata@nxp.com> - 2017-05-24 16:00 +0200
    Re: [PATCH v2] drivers: phy: Add Cortina CS4340 driver Andrew Lunn <andrew@lunn.ch> - 2017-05-24 16:30 +0200
      RE: [PATCH v2] drivers: phy: Add Cortina CS4340 driver Bogdan Purcareata <bogdan.purcareata@nxp.com> - 2017-05-24 16:40 +0200
        Re: [PATCH v2] drivers: phy: Add Cortina CS4340 driver Florian Fainelli <f.fainelli@gmail.com> - 2017-05-24 19:10 +0200
          RE: [PATCH v2] drivers: phy: Add Cortina CS4340 driver Bogdan Purcareata <bogdan.purcareata@nxp.com> - 2017-05-24 19:10 +0200

csiph-web