ath79: ag71xx: apply interface mode to MII0/1_CTRL on ar71xx/ar913x #1271

981213 · 2018-08-11T13:50:50Z

Check commit messages for description :-)

xdarklight · 2018-08-11T14:16:43Z

target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c

+			mii_if = AR71XX_MII1_CTRL_IF_RGMII;
+			break;
+		default:
+			BUG();


https://elixir.bootlin.com/linux/v4.17/source/include/asm-generic/bug.h#L42

Don't use BUG() or BUG_ON() unless there's really no way out [...]
If the (sub)system can somehow continue operating, perhaps with reduced functionality, it's probably not BUG-worthy.

xdarklight · 2018-08-11T14:20:03Z

target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c

@@ -1427,6 +1468,9 @@ static int ag71xx_probe(struct platform_device *pdev)
 		goto err_free;
 	}

+	if (ag->mii_base)
+		ath79_mii_ctrl_set_if(ag, of_property_read_bool(np, "qca,is-mii1"));


are you aware of any other differences between MII0 and MII1?

upstream drivers typically uses a different "compatible" property per actual IP block
I would like to avoid having a "qca,is-mac1" for example property later on, just because there's more differences in other registers

And why eth1 is marked as "qca,is-mii1"? On another device eth0 may be connected this way.

mkresin · 2018-08-11T14:20:53Z

Would you please split the "ath79: ag71xx: fix MII0/1_CTRL settings on ar71xx/ar913x" into multiple atomic changes.

One issue that gets fixed is that the speed settings are written to the interface related bits of the register.

The other change adds the so far missing function that actually sets the interface mode related bits of the register.

I don't really like the "qca,is-mii1" property. But it doesn't look like I'll have the time to check for a better way of doing it today. Just to let you know.

981213 · 2018-08-11T14:41:35Z

What about using a separated dt node for MII_CNTL like the qca,ar9330-gmac in ar9330.dtsi?
The ethernet registers (0x19000000/0x1A000000) are identical (The description for them are written only once in datasheet) but MII0/1_CNTL are written in another section. It seems that MII0/1_CNTL are separated IP blocks for configuring interface mode.

mkresin · 2018-08-11T14:44:53Z

What about using a separated dt node for MII_CNTL like the qca,ar9330-gmac in ar9330.dtsi?

Assuming you mean different compatible, that might work. It is really hard to give a good advice without having the code/datasheets checked so far.

981213 · 2018-08-11T14:56:50Z

I mean something like this in dts:

diff --git a/target/linux/ath79/dts/ar7100.dtsi b/target/linux/ath79/dts/ar7100.dtsi
index 5b9a84197f..5ba887e43b 100644
--- a/target/linux/ath79/dts/ar7100.dtsi
+++ b/target/linux/ath79/dts/ar7100.dtsi
@@ -96,6 +96,16 @@
                                #reset-cells = <1>;
                        };
 
+                       mii0: mii@18070000 {
+                               compatible = "qca,ar7100-mii0";
+                               reg = <0x18070000 0x4>;
+                       };
+
+                       mii1: mii@18070004 {
+                               compatible = "qca,ar7100-mii1";
+                               reg = <0x18070004 0x4>;
+                       };
+
                        pcie0: pcie-controller@180c0000 {
                                compatible = "qca,ar7100-pci";
                                #address-cells = <3>;
@@ -172,8 +182,6 @@
 
 &eth0 {
        compatible = "qca,ar7100-eth";
-       reg = <0x19000000 0x200
-               0x18070000 0x4>;
 
        pll-data = <0x00110000 0x00001099 0x00991099>;
        pll-reg = <0x4 0x10 17>;
@@ -182,6 +190,7 @@
 
        resets = <&rst 9>;
        reset-names = "mac";
+       qca,mii-dev = <&mii0>;
 };
 
 &mdio1 {
@@ -190,8 +199,6 @@
 
 &eth1 {
        compatible = "qca,ar7100-eth";
-       reg = <0x1a000000 0x200
-               0x18070004 0x4>;
 
        pll-data = <0x00110000 0x00001099 0x00991099>;
        pll-reg = <0x4 0x14 19>;
@@ -201,4 +208,5 @@
 
        resets = <&rst 13>;
        reset-names = "mac";
+       qca,mii-dev = <&mii1>;
 };

and use of_iomap/of_iounmap in driver.

mkresin · 2018-08-11T15:13:44Z

looks way cleaner to me. Am i right that reg = <0x19000000 0x200> and reg = <0x1a000000 0x200> need to be kept?

xdarklight · 2018-08-11T15:23:02Z

I mean something like this in dts ...

devicetree should describe the hardware
from my understanding the "eth0" and "eth1" nodes are each describing a MAC/Ethernet controller, while "mdio0" and "mdio1" are each describing an MDIO bus
however, is "MII" it's own device? from my understanding it configures the Ethernet controller's interface type (MII, RMII, GMII, RGMII) and interface speed (10/100/1000 Mbit/s).

if it's not a separate device (but just a separate register because the hardware designers thought it was a good idea to keep it separate) then I would only change the Ethernet controller's compatible string from "qca,ar7100-eth" to "qca,ar7100-eth0" and/or "qca,ar7100-eth1"

I am not very familiar with the ath79 hardware
how would you describe the hardware/IP blocks in this case? based on that we can work out a devicetree binding for it

981213 · 2018-08-11T16:09:55Z

@mkresin

Am i right that reg = <0x19000000 0x200> and reg = <0x1a000000 0x200> need to be kept?

They are defined in ath79.dtsi

@xdarklight
According to address map in datasheet, 0x18000000-0x19000000 belongs to APB bus and both ethernet blocks belongs to AHB bus. I think the only reason why Atheros put MII0/1_CTRL explicitly on a separated bus is that MII0/1_CTRL is actually a separated IP block.

But...

from my understanding it configures the Ethernet controller's interface type (MII, RMII, GMII, RGMII) and interface speed (10/100/1000 Mbit/s).

...since this is the only thing this address space is used for, I don't know why there would be such a separated IP block used to configure only two parameters.
For other ath79 SoCs 0x18070000 is called ETH_CFG in datasheet and it configures interface type of two gmacs / whether a gmac is connected to builtin switch / how PHYs connected to builtin switch etc, mixing up many ethernet related configurations.

how would you describe the hardware/IP blocks in this case?

I might describe this block on ar7100 as a multiplexer (like pinmux of gpio) but I don't know whether it's suitable.

xdarklight · 2018-08-11T17:06:00Z

I might describe this block on ar7100 as a multiplexer (like pinmux of gpio) but I don't know whether it's suitable.

the interface type bits seem like a pinmux controller
the speed configuration bits may simply be a clock divider or duty cycle control, but this would require knowledge of the clock hierarchy (what is the parent of that divider/duty cycle controller)?
it seems that we don't know much about the network PLLs there are raw register values in devicetree for the Ethernet PLL

if you choose to go with that we may end up with something like this:

mii0: mii@18070000 {
    compatible = "qca,ar7100-mii0";
    reg = <0x18070000 0x4>;
    #clock-cells = <0>; /* TODO: what is the parent clock of this clock controller? */
    #pinctrl-cells = <1>;
};

eth0: ethernet@19000000 {
...
    reg = <0x19000000 0x200>;

    pinctrl-names = "mii", "rmii", "gmii", "rgmii";
    pinctrl-0 = <&mii0 MII_CNTL_MII>;
    pinctrl-1 = <&mii0 MII_CNTL_RMII>;
    pinctrl-2 = <&mii0 MII_CNTL_GMII>;
    pinctrl-3 = <&mii0 MII_CNTL_RGMII>;

    clock-names = "txclk";
    clocks = <&mii0>; /* TODO: do we need to call clk_set_rate() or clk_set_duty_cycle() in the Ethernet driver? */
..
};

does the datasheet say anything about the missing information we need to solve these TODOs?

981213 · 2018-08-11T17:27:23Z

Here is a leaked datasheet found on GitHub: https://github.com/Deoptim/atheros/raw/master/AR7100_nowatermark.pdf
The clock is configured at 0x18050010, a separated PLL Control register. (And we currently set raw working values for PLL registers in ag71xx.)
As for speed set at 0x18070000 (on page 76) it only says "interface speed" and there is no other information about what this is used for.

xdarklight · 2018-08-11T17:59:25Z

As for speed set at 0x18070000 (on page 76) it only says "interface speed" and there is no other information about what this is used for.

if it's the actual "interface speed" then I would say that the register belongs to the MAC
do you have a oscilloscope and two boards (one with MII/RMII and another one with GMII/RGMII PHY) to check the signal? random guess: I'd say it changes the TX clock, which means:

speed is not relevant for MII or RMII PHYs
GTXCLK on GMII PHYs or TXC on RGMII PHYs (125MHz for Gbit connections, 25MHz for 100Mbit connections and 2.5MHz for 10Mbit connections)

you can also get feedback from the devicetree maintainers which are reviewing the dt-bindings in the kernel by emailing devicetree@vger.kernel.org
just make sure you explain everything they need to know (as I assume that they prefer email over "github comments")

981213 · 2018-08-12T06:28:21Z

do you have a oscilloscope

Ah... I don't have these things :(

random guess: I'd say it changes the TX clock

I guess that's what PLL registers did. We have three raw values for PLL registers corresponding to 1G/100M/10M.

I've updated the code and used qca,ar7100-eth0/1 as compatible string now.

981213 · 2018-08-12T06:32:56Z

The patch fixing speed settings can be applied first if there are still problems with interface mode. Fixing speed settings will get devices with correct interface mode set by u-boot work.
PS: I doubt whether my way of using compatible string here is correct.

hanipouspilot · 2018-08-12T12:19:37Z

Something is wrong on ar913x on tp-link 1043v1

If I write anything to 0x18070000, then it resets to 0x00000000.

Same with pll-reg 0xb8050010

Writing to 0xb8050014 works, but I am not sure which is the correct reg.

mkresin · 2018-08-12T12:31:35Z

Not really wrong. You need to bring the pll into reset to change the values. You remember the three magic register writes in one if the set pll functions. That is what they are supposed to do.

…

Am 12.08.2018 um 14:19 schrieb Dmitry Tunin ***@***.***>: Something is wrong on ar913x on tp-link 1043v1 If I write anything to 0x18070000, then it resets to 0x00000000. Same with pll-reg 0xb8050010 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

hanipouspilot · 2018-08-12T12:37:34Z

But it worked the same way on dir-825. These are mii registers.

981213 · 2018-08-12T12:44:02Z

Can't reproduce on ar9132 (TL-WR941N v2)

root@OpenWrt:/# devmem 0x18070000
0x00000013
root@OpenWrt:/# devmem 0x18070000 32 0x0
root@OpenWrt:/# devmem 0x18070000
0x00000000
root@OpenWrt:/# devmem 0x18070000 32 0x13
root@OpenWrt:/# devmem 0x18070000
0x00000013
root@OpenWrt:/# devmem 0x18070004
0x00000000
root@OpenWrt:/# devmem 0x18070004 32 0x10
root@OpenWrt:/# devmem 0x18070004
0x00000010
root@OpenWrt:/#

hanipouspilot · 2018-08-12T12:48:21Z

And what are the correct values for a fixed rgmii link 1000? вс, 12 авг. 2018 г. в 15:44, 蝈蝈 <notifications@github.com>:

…

Can't reproduce on ar9132 (TL-WR941N v2) ***@***.***:/# devmem 0x18070000 0x00000013 ***@***.***:/# devmem 0x18070000 32 0x0 ***@***.***:/# devmem 0x18070000 0x00000000 ***@***.***:/# devmem 0x18070000 32 0x13 ***@***.***:/# devmem 0x18070000 0x00000013 ***@***.***:/# devmem 0x18070004 0x00000000 ***@***.***:/# devmem 0x18070004 32 0x10 ***@***.***:/# devmem 0x18070004 0x00000010 ***@***.***:/# — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1271 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJYkt5JJwVr0-vxA_DYC-DU-w_whQCWwks5uQCMagaJpZM4V5LIY> .

hanipouspilot · 2018-08-12T12:51:21Z

Now I have

root@OpenWrt:~# devmem  0x18070000
0x00000012
root@OpenWrt:~# devmem  0x18070004
0x00000011

981213 · 2018-08-12T12:51:37Z

The same as AR7100, it's 0x22. (Code in ar71xx for MII_CNTL is in arch/mips/ath79/dev-eth.c)

hanipouspilot · 2018-08-12T12:52:14Z

So It's 0x22 and 0x10?

981213 · 2018-08-12T12:54:17Z

It seemed that openwrt doesn't use second GMAC on AR9132 for WR1043ND v1 so we don't need to set value at 0x18070004. (I don't know whether factory firmware used GMAC1)

hanipouspilot · 2018-08-12T12:58:01Z

I am confused with which is used. I added debug by mkresin and get this [ 49.068378] MKR: ath79_set_pllval() [ 49.071888] MKR: 0xb8050010 init value: 0x00000000 [ 49.076702] MKR: 0xb8050014 init value: 0x13000a44 [ 49.081513] MKR: speed 1000 [ 49.084328] MKR: wrote 0x1a000000 to offset 0x00000010 and got return code 0 [ 49.091411] MKR: 0xb8050010 final value: 0x00000000 [ 49.096312] MKR: 0xb8050014 final value: 0x13000a44 [ 49.101414] MKR: 0xb8050010 final value: 0x00000000 [ 49.106314] MKR: 0xb8050014 final value: 0x13000a44 [ 49.111223] eth0: link up (1000Mbps/Full duplex) It looks like we are writing to a wrong reg.

…

It seemed that openwrt doesn't use second GMAC on AR9132 for WR1043ND v1 so we don't need to set value at 0x18070004. (I don't know whether factory firmware used GMAC1) — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

hanipouspilot · 2018-08-12T13:07:03Z

@mkresin Not really wrong. You need to bring the pll into reset to change the values. You remember the three magic register writes in one if the set pll functions. That is what they are supposed to do.

But it seems that they do it a wrong way, because we get 0x0 in the register.

981213 · 2018-08-12T13:08:25Z

I suggest reading dev-eth.c once.
And you'll find that you are right :)

hanipouspilot · 2018-08-12T13:26:10Z

@981213 I already tried to change the pll-reg, but the switch didn't work. I'll try to build it again clean, maybe I broke something else.

hanipouspilot · 2018-08-12T13:42:32Z

So I was correct. I broke something. After resetting the tree, the pll works OK.
I manually wrote 0x22, so I am testing your other patches.

981213 · 2018-08-12T13:56:27Z

My commit is still wrong.... PLL shift is 20 instead of 17.

hanipouspilot · 2018-08-12T13:57:36Z

But it worked with 17.

hanipouspilot · 2018-08-12T14:08:58Z

@981213 So I built an image for 1043-v1.
pll-reg is <0x4 0x14 17>
and I used only the speed patch.

All works well.

hanipouspilot · 2018-08-12T14:22:13Z

And it is really 17 in arch/mips/ath79/dev-eth.c

#define AR71XX_ETH0_PLL_SHIFT 17

in ar71xx_regs.h

981213 · 2018-08-12T14:30:03Z

That AR71XX means AR7130/AR7141/AR7161
There is another definition for AR913X. It's not 17 in dev-eth.c:

static void ar91xx_set_speed_ge0(int speed)
{
	u32 val = ath79_get_eth_pll(0, speed);

	ath79_set_pll(AR913X_PLL_REG_ETH_CONFIG, AR913X_PLL_REG_ETH0_INT_CLOCK,
			 val, AR913X_ETH0_PLL_SHIFT);
	ath79_mii_ctrl_set_speed(AR71XX_MII_REG_MII0_CTRL, speed);
}

static void ar91xx_set_speed_ge1(int speed)
{
	u32 val = ath79_get_eth_pll(1, speed);

	ath79_set_pll(AR913X_PLL_REG_ETH_CONFIG, AR913X_PLL_REG_ETH1_INT_CLOCK,
			 val, AR913X_ETH1_PLL_SHIFT);
	ath79_mii_ctrl_set_speed(AR71XX_MII_REG_MII1_CTRL, speed);
}

	case ATH79_SOC_AR9132:
		pdata->has_gbit = 1;
		/* fall through */
	case ATH79_SOC_AR9130:
		if (id == 0)
			pdata->set_speed = ar91xx_set_speed_ge0;
		else
			pdata->set_speed = ar91xx_set_speed_ge1;
		pdata->is_ar91xx = 1;
		break;

hanipouspilot · 2018-08-12T14:33:30Z

Mine is AR9132 with gigabit. We need to set the reg per device then.

hanipouspilot · 2018-08-12T14:38:42Z

Oh, it is fall through. Hm..

hanipouspilot · 2018-08-12T14:56:48Z

So it works with both 17, or 20 shift.

mkresin · 2018-08-13T06:44:19Z

Picked two more commits from the PR. Please rebase on master.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>

981213 · 2018-08-14T12:48:01Z

Honestly I think my approach here is ugly :(
Closed since I guess most U-boot will configure interface mode correctly and I don't have any ideas cleaner than my current one.

hanipouspilot · 2018-08-15T19:52:47Z

@mkresin @xdarklight
So we have a problem on WNDR7x00 devices. mii-if mode is not set correctly.
Actually eth0 and eth1 on ar7100 are not 100% "compatible". They have different ways of setting phy-mode.
So I suggest instead of having ar7100-eth, to split it into ar7100-eth0 and ar7100-eth1 as it was already done in the unmerged commit. But do it everywhere (replace ar7100-eth), not to have two compatible items.
In this case it doesn't look ugly at all.

hnyman · 2018-08-15T20:45:00Z

Yep, looks like the first two merged commits require also this third unmerged commit at least for the WNDR3700 series. It fixes wan (1000 Mbit link) on my WNDR3800.

(I had tested just the wifi and LED things in the past two days with a standalone AP, and did not notice that wan on WNDR3x00 is now broken :-(

hanipouspilot · 2018-08-15T20:47:54Z

@hnyman It was always broken. You probably built with this patch and everyone had an impression it's not really needed for your device. DIR-825 works without it, becuase u-boot sets the mode.

981213 · 2018-08-16T02:36:11Z

But do it everywhere (replace ar7100-eth), not to have two compatible items.
In this case it doesn't look ugly at all.

In this way we'll have to introduce 4 compatible strings (ar7100-eth0/ar7100-eth1/ar9130-eth0/ar9130-eth1) for only one register difference, which I think may noy be worthy.
I'll make this RFC on mailing list.

hanipouspilot · 2018-08-16T05:23:29Z

@981213 Why not of the devices are really different?
Another way with getting address is much more hacky. Thre was someting like 'of_get_address()'.

What is wrong to have different compatible strings for really different devices? It should be seen in DTS that they are different, not hidden in the driver.

981213 · 2018-08-16T13:51:53Z

I'll implement the "compatible string" one next Monday if there are no more feedback from mail list.

981213 mentioned this pull request Aug 11, 2018

ath79: WNDR3800: add wifi #1258

Closed

xdarklight reviewed Aug 11, 2018

View reviewed changes

981213 changed the title ~~ath79: fix pll for ar913x and fix MII0/1_CTRL value for ar71xx/ar913x~~ [WiP] ath79: fix pll for ar913x and fix MII0/1_CTRL value for ar71xx/ar913x Aug 11, 2018

mkresin added target/ath79 pull request/issue for ath79 target work in progress pull request the author is still working on labels Aug 11, 2018

981213 force-pushed the ath79_941 branch from 4a05c89 to baa1a5d Compare August 12, 2018 06:25

981213 changed the title ~~[WiP] ath79: fix pll for ar913x and fix MII0/1_CTRL value for ar71xx/ar913x~~ ath79: fix pll for ar913x and fix MII0/1_CTRL value for ar71xx/ar913x Aug 12, 2018

981213 force-pushed the ath79_941 branch from baa1a5d to c7e42ff Compare August 12, 2018 13:15

981213 force-pushed the ath79_941 branch from c7e42ff to 680771b Compare August 12, 2018 13:58

ath79: ag71xx: apply interface mode to MII0/1_CTRL on ar71xx/ar913x

7416bf6

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>

981213 force-pushed the ath79_941 branch from 680771b to 7416bf6 Compare August 13, 2018 07:28

981213 changed the title ~~ath79: fix pll for ar913x and fix MII0/1_CTRL value for ar71xx/ar913x~~ ath79: ag71xx: apply interface mode to MII0/1_CTRL on ar71xx/ar913x Aug 13, 2018

981213 closed this Aug 14, 2018

ath79: ag71xx: apply interface mode to MII0/1_CTRL on ar71xx/ar913x #1271

ath79: ag71xx: apply interface mode to MII0/1_CTRL on ar71xx/ar913x #1271

Conversation

981213 commented Aug 11, 2018

xdarklight Aug 11, 2018

Choose a reason for hiding this comment

xdarklight Aug 11, 2018

Choose a reason for hiding this comment

hanipouspilot Aug 11, 2018

Choose a reason for hiding this comment

mkresin commented Aug 11, 2018

981213 commented Aug 11, 2018 • edited Loading

mkresin commented Aug 11, 2018

981213 commented Aug 11, 2018

mkresin commented Aug 11, 2018

xdarklight commented Aug 11, 2018

981213 commented Aug 11, 2018 • edited Loading

xdarklight commented Aug 11, 2018

981213 commented Aug 11, 2018

xdarklight commented Aug 11, 2018

981213 commented Aug 12, 2018

981213 commented Aug 12, 2018

hanipouspilot commented Aug 12, 2018 • edited Loading

mkresin commented Aug 12, 2018 via email

hanipouspilot commented Aug 12, 2018

981213 commented Aug 12, 2018

hanipouspilot commented Aug 12, 2018 via email

hanipouspilot commented Aug 12, 2018

981213 commented Aug 12, 2018

hanipouspilot commented Aug 12, 2018

981213 commented Aug 12, 2018

hanipouspilot commented Aug 12, 2018 via email

hanipouspilot commented Aug 12, 2018

981213 commented Aug 12, 2018

hanipouspilot commented Aug 12, 2018

hanipouspilot commented Aug 12, 2018

981213 commented Aug 12, 2018

hanipouspilot commented Aug 12, 2018 • edited Loading

hanipouspilot commented Aug 12, 2018 • edited Loading

hanipouspilot commented Aug 12, 2018 • edited Loading

981213 commented Aug 12, 2018

hanipouspilot commented Aug 12, 2018

hanipouspilot commented Aug 12, 2018

hanipouspilot commented Aug 12, 2018

mkresin commented Aug 13, 2018

981213 commented Aug 14, 2018

hanipouspilot commented Aug 15, 2018 • edited Loading

hnyman commented Aug 15, 2018

hanipouspilot commented Aug 15, 2018

981213 commented Aug 16, 2018 • edited Loading

hanipouspilot commented Aug 16, 2018 • edited Loading

981213 commented Aug 16, 2018

981213 commented Aug 11, 2018 •

edited

Loading

981213 commented Aug 11, 2018 •

edited

Loading

hanipouspilot commented Aug 12, 2018 •

edited

Loading

hanipouspilot commented Aug 12, 2018 •

edited

Loading

hanipouspilot commented Aug 12, 2018 •

edited

Loading

hanipouspilot commented Aug 12, 2018 •

edited

Loading

hanipouspilot commented Aug 15, 2018 •

edited

Loading

981213 commented Aug 16, 2018 •

edited

Loading

hanipouspilot commented Aug 16, 2018 •

edited

Loading