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
>
