diff mbox

[PATCHv2,net-next,11/16] net: mvpp2: handle misc PPv2.1/PPv2.2 differences

Message ID 1482943592-12556-12-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Thomas Petazzoni Dec. 28, 2016, 4:46 p.m. UTC
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(-)

Comments

Russell King (Oracle) Jan. 7, 2017, 9:38 a.m. UTC | #1
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.)
Russell King (Oracle) Jan. 7, 2017, 11:03 a.m. UTC | #2
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.
Marcin Wojtas Jan. 7, 2017, 12:12 p.m. UTC | #3
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
Russell King (Oracle) Jan. 7, 2017, 1:50 p.m. UTC | #4
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.
Thomas Petazzoni Feb. 14, 2017, 2:53 p.m. UTC | #5
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 mbox

Patch

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);