[U-Boot] [PATCH 1/3] net: phy: mscc: add support for VSC8584 PHY

[Quentin Schulz]

Hi Joe,

On Mon, Oct 22, 2018 at 09:52:38PM +0000, Joe Hershberger wrote:
> On Fri, Sep 14, 2018 at 7:50 AM Quentin Schulz
> <quentin.schulz at bootlin.com> wrote:
[...]
> > +#define MEDIA_OP_MODE_MASK               0x0700
> 
> Please use GENMASK() or BIT() for masks.
> 
[...]
> > +/* Extended page GPIO register 00G */
> > +#define MSCC_DW8051_CNTL_STATUS                0
> > +#define MICRO_NSOFT_RESET              0x8000
> 
> Bit definitions such as these should use BIT().
> 

It was purposely done as to keep consistency throughout the driver as
it's not using any GENMASK or BIT currently. I suppose that I should
just not care about that?

[...]

> > +       bus->write(bus, phy0, MDIO_DEVAD_NONE, MSCC_PHY_TEST_PAGE_8, reg);
> > +
> > +       bus->write(bus, phy0, MDIO_DEVAD_NONE, MSCC_EXT_PAGE_ACCESS,
> > +                  MSCC_PHY_PAGE_TR);
> > +
> > +       bus->write(bus, phy0, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_ADDR_16, 0xafa4);
> > +
> > +       reg = bus->read(bus, phy0, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_18);
> > +       reg &= ~0x007f;
> 
> Use GENMASK() and #define
> 
> > +       reg |= 0x0019;
> 
> Magic number... use #define.
> 

I've got no more information from the vendor on those ones. I wish I
could define them something else than MSCC_PHY_REG_TR_DATA_18_MASK_FOO
and MSCC_PHY_REG_TR_DATA_18_BAR.

> > +       bus->write(bus, phy0, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_18, reg);
> > +
> > +       bus->write(bus, phy0, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_ADDR_16, 0x8fa4);
> 
> Magic number... use #define
> 

It's representing an address, I have no other information than that
unfortunately.

> > +
> 
> Comment what this is doing and link the errata that it came from...
> 

No information on the errata (if there's even one). Directly taken from
the baremetal BSP, AFAICT it's optimized electrical settings given by
hardware engineers.

> > +       vsc8584_csr_write(bus, phy0, 0x07fa, 0x0050100f);
> > +       vsc8584_csr_write(bus, phy0, 0x1688, 0x00049f81);
> > +       vsc8584_csr_write(bus, phy0, 0x0f90, 0x00688980);
> > +       vsc8584_csr_write(bus, phy0, 0x03a4, 0x0000d8f0);
> > +       vsc8584_csr_write(bus, phy0, 0x0fc0, 0x00000400);
[...]

Thanks,
Quentin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181031/c2d49e1f/attachment.sig>