| Message ID | 20180816030522.9634-1-gch981213@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [OpenWrt-Devel,RFC] ath79: ag71xx: apply interface mode to MII0/1_CNTL on ar71xx/ar913x | expand |
Hi, On 16 August 2018 at 05:05, Chuanhong Guo <gch981213@gmail.com> wrote: > Signed-off-by: Chuanhong Guo <gch981213@gmail.com> > --- > 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 0x18070000/ > 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 = <0x19000000 0x200 > 0x18070000 0x4>; > > @@ -189,7 +189,7 @@ > }; > > ð1 { > - compatible = "qca,ar7100-eth"; > + compatible = "qca,ar7100-eth1", "qca,ar7100-eth"; > reg = <0x1a000000 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 = <0x19000000 0x200 > 0x18070000 0x4>; > pll-data = <0x1a000000 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 (ag->mii_base) { > + if (of_device_is_compatible(np, "qca,ar7100-eth0")) > + ath79_mii0_ctrl_set_if(ag); > + else if (of_device_is_compatible(np, "qca,ar7100-eth1")) > + ath79_mii1_ctrl_set_if(ag); > + } > + > netif_napi_add(dev, &ag->napi, ag71xx_poll, AG71XX_NAPI_WEIGHT); > > ag71xx_wr(ag, AG71XX_REG_MAC_CFG1, 0); > -- > 2.17.1 > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
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. Hi, On Fri, Aug 17, 2018 at 12:05 PM Jonas Gorski <jonas.gorski@gmail.com> wrote: > > Hi, > > On 16 August 2018 at 05:05, Chuanhong Guo <gch981213@gmail.com> wrote: > > Signed-off-by: Chuanhong Guo <gch981213@gmail.com> > > --- > > 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 0x18070000/ > > 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
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. On Thu, Aug 16, 2018 at 5:06 AM Chuanhong Guo <gch981213@gmail.com> wrote: > > Signed-off-by: Chuanhong Guo <gch981213@gmail.com> > --- > 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 0x18070000/ > 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
08/17/2018 09:45 PM, Martin Blumenstingl: > On Thu, Aug 16, 2018 at 5:06 AM Chuanhong Guo <gch981213@gmail.com> wrote: >> >> Signed-off-by: Chuanhong Guo <gch981213@gmail.com> >> --- >> 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 0x18070000/ >> 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
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 = <0x19000000 0x200 0x18070000 0x4>; @@ -189,7 +189,7 @@ }; ð1 { - compatible = "qca,ar7100-eth"; + compatible = "qca,ar7100-eth1", "qca,ar7100-eth"; reg = <0x1a000000 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 = <0x19000000 0x200 0x18070000 0x4>; pll-data = <0x1a000000 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 (ag->mii_base) { + if (of_device_is_compatible(np, "qca,ar7100-eth0")) + ath79_mii0_ctrl_set_if(ag); + else if (of_device_is_compatible(np, "qca,ar7100-eth1")) + ath79_mii1_ctrl_set_if(ag); + } + netif_napi_add(dev, &ag->napi, ag71xx_poll, AG71XX_NAPI_WEIGHT); ag71xx_wr(ag, AG71XX_REG_MAC_CFG1, 0);
Signed-off-by: Chuanhong Guo <gch981213@gmail.com> --- 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 0x18070000/ 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(-)