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

Quentin Schulz
2018-10-31T08:20:10Z

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@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>