Re: [PATCH v3 20/22] net: phy: Add basic driver for MV88E6XXX switches from Marvell

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Mon, Oct 15, 2018 at 2:19 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Andrey.
>
> Some random nits while browsing the code.
>
> On Sun, Oct 14, 2018 at 07:21:23PM -0700, Andrey Smirnov wrote:
> > Port a very abridged version of MV88E6XXX DSA driver from Linux
> > kernel. Currently only internal MDIO bus connected to switch PHYs is
> > exposed.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> >
> > net: phy: mv88e6xxx: Expose internal MDIO bus
> > ---
> >  drivers/net/phy/Kconfig             |   6 +
> >  drivers/net/phy/Makefile            |   1 +
> >  drivers/net/phy/mv88e6xxx/Makefile  |   5 +
> >  drivers/net/phy/mv88e6xxx/chip.c    | 723 ++++++++++++++++++++++++++++
> >  drivers/net/phy/mv88e6xxx/chip.h    |  61 +++
> >  drivers/net/phy/mv88e6xxx/global2.c | 124 +++++
> >  drivers/net/phy/mv88e6xxx/global2.h |  41 ++
> >  drivers/net/phy/mv88e6xxx/port.c    |  20 +
> >  drivers/net/phy/mv88e6xxx/port.h    |  89 ++++
> >  9 files changed, 1070 insertions(+)
> >  create mode 100644 drivers/net/phy/mv88e6xxx/Makefile
> >  create mode 100644 drivers/net/phy/mv88e6xxx/chip.c
> >  create mode 100644 drivers/net/phy/mv88e6xxx/chip.h
> >  create mode 100644 drivers/net/phy/mv88e6xxx/global2.c
> >  create mode 100644 drivers/net/phy/mv88e6xxx/global2.h
> >  create mode 100644 drivers/net/phy/mv88e6xxx/port.c
> >  create mode 100644 drivers/net/phy/mv88e6xxx/port.h
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index 79fb917ee..3b1a6ea7e 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -48,6 +48,12 @@ config SMSC_PHY
> >       ---help---
> >         Currently supports the LAN83C185, LAN8187 and LAN8700 PHYs
> >
> > +config NET_DSA_MV88E6XXX
> > +     tristate "Marvell 88E6xxx Ethernet switch fabric support"
> > +     help
> > +       This driver adds support for most of the Marvell 88E6xxx models of
> > +       Ethernet switch chips, except 88E6060.
> > +
> >  comment "MII bus device drivers"
> >
> >  config MDIO_MVEBU
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> > index 4424054d9..e4d9ec65a 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -7,6 +7,7 @@ obj-$(CONFIG_MARVELL_PHY)     += marvell.o
> >  obj-$(CONFIG_MICREL_PHY)     += micrel.o
> >  obj-$(CONFIG_NATIONAL_PHY)   += national.o
> >  obj-$(CONFIG_SMSC_PHY)               += smsc.o
> > +obj-$(CONFIG_NET_DSA_MV88E6XXX)      += mv88e6xxx/
> >
> >  obj-$(CONFIG_MDIO_MVEBU)     += mdio-mvebu.o
> >  obj-$(CONFIG_MDIO_BITBANG)   += mdio-bitbang.o
> > diff --git a/drivers/net/phy/mv88e6xxx/Makefile b/drivers/net/phy/mv88e6xxx/Makefile
> > new file mode 100644
> > index 000000000..8c8ba78cd
> > --- /dev/null
> > +++ b/drivers/net/phy/mv88e6xxx/Makefile
> > @@ -0,0 +1,5 @@
> > +obj-y += mv88e6xxx.o
> > +
> > +mv88e6xxx-objs := chip.o
> > +mv88e6xxx-objs += global2.o
> > +mv88e6xxx-objs += port.o
> > \ No newline at end of file
>
> Another way to write the above would be:
> mv88e6xxx-y := chip.o
> mv88e6xxx-y += global2.o
> mv88e6xxx-y += port.o
>
> And if you change this then you can alos add the missing newline
>
> As the kernel uses -objs you will likely keep current syntax,
> just wanted to point in the direction of the new way to express this.
>

I'll fix the lack of newline in v4. As you mentioned this is a
straight copy of what's in kernel and I tried to keep as much original
code intact as possible. Thanks for the pointer, though.

>
> > +static int mv88e6xxx_probe(struct device_d *dev)
> > +{
> > +     struct device_node *np = dev->device_node;
> > +     struct device_node *mdio_node;
> > +     struct mv88e6xxx_chip *chip;
> > +     enum of_gpio_flags of_flags;
> > +     int err;
> > +     u32 reg;
> > +
> > +     err = of_property_read_u32(np, "reg", &reg);
> > +     if (err) {
> > +             dev_err(dev, "Couldn't determine switch MIDO address\n");
> > +             return err;
> > +     }
> > +
> > +     if (reg) {
> > +             dev_err(dev, "Only single-chip address mode is supported\n");
> > +             return -ENOTSUPP;
> > +     }
> > +
> > +     chip = xzalloc(sizeof(struct mv88e6xxx_chip));
> > +     chip->dev = dev;
> > +     chip->info = of_device_get_match_data(dev);
> > +
> > +     chip->parent_miibus = of_mdio_find_bus(np->parent);
> > +     if (!chip->parent_miibus)
> > +             return -EPROBE_DEFER;
> > +
> > +     chip->reset = of_get_named_gpio_flags(np, "reset-gpios", 0, &of_flags);
> > +     if (gpio_is_valid(chip->reset)) {
> > +             unsigned long flags = GPIOF_OUT_INIT_INACTIVE;
> > +             char *name;
> > +
> > +             if (of_flags & OF_GPIO_ACTIVE_LOW)
> > +                     flags |= GPIOF_ACTIVE_LOW;
> > +
> > +             name = basprintf("%s reset", dev_name(dev));
> > +             err = gpio_request_one(chip->reset, flags, name);
> > +             if (err < 0)
> > +                     return err;
> > +             /*
> > +              * We assume that reset line was previously held low
> > +              * and give the switch time to initialize before
> > +              * trying to read its registers
> > +              */
> > +             mv88e6xxx_hardware_reset_delay();
> > +     }
> > +
> > +     err = mv88e6xxx_detect(chip);
> > +     if (err)
> > +             return err;
> > +
> > +     err = mv88e6xxx_switch_reset(chip);
> > +     if (err)
> > +             return err;
> > +     /*
> > +      * In single-chip address mode addresses 0x10 - 0x1f are
> > +      * reserved to access various switch registers and do not
> > +      * correspond to any PHYs, so we mask them to pervent from
> > +      * being exposed as phy devices
> > +      */
> > +     chip->parent_miibus->phy_mask |= GENMASK(0x1f, 0x10);
> > +     /*
> > +      * Address 0x0f on internal bus is dedicated to SERDES
> > +      * registers and won't be very useful against standard PHY
> > +      * driver
> > +      */
> > +     chip->miibus.phy_mask |= GENMASK(0x1f, 0x0f);
> > +
> > +     chip->miibus.read = mv88e6xxx_mdio_read;
> > +     chip->miibus.write = mv88e6xxx_mdio_write;
>
> The function pointers are hardcoded here.
> But we have them in chip->info->ops - where we can
> have chip specific variants.
> I assume it would be more correct to copy them from the ops structure?
>

I am not sure I see why that would be. Mv88e6xxx_mdio_read() and
mv88e6xxx_mdio_write() are both wrappers around
chip->info->ops->phy_read/phy_write that also have code to handle the
case when either phy_read/phy_write are not specified.
MV88e6xxx_mdio_read() also have special workaround code to deal with
bogus values programmed in on of the ID registers on MV88E6390.
Besides those two functions are not Barebox specific and come directly
from corresponding Linux driver (sans the needless locking), so, IMHO,
they should stay as is.

>
> > +#endif /* _MV88E6XXX_CHIP_H */
> > \ No newline at end of file
> Hmm...
>
>
> > +#endif /* _MV88E6XXX_GLOBAL2_H */
> > \ No newline at end of file
> Hmmm...
>
> > +#endif /* _MV88E6XXX_PORT_H */
> > \ No newline at end of file
> Again
>

Will fix the newlines in v4.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox