| Message ID | 20201219162153.23126-1-dqfext@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next] net: dsa: mt7530: rename MT7621 compatible | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst WARNING: line length of 83 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
> --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = { > }; > > static const struct of_device_id mt7530_of_match[] = { > - { .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], }, > + { .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], }, > { .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], }, > { .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], }, > { /* sentinel */ }, This will break backwards compatibility with existing DT blobs. You need to keep the old "mediatek,mt7621", but please add a comment that it is deprecated. Andrew
On 12/19/2020 8:26 AM, Andrew Lunn wrote: >> --- a/drivers/net/dsa/mt7530.c >> +++ b/drivers/net/dsa/mt7530.c >> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = { >> }; >> >> static const struct of_device_id mt7530_of_match[] = { >> - { .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], }, >> + { .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], }, >> { .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], }, >> { .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], }, >> { /* sentinel */ }, > > This will break backwards compatibility with existing DT blobs. You > need to keep the old "mediatek,mt7621", but please add a comment that > it is deprecated. Besides, adding -gsw would make it inconsistent with the existing matching compatible strings. While it's not ideal to have the same top-level SoC compatible and having another sub-node within that SoC's DTS have the same compatible, given this would be break backwards compatibility, cannot you stay with what is defined today?
Hi Andrew, Florian, On Sat, Dec 19, 2020 at 09:07:13AM -0800, Florian Fainelli wrote: > On 12/19/2020 8:26 AM, Andrew Lunn wrote: > >> --- a/drivers/net/dsa/mt7530.c > >> +++ b/drivers/net/dsa/mt7530.c > >> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = { > >> }; > >> > >> static const struct of_device_id mt7530_of_match[] = { > >> - { .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], }, > >> + { .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], }, > >> { .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], }, > >> { .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], }, > >> { /* sentinel */ }, > > > > This will break backwards compatibility with existing DT blobs. You > > need to keep the old "mediatek,mt7621", but please add a comment that > > it is deprecated. > > Besides, adding -gsw would make it inconsistent with the existing > matching compatible strings. While it's not ideal to have the same > top-level SoC compatible and having another sub-node within that SoC's > DTS have the same compatible, given this would be break backwards > compatibility, cannot you stay with what is defined today? The MT7621 device tree is in staging. I suppose that some amount of breaking changes could be tolerated? But Qingfang, I'm confused when looking at drivers/staging/mt7621-dts/mt7621.dtsi. /ethernet@1e100000/mdio-bus { switch0: switch0@0 { compatible = "mediatek,mt7621"; #address-cells = <1>; #size-cells = <0>; reg = <0>; mediatek,mcm; resets = <&rstctrl 2>; reset-names = "mcm"; ports { #address-cells = <1>; #size-cells = <0>; reg = <0>; port@0 { status = "off"; reg = <0>; label = "lan0"; }; port@1 { status = "off"; reg = <1>; label = "lan1"; }; port@2 { status = "off"; reg = <2>; label = "lan2"; }; port@3 { status = "off"; reg = <3>; label = "lan3"; }; port@4 { status = "off"; reg = <4>; label = "lan4"; }; port@6 { reg = <6>; label = "cpu"; ethernet = <&gmac0>; phy-mode = "trgmii"; fixed-link { speed = <1000>; full-duplex; }; }; }; }; }; / { gsw: gsw@1e110000 { compatible = "mediatek,mt7621-gsw"; reg = <0x1e110000 0x8000>; interrupt-parent = <&gic>; interrupts = <GIC_SHARED 23 IRQ_TYPE_LEVEL_HIGH>; }; }; What is the platform device at the memory address 1e110000? There is no driver for it. The documentation only has me even more confused: Mediatek Gigabit Switch ======================= The mediatek gigabit switch can be found on Mediatek SoCs (mt7620, mt7621). Required properties: - compatible: Should be "mediatek,mt7620-gsw" or "mediatek,mt7621-gsw" - reg: Address and length of the register set for the device - interrupts: Should contain the gigabit switches interrupt - resets: Should contain the gigabit switches resets - reset-names: Should contain the reset names "gsw" Example: gsw@10110000 { compatible = "ralink,mt7620-gsw"; <- notice how even the example is bad and inconsistent reg = <0x10110000 8000>; resets = <&rstctrl 23>; reset-names = "gsw"; interrupt-parent = <&intc>; interrupts = <17>; }; Does the MT7621 contain two Ethernet switches, one accessed over MMIO and another over MDIO? Or is it the same switch? I don't understand. What is the relationship between the new compatible that you're proposing, Qingfang, and the existing device tree bindings?
Hi Vladimir, On Sun, Dec 20, 2020 at 3:48 AM Vladimir Oltean <olteanv@gmail.com> wrote: > > Hi Andrew, Florian, > > On Sat, Dec 19, 2020 at 09:07:13AM -0800, Florian Fainelli wrote: > > On 12/19/2020 8:26 AM, Andrew Lunn wrote: > > >> --- a/drivers/net/dsa/mt7530.c > > >> +++ b/drivers/net/dsa/mt7530.c > > >> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = { > > >> }; > > >> > > >> static const struct of_device_id mt7530_of_match[] = { > > >> - { .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], }, > > >> + { .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], }, > > >> { .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], }, > > >> { .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], }, > > >> { /* sentinel */ }, > > > > > > This will break backwards compatibility with existing DT blobs. You > > > need to keep the old "mediatek,mt7621", but please add a comment that > > > it is deprecated. > > > > Besides, adding -gsw would make it inconsistent with the existing > > matching compatible strings. While it's not ideal to have the same > > top-level SoC compatible and having another sub-node within that SoC's > > DTS have the same compatible, given this would be break backwards > > compatibility, cannot you stay with what is defined today? > > The MT7621 device tree is in staging. I suppose that some amount of > breaking changes could be tolerated? > > But Qingfang, I'm confused when looking at drivers/staging/mt7621-dts/mt7621.dtsi. > > /ethernet@1e100000/mdio-bus { > switch0: switch0@0 { > compatible = "mediatek,mt7621"; > #address-cells = <1>; > #size-cells = <0>; > reg = <0>; > mediatek,mcm; > resets = <&rstctrl 2>; > reset-names = "mcm"; > > ports { > #address-cells = <1>; > #size-cells = <0>; > reg = <0>; > port@0 { > status = "off"; > reg = <0>; > label = "lan0"; > }; > port@1 { > status = "off"; > reg = <1>; > label = "lan1"; > }; > port@2 { > status = "off"; > reg = <2>; > label = "lan2"; > }; > port@3 { > status = "off"; > reg = <3>; > label = "lan3"; > }; > port@4 { > status = "off"; > reg = <4>; > label = "lan4"; > }; > port@6 { > reg = <6>; > label = "cpu"; > ethernet = <&gmac0>; > phy-mode = "trgmii"; > fixed-link { > speed = <1000>; > full-duplex; > }; > }; > }; > }; > }; > > / { > gsw: gsw@1e110000 { > compatible = "mediatek,mt7621-gsw"; > reg = <0x1e110000 0x8000>; > interrupt-parent = <&gic>; > interrupts = <GIC_SHARED 23 IRQ_TYPE_LEVEL_HIGH>; > }; > }; > > What is the platform device at the memory address 1e110000? > There is no driver for it. The documentation only has me even more > confused: > > Mediatek Gigabit Switch > ======================= > > The mediatek gigabit switch can be found on Mediatek SoCs (mt7620, mt7621). > > Required properties: > - compatible: Should be "mediatek,mt7620-gsw" or "mediatek,mt7621-gsw" > - reg: Address and length of the register set for the device > - interrupts: Should contain the gigabit switches interrupt > - resets: Should contain the gigabit switches resets > - reset-names: Should contain the reset names "gsw" > > Example: > > gsw@10110000 { > compatible = "ralink,mt7620-gsw"; <- notice how even the example is bad and inconsistent > reg = <0x10110000 8000>; > > resets = <&rstctrl 23>; > reset-names = "gsw"; > > interrupt-parent = <&intc>; > interrupts = <17>; > }; > > Does the MT7621 contain two Ethernet switches, one accessed over MMIO > and another over MDIO? Or is it the same switch? I don't understand. > What is the relationship between the new compatible that you're > proposing, Qingfang, and the existing device tree bindings? The current dtsi is copied from OpenWrt, so the existing "mt7621-gsw" / "mt7620-gsw" compatible is for their swconfig driver. MT7621 has only one switch, accessed over MDIO, so the reg property has no effect. Should this patch be accepted, the existing gsw nodes can be dropped.
On Sun, Dec 20, 2020 at 12:48:08PM +0800, DENG Qingfang wrote: > Hi Vladimir, > > On Sun, Dec 20, 2020 at 3:48 AM Vladimir Oltean <olteanv@gmail.com> wrote: > > > > Hi Andrew, Florian, > > > > On Sat, Dec 19, 2020 at 09:07:13AM -0800, Florian Fainelli wrote: > > > On 12/19/2020 8:26 AM, Andrew Lunn wrote: > > > >> --- a/drivers/net/dsa/mt7530.c > > > >> +++ b/drivers/net/dsa/mt7530.c > > > >> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = { > > > >> }; > > > >> > > > >> static const struct of_device_id mt7530_of_match[] = { > > > >> - { .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], }, > > > >> + { .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], }, > > > >> { .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], }, > > > >> { .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], }, > > > >> { /* sentinel */ }, > > > > > > > > This will break backwards compatibility with existing DT blobs. You > > > > need to keep the old "mediatek,mt7621", but please add a comment that > > > > it is deprecated. > > > > > > Besides, adding -gsw would make it inconsistent with the existing > > > matching compatible strings. While it's not ideal to have the same > > > top-level SoC compatible and having another sub-node within that SoC's > > > DTS have the same compatible, given this would be break backwards > > > compatibility, cannot you stay with what is defined today? > > > > The MT7621 device tree is in staging. I suppose that some amount of > > breaking changes could be tolerated? > > > > But Qingfang, I'm confused when looking at drivers/staging/mt7621-dts/mt7621.dtsi. > > > > /ethernet@1e100000/mdio-bus { > > switch0: switch0@0 { > > compatible = "mediatek,mt7621"; > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <0>; > > mediatek,mcm; > > resets = <&rstctrl 2>; > > reset-names = "mcm"; > > > > ports { > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <0>; > > port@0 { > > status = "off"; > > reg = <0>; > > label = "lan0"; > > }; > > port@1 { > > status = "off"; > > reg = <1>; > > label = "lan1"; > > }; > > port@2 { > > status = "off"; > > reg = <2>; > > label = "lan2"; > > }; > > port@3 { > > status = "off"; > > reg = <3>; > > label = "lan3"; > > }; > > port@4 { > > status = "off"; > > reg = <4>; > > label = "lan4"; > > }; > > port@6 { > > reg = <6>; > > label = "cpu"; > > ethernet = <&gmac0>; > > phy-mode = "trgmii"; > > fixed-link { > > speed = <1000>; > > full-duplex; > > }; > > }; > > }; > > }; > > }; > > > > / { > > gsw: gsw@1e110000 { > > compatible = "mediatek,mt7621-gsw"; > > reg = <0x1e110000 0x8000>; > > interrupt-parent = <&gic>; > > interrupts = <GIC_SHARED 23 IRQ_TYPE_LEVEL_HIGH>; > > }; > > }; > > > > What is the platform device at the memory address 1e110000? > > There is no driver for it. The documentation only has me even more > > confused: > > > > Mediatek Gigabit Switch > > ======================= > > > > The mediatek gigabit switch can be found on Mediatek SoCs (mt7620, mt7621). > > > > Required properties: > > - compatible: Should be "mediatek,mt7620-gsw" or "mediatek,mt7621-gsw" > > - reg: Address and length of the register set for the device > > - interrupts: Should contain the gigabit switches interrupt > > - resets: Should contain the gigabit switches resets > > - reset-names: Should contain the reset names "gsw" > > > > Example: > > > > gsw@10110000 { > > compatible = "ralink,mt7620-gsw"; <- notice how even the example is bad and inconsistent > > reg = <0x10110000 8000>; > > > > resets = <&rstctrl 23>; > > reset-names = "gsw"; > > > > interrupt-parent = <&intc>; > > interrupts = <17>; > > }; > > > > Does the MT7621 contain two Ethernet switches, one accessed over MMIO > > and another over MDIO? Or is it the same switch? I don't understand. > > What is the relationship between the new compatible that you're > > proposing, Qingfang, and the existing device tree bindings? > > The current dtsi is copied from OpenWrt, so the existing "mt7621-gsw" > / "mt7620-gsw" compatible is for their swconfig driver. > MT7621 has only one switch, accessed over MDIO, so the reg property > has no effect. > > Should this patch be accepted, the existing gsw nodes can be dropped. But still, what is at memory address 0x1e110000, if the switch is accessed over MDIO?
On Sun, Dec 20, 2020 at 3:49 PM Vladimir Oltean <olteanv@gmail.com> wrote: > But still, what is at memory address 0x1e110000, if the switch is > accessed over MDIO? It's "Ethernet GMAC", handled by mtk_eth_soc.
On Sun, Dec 20, 2020 at 04:36:27PM +0800, DENG Qingfang wrote: > On Sun, Dec 20, 2020 at 3:49 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > But still, what is at memory address 0x1e110000, if the switch is > > accessed over MDIO? > > It's "Ethernet GMAC", handled by mtk_eth_soc. I see. You have some work to do with that device tree.
On Sun, Dec 20, 2020 at 12:21:53AM +0800, DENG Qingfang wrote: > MT7621 is a SoC, so using "mediatek,mt7621" as its compatible is ambiguous. > Rename it to "mediatek,mt7621-gsw". > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > --- I would say that you need to resolve the situation with the docs at Documentation/devicetree/bindings/net/mediatek,mt7620-gsw.txt and with the bindings at drivers/staging/mt7621-dts/mt7621.dtsi first (or in the same series). And still then, it would be nice if you could preserve compatibility with the existing bindings at least for a while.
Hi! On Sun, Dec 20, 2020 at 1:10 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 12/19/2020 8:26 AM, Andrew Lunn wrote: > >> --- a/drivers/net/dsa/mt7530.c > >> +++ b/drivers/net/dsa/mt7530.c > >> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = { > >> }; > >> > >> static const struct of_device_id mt7530_of_match[] = { > >> - { .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], }, > >> + { .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], }, > >> { .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], }, > >> { .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], }, > >> { /* sentinel */ }, > > > > This will break backwards compatibility with existing DT blobs. You > > need to keep the old "mediatek,mt7621", but please add a comment that > > it is deprecated. > > Besides, adding -gsw would make it inconsistent with the existing > matching compatible strings. While it's not ideal to have the same > top-level SoC compatible and having another sub-node within that SoC's > DTS have the same compatible, given this would be break backwards > compatibility, cannot you stay with what is defined today? U-boot for MT7621 doesn't support device tree, and the kernel image is always packaged with dt. Do we need to maintain backward compatibility at all in this situation?
diff --git a/Documentation/devicetree/bindings/net/dsa/mt7530.txt b/Documentation/devicetree/bindings/net/dsa/mt7530.txt index 560369efad6c..a9c8492296b3 100644 --- a/Documentation/devicetree/bindings/net/dsa/mt7530.txt +++ b/Documentation/devicetree/bindings/net/dsa/mt7530.txt @@ -4,7 +4,7 @@ Mediatek MT7530 Ethernet switch Required properties: - compatible: may be compatible = "mediatek,mt7530" - or compatible = "mediatek,mt7621" + or compatible = "mediatek,mt7621-gsw" or compatible = "mediatek,mt7531" - #address-cells: Must be 1. - #size-cells: Must be 0. @@ -35,7 +35,7 @@ Required properties for the child nodes within ports container: user ports. - phy-mode: String, the following values are acceptable for port labeled "cpu": - If compatible mediatek,mt7530 or mediatek,mt7621 is set, + If compatible mediatek,mt7530 or mediatek,mt7621-gsw is set, must be either "trgmii" or "rgmii" If compatible mediatek,mt7531 is set, must be either "sgmii", "1000base-x" or "2500base-x" @@ -168,7 +168,7 @@ Example 2: MT7621: Port 4 is WAN port: 2nd GMAC -> Port 5 -> PHY port 4. }; mt7530: switch@1f { - compatible = "mediatek,mt7621"; + compatible = "mediatek,mt7621-gsw"; #address-cells = <1>; #size-cells = <0>; reg = <0x1f>; @@ -251,7 +251,7 @@ Example 3: MT7621: Port 5 is connected to external PHY: Port 5 -> external PHY. }; mt7530: switch@1f { - compatible = "mediatek,mt7621"; + compatible = "mediatek,mt7621-gsw"; #address-cells = <1>; #size-cells = <0>; reg = <0x1f>; diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 55c8baa31e5d..347845d66671 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = { }; static const struct of_device_id mt7530_of_match[] = { - { .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], }, + { .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], }, { .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], }, { .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], }, { /* sentinel */ },
MT7621 is a SoC, so using "mediatek,mt7621" as its compatible is ambiguous. Rename it to "mediatek,mt7621-gsw". Signed-off-by: DENG Qingfang <dqfext@gmail.com> --- Documentation/devicetree/bindings/net/dsa/mt7530.txt | 8 ++++---- drivers/net/dsa/mt7530.c | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-)