mbox series

[v2,net-next,0/3] net: stmmac: RK3566

Message ID 20210421203409.40717-1-ezequiel@collabora.com (mailing list archive)
Headers show
Series net: stmmac: RK3566 | expand

Message

Ezequiel Garcia April 21, 2021, 8:34 p.m. UTC
Now that RK3568 SoC devicetree upstreaming is happening [1],
here's another round for the RK3566 dwmac. There wasn't any clear
consensus on how to implement the two interfaces present
on RK3568, so I decided to drop that and just submit RK3566 for now.

This has been tested on a Pine64 RK3566 Quartz64 Model B board,
DHCP and iperf are looking good.

For all the people testing, here's Quartz 64 Model B device tree
snippet:

        gmac1: ethernet@fe010000 {
                compatible = "rockchip,rk3566-gmac", "snps,dwmac-4.20a";
                reg = <0x0 0xfe010000 0x0 0x10000>;
                interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>,
                             <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
                interrupt-names = "macirq", "eth_wake_irq";
                rockchip,grf = <&grf>;
                clocks = <&cru SCLK_GMAC1>, <&cru SCLK_GMAC1_RX_TX>,
                         <&cru SCLK_GMAC1_RX_TX>, <&cru CLK_MAC1_REFOUT>,
                         <&cru ACLK_GMAC1>, <&cru PCLK_GMAC1>,
                         <&cru SCLK_GMAC1_RX_TX>, <&cru CLK_GMAC1_PTP_REF>;
                clock-names = "stmmaceth", "mac_clk_rx",
                              "mac_clk_tx", "clk_mac_refout",
                              "aclk_mac", "pclk_mac",
                              "clk_mac_speed", "ptp_ref";
                resets = <&cru SRST_A_GMAC1>;
                reset-names = "stmmaceth";

                snps,mixed-burst;
                snps,tso;

                snps,axi-config = <&gmac1_stmmac_axi_setup>;
                snps,mtl-rx-config = <&gmac1_mtl_rx_setup>;
                snps,mtl-tx-config = <&gmac1_mtl_tx_setup>;
                status = "disabled";

                mdio1: mdio {
                        compatible = "snps,dwmac-mdio";
                        #address-cells = <0x1>;
                        #size-cells = <0x0>;
                };

                gmac1_stmmac_axi_setup: stmmac-axi-config {
                        snps,wr_osr_lmt = <4>;
                        snps,rd_osr_lmt = <8>;
                        snps,blen = <0 0 0 0 16 8 4>;
                };

                gmac1_mtl_rx_setup: rx-queues-config {
                        snps,rx-queues-to-use = <1>;
                        queue0 {};
                };

                gmac1_mtl_tx_setup: tx-queues-config {
                        snps,tx-queues-to-use = <1>;
                        queue0 {};
                };
        };

While here, I'm adding a small patch from David Wu, for some
sanity checks for dwmac-rockchip-specific non-NULL ops.

Thanks!

[1] http://lore.kernel.org/r/20210421065921.23917-1-cl@rock-chips.com

David Wu (2):
  net: stmmac: dwmac-rk: Check platform-specific ops
  net: stmmac: Add RK3566 SoC support

Ezequiel Garcia (1):
  net: stmmac: Don't set has_gmac if has_gmac4 is set

 .../bindings/net/rockchip-dwmac.txt           |   1 +
 .../net/ethernet/stmicro/stmmac/dwmac-rk.c    | 126 +++++++++++++++++-
 2 files changed, 124 insertions(+), 3 deletions(-)

Comments

David Wu April 23, 2021, 9:24 a.m. UTC | #1
Hi Ezequiel,

在 2021/4/22 上午4:34, Ezequiel Garcia 写道:
> Now that RK3568 SoC devicetree upstreaming is happening [1],
> here's another round for the RK3566 dwmac. There wasn't any clear
> consensus on how to implement the two interfaces present
> on RK3568, so I decided to drop that and just submit RK3566 for now.
> 
> This has been tested on a Pine64 RK3566 Quartz64 Model B board,
> DHCP and iperf are looking good.
> 
> For all the people testing, here's Quartz 64 Model B device tree
> snippet:
> 
>          gmac1: ethernet@fe010000 {
>                  compatible = "rockchip,rk3566-gmac", "snps,dwmac-4.20a";

It is better to use "rockchip,rk3568-gmac" here, "rockchip,rk3566-gmac" 
is not compatible, 3568 has two gmacs, which are compatible with 3566.

If there is no better way, using bus_id from alias is good, it is a 
fixed id, and U-Boot also use the id to write MAC address into kernel DTB.

plat->bus_id = of_alias_get_id(np, "ethernet");

>                  reg = <0x0 0xfe010000 0x0 0x10000>;
>                  interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>,
>                               <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
>                  interrupt-names = "macirq", "eth_wake_irq";
>                  rockchip,grf = <&grf>;
>                  clocks = <&cru SCLK_GMAC1>, <&cru SCLK_GMAC1_RX_TX>,
>                           <&cru SCLK_GMAC1_RX_TX>, <&cru CLK_MAC1_REFOUT>,
>                           <&cru ACLK_GMAC1>, <&cru PCLK_GMAC1>,
>                           <&cru SCLK_GMAC1_RX_TX>, <&cru CLK_GMAC1_PTP_REF>;
>                  clock-names = "stmmaceth", "mac_clk_rx",
>                                "mac_clk_tx", "clk_mac_refout",
>                                "aclk_mac", "pclk_mac",
>                                "clk_mac_speed", "ptp_ref";
>                  resets = <&cru SRST_A_GMAC1>;
>                  reset-names = "stmmaceth";
> 
>                  snps,mixed-burst;
>                  snps,tso;
> 
>                  snps,axi-config = <&gmac1_stmmac_axi_setup>;
>                  snps,mtl-rx-config = <&gmac1_mtl_rx_setup>;
>                  snps,mtl-tx-config = <&gmac1_mtl_tx_setup>;
>                  status = "disabled";
> 
>                  mdio1: mdio {
>                          compatible = "snps,dwmac-mdio";
>                          #address-cells = <0x1>;
>                          #size-cells = <0x0>;
>                  };
> 
>                  gmac1_stmmac_axi_setup: stmmac-axi-config {
>                          snps,wr_osr_lmt = <4>;
>                          snps,rd_osr_lmt = <8>;
>                          snps,blen = <0 0 0 0 16 8 4>;
>                  };
> 
>                  gmac1_mtl_rx_setup: rx-queues-config {
>                          snps,rx-queues-to-use = <1>;
>                          queue0 {};
>                  };
> 
>                  gmac1_mtl_tx_setup: tx-queues-config {
>                          snps,tx-queues-to-use = <1>;
>                          queue0 {};
>                  };
>          };
> 
> While here, I'm adding a small patch from David Wu, for some
> sanity checks for dwmac-rockchip-specific non-NULL ops.
> 
> Thanks!
> 
> [1] http://lore.kernel.org/r/20210421065921.23917-1-cl@rock-chips.com
> 
> David Wu (2):
>    net: stmmac: dwmac-rk: Check platform-specific ops
>    net: stmmac: Add RK3566 SoC support
> 
> Ezequiel Garcia (1):
>    net: stmmac: Don't set has_gmac if has_gmac4 is set
> 
>   .../bindings/net/rockchip-dwmac.txt           |   1 +
>   .../net/ethernet/stmicro/stmmac/dwmac-rk.c    | 126 +++++++++++++++++-
>   2 files changed, 124 insertions(+), 3 deletions(-)
>
Ezequiel Garcia April 23, 2021, 1:57 p.m. UTC | #2
Hi David,

Thanks a lot for reviewing.

On Fri, 2021-04-23 at 17:24 +0800, David Wu wrote:
> Hi Ezequiel,
> 
> 在 2021/4/22 上午4:34, Ezequiel Garcia 写道:
> > Now that RK3568 SoC devicetree upstreaming is happening [1],
> > here's another round for the RK3566 dwmac. There wasn't any clear
> > consensus on how to implement the two interfaces present
> > on RK3568, so I decided to drop that and just submit RK3566 for now.
> > 
> > This has been tested on a Pine64 RK3566 Quartz64 Model B board,
> > DHCP and iperf are looking good.
> > 
> > For all the people testing, here's Quartz 64 Model B device tree
> > snippet:
> > 
> >          gmac1: ethernet@fe010000 {
> >                  compatible = "rockchip,rk3566-gmac", "snps,dwmac-4.20a";
> 
> It is better to use "rockchip,rk3568-gmac" here, "rockchip,rk3566-gmac" 
> is not compatible, 3568 has two gmacs, which are compatible with 3566.
> 
> If there is no better way, using bus_id from alias is good, it is a 
> fixed id, and U-Boot also use the id to write MAC address into kernel DTB.
> 
> plat->bus_id = of_alias_get_id(np, "ethernet");
> 

This was discussed, see ChenYu's reply. I think the idea
is considered fragile:

https://lore.kernel.org/netdev/CAGb2v67ZBR=XDFPeXQc429HNu_dbY__-KN50tvBW44fXMs78_w@mail.gmail.com/

However, I agree with you about going back to just "rockchip,rk3568-gmac".
Adding rockchip,rk3566-gmac maybe wasn't a great idea :-)

The most accepted way forward seems Heiko's proposal of hardcoding the mmio
addresses to identify each block, as it's done in the DSI driver:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c#n1170

Would you agree with it?

Thanks!
Ezequiel
 
> >                  reg = <0x0 0xfe010000 0x0 0x10000>;
> >                  interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>,
> >                               <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
> >                  interrupt-names = "macirq", "eth_wake_irq";
> >                  rockchip,grf = <&grf>;
> >                  clocks = <&cru SCLK_GMAC1>, <&cru SCLK_GMAC1_RX_TX>,
> >                           <&cru SCLK_GMAC1_RX_TX>, <&cru CLK_MAC1_REFOUT>,
> >                           <&cru ACLK_GMAC1>, <&cru PCLK_GMAC1>,
> >                           <&cru SCLK_GMAC1_RX_TX>, <&cru CLK_GMAC1_PTP_REF>;
> >                  clock-names = "stmmaceth", "mac_clk_rx",
> >                                "mac_clk_tx", "clk_mac_refout",
> >                                "aclk_mac", "pclk_mac",
> >                                "clk_mac_speed", "ptp_ref";
> >                  resets = <&cru SRST_A_GMAC1>;
> >                  reset-names = "stmmaceth";
> > 
> >                  snps,mixed-burst;
> >                  snps,tso;
> > 
> >                  snps,axi-config = <&gmac1_stmmac_axi_setup>;
> >                  snps,mtl-rx-config = <&gmac1_mtl_rx_setup>;
> >                  snps,mtl-tx-config = <&gmac1_mtl_tx_setup>;
> >                  status = "disabled";
> > 
> >                  mdio1: mdio {
> >                          compatible = "snps,dwmac-mdio";
> >                          #address-cells = <0x1>;
> >                          #size-cells = <0x0>;
> >                  };
> > 
> >                  gmac1_stmmac_axi_setup: stmmac-axi-config {
> >                          snps,wr_osr_lmt = <4>;
> >                          snps,rd_osr_lmt = <8>;
> >                          snps,blen = <0 0 0 0 16 8 4>;
> >                  };
> > 
> >                  gmac1_mtl_rx_setup: rx-queues-config {
> >                          snps,rx-queues-to-use = <1>;
> >                          queue0 {};
> >                  };
> > 
> >                  gmac1_mtl_tx_setup: tx-queues-config {
> >                          snps,tx-queues-to-use = <1>;
> >                          queue0 {};
> >                  };
> >          };
> > 
> > While here, I'm adding a small patch from David Wu, for some
> > sanity checks for dwmac-rockchip-specific non-NULL ops.
> > 
> > Thanks!
> > 
> > [1] http://lore.kernel.org/r/20210421065921.23917-1-cl@rock-chips.com
> > 
> > David Wu (2):
> >    net: stmmac: dwmac-rk: Check platform-specific ops
> >    net: stmmac: Add RK3566 SoC support
> > 
> > Ezequiel Garcia (1):
> >    net: stmmac: Don't set has_gmac if has_gmac4 is set
> > 
> >   .../bindings/net/rockchip-dwmac.txt           |   1 +
> >   .../net/ethernet/stmicro/stmmac/dwmac-rk.c    | 126 +++++++++++++++++-
> >   2 files changed, 124 insertions(+), 3 deletions(-)
> > 
> 
>
David Wu April 25, 2021, 9:07 a.m. UTC | #3
Hi Ezequiel,

在 2021/4/23 下午9:57, Ezequiel Garcia 写道:
> Hi David,
> 
> Thanks a lot for reviewing.
> 
> On Fri, 2021-04-23 at 17:24 +0800, David Wu wrote:
>> Hi Ezequiel,
>>
>> 在 2021/4/22 上午4:34, Ezequiel Garcia 写道:
>>> Now that RK3568 SoC devicetree upstreaming is happening [1],
>>> here's another round for the RK3566 dwmac. There wasn't any clear
>>> consensus on how to implement the two interfaces present
>>> on RK3568, so I decided to drop that and just submit RK3566 for now.
>>>
>>> This has been tested on a Pine64 RK3566 Quartz64 Model B board,
>>> DHCP and iperf are looking good.
>>>
>>> For all the people testing, here's Quartz 64 Model B device tree
>>> snippet:
>>>
>>>           gmac1: ethernet@fe010000 {
>>>                   compatible = "rockchip,rk3566-gmac", "snps,dwmac-4.20a";
>>
>> It is better to use "rockchip,rk3568-gmac" here, "rockchip,rk3566-gmac"
>> is not compatible, 3568 has two gmacs, which are compatible with 3566.
>>
>> If there is no better way, using bus_id from alias is good, it is a
>> fixed id, and U-Boot also use the id to write MAC address into kernel DTB.
>>
>> plat->bus_id = of_alias_get_id(np, "ethernet");
>>
> 
> This was discussed, see ChenYu's reply. I think the idea
> is considered fragile:
> 
> https://lore.kernel.org/netdev/CAGb2v67ZBR=XDFPeXQc429HNu_dbY__-KN50tvBW44fXMs78_w@mail.gmail.com/
> 
> However, I agree with you about going back to just "rockchip,rk3568-gmac".
> Adding rockchip,rk3566-gmac maybe wasn't a great idea :-)
> 
> The most accepted way forward seems Heiko's proposal of hardcoding the mmio
> addresses to identify each block, as it's done in the DSI driver:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c#n1170
> 
> Would you agree with it?
> 

Yes, I agree.
I also think the alias should be rarely changed. The DSI mmio addresses 
example here can be used for reference, and it seems that it will be 
stronger than alias.

> Thanks!
> Ezequiel
>   
>>>                   reg = <0x0 0xfe010000 0x0 0x10000>;
>>>                   interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>,
>>>                                <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
>>>                   interrupt-names = "macirq", "eth_wake_irq";
>>>                   rockchip,grf = <&grf>;
>>>                   clocks = <&cru SCLK_GMAC1>, <&cru SCLK_GMAC1_RX_TX>,
>>>                            <&cru SCLK_GMAC1_RX_TX>, <&cru CLK_MAC1_REFOUT>,
>>>                            <&cru ACLK_GMAC1>, <&cru PCLK_GMAC1>,
>>>                            <&cru SCLK_GMAC1_RX_TX>, <&cru CLK_GMAC1_PTP_REF>;
>>>                   clock-names = "stmmaceth", "mac_clk_rx",
>>>                                 "mac_clk_tx", "clk_mac_refout",
>>>                                 "aclk_mac", "pclk_mac",
>>>                                 "clk_mac_speed", "ptp_ref";
>>>                   resets = <&cru SRST_A_GMAC1>;
>>>                   reset-names = "stmmaceth";
>>>
>>>                   snps,mixed-burst;
>>>                   snps,tso;
>>>
>>>                   snps,axi-config = <&gmac1_stmmac_axi_setup>;
>>>                   snps,mtl-rx-config = <&gmac1_mtl_rx_setup>;
>>>                   snps,mtl-tx-config = <&gmac1_mtl_tx_setup>;
>>>                   status = "disabled";
>>>
>>>                   mdio1: mdio {
>>>                           compatible = "snps,dwmac-mdio";
>>>                           #address-cells = <0x1>;
>>>                           #size-cells = <0x0>;
>>>                   };
>>>
>>>                   gmac1_stmmac_axi_setup: stmmac-axi-config {
>>>                           snps,wr_osr_lmt = <4>;
>>>                           snps,rd_osr_lmt = <8>;
>>>                           snps,blen = <0 0 0 0 16 8 4>;
>>>                   };
>>>
>>>                   gmac1_mtl_rx_setup: rx-queues-config {
>>>                           snps,rx-queues-to-use = <1>;
>>>                           queue0 {};
>>>                   };
>>>
>>>                   gmac1_mtl_tx_setup: tx-queues-config {
>>>                           snps,tx-queues-to-use = <1>;
>>>                           queue0 {};
>>>                   };
>>>           };
>>>
>>> While here, I'm adding a small patch from David Wu, for some
>>> sanity checks for dwmac-rockchip-specific non-NULL ops.
>>>
>>> Thanks!
>>>
>>> [1] http://lore.kernel.org/r/20210421065921.23917-1-cl@rock-chips.com
>>>
>>> David Wu (2):
>>>     net: stmmac: dwmac-rk: Check platform-specific ops
>>>     net: stmmac: Add RK3566 SoC support
>>>
>>> Ezequiel Garcia (1):
>>>     net: stmmac: Don't set has_gmac if has_gmac4 is set
>>>
>>>    .../bindings/net/rockchip-dwmac.txt           |   1 +
>>>    .../net/ethernet/stmicro/stmmac/dwmac-rk.c    | 126 +++++++++++++++++-
>>>    2 files changed, 124 insertions(+), 3 deletions(-)
>>>
>>
>>
> 
> 
> 
> 
>