Re: [OpenWrt-Devel] [PATCH] [RFC] ath79: ag71xx: apply interface mode to MII0/1_CNTL on ar71xx/ar913x
08/17/2018 09:45 PM, Martin Blumenstingl: On Thu, Aug 16, 2018 at 5:06 AM Chuanhong Guo wrote: Signed-off-by: Chuanhong Guo --- RFC: Previous discussion about this patch can be found on GitHub PR#1271. This patch applies correct interface mode to MII0/1_CNTL register at 0x1807/ 0x18070004. But there is a small difference in values for these two registers: | GMAC0| GMAC1 | |--|-| | 0 GMII | 0 RGMII | | 1 MII| 1 RMII | | 2 RGMII | | | 3 RMII | | I currently have 4 ways of dealing with this: 1. Use a bool value in dts indicating whether this is the second GMAC. This one is pretty dirty and I dropped it. 2. Split MII_CNTL into separated dt node and use different compatible for them like we did for ETH_CFG (gmac node) on ar933x and later SoCs. After some discussion on GitHub it turns out to be unreasonable to treat those in separated nodes. 3. Use ar7100-eth0/ar7100-eth1 as compatible string. This is what I've done in this patch I sent here. But I think my way of using compatible string here is ugly :( A possible cleaner implementation would be introducing ar7100-eth0/ar7100-eth1/ ar9130-eth0/ar9130-eth1 to replace ar7100-eth/ar9130-eth. But I doubt whether introducing 4 new compatible strings for such a slight difference is worthy. I think the naming "ar7100-eth0" / "ar7100-eth1" was my idea maybe it's not super intuitive since it reads as if the target Ethernet device will be called "eth0" or "eth1" - this is not what I meant with this naming. Yeah, I don't like the naming exactly because for the reason of easy misinterpreting. I wonder if "ar7100-mii0-eth" / "ar7100-mii1-eth" is any better. This one I like. other ideas are highly welcome Haha, nice try! Mathias ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH] [RFC] ath79: ag71xx: apply interface mode to MII0/1_CNTL on ar71xx/ar913x
The sender domain has a DMARC Reject/Quarantine policy which disallows sending mailing list messages using the original "From" header. To mitigate this problem, the original message has been wrapped automatically by the mailing list software.--- Begin Message --- On Thu, Aug 16, 2018 at 5:06 AM Chuanhong Guo wrote: > > Signed-off-by: Chuanhong Guo > --- > RFC: > Previous discussion about this patch can be found on GitHub PR#1271. > This patch applies correct interface mode to MII0/1_CNTL register at > 0x1807/ > 0x18070004. But there is a small difference in values for these two registers: > | GMAC0| GMAC1 | > |--|-| > | 0 GMII | 0 RGMII | > | 1 MII| 1 RMII | > | 2 RGMII | | > | 3 RMII | | > I currently have 4 ways of dealing with this: > 1. Use a bool value in dts indicating whether this is the second GMAC. This > one >is pretty dirty and I dropped it. > 2. Split MII_CNTL into separated dt node and use different compatible for them >like we did for ETH_CFG (gmac node) on ar933x and later SoCs. After some > discussion >on GitHub it turns out to be unreasonable to treat those in separated > nodes. > 3. Use ar7100-eth0/ar7100-eth1 as compatible string. This is what I've done in >this patch I sent here. But I think my way of using compatible string here > is ugly :( >A possible cleaner implementation would be introducing > ar7100-eth0/ar7100-eth1/ >ar9130-eth0/ar9130-eth1 to replace ar7100-eth/ar9130-eth. But I doubt > whether >introducing 4 new compatible strings for such a slight difference is > worthy. I think the naming "ar7100-eth0" / "ar7100-eth1" was my idea maybe it's not super intuitive since it reads as if the target Ethernet device will be called "eth0" or "eth1" - this is not what I meant with this naming. I wonder if "ar7100-mii0-eth" / "ar7100-mii1-eth" is any better. other ideas are highly welcome "There are only two hard things in Computer Science: cache invalidation and naming things." -- Phil Karlton Regards Martin --- End Message --- ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH] [RFC] ath79: ag71xx: apply interface mode to MII0/1_CNTL on ar71xx/ar913x
The sender domain has a DMARC Reject/Quarantine policy which disallows sending mailing list messages using the original "From" header. To mitigate this problem, the original message has been wrapped automatically by the mailing list software.--- Begin Message --- Hi, On Fri, Aug 17, 2018 at 12:05 PM Jonas Gorski wrote: > > Hi, > > On 16 August 2018 at 05:05, Chuanhong Guo wrote: > > Signed-off-by: Chuanhong Guo > > --- > > RFC: > > Previous discussion about this patch can be found on GitHub PR#1271. > > This patch applies correct interface mode to MII0/1_CNTL register at > > 0x1807/ > > 0x18070004. But there is a small difference in values for these two > > registers: > > | GMAC0| GMAC1 | > > |--|-| > > | 0 GMII | 0 RGMII | > > | 1 MII| 1 RMII | > > | 2 RGMII | | > > | 3 RMII | | > > I currently have 4 ways of dealing with this: > > 1. Use a bool value in dts indicating whether this is the second GMAC. This > > one > >is pretty dirty and I dropped it. > > 2. Split MII_CNTL into separated dt node and use different compatible for > > them > >like we did for ETH_CFG (gmac node) on ar933x and later SoCs. After some > > discussion > >on GitHub it turns out to be unreasonable to treat those in separated > > nodes. > > 3. Use ar7100-eth0/ar7100-eth1 as compatible string. This is what I've done > > in > >this patch I sent here. But I think my way of using compatible string > > here is ugly :( > >A possible cleaner implementation would be introducing > > ar7100-eth0/ar7100-eth1/ > >ar9130-eth0/ar9130-eth1 to replace ar7100-eth/ar9130-eth. But I doubt > > whether > >introducing 4 new compatible strings for such a slight difference is > > worthy. as already stated on the github PR I prefer option #3 > > 4. Somehow write all the possible values for each interface mode in device > > tree. > >e.g. qca,if-modes = "gmii","mii","rgmii","rmii"; for eth0 and > > qca,if-modes = "rgmii","rmii"; for eth1. this would encode register values in a devicetree property (index of each string is the register value) I believe that upstream devicetree maintainers don't want this anymore the rule of thumb is: devicetree should describe the hardware (not register contents or driver settings) because it should be "universal" (some bootloaders use it, Linux uses it, ...) if two devices are compatible they can use the same compatible string however, if they are different (even just slightly) they should get their own compatible string (see the stmmac bindings upstream for an example: Documentation/devicetree/bindings/net/stmmac.txt contains the original stmmac bindings while Documentation/devicetree/bindings/net/meson-dwmac.txt / Documentation/devicetree/bindings/net/stm32-dwmac.txt define new compatible strings because each vendor has defined it's own extensions to the original stmmac IP block) > > Which one is better? Is there any other possible solutions for this problem? > > I'd prefer 4, or a modified 3: > > do something like > > aliases { > > ethernet0 = ð0; > ethernet1 = ð1; > }; > > > and then you can find out if you are eth0 or eth1 by calling > > id = of_alias_get_id(node, "ethernet"); > > and maybe warn (and don't configure mii mode) if it returns neither 0 or 1. this is an interesting idea do you have any upstream example where this behavior was reviewed (and ACK'ed) by the upstream devicetree maintainers? (I found some examples relying on that exact behavior, but I'm not sure if the devicetree maintainers know about this code/want this solution to be used in the future) Regards Martin --- End Message --- ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH] [RFC] ath79: ag71xx: apply interface mode to MII0/1_CNTL on ar71xx/ar913x
Hi, On 16 August 2018 at 05:05, Chuanhong Guo wrote: > Signed-off-by: Chuanhong Guo > --- > RFC: > Previous discussion about this patch can be found on GitHub PR#1271. > This patch applies correct interface mode to MII0/1_CNTL register at > 0x1807/ > 0x18070004. But there is a small difference in values for these two registers: > | GMAC0| GMAC1 | > |--|-| > | 0 GMII | 0 RGMII | > | 1 MII| 1 RMII | > | 2 RGMII | | > | 3 RMII | | > I currently have 4 ways of dealing with this: > 1. Use a bool value in dts indicating whether this is the second GMAC. This > one >is pretty dirty and I dropped it. > 2. Split MII_CNTL into separated dt node and use different compatible for them >like we did for ETH_CFG (gmac node) on ar933x and later SoCs. After some > discussion >on GitHub it turns out to be unreasonable to treat those in separated > nodes. > 3. Use ar7100-eth0/ar7100-eth1 as compatible string. This is what I've done in >this patch I sent here. But I think my way of using compatible string here > is ugly :( >A possible cleaner implementation would be introducing > ar7100-eth0/ar7100-eth1/ >ar9130-eth0/ar9130-eth1 to replace ar7100-eth/ar9130-eth. But I doubt > whether >introducing 4 new compatible strings for such a slight difference is > worthy. > 4. Somehow write all the possible values for each interface mode in device > tree. >e.g. qca,if-modes = "gmii","mii","rgmii","rmii"; for eth0 and > qca,if-modes = "rgmii","rmii"; for eth1. > > Which one is better? Is there any other possible solutions for this problem? I'd prefer 4, or a modified 3: do something like aliases { ethernet0 = ð0; ethernet1 = ð1; }; and then you can find out if you are eth0 or eth1 by calling id = of_alias_get_id(node, "ethernet"); and maybe warn (and don't configure mii mode) if it returns neither 0 or 1. Regards Jonas > > target/linux/ath79/dts/ar7100.dtsi| 4 +- > target/linux/ath79/dts/ar9132.dtsi| 2 +- > .../net/ethernet/atheros/ag71xx/ag71xx_main.c | 61 +++ > 3 files changed, 64 insertions(+), 3 deletions(-) > > diff --git a/target/linux/ath79/dts/ar7100.dtsi > b/target/linux/ath79/dts/ar7100.dtsi > index 8994a7d688..89c17bcede 100644 > --- a/target/linux/ath79/dts/ar7100.dtsi > +++ b/target/linux/ath79/dts/ar7100.dtsi > @@ -171,7 +171,7 @@ > }; > > ð0 { > - compatible = "qca,ar7100-eth"; > + compatible = "qca,ar7100-eth0", "qca,ar7100-eth"; > reg = <0x1900 0x200 > 0x1807 0x4>; > > @@ -189,7 +189,7 @@ > }; > > ð1 { > - compatible = "qca,ar7100-eth"; > + compatible = "qca,ar7100-eth1", "qca,ar7100-eth"; > reg = <0x1a00 0x200 > 0x18070004 0x4>; > > diff --git a/target/linux/ath79/dts/ar9132.dtsi > b/target/linux/ath79/dts/ar9132.dtsi > index 9d8ddcf9ba..a136b06e69 100644 > --- a/target/linux/ath79/dts/ar9132.dtsi > +++ b/target/linux/ath79/dts/ar9132.dtsi > @@ -185,7 +185,7 @@ > }; > > ð0 { > - compatible = "qca,ar9130-eth", "syscon"; > + compatible = "qca,ar9130-eth", "qca,ar7100-eth0", "syscon"; > reg = <0x1900 0x200 > 0x1807 0x4>; > pll-data = <0x1a00 0x13000a44 0x00441099>; > diff --git > a/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c > b/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c > index 1e0bb6937f..5ea9ef40d2 100644 > --- > a/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c > +++ > b/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c > @@ -529,6 +529,60 @@ static void ath79_set_pll(struct ag71xx *ag) > udelay(100); > } > > +static void ath79_mii_ctrl_set_if(struct ag71xx *ag, unsigned int mii_if) > +{ > + u32 t; > + > + t = __raw_readl(ag->mii_base); > + t &= ~(AR71XX_MII_CTRL_IF_MASK); > + t |= (mii_if & AR71XX_MII_CTRL_IF_MASK); > + __raw_writel(t, ag->mii_base); > +} > + > +static void ath79_mii0_ctrl_set_if(struct ag71xx *ag) > +{ > + unsigned int mii_if; > + > + switch (ag->phy_if_mode) { > + case PHY_INTERFACE_MODE_MII: > + mii_if = AR71XX_MII0_CTRL_IF_MII; > + break; > + case PHY_INTERFACE_MODE_GMII: > + mii_if = AR71XX_MII0_CTRL_IF_GMII; > + break; > + case PHY_INTERFACE_MODE_RGMII: > + mii_if = AR71XX_MII0_CTRL_IF_RGMII; > + break; > + case PHY_INTERFACE_MODE_RMII: > + mii_if = AR71XX_MII0_CTRL_IF_RMII; > + break; > + default: > + WARN(1, "Impossible PHY mode defined.\n"); > + return; > + } > + > + ath79_mii_ctrl_set_if(ag, mii_if); > +} > + > +static void ath79_mii1_ctrl_set_if(struct ag71xx *ag) > +{ > + unsigned int mii_if; > + > + switch
[OpenWrt-Devel] [PATCH] [RFC] ath79: ag71xx: apply interface mode to MII0/1_CNTL on ar71xx/ar913x
Signed-off-by: Chuanhong Guo --- RFC: Previous discussion about this patch can be found on GitHub PR#1271. This patch applies correct interface mode to MII0/1_CNTL register at 0x1807/ 0x18070004. But there is a small difference in values for these two registers: | GMAC0| GMAC1 | |--|-| | 0 GMII | 0 RGMII | | 1 MII| 1 RMII | | 2 RGMII | | | 3 RMII | | I currently have 4 ways of dealing with this: 1. Use a bool value in dts indicating whether this is the second GMAC. This one is pretty dirty and I dropped it. 2. Split MII_CNTL into separated dt node and use different compatible for them like we did for ETH_CFG (gmac node) on ar933x and later SoCs. After some discussion on GitHub it turns out to be unreasonable to treat those in separated nodes. 3. Use ar7100-eth0/ar7100-eth1 as compatible string. This is what I've done in this patch I sent here. But I think my way of using compatible string here is ugly :( A possible cleaner implementation would be introducing ar7100-eth0/ar7100-eth1/ ar9130-eth0/ar9130-eth1 to replace ar7100-eth/ar9130-eth. But I doubt whether introducing 4 new compatible strings for such a slight difference is worthy. 4. Somehow write all the possible values for each interface mode in device tree. e.g. qca,if-modes = "gmii","mii","rgmii","rmii"; for eth0 and qca,if-modes = "rgmii","rmii"; for eth1. Which one is better? Is there any other possible solutions for this problem? target/linux/ath79/dts/ar7100.dtsi| 4 +- target/linux/ath79/dts/ar9132.dtsi| 2 +- .../net/ethernet/atheros/ag71xx/ag71xx_main.c | 61 +++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/target/linux/ath79/dts/ar7100.dtsi b/target/linux/ath79/dts/ar7100.dtsi index 8994a7d688..89c17bcede 100644 --- a/target/linux/ath79/dts/ar7100.dtsi +++ b/target/linux/ath79/dts/ar7100.dtsi @@ -171,7 +171,7 @@ }; ð0 { - compatible = "qca,ar7100-eth"; + compatible = "qca,ar7100-eth0", "qca,ar7100-eth"; reg = <0x1900 0x200 0x1807 0x4>; @@ -189,7 +189,7 @@ }; ð1 { - compatible = "qca,ar7100-eth"; + compatible = "qca,ar7100-eth1", "qca,ar7100-eth"; reg = <0x1a00 0x200 0x18070004 0x4>; diff --git a/target/linux/ath79/dts/ar9132.dtsi b/target/linux/ath79/dts/ar9132.dtsi index 9d8ddcf9ba..a136b06e69 100644 --- a/target/linux/ath79/dts/ar9132.dtsi +++ b/target/linux/ath79/dts/ar9132.dtsi @@ -185,7 +185,7 @@ }; ð0 { - compatible = "qca,ar9130-eth", "syscon"; + compatible = "qca,ar9130-eth", "qca,ar7100-eth0", "syscon"; reg = <0x1900 0x200 0x1807 0x4>; pll-data = <0x1a00 0x13000a44 0x00441099>; diff --git a/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c b/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c index 1e0bb6937f..5ea9ef40d2 100644 --- a/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c +++ b/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c @@ -529,6 +529,60 @@ static void ath79_set_pll(struct ag71xx *ag) udelay(100); } +static void ath79_mii_ctrl_set_if(struct ag71xx *ag, unsigned int mii_if) +{ + u32 t; + + t = __raw_readl(ag->mii_base); + t &= ~(AR71XX_MII_CTRL_IF_MASK); + t |= (mii_if & AR71XX_MII_CTRL_IF_MASK); + __raw_writel(t, ag->mii_base); +} + +static void ath79_mii0_ctrl_set_if(struct ag71xx *ag) +{ + unsigned int mii_if; + + switch (ag->phy_if_mode) { + case PHY_INTERFACE_MODE_MII: + mii_if = AR71XX_MII0_CTRL_IF_MII; + break; + case PHY_INTERFACE_MODE_GMII: + mii_if = AR71XX_MII0_CTRL_IF_GMII; + break; + case PHY_INTERFACE_MODE_RGMII: + mii_if = AR71XX_MII0_CTRL_IF_RGMII; + break; + case PHY_INTERFACE_MODE_RMII: + mii_if = AR71XX_MII0_CTRL_IF_RMII; + break; + default: + WARN(1, "Impossible PHY mode defined.\n"); + return; + } + + ath79_mii_ctrl_set_if(ag, mii_if); +} + +static void ath79_mii1_ctrl_set_if(struct ag71xx *ag) +{ + unsigned int mii_if; + + switch (ag->phy_if_mode) { + case PHY_INTERFACE_MODE_RMII: + mii_if = AR71XX_MII1_CTRL_IF_RMII; + break; + case PHY_INTERFACE_MODE_RGMII: + mii_if = AR71XX_MII1_CTRL_IF_RGMII; + break; + default: + WARN(1, "Impossible PHY mode defined.\n"); + return; + } + + ath79_mii_ctrl_set_if(ag, mii_if); +} + static void ath79_mii_ctrl_set_speed(struct ag71xx *ag) { unsigned int mii_speed; @@ -1427,6 +1481,13 @@ static int ag71xx_probe(struct platform_device *pdev) goto err_free; } + if (a