| Message ID | 20230413191541.1073027-4-ahalaney@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add EMAC3 support for sa8540p-ride (devicetree/clk bits) | expand |
Quoting Andrew Halaney (2023-04-13 12:15:41) > arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 179 ++++++++++++++++++++++ > 1 file changed, 179 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > index 40db5aa0803c..650cd54f418e 100644 > --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > @@ -28,6 +28,65 @@ aliases { > chosen { > stdout-path = "serial0:115200n8"; > }; > + > + mtl_rx_setup: rx-queues-config { Is there a reason why this isn't a child of an ethernet node? > + snps,rx-queues-to-use = <1>; > + snps,rx-sched-sp; > +
On Thu, Apr 13, 2023 at 03:05:26PM -0700, Stephen Boyd wrote: > Quoting Andrew Halaney (2023-04-13 14:01:27) > > On Thu, Apr 13, 2023 at 01:47:19PM -0700, Stephen Boyd wrote: > > > Quoting Andrew Halaney (2023-04-13 12:15:41) > > > > arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 179 ++++++++++++++++++++++ > > > > 1 file changed, 179 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > > > > index 40db5aa0803c..650cd54f418e 100644 > > > > --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > > > > +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > > > > @@ -28,6 +28,65 @@ aliases { > > > > chosen { > > > > stdout-path = "serial0:115200n8"; > > > > }; > > > > + > > > > + mtl_rx_setup: rx-queues-config { > > > > > > Is there a reason why this isn't a child of an ethernet node? > > > > > > > > > > I debated if it was more appropriate to: > > > > 1. make a duplicate in each ethernet node (ethernet0/1) > > 2. Put it in one and reference from both > > 3. have it floating around independent like this, similar to what is > > done in sa8155p-adp.dts[0] > > > > I chose 3 as it seemed cleanest, but if there's a good argument for a > > different approach I'm all ears! > > I wonder if it allows the binding checker to catch bad properties by > having it under the ethernet node? That's the only thing I can think of > that may be improved, but I'll let binding reviewers comment here. > Thanks, I was curious so I played around to answer the question via testing, and you're right... rx-queues-config/tx-queues-config aren't evaluated unless they sit under the node with the compatible (i.e. it doesn't just follow the phandle and evaluate). That makes sense to me I suppose. So, I guess, would maintainers prefer to see option (1) or (2) above? I want that thing evaluated. Option 1., above, has duplicated configuration, but is probably a more accurate representation of the hardware description. Option 2., above, doesn't duplicate rx-queues-config/tx-queues-config, but is a weirder representation of hardware description, and only complains once (which is fine since it's shared) when the binding checker runs (i.e. only the etherent parent containing rx-queues-config yells). In the below example you can see what I mean by the "only complains once" comment as well as illustration that the patchset as is doesn't allow rx-queues-config/tx-queues-config to be validated by dt-binding checks: (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % # Purposely introduce a dt-binding error on top of the current patchset :( (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % git diff :( diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts index 650cd54f418e..ecb0000db4e2 100644 --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts @@ -54,7 +54,7 @@ queue2 { queue3 { snps,avb-algorithm; - snps,map-to-dma-channel = <0x3>; + snps,map-to-dma-channel = "not-correct"; snps,priority = <0xc>; }; }; (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % make CHECK_DTBS=y DT_SCHEMA_FILES=/net/qcom,ethqos.yaml qcom/sa8540p-ride.dtb DTC_CHK arch/arm64/boot/dts/qcom/sa8540p-ride.dtb (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % # That should have failed (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % # Move the whole node under ethernet0, have ethernet1 reference via phandle only still (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % git diff | cat diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts index 650cd54f418e..451246936731 100644 --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts @@ -29,35 +29,6 @@ chosen { stdout-path = "serial0:115200n8"; }; - mtl_rx_setup: rx-queues-config { - snps,rx-queues-to-use = <1>; - snps,rx-sched-sp; - - queue0 { - snps,dcb-algorithm; - snps,map-to-dma-channel = <0x0>; - snps,route-up; - snps,priority = <0x1>; - }; - - queue1 { - snps,dcb-algorithm; - snps,map-to-dma-channel = <0x1>; - snps,route-ptp; - }; - - queue2 { - snps,avb-algorithm; - snps,map-to-dma-channel = <0x2>; - snps,route-avcp; - }; - - queue3 { - snps,avb-algorithm; - snps,map-to-dma-channel = <0x3>; - snps,priority = <0xc>; - }; - }; mtl_tx_setup: tx-queues-config { snps,tx-queues-to-use = <1>; @@ -223,6 +194,36 @@ ðernet0 { status = "okay"; + mtl_rx_setup: rx-queues-config { + snps,rx-queues-to-use = <1>; + snps,rx-sched-sp; + + queue0 { + snps,dcb-algorithm; + snps,map-to-dma-channel = <0x0>; + snps,route-up; + snps,priority = <0x1>; + }; + + queue1 { + snps,dcb-algorithm; + snps,map-to-dma-channel = <0x1>; + snps,route-ptp; + }; + + queue2 { + snps,avb-algorithm; + snps,map-to-dma-channel = <0x2>; + snps,route-avcp; + }; + + queue3 { + snps,avb-algorithm; + snps,map-to-dma-channel = "not-correct"; + snps,priority = <0xc>; + }; + }; + mdio { compatible = "snps,dwmac-mdio"; #address-cells = <1>; (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % make CHECK_DTBS=y DT_SCHEMA_FILES=/net/qcom,ethqos.yaml qcom/sa8540p-ride.dtb DTC_CHK arch/arm64/boot/dts/qcom/sa8540p-ride.dtb /home/ahalaney/git/redhat/stmmac/arch/arm64/boot/dts/qcom/sa8540p-ride.dtb: ethernet@20000: rx-queues-config:queue3:snps,map-to-dma-channel:0: [1852797997, 1668248178, 1701016576] is too long From schema: /home/ahalaney/git/redhat/stmmac/Documentation/devicetree/bindings/net/qcom,ethqos.yaml /home/ahalaney/git/redhat/stmmac/arch/arm64/boot/dts/qcom/sa8540p-ride.dtb: ethernet@20000: Unevaluated properties are not allowed ('max-speed', 'mdio', 'phy-handle', 'phy-mode', 'power-domains', 'rx-fifo-depth', 'rx-queues-config', 'snps,mtl-rx-config', 'snps,mtl-tx-config', 'snps,pbl', 'snps,tso', 'tx-fifo-depth' were unexpected) From schema: /home/ahalaney/git/redhat/stmmac/Documentation/devicetree/bindings/net/qcom,ethqos.yaml (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % # That warned as expected, since snps,dwmac.yaml failed on rx-queues-config it warns, \ and since part of the schema failed its not inherited (hence the Unevaluated properties warning following) \ also note how only ethernet0 (@20000) is evaluating rx-queues-config since that's where the rx-queues-config node lives (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % Thanks for the review! - Andrew
Hey Krzysztof, Some advice below would be appreciated. I discussed offline with Bjorn between the two approaches below but it sounded like ultimately we'd defer to your preference here! On Fri, Apr 14, 2023 at 09:58:44AM -0500, Andrew Halaney wrote: > On Thu, Apr 13, 2023 at 03:05:26PM -0700, Stephen Boyd wrote: > > Quoting Andrew Halaney (2023-04-13 14:01:27) > > > On Thu, Apr 13, 2023 at 01:47:19PM -0700, Stephen Boyd wrote: > > > > Quoting Andrew Halaney (2023-04-13 12:15:41) > > > > > arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 179 ++++++++++++++++++++++ > > > > > 1 file changed, 179 insertions(+) > > > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > > > > > index 40db5aa0803c..650cd54f418e 100644 > > > > > --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > > > > > +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > > > > > @@ -28,6 +28,65 @@ aliases { > > > > > chosen { > > > > > stdout-path = "serial0:115200n8"; > > > > > }; > > > > > + > > > > > + mtl_rx_setup: rx-queues-config { > > > > > > > > Is there a reason why this isn't a child of an ethernet node? > > > > > > > > > > > > > > I debated if it was more appropriate to: > > > > > > 1. make a duplicate in each ethernet node (ethernet0/1) > > > 2. Put it in one and reference from both > > > 3. have it floating around independent like this, similar to what is > > > done in sa8155p-adp.dts[0] > > > > > > I chose 3 as it seemed cleanest, but if there's a good argument for a > > > different approach I'm all ears! > > > > I wonder if it allows the binding checker to catch bad properties by > > having it under the ethernet node? That's the only thing I can think of > > that may be improved, but I'll let binding reviewers comment here. > > > > Thanks, I was curious so I played around to answer the question via > testing, and you're right... rx-queues-config/tx-queues-config aren't > evaluated unless they sit under the node with the compatible (i.e. it > doesn't just follow the phandle and evaluate). That makes sense to me I > suppose. > > So, I guess, would maintainers prefer to see option (1) or (2) above? I > want that thing evaluated. > > Option 1., above, has duplicated configuration, but is probably a more accurate > representation of the hardware description. > > Option 2., above, doesn't duplicate rx-queues-config/tx-queues-config, > but is a weirder representation of hardware description, and only > complains once (which is fine since it's shared) when the binding checker > runs (i.e. only the etherent parent containing rx-queues-config yells). > For what it is worth, I prefer option 1 above :) > In the below example you can see what I mean by the "only complains > once" comment as well as illustration that the patchset as is doesn't > allow rx-queues-config/tx-queues-config to be validated by dt-binding > checks: > > (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % # Purposely introduce a dt-binding error on top of the current patchset :( > (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % git diff :( > diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > index 650cd54f418e..ecb0000db4e2 100644 > --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > @@ -54,7 +54,7 @@ queue2 { > > queue3 { > snps,avb-algorithm; > - snps,map-to-dma-channel = <0x3>; > + snps,map-to-dma-channel = "not-correct"; > snps,priority = <0xc>; > }; > }; > (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % make CHECK_DTBS=y DT_SCHEMA_FILES=/net/qcom,ethqos.yaml qcom/sa8540p-ride.dtb > DTC_CHK arch/arm64/boot/dts/qcom/sa8540p-ride.dtb > (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % # That should have failed > (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % # Move the whole node under ethernet0, have ethernet1 reference via phandle only still > (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % git diff | cat > diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > index 650cd54f418e..451246936731 100644 > --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > @@ -29,35 +29,6 @@ chosen { > stdout-path = "serial0:115200n8"; > }; > > - mtl_rx_setup: rx-queues-config { > - snps,rx-queues-to-use = <1>; > - snps,rx-sched-sp; > - > - queue0 { > - snps,dcb-algorithm; > - snps,map-to-dma-channel = <0x0>; > - snps,route-up; > - snps,priority = <0x1>; > - }; > - > - queue1 { > - snps,dcb-algorithm; > - snps,map-to-dma-channel = <0x1>; > - snps,route-ptp; > - }; > - > - queue2 { > - snps,avb-algorithm; > - snps,map-to-dma-channel = <0x2>; > - snps,route-avcp; > - }; > - > - queue3 { > - snps,avb-algorithm; > - snps,map-to-dma-channel = <0x3>; > - snps,priority = <0xc>; > - }; > - }; > > mtl_tx_setup: tx-queues-config { > snps,tx-queues-to-use = <1>; > @@ -223,6 +194,36 @@ ðernet0 { > > status = "okay"; > > + mtl_rx_setup: rx-queues-config { > + snps,rx-queues-to-use = <1>; > + snps,rx-sched-sp; > + > + queue0 { > + snps,dcb-algorithm; > + snps,map-to-dma-channel = <0x0>; > + snps,route-up; > + snps,priority = <0x1>; > + }; > + > + queue1 { > + snps,dcb-algorithm; > + snps,map-to-dma-channel = <0x1>; > + snps,route-ptp; > + }; > + > + queue2 { > + snps,avb-algorithm; > + snps,map-to-dma-channel = <0x2>; > + snps,route-avcp; > + }; > + > + queue3 { > + snps,avb-algorithm; > + snps,map-to-dma-channel = "not-correct"; > + snps,priority = <0xc>; > + }; > + }; > + > mdio { > compatible = "snps,dwmac-mdio"; > #address-cells = <1>; > (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % make CHECK_DTBS=y DT_SCHEMA_FILES=/net/qcom,ethqos.yaml qcom/sa8540p-ride.dtb > DTC_CHK arch/arm64/boot/dts/qcom/sa8540p-ride.dtb > /home/ahalaney/git/redhat/stmmac/arch/arm64/boot/dts/qcom/sa8540p-ride.dtb: ethernet@20000: rx-queues-config:queue3:snps,map-to-dma-channel:0: [1852797997, 1668248178, 1701016576] is too long > From schema: /home/ahalaney/git/redhat/stmmac/Documentation/devicetree/bindings/net/qcom,ethqos.yaml > /home/ahalaney/git/redhat/stmmac/arch/arm64/boot/dts/qcom/sa8540p-ride.dtb: ethernet@20000: Unevaluated properties are not allowed ('max-speed', 'mdio', 'phy-handle', 'phy-mode', 'power-domains', 'rx-fifo-depth', 'rx-queues-config', 'snps,mtl-rx-config', 'snps,mtl-tx-config', 'snps,pbl', 'snps,tso', 'tx-fifo-depth' were unexpected) > From schema: /home/ahalaney/git/redhat/stmmac/Documentation/devicetree/bindings/net/qcom,ethqos.yaml > (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % # That warned as expected, since snps,dwmac.yaml failed on rx-queues-config it warns, \ > and since part of the schema failed its not inherited (hence the Unevaluated properties warning following) \ > also note how only ethernet0 (@20000) is evaluating rx-queues-config since that's where the rx-queues-config node lives > (dtschema) ahalaney@halaney-x13s ~/git/redhat/stmmac (git)-[stmmac] % > > Thanks for the review! > - Andrew
diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts index 40db5aa0803c..650cd54f418e 100644 --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts @@ -28,6 +28,65 @@ aliases { chosen { stdout-path = "serial0:115200n8"; }; + + mtl_rx_setup: rx-queues-config { + snps,rx-queues-to-use = <1>; + snps,rx-sched-sp; + + queue0 { + snps,dcb-algorithm; + snps,map-to-dma-channel = <0x0>; + snps,route-up; + snps,priority = <0x1>; + }; + + queue1 { + snps,dcb-algorithm; + snps,map-to-dma-channel = <0x1>; + snps,route-ptp; + }; + + queue2 { + snps,avb-algorithm; + snps,map-to-dma-channel = <0x2>; + snps,route-avcp; + }; + + queue3 { + snps,avb-algorithm; + snps,map-to-dma-channel = <0x3>; + snps,priority = <0xc>; + }; + }; + + mtl_tx_setup: tx-queues-config { + snps,tx-queues-to-use = <1>; + snps,tx-sched-sp; + + queue0 { + snps,dcb-algorithm; + }; + + queue1 { + snps,dcb-algorithm; + }; + + queue2 { + snps,avb-algorithm; + snps,send_slope = <0x1000>; + snps,idle_slope = <0x1000>; + snps,high_credit = <0x3e800>; + snps,low_credit = <0xffc18000>; + }; + + queue3 { + snps,avb-algorithm; + snps,send_slope = <0x1000>; + snps,idle_slope = <0x1000>; + snps,high_credit = <0x3e800>; + snps,low_credit = <0xffc18000>; + }; + }; }; &apps_rsc { @@ -151,6 +210,66 @@ vreg_l8g: ldo8 { }; }; +ðernet0 { + snps,mtl-rx-config = <&mtl_rx_setup>; + snps,mtl-tx-config = <&mtl_tx_setup>; + + max-speed = <1000>; + phy-handle = <&rgmii_phy>; + phy-mode = "rgmii-txid"; + + pinctrl-names = "default"; + pinctrl-0 = <ðernet0_default>; + + status = "okay"; + + mdio { + compatible = "snps,dwmac-mdio"; + #address-cells = <1>; + #size-cells = <0>; + + /* Marvell 88EA1512 */ + rgmii_phy: phy@8 { + reg = <0x8>; + + interrupts-extended = <&tlmm 127 IRQ_TYPE_EDGE_FALLING>; + + reset-gpios = <&pmm8540c_gpios 1 GPIO_ACTIVE_LOW>; + reset-assert-us = <11000>; + reset-deassert-us = <70000>; + + device_type = "ethernet-phy"; + + /* Set to RGMII_SGMII mode and soft reset. Turn off auto-negotiation + * from userspace to talk to the switch on the SGMII side of things + */ + marvell,reg-init = + /* Set MODE[2:0] to RGMII_SGMII */ + <0x12 0x14 0xfff8 0x4>, + /* Soft reset required after changing MODE[2:0] */ + <0x12 0x14 0x7fff 0x8000>; + }; + }; +}; + +ðernet1 { + snps,mtl-rx-config = <&mtl_rx_setup>; + snps,mtl-tx-config = <&mtl_tx_setup>; + + max-speed = <1000>; + phy-mode = "rgmii-txid"; + + pinctrl-names = "default"; + pinctrl-0 = <ðernet1_default>; + + status = "okay"; + + fixed-link { + speed = <1000>; + full-duplex; + }; +}; + &i2c0 { pinctrl-names = "default"; pinctrl-0 = <&i2c0_default>; @@ -316,6 +435,66 @@ &xo_board_clk { /* PINCTRL */ &tlmm { + ethernet0_default: ethernet0-default-state { + mdc-pins { + pins = "gpio175"; + function = "rgmii_0"; + drive-strength = <16>; + bias-pull-up; + }; + + mdio-pins { + pins = "gpio176"; + function = "rgmii_0"; + drive-strength = <16>; + bias-pull-up; + }; + + rgmii-tx-pins { + pins = "gpio183", "gpio184", "gpio185", "gpio186", "gpio187", "gpio188"; + function = "rgmii_0"; + drive-strength = <16>; + bias-pull-up; + }; + + rgmii-rx-pins { + pins = "gpio177", "gpio178", "gpio179", "gpio180", "gpio181", "gpio182"; + function = "rgmii_0"; + drive-strength = <16>; + bias-disable; + }; + }; + + ethernet1_default: ethernet1-default-state { + mdc-pins { + pins = "gpio97"; + function = "rgmii_1"; + drive-strength = <16>; + bias-pull-up; + }; + + mdio-pins { + pins = "gpio98"; + function = "rgmii_1"; + drive-strength = <16>; + bias-pull-up; + }; + + rgmii-tx-pins { + pins = "gpio105", "gpio106", "gpio107", "gpio108", "gpio109", "gpio110"; + function = "rgmii_1"; + drive-strength = <16>; + bias-pull-up; + }; + + rgmii-rx-pins { + pins = "gpio99", "gpio100", "gpio101", "gpio102", "gpio103", "gpio104"; + function = "rgmii_1"; + drive-strength = <16>; + bias-disable; + }; + }; + i2c0_default: i2c0-default-state { /* To USB7002T-I/KDXVA0 USB hub (SIP1 only) */ pins = "gpio135", "gpio136";
Enable both the MACs found on the board. ethernet0 and ethernet1 both ultimately go to a series of on board switches which aren't managed by this processor. ethernet0 is connected to a Marvell 88EA1512 phy via RGMII. That goes to the series of switches via SGMII on the "media" side of the phy. RGMII_SGMII mode is enabled via devicetree register descriptions. The switch on the "media" side has auto-negotiation disabled, so configuration from userspace similar to: ethtool -s eth0 autoneg off speed 1000 duplex full is necessary to get traffic flowing on that interface. ethernet1 is in a mac2mac/fixed-link configuration going to the same series of switches directly via RGMII. Tested-by: Brian Masney <bmasney@redhat.com> Signed-off-by: Andrew Halaney <ahalaney@redhat.com> --- This patch is why there's a v5, I couldn't ignore the needless interrupts-parent Konrad pointed out in the sa8155-adp.dts over at: https://lore.kernel.org/linux-arm-msm/88d41729-86be-95cb-2fda-1b809f07ed6b@linaro.org/ Changes since v4: * Remove needless interrupt-parent (Konrad) * Add Tested-by (Brian) Changes since v3: * Compatible goes first in node (Krzysztof) Changes since v1 and v2: * None arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 179 ++++++++++++++++++++++ 1 file changed, 179 insertions(+)