| Message ID | 20230320221617.236323-1-ahalaney@redhat.com |
|---|---|
| Headers | show |
| Series | Add EMAC3 support for sa8540p-ride | expand |
On Mon, 20 Mar 2023 17:16:05 -0500 Andrew Halaney wrote: > This is a forward port / upstream refactor of code delivered > downstream by Qualcomm over at [0] to enable the DWMAC5 based > implementation called EMAC3 on the sa8540p-ride dev board. > > From what I can tell with the board schematic in hand, > as well as the code delivered, the main changes needed are: > > 1. A new address space layout for /dwmac5/EMAC3 MTL/DMA regs > 2. A new programming sequence required for the EMAC3 base platforms > > This series makes those adaptations as well as other housekeeping items > such as converting dt-bindings to yaml, adding clock descriptions, etc. > > [0] https://git.codelinaro.org/clo/la/kernel/ark-5.14/-/commit/510235ad02d7f0df478146fb00d7a4ba74821b17 > > v1: https://lore.kernel.org/netdev/20230313165620.128463-1-ahalaney@redhat.com/ At a glance 1-4,8-12 need to go via networking, 5 via clock tree, and 6,7 via ARM/Qualcomm. AFAICT there are no strong (compile) dependencies so we can each merge our chunk and they will meet in Linus's tree? If so please repost just the networking stuff for net-next, and the other bits to respective trees, as separate series.
On Mon, 20 Mar 2023 17:16:15 -0500 Andrew Halaney wrote: > static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos) > { > + int phy_mode; > + int phase_shift; nit: invert the order, we like variable declaration lines longest to shortest > + /* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */ > + phy_mode = device_get_phy_mode(ðqos->pdev->dev); > + if (phy_mode == PHY_INTERFACE_MODE_RGMII_ID || phy_mode == PHY_INTERFACE_MODE_RGMII_TXID) Let's try to stick to 80 chars where reasonable, this would be easier to read as 2 lines. > + phase_shift = 0; > + else > + phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN; > + > /* Disable loopback mode */ > rgmii_updatel(ethqos, RGMII_CONFIG2_TX_TO_RX_LOOPBACK_EN, > 0, RGMII_IO_MACRO_CONFIG2); > @@ -300,9 +310,9 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos) > RGMII_CONFIG_PROG_SWAP, RGMII_IO_MACRO_CONFIG); > rgmii_updatel(ethqos, RGMII_CONFIG2_DATA_DIVIDE_CLK_SEL, > 0, RGMII_IO_MACRO_CONFIG2); > + > rgmii_updatel(ethqos, RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN, > - RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN, > - RGMII_IO_MACRO_CONFIG2); > + phase_shift, RGMII_IO_MACRO_CONFIG2); here and in couple more places looks like indentation got broken? continuation line should start under the opening bracket + 1.
On Mon, 20 Mar 2023 17:16:14 -0500 Andrew Halaney wrote: > The next approach that was checked was to have a function pointer > embedded inside a structure that does the appropriate conversion based > on the variant that's in use. However, some of the function definitions > are like the following: > > void emac3_set_rx_ring_len(void __iomem *ioaddr, u32 len, u32 chan) I checked a couple of callbacks and they seem to all be called with priv->iomem as an arg, so there is no strong reason to pass iomem instead of priv / hw. Or at least not to pass both.. I think that's a better approach than adding the wrappers :( Are you familiar with coccinelle / spatch? It's often better than just regexps for refactoring, maybe it can help?
On Mon, Mar 20, 2023 at 08:28:02PM -0700, Jakub Kicinski wrote: > On Mon, 20 Mar 2023 17:16:05 -0500 Andrew Halaney wrote: > > This is a forward port / upstream refactor of code delivered > > downstream by Qualcomm over at [0] to enable the DWMAC5 based > > implementation called EMAC3 on the sa8540p-ride dev board. > > > > From what I can tell with the board schematic in hand, > > as well as the code delivered, the main changes needed are: > > > > 1. A new address space layout for /dwmac5/EMAC3 MTL/DMA regs > > 2. A new programming sequence required for the EMAC3 base platforms > > > > This series makes those adaptations as well as other housekeeping items > > such as converting dt-bindings to yaml, adding clock descriptions, etc. > > > > [0] https://git.codelinaro.org/clo/la/kernel/ark-5.14/-/commit/510235ad02d7f0df478146fb00d7a4ba74821b17 > > > > v1: https://lore.kernel.org/netdev/20230313165620.128463-1-ahalaney@redhat.com/ > > At a glance 1-4,8-12 need to go via networking, 5 via clock tree, > and 6,7 via ARM/Qualcomm. > > AFAICT there are no strong (compile) dependencies so we can each merge > our chunk and they will meet in Linus's tree? If so please repost just > the networking stuff for net-next, and the other bits to respective > trees, as separate series. > That makes sense to me, thanks for the advice. The only note is that 5 (the clk patch) is depended on by 6/7 to compile (they use the header value in 5)... So I'll keep those together! So all in all it will be the dt-binding changes + stmmac changes in one series for networking, and the clock + devicetree changes via ARM/Qualcomm if I am following properly. I'll go that route for v3 and link here (just to make finding the split easier) unless someone objects (got some time as I need to refactor based on series feedback)! Thanks, Andrew
On 20.03.2023 23:16, Andrew Halaney wrote: > This platform has 2 MACs integrated in it, go ahead and describe them. > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > --- > > Changes since v1: > * None > > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 53 ++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > index 0d02599d8867..a63e8e81a8c4 100644 > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > @@ -761,6 +761,59 @@ soc: soc@0 { > ranges = <0 0 0 0 0x10 0>; > dma-ranges = <0 0 0 0 0x10 0>; > > + ethernet0: ethernet@20000 { > + compatible = "qcom,sc8280xp-ethqos"; > + reg = <0x0 0x00020000 0x0 0x10000>, > + <0x0 0x00036000 0x0 0x100>; Please correct the indentation here. > + reg-names = "stmmaceth", "rgmii"; > + > + clocks = <&gcc GCC_EMAC0_AXI_CLK>, > + <&gcc GCC_EMAC0_SLV_AHB_CLK>, > + <&gcc GCC_EMAC0_PTP_CLK>, > + <&gcc GCC_EMAC0_RGMII_CLK>; Please correct the indentation here. > + clock-names = "stmmaceth", "pclk", "ptp_ref", "rgmii"; Please turn this into a vertical list. > + > + interrupts = <GIC_SPI 946 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 936 IRQ_TYPE_LEVEL_HIGH>; Please correct the indentation here. Same for the other node. Konrad > + interrupt-names = "macirq", "eth_lpi"; > + iommus = <&apps_smmu 0x4c0 0xf>; > + power-domains = <&gcc EMAC_0_GDSC>; > + > + snps,tso; > + snps,pbl = <32>; > + rx-fifo-depth = <4096>; > + tx-fifo-depth = <4096>; > + > + status = "disabled"; > + }; > + > + ethernet1: ethernet@23000000 { > + compatible = "qcom,sc8280xp-ethqos"; > + reg = <0x0 0x23000000 0x0 0x10000>, > + <0x0 0x23016000 0x0 0x100>; > + reg-names = "stmmaceth", "rgmii"; > + > + clocks = <&gcc GCC_EMAC1_AXI_CLK>, > + <&gcc GCC_EMAC1_SLV_AHB_CLK>, > + <&gcc GCC_EMAC1_PTP_CLK>, > + <&gcc GCC_EMAC1_RGMII_CLK>; > + clock-names = "stmmaceth", "pclk", "ptp_ref", "rgmii"; > + > + interrupts = <GIC_SPI 929 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 919 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "macirq", "eth_lpi"; > + > + iommus = <&apps_smmu 0x40 0xf>; > + power-domains = <&gcc EMAC_1_GDSC>; > + > + snps,tso; > + snps,pbl = <32>; > + rx-fifo-depth = <4096>; > + tx-fifo-depth = <4096>; > + > + status = "disabled"; > + }; > + > gcc: clock-controller@100000 { > compatible = "qcom,gcc-sc8280xp"; > reg = <0x0 0x00100000 0x0 0x1f0000>;
On Mon, Mar 20, 2023 at 08:41:53PM -0700, Jakub Kicinski wrote: > On Mon, 20 Mar 2023 17:16:14 -0500 Andrew Halaney wrote: > > The next approach that was checked was to have a function pointer > > embedded inside a structure that does the appropriate conversion based > > on the variant that's in use. However, some of the function definitions > > are like the following: > > > > void emac3_set_rx_ring_len(void __iomem *ioaddr, u32 len, u32 chan) > > I checked a couple of callbacks and they seem to all be called with > priv->iomem as an arg, so there is no strong reason to pass iomem > instead of priv / hw. Or at least not to pass both.. > > I think that's a better approach than adding the wrappers :( > > Are you familiar with coccinelle / spatch? It's often better than > just regexps for refactoring, maybe it can help? > No worries, I'll try and refactor as you mentioned. Looking at it some this afternoon makes me think I'll try something like this: diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h index 16a7421715cb..75c55f696c7a 100644 --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h @@ -12,7 +12,7 @@ ({ \ int __result = -EINVAL; \ if ((__priv)->hw->__module && (__priv)->hw->__module->__cname) { \ - (__priv)->hw->__module->__cname((__arg0), ##__args); \ + (__priv)->hw->__module->__cname((__priv), (__arg0), ##__args); \ __result = 0; \ } \ __result; \ @@ -21,7 +21,7 @@ ({ \ int __result = -EINVAL; \ if ((__priv)->hw->__module && (__priv)->hw->__module->__cname) \ - __result = (__priv)->hw->__module->__cname((__arg0), ##__args); \ + __result = (__priv)->hw->__module->__cname((__priv), (__arg0), ##__args); \ __result; \ }) @@ -34,68 +34,68 @@ struct dma_edesc; /* Descriptors helpers */ struct stmmac_desc_ops { /* DMA RX descriptor ring initialization */ - void (*init_rx_desc)(struct dma_desc *p, int disable_rx_ic, int mode, - int end, int bfsize); + void (*init_rx_desc)(struct stmmac_priv *priv, struct dma_desc *p, + int disable_rx_ic, int mode, int end, int bfsize); /* DMA TX descriptor ring initialization */ (...) and then, I'll add something like: diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index a152678b82b7..f5f406a09ae3 100644 --- a/include/linux/stmmac.h +++ b/include/linux/stmmac.h @@ -273,5 +273,7 @@ struct plat_stmmacenet_data { bool use_phy_wol; bool sph_disable; bool serdes_up_after_phy_linkup; + u32 mtl_base; + u32 mtl_offset; }; #endif and rewrite: #define MTL_CHANX_BASE_ADDR(x) (MTL_CHAN_BASE_ADDR + \ (x * MTL_CHAN_BASE_OFFSET)) to use mtl_base/offset if they exist, and so on for the DMA versions, etc... I'm sure I'll probably run into some issue and change course slightly, but thought I'd post a hint of the path to make sure I'm not way off the mark. Thanks for your feedback, Andrew
On Tue, Mar 21, 2023 at 01:44:35PM -0500, Andrew Halaney wrote: > On Mon, Mar 20, 2023 at 08:28:02PM -0700, Jakub Kicinski wrote: > > On Mon, 20 Mar 2023 17:16:05 -0500 Andrew Halaney wrote: > > > This is a forward port / upstream refactor of code delivered > > > downstream by Qualcomm over at [0] to enable the DWMAC5 based > > > implementation called EMAC3 on the sa8540p-ride dev board. > > > > > > From what I can tell with the board schematic in hand, > > > as well as the code delivered, the main changes needed are: > > > > > > 1. A new address space layout for /dwmac5/EMAC3 MTL/DMA regs > > > 2. A new programming sequence required for the EMAC3 base platforms > > > > > > This series makes those adaptations as well as other housekeeping items > > > such as converting dt-bindings to yaml, adding clock descriptions, etc. > > > > > > [0] https://git.codelinaro.org/clo/la/kernel/ark-5.14/-/commit/510235ad02d7f0df478146fb00d7a4ba74821b17 > > > > > > v1: https://lore.kernel.org/netdev/20230313165620.128463-1-ahalaney@redhat.com/ > > > > At a glance 1-4,8-12 need to go via networking, 5 via clock tree, > > and 6,7 via ARM/Qualcomm. > > > > AFAICT there are no strong (compile) dependencies so we can each merge > > our chunk and they will meet in Linus's tree? If so please repost just > > the networking stuff for net-next, and the other bits to respective > > trees, as separate series. > > > > That makes sense to me, thanks for the advice. > > The only note is that 5 (the clk patch) is depended on by 6/7 to > compile (they use the header value in 5)... So I'll keep those together! > Sounds good to me! Regards, Bjorn > So all in all it will be the dt-binding changes + stmmac changes in one > series for networking, and the clock + devicetree changes via > ARM/Qualcomm if I am following properly. > > I'll go that route for v3 and link here (just to make finding the split > easier) unless someone objects (got some time as I need to refactor > based on series feedback)! > > Thanks, > Andrew >
This is a forward port / upstream refactor of code delivered downstream by Qualcomm over at [0] to enable the DWMAC5 based implementation called EMAC3 on the sa8540p-ride dev board. From what I can tell with the board schematic in hand, as well as the code delivered, the main changes needed are: 1. A new address space layout for /dwmac5/EMAC3 MTL/DMA regs 2. A new programming sequence required for the EMAC3 base platforms This series makes those adaptations as well as other housekeeping items such as converting dt-bindings to yaml, adding clock descriptions, etc. [0] https://git.codelinaro.org/clo/la/kernel/ark-5.14/-/commit/510235ad02d7f0df478146fb00d7a4ba74821b17 v1: https://lore.kernel.org/netdev/20230313165620.128463-1-ahalaney@redhat.com/ Thanks, Andrew Andrew Halaney (8): dt-bindings: net: qcom,ethqos: Add Qualcomm sc8280xp compatibles clk: qcom: gcc-sc8280xp: Add EMAC GDSCs arm64: dts: qcom: sc8280xp: Add ethernet nodes arm64: dts: qcom: sa8540p-ride: Add ethernet nodes net: stmmac: Remove unnecessary if statement brackets net: stmmac: dwmac-qcom-ethqos: Respect phy-mode and TX delay net: stmmac: dwmac-qcom-ethqos: Use loopback_en for all speeds net: stmmac: dwmac-qcom-ethqos: Add EMAC3 support Bhupesh Sharma (3): dt-bindings: net: snps,dwmac: Update interrupt-names dt-bindings: net: snps,dwmac: Add Qualcomm Ethernet ETHQOS compatibles dt-bindings: net: qcom,ethqos: Convert bindings to yaml Brian Masney (1): net: stmmac: Add EMAC3 variant of dwmac4 .../devicetree/bindings/net/qcom,ethqos.txt | 66 ---- .../devicetree/bindings/net/qcom,ethqos.yaml | 111 ++++++ .../devicetree/bindings/net/snps,dwmac.yaml | 9 +- MAINTAINERS | 2 +- arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 181 ++++++++++ arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 53 +++ drivers/clk/qcom/gcc-sc8280xp.c | 18 + .../stmicro/stmmac/dwmac-qcom-ethqos.c | 161 ++++++--- drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 32 +- .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 235 ++++++++++-- .../net/ethernet/stmicro/stmmac/dwmac4_dma.c | 334 ++++++++++++++---- .../net/ethernet/stmicro/stmmac/dwmac4_dma.h | 38 ++ .../net/ethernet/stmicro/stmmac/dwmac4_lib.c | 144 ++++++-- drivers/net/ethernet/stmicro/stmmac/hwif.c | 29 +- drivers/net/ethernet/stmicro/stmmac/hwif.h | 2 + .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 6 +- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 17 +- .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 9 +- .../net/ethernet/stmicro/stmmac/stmmac_ptp.c | 4 +- include/dt-bindings/clock/qcom,gcc-sc8280xp.h | 2 + include/linux/stmmac.h | 1 + 21 files changed, 1196 insertions(+), 258 deletions(-) delete mode 100644 Documentation/devicetree/bindings/net/qcom,ethqos.txt create mode 100644 Documentation/devicetree/bindings/net/qcom,ethqos.yaml