| Message ID | 1482943592-12556-12-git-send-email-thomas.petazzoni@free-electrons.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Dec 28, 2016 at 05:46:27PM +0100, Thomas Petazzoni wrote: > @@ -6511,7 +6515,9 @@ static int mvpp2_port_probe(struct platform_device *pdev, > dev_err(&pdev->dev, "failed to init port %d\n", id); > goto err_free_stats; > } > - mvpp2_port_power_up(port); > + > + if (priv->hw_version == MVPP21) > + mvpp21_port_power_up(port); This has the side effect that nothing clears the port reset bit in the GMAC, which means there's no hope of the interface working - with the reset bit set, the port is well and truely held in "link down" state. In any case, the GMAC part is much the same as mvneta, and I think that code should be shared rather than writing new versions of it. There are some subtle differences between neta, pp2.1 and pp2.2, but it's entirely doable (I have an implementation here as I wasn't going to duplicate this code for my phylink conversion.)
On Wed, Dec 28, 2016 at 05:46:27PM +0100, Thomas Petazzoni wrote: > +#define MVPP22_SMI_MISC_CFG_REG 0x2a204 > +#define MVPP22_SMI_POLLING_EN BIT(10) > + ... > + if (priv->hw_version == MVPP21) { > + val = readl(priv->lms_base + MVPP2_PHY_AN_CFG0_REG); > + val |= MVPP2_PHY_AN_STOP_SMI0_MASK; > + writel(val, priv->lms_base + MVPP2_PHY_AN_CFG0_REG); > + } else { > + val = readl(priv->iface_base + MVPP22_SMI_MISC_CFG_REG); > + val &= ~MVPP22_SMI_POLLING_EN; > + writel(val, priv->iface_base + MVPP22_SMI_MISC_CFG_REG); > + } The MVPP22_SMI_MISC_CFG_REG register is within the MDIO driver's register set, although the mvmdio driver does not access this register. Shouldn't this be taken care of by the mvmdio driver? Also, a point that I've noticed while reviewing this is the mvmdio driver also accesses these registers: #define MVMDIO_ERR_INT_CAUSE 0x007C #define MVMDIO_ERR_INT_MASK 0x0080 in addition to the un-named register at offset 0. The driver writes to these registers unconditionally when unbinding: writel(0, dev->regs + MVMDIO_ERR_INT_MASK); However, the various bindings for the driver have: arch/arm/boot/dts/armada-370-xp.dtsi: compatible = "marvell,orion-mdio"; arch/arm/boot/dts/armada-370-xp.dtsi- reg = <0x72004 0x4>; arch/arm/boot/dts/armada-375.dtsi: compatible = "marvell,orion-mdio"; arch/arm/boot/dts/armada-375.dtsi- reg = <0xc0054 0x4>; arch/arm/boot/dts/dove.dtsi: compatible = "marvell,orion-mdio"; arch/arm/boot/dts/dove.dtsi- #address-cells = <1>; arch/arm/boot/dts/dove.dtsi- #size-cells = <0>; arch/arm/boot/dts/dove.dtsi- reg = <0x72004 0x84>; arch/arm/boot/dts/orion5x.dtsi: compatible = "marvell,orion-mdio"; arch/arm/boot/dts/orion5x.dtsi- #address-cells = <1>; arch/arm/boot/dts/orion5x.dtsi- #size-cells = <0>; arch/arm/boot/dts/orion5x.dtsi- reg = <0x72004 0x84>; arch/arm/boot/dts/kirkwood.dtsi: compatible = "marvell,orion-mdio"; arch/arm/boot/dts/kirkwood.dtsi- #address-cells = <1>; arch/arm/boot/dts/kirkwood.dtsi- #size-cells = <0>; arch/arm/boot/dts/kirkwood.dtsi- reg = <0x72004 0x84>; arch/arm/boot/dts/armada-38x.dtsi: compatible = "marvell,orion-mdio"; arch/arm/boot/dts/armada-38x.dtsi- reg = <0x72004 0x4>; So, for many of these, we're accessing registers outside of the given binding, which sounds incorrect. I guess that write should be conditional upon an interrupt being present. The binding document says: - reg: address and length of the SMI register which is clearly wrong for those cases where the interrupt is used. I also notice that the binding for CP110 uses a register size of 0x10 (even in your tree) - but I guess this should be 4. I'm starting to wonder whether the orion-mdio driver really is a separate chunk of hardware that warrants a separate description in DT from the ethernet controller - it appears in all cases to be embedded with an ethernet controller, sharing its register space and at least some of the ethernet controllers clocks. That says to me that it isn't an independent functional unit of hardware.
Hi Russel, 2017-01-07 12:03 GMT+01:00 Russell King - ARM Linux <linux@armlinux.org.uk>: > On Wed, Dec 28, 2016 at 05:46:27PM +0100, Thomas Petazzoni wrote: >> +#define MVPP22_SMI_MISC_CFG_REG 0x2a204 >> +#define MVPP22_SMI_POLLING_EN BIT(10) >> + > ... >> + if (priv->hw_version == MVPP21) { >> + val = readl(priv->lms_base + MVPP2_PHY_AN_CFG0_REG); >> + val |= MVPP2_PHY_AN_STOP_SMI0_MASK; >> + writel(val, priv->lms_base + MVPP2_PHY_AN_CFG0_REG); >> + } else { >> + val = readl(priv->iface_base + MVPP22_SMI_MISC_CFG_REG); >> + val &= ~MVPP22_SMI_POLLING_EN; >> + writel(val, priv->iface_base + MVPP22_SMI_MISC_CFG_REG); >> + } > > The MVPP22_SMI_MISC_CFG_REG register is within the MDIO driver's > register set, although the mvmdio driver does not access this register. > Shouldn't this be taken care of by the mvmdio driver? > > Also, a point that I've noticed while reviewing this is the mvmdio > driver also accesses these registers: > > #define MVMDIO_ERR_INT_CAUSE 0x007C > #define MVMDIO_ERR_INT_MASK 0x0080 > > in addition to the un-named register at offset 0. The driver writes > to these registers unconditionally when unbinding: > > writel(0, dev->regs + MVMDIO_ERR_INT_MASK); > > However, the various bindings for the driver have: > > arch/arm/boot/dts/armada-370-xp.dtsi: compatible = "marvell,orion-mdio"; > arch/arm/boot/dts/armada-370-xp.dtsi- reg = <0x72004 0x4>; > arch/arm/boot/dts/armada-375.dtsi: compatible = "marvell,orion-mdio"; > arch/arm/boot/dts/armada-375.dtsi- reg = <0xc0054 0x4>; > arch/arm/boot/dts/dove.dtsi: compatible = "marvell,orion-mdio"; > arch/arm/boot/dts/dove.dtsi- #address-cells = <1>; > arch/arm/boot/dts/dove.dtsi- #size-cells = <0>; > arch/arm/boot/dts/dove.dtsi- reg = <0x72004 0x84>; > arch/arm/boot/dts/orion5x.dtsi: compatible = "marvell,orion-mdio"; > arch/arm/boot/dts/orion5x.dtsi- #address-cells = <1>; > arch/arm/boot/dts/orion5x.dtsi- #size-cells = <0>; > arch/arm/boot/dts/orion5x.dtsi- reg = <0x72004 0x84>; > arch/arm/boot/dts/kirkwood.dtsi: compatible = "marvell,orion-mdio"; > arch/arm/boot/dts/kirkwood.dtsi- #address-cells = <1>; > arch/arm/boot/dts/kirkwood.dtsi- #size-cells = <0>; > arch/arm/boot/dts/kirkwood.dtsi- reg = <0x72004 0x84>; > arch/arm/boot/dts/armada-38x.dtsi: compatible = "marvell,orion-mdio"; > arch/arm/boot/dts/armada-38x.dtsi- reg = <0x72004 0x4>; > > So, for many of these, we're accessing registers outside of the given > binding, which sounds incorrect. I guess that write should be > conditional upon an interrupt being present. > > The binding document says: > > - reg: address and length of the SMI register > > which is clearly wrong for those cases where the interrupt is used. > > I also notice that the binding for CP110 uses a register size of 0x10 > (even in your tree) - but I guess this should be 4. > > I'm starting to wonder whether the orion-mdio driver really is a > separate chunk of hardware that warrants a separate description in > DT from the ethernet controller - it appears in all cases to be > embedded with an ethernet controller, sharing its register space > and at least some of the ethernet controllers clocks. That says > to me that it isn't an independent functional unit of hardware. > In fact there is common SMI bus, but each port has its own register set to control it (it's true at least for Neta and PP2). There is also an option to use HW polling - every 1s hardware checks PHY over SMI and updates MAC registers of each port independently. I was able to use those successfully in other implementations. However we are supposed to use libphy in Linux and I'm afraid we have to use single instance that controls single SMI bus - I think current implementation is a compromise between HW and libphy demands. Best regards, Marcin
On Sat, Jan 07, 2017 at 01:12:35PM +0100, Marcin Wojtas wrote: > In fact there is common SMI bus, but each port has its own register > set to control it (it's true at least for Neta and PP2). There is also > an option to use HW polling - every 1s hardware checks PHY over SMI > and updates MAC registers of each port independently. I was able to > use those successfully in other implementations. > > However we are supposed to use libphy in Linux and I'm afraid we have > to use single instance that controls single SMI bus - I think current > implementation is a compromise between HW and libphy demands. One of the "DT rules" is that DT only describes the hardware in an implementation independent way. It should not describe a particular implementation's view of the hardware. "libphy demands" sounds very much like an implementation dictating the DT description, which sounds to me very wrong and against the goals and purposes of DT. Anyway, I don't think it's too bad - a possible solution here would be to have the existing MDIO driver as a library, which can be instantiated by the Neta and PP2 drivers. This could be done in DT (taking dove as an example) as: eth: ethernet-ctrl@72000 { compatible = "marvell,orion-eth"; reg = <0x72000 0x4000>; ranges = <0 0x72000 0 0x4000>; ... mdio: mdio@4 { compatible = "marvell,orion-mdio"; reg = <4>; ... ... phys ... }; }; I don't think that would require that big a change - and it could be done in a way that compatibility with the existing DT descriptions is maintained very cheaply. Now, I'm not suggesting that mdio@4 should be created by a platform device via marking ethernet-ctrl@72000 with "simple-bus", but it's something that should be created by the ethernet driver if present. The compatible string is there so we can identify that this is a mdio node, and which type of mdio it is (the Armada 8k has this type and a separate clause-45 xmdio implementation, and we need to distinguish those.) What that means is that we no longer have to worry about clocks and overlapping register regions and the like, and can deal with the ethernet driver wanting to access the SMI registers as well. We would need the ethernet driver to be capable of instantiation even with no ports enabled, so cases where the MDIO interface is used with other ethernet controllers continues to be supportable (eg, the Armada 8040 case where the slave CP110 ethernet controller is used with PHYs connected to the master CP110 ethernet controller's MDIO buses - which afaik aren't shared between the two CP110 dies.) However, I'd like to see libphy become more flexible and support hardware polled mode of operation, since libphy provides a nice library of functions for accessing various phy features (like setting the advertisment, EEE modes, flow control, etc.) Even with hardware polling, we should still describe the PHY in DT, because PHY is part of the hardware description, and we need to know where it is in order to program these phy features.
Hello, On Sat, 7 Jan 2017 09:38:34 +0000, Russell King - ARM Linux wrote: > This has the side effect that nothing clears the port reset bit in the > GMAC, which means there's no hope of the interface working - with the > reset bit set, the port is well and truely held in "link down" state. Things were working fine here on Armada 7K/8K even without calling mvpp21_port_power_up(port). But I've looked into more details, and in fact the whole function makes sense on PPv2.2, except the mvpp2_port_fc_adv_enable(port) part of it. So I've replaced this with: mvpp2_port_mii_set(port); mvpp2_port_periodic_xon_disable(port); if (priv->hw_version == MVPP21) mvpp2_port_fc_adv_enable(port); mvpp2_port_reset(port); This will be in my v3. > In any case, the GMAC part is much the same as mvneta, and I think > that code should be shared rather than writing new versions of it. Possibly, but it's really a separate thing: this is completely independent from introducing PPv2.2 support, it's something that already exists. The patch series is already 16 patches long in its v2, and is going to even a bit longer in its v3, so I would really like to stick to just PPv2.2 support. Thanks! Thomas
diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c index 389cc62..eb55576 100644 --- a/drivers/net/ethernet/marvell/mvpp2.c +++ b/drivers/net/ethernet/marvell/mvpp2.c @@ -304,6 +304,9 @@ #define MVPP2_GMAC_TX_FIFO_MIN_TH_MASK(v) (((v) << 6) & \ MVPP2_GMAC_TX_FIFO_MIN_TH_ALL_MASK) +#define MVPP22_SMI_MISC_CFG_REG 0x2a204 +#define MVPP22_SMI_POLLING_EN BIT(10) + #define MVPP22_PORT_BASE 0x30e00 #define MVPP22_PORT_OFFSET 0x1000 @@ -5823,7 +5826,7 @@ static int mvpp2_check_ringparam_valid(struct net_device *dev, return 0; } -static void mvpp2_get_mac_address(struct mvpp2_port *port, unsigned char *addr) +static void mvpp21_get_mac_address(struct mvpp2_port *port, unsigned char *addr) { u32 mac_addr_l, mac_addr_m, mac_addr_h; @@ -6272,7 +6275,7 @@ static const struct ethtool_ops mvpp2_eth_tool_ops = { /* Driver initialization */ -static void mvpp2_port_power_up(struct mvpp2_port *port) +static void mvpp21_port_power_up(struct mvpp2_port *port) { mvpp2_port_mii_set(port); mvpp2_port_periodic_xon_disable(port); @@ -6491,7 +6494,8 @@ static int mvpp2_port_probe(struct platform_device *pdev, mac_from = "device tree"; ether_addr_copy(dev->dev_addr, dt_mac_addr); } else { - mvpp2_get_mac_address(port, hw_mac_addr); + if (priv->hw_version == MVPP21) + mvpp21_get_mac_address(port, hw_mac_addr); if (is_valid_ether_addr(hw_mac_addr)) { mac_from = "hardware"; ether_addr_copy(dev->dev_addr, hw_mac_addr); @@ -6511,7 +6515,9 @@ static int mvpp2_port_probe(struct platform_device *pdev, dev_err(&pdev->dev, "failed to init port %d\n", id); goto err_free_stats; } - mvpp2_port_power_up(port); + + if (priv->hw_version == MVPP21) + mvpp21_port_power_up(port); port->pcpu = alloc_percpu(struct mvpp2_port_pcpu); if (!port->pcpu) { @@ -6654,9 +6660,15 @@ static int mvpp2_init(struct platform_device *pdev, struct mvpp2 *priv) mvpp2_conf_mbus_windows(dram_target_info, priv); /* Disable HW PHY polling */ - val = readl(priv->lms_base + MVPP2_PHY_AN_CFG0_REG); - val |= MVPP2_PHY_AN_STOP_SMI0_MASK; - writel(val, priv->lms_base + MVPP2_PHY_AN_CFG0_REG); + if (priv->hw_version == MVPP21) { + val = readl(priv->lms_base + MVPP2_PHY_AN_CFG0_REG); + val |= MVPP2_PHY_AN_STOP_SMI0_MASK; + writel(val, priv->lms_base + MVPP2_PHY_AN_CFG0_REG); + } else { + val = readl(priv->iface_base + MVPP22_SMI_MISC_CFG_REG); + val &= ~MVPP22_SMI_POLLING_EN; + writel(val, priv->iface_base + MVPP22_SMI_MISC_CFG_REG); + } /* Allocate and initialize aggregated TXQs */ priv->aggr_txqs = devm_kcalloc(&pdev->dev, num_present_cpus(), @@ -6681,8 +6693,9 @@ static int mvpp2_init(struct platform_device *pdev, struct mvpp2 *priv) for (i = 0; i < MVPP2_MAX_PORTS; i++) mvpp2_write(priv, MVPP2_ISR_RXQ_GROUP_REG(i), rxq_number); - writel(MVPP2_EXT_GLOBAL_CTRL_DEFAULT, - priv->lms_base + MVPP2_MNG_EXTENDED_GLOBAL_CTRL_REG); + if (priv->hw_version == MVPP21) + writel(MVPP2_EXT_GLOBAL_CTRL_DEFAULT, + priv->lms_base + MVPP2_MNG_EXTENDED_GLOBAL_CTRL_REG); /* Allow cache snoop when transmiting packets */ mvpp2_write(priv, MVPP2_TX_SNOOP_REG, 0x1);
This commit handles a few miscellaneous differences between PPv2.1 and PPv2.2 in different areas, where code done for PPv2.1 doesn't apply for PPv2.2 or needs to be adjusted (getting the MAC address, disabling PHY polling, etc.). Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- drivers/net/ethernet/marvell/mvpp2.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)