From: Russell King - ARM Linux <linux@armlinux.org.uk> To: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Cc: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>, devicetree@vger.kernel.org, Rob Herring <robh+dt@kernel.org>, Ian Campbell <ijc+devicetree@hellion.org.uk>, Pawel Moll <pawel.moll@arm.com>, Mark Rutland <mark.rutland@arm.com>, Kumar Gala <galak@codeaurora.org>, Andrew Lunn <andrew@lunn.ch>, Yehuda Yitschak <yehuday@marvell.com>, Jason Cooper <jason@lakedaemon.net>, Hanna Hawa <hannah@marvell.com>, Nadav Haklai <nadavh@marvell.com>, Gregory Clement <gregory.clement@free-electrons.com>, Stefan Chulski <stefanc@marvell.com>, Marcin Wojtas <mw@semihalf.com>, linux-arm-kernel@lists.infradead.org, Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> Subject: Re: [PATCHv2 net-next 11/16] net: mvpp2: handle misc PPv2.1/PPv2.2 differences Date: Sat, 7 Jan 2017 11:03:51 +0000 [thread overview] Message-ID: <20170107110351.GK14217@n2100.armlinux.org.uk> (raw) In-Reply-To: <1482943592-12556-12-git-send-email-thomas.petazzoni@free-electrons.com> 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. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
WARNING: multiple messages have this Message-ID (diff)
From: linux@armlinux.org.uk (Russell King - ARM Linux) To: linux-arm-kernel@lists.infradead.org Subject: [PATCHv2 net-next 11/16] net: mvpp2: handle misc PPv2.1/PPv2.2 differences Date: Sat, 7 Jan 2017 11:03:51 +0000 [thread overview] Message-ID: <20170107110351.GK14217@n2100.armlinux.org.uk> (raw) In-Reply-To: <1482943592-12556-12-git-send-email-thomas.petazzoni@free-electrons.com> 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. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
next prev parent reply other threads:[~2017-01-07 11:04 UTC|newest] Thread overview: 92+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-12-28 16:46 [PATCHv2 net-next 00/16] net: mvpp2: add basic support for PPv2.2 Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni [not found] ` <1482943592-12556-1-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2016-12-28 16:46 ` [PATCHv2 net-next 01/16] dt-bindings: net: update Marvell PPv2 binding for PPv2.2 support Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni [not found] ` <1482943592-12556-2-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-01-03 20:18 ` Rob Herring 2017-01-03 20:18 ` Rob Herring 2017-02-02 16:56 ` Thomas Petazzoni 2017-02-02 16:56 ` Thomas Petazzoni 2017-02-03 16:48 ` Russell King - ARM Linux 2017-02-03 16:48 ` Russell King - ARM Linux 2017-02-14 14:25 ` Thomas Petazzoni 2017-02-14 14:25 ` Thomas Petazzoni [not found] ` <20170214152503.602878cb-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-02-21 10:12 ` Thomas Petazzoni 2017-02-21 10:12 ` Thomas Petazzoni 2017-01-07 9:32 ` Russell King - ARM Linux 2017-01-07 9:32 ` Russell King - ARM Linux 2017-02-02 16:44 ` Thomas Petazzoni 2017-02-02 16:44 ` Thomas Petazzoni 2016-12-28 16:46 ` [PATCHv2 net-next 02/16] net: mvpp2: add and use accessors for TX/RX descriptors Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni 2016-12-28 16:46 ` [PATCHv2 net-next 03/16] net: mvpp2: add hw_version field in "struct mvpp2" Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni 2016-12-28 16:46 ` [PATCHv2 net-next 07/16] net: mvpp2: adapt the mvpp2_rxq_*_pool_set functions to PPv2.2 Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni 2016-12-28 16:46 ` [PATCHv2 net-next 14/16] net: mvpp2: adapt rxq distribution " Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni 2016-12-28 16:46 ` [PATCHv2 net-next 16/16] net: mvpp2: finally add the PPv2.2 compatible string Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni 2016-12-28 17:06 ` [PATCHv2 net-next 00/16] net: mvpp2: add basic support for PPv2.2 David Miller 2016-12-28 17:06 ` David Miller [not found] ` <20161228.120644.1166014191192724301.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> 2016-12-28 21:08 ` Thomas Petazzoni 2016-12-28 21:08 ` Thomas Petazzoni 2016-12-28 16:46 ` [PATCHv2 net-next 04/16] net: mvpp2: introduce an intermediate union for the TX/RX descriptors Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni 2016-12-28 16:46 ` [PATCHv2 net-next 05/16] net: mvpp2: introduce PPv2.2 HW descriptors and adapt accessors Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni 2017-01-06 14:29 ` Russell King - ARM Linux 2017-01-06 14:29 ` Russell King - ARM Linux 2017-01-06 14:44 ` Robin Murphy 2017-01-06 14:44 ` Robin Murphy [not found] ` <113811b6-79a4-9c66-d302-add9fb0c5b1a-5wv7dgnIgG8@public.gmane.org> 2017-02-03 13:24 ` Thomas Petazzoni 2017-02-03 13:24 ` Thomas Petazzoni 2017-02-03 14:05 ` Robin Murphy 2017-02-03 14:05 ` Robin Murphy [not found] ` <231514f4-2e35-8bde-4469-aada833635aa-5wv7dgnIgG8@public.gmane.org> 2017-02-03 15:02 ` Thomas Petazzoni 2017-02-03 15:02 ` Thomas Petazzoni [not found] ` <20170203160227.08b40c58-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-02-03 16:31 ` Robin Murphy 2017-02-03 16:31 ` Robin Murphy 2017-02-03 15:54 ` Russell King - ARM Linux 2017-02-03 15:54 ` Russell King - ARM Linux 2017-02-04 13:59 ` Thomas Petazzoni 2017-02-04 13:59 ` Thomas Petazzoni 2017-02-06 12:43 ` David Laight 2017-02-06 12:43 ` David Laight 2017-02-06 12:43 ` David Laight 2016-12-28 16:46 ` [PATCHv2 net-next 06/16] net: mvpp2: adjust the allocation/free of BM pools for PPv2.2 Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni 2017-01-06 14:32 ` Russell King - ARM Linux 2017-01-06 14:32 ` Russell King - ARM Linux 2016-12-28 16:46 ` [PATCHv2 net-next 08/16] net: mvpp2: adapt mvpp2_defaults_set() to PPv2.2 Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni 2016-12-28 16:46 ` [PATCHv2 net-next 09/16] net: mvpp2: adjust mvpp2_{rxq,txq}_init for PPv2.2 Thomas Petazzoni 2016-12-28 16:46 ` [PATCHv2 net-next 09/16] net: mvpp2: adjust mvpp2_{rxq, txq}_init " Thomas Petazzoni 2016-12-28 16:46 ` [PATCHv2 net-next 10/16] net: mvpp2: handle register mapping and access " Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni 2017-01-06 14:46 ` Russell King - ARM Linux 2017-01-06 14:46 ` Russell King - ARM Linux [not found] ` <20170106144648.GE14217-l+eeeJia6m9URfEZ8mYm6t73F7V6hmMc@public.gmane.org> 2017-03-02 8:45 ` Thomas Petazzoni 2017-03-02 8:45 ` Thomas Petazzoni 2016-12-28 16:46 ` [PATCHv2 net-next 11/16] net: mvpp2: handle misc PPv2.1/PPv2.2 differences Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni 2017-01-07 9:38 ` Russell King - ARM Linux 2017-01-07 9:38 ` Russell King - ARM Linux [not found] ` <20170107093834.GJ14217-l+eeeJia6m9URfEZ8mYm6t73F7V6hmMc@public.gmane.org> 2017-01-07 20:10 ` Russell King - ARM Linux 2017-01-07 20:10 ` Russell King - ARM Linux 2017-02-14 14:53 ` Thomas Petazzoni 2017-02-14 14:53 ` Thomas Petazzoni 2017-01-07 11:03 ` Russell King - ARM Linux [this message] 2017-01-07 11:03 ` Russell King - ARM Linux 2017-01-07 12:12 ` Marcin Wojtas 2017-01-07 12:12 ` Marcin Wojtas [not found] ` <CAPv3WKeQ=fj2cKPyJ2NqCaAv55cOyWodujKwj3-v5iCrDYNcmA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-01-07 13:50 ` Russell King - ARM Linux 2017-01-07 13:50 ` Russell King - ARM Linux 2017-01-07 13:50 ` Russell King - ARM Linux 2016-12-28 16:46 ` [PATCHv2 net-next 12/16] net: mvpp2: add AXI bridge initialization for PPv2.2 Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni 2016-12-28 16:46 ` [PATCHv2 net-next 13/16] net: mvpp2: rework RXQ interrupt group " Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni 2016-12-28 16:46 ` [PATCHv2 net-next 15/16] net: mvpp2: add support for an additional clock needed " Thomas Petazzoni 2016-12-28 16:46 ` Thomas Petazzoni [not found] ` <1482943592-12556-16-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-01-07 9:29 ` Russell King - ARM Linux 2017-01-07 9:29 ` Russell King - ARM Linux
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170107110351.GK14217@n2100.armlinux.org.uk \ --to=linux@armlinux.org.uk \ --cc=andrew@lunn.ch \ --cc=davem@davemloft.net \ --cc=devicetree@vger.kernel.org \ --cc=galak@codeaurora.org \ --cc=gregory.clement@free-electrons.com \ --cc=hannah@marvell.com \ --cc=ijc+devicetree@hellion.org.uk \ --cc=jason@lakedaemon.net \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=mark.rutland@arm.com \ --cc=mw@semihalf.com \ --cc=nadavh@marvell.com \ --cc=netdev@vger.kernel.org \ --cc=pawel.moll@arm.com \ --cc=robh+dt@kernel.org \ --cc=sebastian.hesselbarth@gmail.com \ --cc=stefanc@marvell.com \ --cc=thomas.petazzoni@free-electrons.com \ --cc=yehuday@marvell.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.