| Message ID | 20170612095745.11300-8-antoine.tenart@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 12, 2017 at 11:57:43AM +0200, Antoine Tenart wrote: > +static void orion_mdio_xsmi_start_read_op(struct orion_mdio_dev *dev, > + int mii_id, int regnum) > +{ > + u16 dev_addr = (regnum >> 16) & GENMASK(4, 0); > + > + writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG); > + writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) | > + (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) | > + MVMDIO_XSMI_READ_OPERATION, > + dev->regs + MVMDIO_XSMI_MGNT_REG); So what happens if this function gets passed a Clause 22 formatted request? Both regnum and mii_id are in the range 0-31. MII_ADDR_C45 in regnum is clear. The answer is we produce two Clause 45 frames on the bus, which is certainly not correct. You need to trap and error out if MII_ADDR_C45 is not set (as I've already said previously.) The SMI operations need to do the reverse - they need to fail if they receive a regnum with MII_ADDR_C45 set, as they are unable to produce Clause 45 frames. > +static void orion_mdio_xsmi_write_op(struct orion_mdio_dev *dev, int mii_id, > + int regnum, u16 value) > +{ > + u16 dev_addr = (regnum >> 16) & GENMASK(4, 0); > + > + writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG); > + writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) | > + (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) | > + MVMDIO_XSMI_WRITE_OPERATION | value, > + dev->regs + MVMDIO_XSMI_MGNT_REG); It's even more important here, as a Clause 22 write will produce a valid Clause 45 pair of frames with dev_addr = 0. Again, the corresponding SMI code needs to reject Clause 45 requests as the SMI interface is unable to produce Clause 45 frames.
On Mon, Jun 12, 2017 at 11:57:43AM +0200, Antoine Tenart wrote: > +static const struct orion_mdio_ops *orion_mdio_get_ops(struct orion_mdio_dev *dev, > + int regnum) > +{ > + if (dev->bus_type == BUS_TYPE_XSMI && (regnum & MII_ADDR_C45)) > + return &orion_mdio_xsmi_ops; > + else if (dev->bus_type == BUS_TYPE_SMI) > + return &orion_mdio_smi_ops; > + > + return ERR_PTR(-EOPNOTSUPP); > +} Oh, this is where you're doing it - I'm not sure having this complexity is really necessary - there is no dynamic choice between the two. This seems to be way over-engineered. You might as well make the SMI operations fail if MII_ADDR_C45 is set, and the XSMI operations fail if MII_ADDR_C45 is not set. Hmm, I think this whole driver is over-engineered: 1. the mdio read/write functions implement their own locking. At the MDIO level, there is already locking in the form of a per-bus lock "bus->mdio_lock" which will be taken whenever either of these functions is called. So the driver's "dev->lock" is redundant. 2. with the redundant locking removed, orion_mdio_write() becomes a call to orion_mdio_wait_ready() followed by a call to dev->ops->write. It seems that orion_mdio_wait_ready() could be a library function shared between a SMI version of orion_mdio_write() and a XSMI version. 3. the same is really true of orion_mdio_read(), although that function is a little more complex in itself, the result would actually end up being simpler. With those changes together, it elimates "struct orion_mdio_ops" entirely, and I think makes the driver smaller, simpler, and cleaner.
On Mon, Jun 12, 2017 at 11:17:39AM +0100, Russell King - ARM Linux wrote: > On Mon, Jun 12, 2017 at 11:57:43AM +0200, Antoine Tenart wrote: > > +static const struct orion_mdio_ops *orion_mdio_get_ops(struct orion_mdio_dev *dev, > > + int regnum) > > +{ > > + if (dev->bus_type == BUS_TYPE_XSMI && (regnum & MII_ADDR_C45)) > > + return &orion_mdio_xsmi_ops; > > + else if (dev->bus_type == BUS_TYPE_SMI) > > + return &orion_mdio_smi_ops; > > + > > + return ERR_PTR(-EOPNOTSUPP); > > +} > > Oh, this is where you're doing it - I'm not sure having this complexity > is really necessary - there is no dynamic choice between the two. This > seems to be way over-engineered. > > You might as well make the SMI operations fail if MII_ADDR_C45 is set, > and the XSMI operations fail if MII_ADDR_C45 is not set. > > Hmm, I think this whole driver is over-engineered: > > 1. the mdio read/write functions implement their own locking. > > At the MDIO level, there is already locking in the form of a per-bus > lock "bus->mdio_lock" which will be taken whenever either of these > functions is called. So the driver's "dev->lock" is redundant. > > 2. with the redundant locking removed, orion_mdio_write() becomes a > call to orion_mdio_wait_ready() followed by a call to dev->ops->write. > It seems that orion_mdio_wait_ready() could be a library function > shared between a SMI version of orion_mdio_write() and a XSMI version. > > 3. the same is really true of orion_mdio_read(), although that function > is a little more complex in itself, the result would actually end up > being simpler. > > With those changes together, it elimates "struct orion_mdio_ops" entirely, > and I think makes the driver smaller, simpler, and cleaner. I wasn't able to completely get rid of the ops structure, but I think this is cleaner. I haven't tested these two patches yet. drivers/net/ethernet/marvell/mvmdio.c | 276 +++++++++++++++------------------- 1 file changed, 123 insertions(+), 153 deletions(-)
Hi Russell, On Mon, Jun 12, 2017 at 11:17:39AM +0100, Russell King - ARM Linux wrote: > On Mon, Jun 12, 2017 at 11:57:43AM +0200, Antoine Tenart wrote: > > +static const struct orion_mdio_ops *orion_mdio_get_ops(struct orion_mdio_dev *dev, > > + int regnum) > > +{ > > + if (dev->bus_type == BUS_TYPE_XSMI && (regnum & MII_ADDR_C45)) > > + return &orion_mdio_xsmi_ops; > > + else if (dev->bus_type == BUS_TYPE_SMI) > > + return &orion_mdio_smi_ops; > > + > > + return ERR_PTR(-EOPNOTSUPP); > > +} > > Oh, this is where you're doing it - I'm not sure having this complexity > is really necessary - there is no dynamic choice between the two. This > seems to be way over-engineered. You're right, there is no dynamic choice between the two. The advantage is the logic of the read/write operations are not duplicated. > You might as well make the SMI operations fail if MII_ADDR_C45 is set, > and the XSMI operations fail if MII_ADDR_C45 is not set. This check is already done for xSMI operations. But this should also be the case for SMI ones, you're right. > 1. the mdio read/write functions implement their own locking. > > At the MDIO level, there is already locking in the form of a per-bus > lock "bus->mdio_lock" which will be taken whenever either of these > functions is called. So the driver's "dev->lock" is redundant. OK, that's a good rework to add in the series. > 2. with the redundant locking removed, orion_mdio_write() becomes a > call to orion_mdio_wait_ready() followed by a call to dev->ops->write. > It seems that orion_mdio_wait_ready() could be a library function > shared between a SMI version of orion_mdio_write() and a XSMI version. > > 3. the same is really true of orion_mdio_read(), although that function > is a little more complex in itself, the result would actually end up > being simpler. I'm not completely convinced as the read and write functions end up being duplicated. One for SMI operations and one for xSMI. But I don't really care, if you think this is better let's go for it. Should I add your first patch in my series and squash the second one in patch 7/9? (I'll also remove the bus_type member from the private struct, as well as the one line it's used in the probe). Thanks! Antoine
diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig index da6fb825afea..205bb7e683b7 100644 --- a/drivers/net/ethernet/marvell/Kconfig +++ b/drivers/net/ethernet/marvell/Kconfig @@ -35,9 +35,9 @@ config MVMDIO depends on HAS_IOMEM select PHYLIB ---help--- - This driver supports the MDIO interface found in the network - interface units of the Marvell EBU SoCs (Kirkwood, Orion5x, - Dove, Armada 370 and Armada XP). + This driver supports the MDIO and xMDIO interfaces found in + the network interface units of the Marvell EBU SoCs (Kirkwood, + Orion5x, Dove, Armada 370, Armada XP, Armada 7k and Armada 8k). This driver is used by the MV643XX_ETH and MVNETA drivers. diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c index 41c75d7b84fb..c43747f0be0b 100644 --- a/drivers/net/ethernet/marvell/mvmdio.c +++ b/drivers/net/ethernet/marvell/mvmdio.c @@ -24,6 +24,7 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/of_device.h> #include <linux/of_mdio.h> #include <linux/phy.h> #include <linux/platform_device.h> @@ -41,6 +42,15 @@ #define MVMDIO_ERR_INT_SMI_DONE 0x00000010 #define MVMDIO_ERR_INT_MASK 0x0080 +#define MVMDIO_XSMI_MGNT_REG 0x0 +#define MVMDIO_XSMI_PHYADDR_SHIFT 16 +#define MVMDIO_XSMI_DEVADDR_SHIFT 21 +#define MVMDIO_XSMI_WRITE_OPERATION (0x5 << 26) +#define MVMDIO_XSMI_READ_OPERATION (0x7 << 26) +#define MVMDIO_XSMI_READ_VALID BIT(29) +#define MVMDIO_XSMI_BUSY BIT(30) +#define MVMDIO_XSMI_ADDR_REG 0x8 + /* * SMI Timeout measurements: * - Kirkwood 88F6281 (Globalscale Dreamplug): 45us to 95us (Interrupt) @@ -50,6 +60,14 @@ #define MVMDIO_SMI_POLL_INTERVAL_MIN 45 #define MVMDIO_SMI_POLL_INTERVAL_MAX 55 +#define MVMDIO_XSMI_POLL_INTERVAL_MIN 150 +#define MVMDIO_XSMI_POLL_INTERVAL_MAX 160 + +enum orion_mdio_bus_type { + BUS_TYPE_SMI, + BUS_TYPE_XSMI +}; + struct orion_mdio_dev { struct mutex lock; void __iomem *regs; @@ -62,6 +80,8 @@ struct orion_mdio_dev { */ int err_interrupt; wait_queue_head_t smi_busy_wait; + + enum orion_mdio_bus_type bus_type; }; struct orion_mdio_ops { @@ -75,6 +95,7 @@ struct orion_mdio_ops { unsigned int poll_interval_max; }; +/* mdio smi */ static int orion_mdio_smi_is_done(struct orion_mdio_dev *dev) { return !(readl(dev->regs) & MVMDIO_SMI_BUSY); @@ -120,6 +141,68 @@ static const struct orion_mdio_ops orion_mdio_smi_ops = { .poll_interval_max = MVMDIO_SMI_POLL_INTERVAL_MAX, }; +/* xmdio xsmi */ +static int orion_mdio_xsmi_is_done(struct orion_mdio_dev *dev) +{ + return !(readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & MVMDIO_XSMI_BUSY); +} + +static int orion_mdio_xsmi_is_read_valid(struct orion_mdio_dev *dev) +{ + return readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & MVMDIO_XSMI_READ_VALID; +} + +static void orion_mdio_xsmi_start_read_op(struct orion_mdio_dev *dev, + int mii_id, int regnum) +{ + u16 dev_addr = (regnum >> 16) & GENMASK(4, 0); + + writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG); + writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) | + (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) | + MVMDIO_XSMI_READ_OPERATION, + dev->regs + MVMDIO_XSMI_MGNT_REG); +} + +static u16 orion_mdio_xsmi_read_op(struct orion_mdio_dev *dev) +{ + return readl(dev->regs + MVMDIO_XSMI_MGNT_REG) & GENMASK(15, 0); +} + +static void orion_mdio_xsmi_write_op(struct orion_mdio_dev *dev, int mii_id, + int regnum, u16 value) +{ + u16 dev_addr = (regnum >> 16) & GENMASK(4, 0); + + writel(regnum & GENMASK(15, 0), dev->regs + MVMDIO_XSMI_ADDR_REG); + writel((mii_id << MVMDIO_XSMI_PHYADDR_SHIFT) | + (dev_addr << MVMDIO_XSMI_DEVADDR_SHIFT) | + MVMDIO_XSMI_WRITE_OPERATION | value, + dev->regs + MVMDIO_XSMI_MGNT_REG); +} + +static const struct orion_mdio_ops orion_mdio_xsmi_ops = { + .is_done = orion_mdio_xsmi_is_done, + .is_read_valid = orion_mdio_xsmi_is_read_valid, + .start_read = orion_mdio_xsmi_start_read_op, + .read = orion_mdio_xsmi_read_op, + .write = orion_mdio_xsmi_write_op, + + .poll_interval_min = MVMDIO_XSMI_POLL_INTERVAL_MIN, + .poll_interval_max = MVMDIO_XSMI_POLL_INTERVAL_MAX, +}; + +static const struct orion_mdio_ops *orion_mdio_get_ops(struct orion_mdio_dev *dev, + int regnum) +{ + if (dev->bus_type == BUS_TYPE_XSMI && (regnum & MII_ADDR_C45)) + return &orion_mdio_xsmi_ops; + else if (dev->bus_type == BUS_TYPE_SMI) + return &orion_mdio_smi_ops; + + return ERR_PTR(-EOPNOTSUPP); +} + /* Wait for the SMI unit to be ready for another operation */ static int orion_mdio_wait_ready(const struct orion_mdio_ops *ops, @@ -164,9 +247,13 @@ static int orion_mdio_read(struct mii_bus *bus, int mii_id, int regnum) { struct orion_mdio_dev *dev = bus->priv; - const struct orion_mdio_ops *ops = &orion_mdio_smi_ops; + const struct orion_mdio_ops *ops; int ret; + ops = orion_mdio_get_ops(dev, regnum); + if (IS_ERR(ops)) + return PTR_ERR(ops); + mutex_lock(&dev->lock); ret = orion_mdio_wait_ready(ops, bus); @@ -195,9 +282,13 @@ static int orion_mdio_write(struct mii_bus *bus, int mii_id, int regnum, u16 value) { struct orion_mdio_dev *dev = bus->priv; - const struct orion_mdio_ops *ops = &orion_mdio_smi_ops; + const struct orion_mdio_ops *ops; int ret; + ops = orion_mdio_get_ops(dev, regnum); + if (IS_ERR(ops)) + return PTR_ERR(ops); + mutex_lock(&dev->lock); ret = orion_mdio_wait_ready(ops, bus); @@ -288,6 +379,9 @@ static int orion_mdio_probe(struct platform_device *pdev) return -EPROBE_DEFER; } + dev->bus_type = + (enum orion_mdio_bus_type)of_device_get_match_data(&pdev->dev); + mutex_init(&dev->lock); if (pdev->dev.of_node) @@ -338,7 +432,8 @@ static int orion_mdio_remove(struct platform_device *pdev) } static const struct of_device_id orion_mdio_match[] = { - { .compatible = "marvell,orion-mdio" }, + { .compatible = "marvell,orion-mdio", .data = (void *)BUS_TYPE_SMI }, + { .compatible = "marvell,xmdio", .data = (void *)BUS_TYPE_XSMI }, { } }; MODULE_DEVICE_TABLE(of, orion_mdio_match);
This patch adds the xmdio xsmi interface support in the mvmdio driver. This interface is used in Ethernet controllers on Marvell 370, 7k and 8k (as of now). The xsmi interface supported by this driver complies with the IEEE 802.3 clause 45. The xSMI interface is used by 10GbE devices. Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> --- drivers/net/ethernet/marvell/Kconfig | 6 +- drivers/net/ethernet/marvell/mvmdio.c | 101 +++++++++++++++++++++++++++++++++- 2 files changed, 101 insertions(+), 6 deletions(-)