Re: [PATCH v3 1/3] net: phy: mdio: add IPQ40xx MDIO driver

[Robert Marko]

On Thu, Apr 16, 2020 at 1:48 AM Andrew Lunn <andrew@xxxxxxx> wrote:
>
> Hi Robert
>
> I should of said this earlier. With a patch set, you should include a
> cover note, patch 0 of X, explaining the big picture of what the
> patches do.

Sorry, I thought that was a relatively small patch set and cover
letter was not needed.
>
> Also, for network patches, the subject line should indicate which tree
> these patches are for. So
>
> [PATCH net-next v3 0/3]
OK, this is my first contribution to networking and I did not check first.
I am used to sending patches without it.
Now I know for the future,
Thanks
>
> On Wed, Apr 15, 2020 at 05:02:43PM +0200, Robert Marko wrote:
> > This patch adds the driver for the MDIO interface
> > inside of Qualcomm IPQ40xx series SoC-s.
> >
> > Signed-off-by: Christian Lamparter <chunkeey@xxxxxxxxx>
> > Signed-off-by: Robert Marko <robert.marko@xxxxxxxxxx>
> > Cc: Luka Perkov <luka.perkov@xxxxxxxxxx>
> > ---
> > Changes from v2 to v3:
> > * Rename registers
> > * Remove unnecessary variable initialisations
> > * Switch to readl_poll_timeout() instead of custom solution
> > * Drop unused header
> >
> > Changes from v1 to v2:
> > * Remove magic default value
> > * Remove lockdep_assert_held
> > * Add C45 check
> > * Simplify the driver
> > * Drop device and mii_bus structs from private struct
> > * Use devm_mdiobus_alloc_size()
> >
> >  drivers/net/phy/Kconfig        |   7 ++
> >  drivers/net/phy/Makefile       |   1 +
> >  drivers/net/phy/mdio-ipq40xx.c | 160 +++++++++++++++++++++++++++++++++
> >  3 files changed, 168 insertions(+)
> >  create mode 100644 drivers/net/phy/mdio-ipq40xx.c
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index 3fa33d27eeba..23bb5db033e3 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -157,6 +157,13 @@ config MDIO_I2C
> >
> >         This is library mode.
> >
> > +config MDIO_IPQ40XX
> > +     tristate "Qualcomm IPQ40xx MDIO interface"
> > +     depends on HAS_IOMEM && OF_MDIO
> > +     help
> > +       This driver supports the MDIO interface found in Qualcomm
> > +       IPQ40xx series Soc-s.
> > +
> >  config MDIO_IPQ8064
> >       tristate "Qualcomm IPQ8064 MDIO interface support"
> >       depends on HAS_IOMEM && OF_MDIO
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> > index 2f5c7093a65b..36aafc6128c4 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_MDIO_CAVIUM)   += mdio-cavium.o
> >  obj-$(CONFIG_MDIO_GPIO)              += mdio-gpio.o
> >  obj-$(CONFIG_MDIO_HISI_FEMAC)        += mdio-hisi-femac.o
> >  obj-$(CONFIG_MDIO_I2C)               += mdio-i2c.o
> > +obj-$(CONFIG_MDIO_IPQ40XX)   += mdio-ipq40xx.o
> >  obj-$(CONFIG_MDIO_IPQ8064)   += mdio-ipq8064.o
> >  obj-$(CONFIG_MDIO_MOXART)    += mdio-moxart.o
> >  obj-$(CONFIG_MDIO_MSCC_MIIM) += mdio-mscc-miim.o
> > diff --git a/drivers/net/phy/mdio-ipq40xx.c b/drivers/net/phy/mdio-ipq40xx.c
> > new file mode 100644
> > index 000000000000..acf1230341bd
> > --- /dev/null
> > +++ b/drivers/net/phy/mdio-ipq40xx.c
> > @@ -0,0 +1,160 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> > +/* Copyright (c) 2015, The Linux Foundation. All rights reserved. */
> > +/* Copyright (c) 2020 Sartura Ltd. */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_mdio.h>
> > +#include <linux/phy.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define MDIO_ADDR_REG                                0x44
> > +#define MDIO_DATA_WRITE_REG                  0x48
> > +#define MDIO_DATA_READ_REG                   0x4c
> > +#define MDIO_CMD_REG                         0x50
> > +#define MDIO_CMD_ACCESS_BUSY         BIT(16)
> > +#define MDIO_CMD_ACCESS_START                BIT(8)
> > +#define MDIO_CMD_ACCESS_CODE_READ    0
> > +#define MDIO_CMD_ACCESS_CODE_WRITE   1
> > +
> > +#define IPQ40XX_MDIO_TIMEOUT 10000
> > +#define IPQ40XX_MDIO_SLEEP           10
> > +
> > +struct ipq40xx_mdio_data {
> > +     void __iomem    *membase;
> > +};
> > +
> > +static int ipq40xx_mdio_wait_busy(struct mii_bus *bus)
> > +{
> > +     struct ipq40xx_mdio_data *priv = bus->priv;
> > +     unsigned int busy;
> > +
> > +     return readl_poll_timeout(priv->membase + MDIO_CMD_REG, busy,
> > +                               (busy & MDIO_CMD_ACCESS_BUSY) == 0,
> > +                               IPQ40XX_MDIO_SLEEP, IPQ40XX_MDIO_TIMEOUT);
>
> Do you have any documentation about _START and _BUSY? You are making
> the assumption that the next read after writing the START bit will
> have the BUSY bit set. That the hardware reacts that fast. It is not
> an unreasonable assumption, but i've seen more designed where the
> START bit is also the BUSY bit, so the write implicitly sets the busy
> bit, and the hardware needs to clear it when it is done.
No, I don't have any docs at all for the SoC.
It's all based on GPL drivers from SDK and QCA comments in
the drivers as well as U-boot drivers.
>
> As i said, this is not unreasonable, so:
>
> Reviewed-by: Andrew Lunn <andrew@xxxxxxx>
>
>     Andrew
>