[RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[DENG Qingfang]

Hi Vladimir,

On Sun, Dec 20, 2020 at 3:48 AM Vladimir Oltean <olteanv at 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 at 1e100000/mdio-bus {
>         switch0: switch0 at 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 at 0 {
>                                 status = "off";
>                                 reg = <0>;
>                                 label = "lan0";
>                         };
>                         port at 1 {
>                                 status = "off";
>                                 reg = <1>;
>                                 label = "lan1";
>                         };
>                         port at 2 {
>                                 status = "off";
>                                 reg = <2>;
>                                 label = "lan2";
>                         };
>                         port at 3 {
>                                 status = "off";
>                                 reg = <3>;
>                                 label = "lan3";
>                         };
>                         port at 4 {
>                                 status = "off";
>                                 reg = <4>;
>                                 label = "lan4";
>                         };
>                         port at 6 {
>                                 reg = <6>;
>                                 label = "cpu";
>                                 ethernet = <&gmac0>;
>                                 phy-mode = "trgmii";
>                                 fixed-link {
>                                         speed = <1000>;
>                                         full-duplex;
>                                 };
>                         };
>                 };
>         };
> };
>
> / {
>         gsw: gsw at 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 at 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.