mbox series

[net-next,v2,00/12] Add EMAC3 support for sa8540p-ride

Message ID 20230320221617.236323-1-ahalaney@redhat.com
Headers show
Series Add EMAC3 support for sa8540p-ride | expand

Message

Andrew Halaney March 20, 2023, 10:16 p.m. UTC 
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.

Comments

Jakub Kicinski March 21, 2023, 3:28 a.m. UTC | #1 
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.
Jakub Kicinski March 21, 2023, 3:41 a.m. UTC | #2 
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?
Krzysztof Kozlowski March 21, 2023, 6:46 a.m. UTC | #3 
On 20/03/2023 23:16, Andrew Halaney wrote:
> From: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> 
> As commit fc191af1bb0d ("net: stmmac: platform: Fix misleading
> interrupt error msg") noted, not every stmmac based platform
> makes use of the 'eth_wake_irq' or 'eth_lpi' interrupts.
> 
> So, update the 'interrupt-names' inside 'snps,dwmac' YAML
> bindings to reflect the same.
> 
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
Krzysztof Kozlowski March 21, 2023, 6:47 a.m. UTC | #4 
On 20/03/2023 23:16, Andrew Halaney wrote:
> The sc8280xp has a new version of the ETHQOS hardware in it, EMAC v3.
> Add a compatible for this.
> 
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> ---
> 
> Changes since v1:
> 	* Alphabetical sorting (Krzysztof)
> 
>  Documentation/devicetree/bindings/net/qcom,ethqos.yaml | 1 +
>  Documentation/devicetree/bindings/net/snps,dwmac.yaml  | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
> index 88234a2010b1..c60248e17e5a 100644
> --- a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
> @@ -21,6 +21,7 @@ properties:
>      enum:
>        - qcom,qcs404-ethqos
>        - qcom,sm8150-ethqos
> +      - qcom,sc8280xp-ethqos

This still needs sort.

Best regards,
Krzysztof
Konrad Dybcio March 21, 2023, 7:33 p.m. UTC | #5 
On 20.03.2023 23:16, Andrew Halaney wrote:
> Add the EMAC GDSCs to allow the EMAC hardware to be enabled.
> 
> Acked-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
> 
> I'm still unsure if Bjorn wants to take this patch or net-dev, and how I am
> supposed to indicate such other than commenting here (per Stephen's
> comment on v1): https://lore.kernel.org/netdev/e5cb46e8874b12dbe438be12ee0cf949.sboyd@kernel.org/#t
> 
> Changes since v1:
> 	* Add Stephen's Acked-by
> 	* Explicitly tested on x13s laptop with no noticeable side effect (Konrad)
> 
>  drivers/clk/qcom/gcc-sc8280xp.c               | 18 ++++++++++++++++++
>  include/dt-bindings/clock/qcom,gcc-sc8280xp.h |  2 ++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c
> index b3198784e1c3..04a99dbaa57e 100644
> --- a/drivers/clk/qcom/gcc-sc8280xp.c
> +++ b/drivers/clk/qcom/gcc-sc8280xp.c
> @@ -6873,6 +6873,22 @@ static struct gdsc usb30_sec_gdsc = {
>  	.pwrsts = PWRSTS_RET_ON,
>  };
>  
> +static struct gdsc emac_0_gdsc = {
> +	.gdscr = 0xaa004,
> +	.pd = {
> +		.name = "emac_0_gdsc",
> +	},
> +	.pwrsts = PWRSTS_OFF_ON,
> +};
> +
> +static struct gdsc emac_1_gdsc = {
> +	.gdscr = 0xba004,
> +	.pd = {
> +		.name = "emac_1_gdsc",
> +	},
> +	.pwrsts = PWRSTS_OFF_ON,
> +};
> +
>  static struct clk_regmap *gcc_sc8280xp_clocks[] = {
>  	[GCC_AGGRE_NOC_PCIE0_TUNNEL_AXI_CLK] = &gcc_aggre_noc_pcie0_tunnel_axi_clk.clkr,
>  	[GCC_AGGRE_NOC_PCIE1_TUNNEL_AXI_CLK] = &gcc_aggre_noc_pcie1_tunnel_axi_clk.clkr,
> @@ -7351,6 +7367,8 @@ static struct gdsc *gcc_sc8280xp_gdscs[] = {
>  	[USB30_MP_GDSC] = &usb30_mp_gdsc,
>  	[USB30_PRIM_GDSC] = &usb30_prim_gdsc,
>  	[USB30_SEC_GDSC] = &usb30_sec_gdsc,
> +	[EMAC_0_GDSC] = &emac_0_gdsc,
> +	[EMAC_1_GDSC] = &emac_1_gdsc,
>  };
>  
>  static const struct clk_rcg_dfs_data gcc_dfs_clocks[] = {
> diff --git a/include/dt-bindings/clock/qcom,gcc-sc8280xp.h b/include/dt-bindings/clock/qcom,gcc-sc8280xp.h
> index cb2fb638825c..721105ea4fad 100644
> --- a/include/dt-bindings/clock/qcom,gcc-sc8280xp.h
> +++ b/include/dt-bindings/clock/qcom,gcc-sc8280xp.h
> @@ -492,5 +492,7 @@
>  #define USB30_MP_GDSC					9
>  #define USB30_PRIM_GDSC					10
>  #define USB30_SEC_GDSC					11
> +#define EMAC_0_GDSC					12
> +#define EMAC_1_GDSC					13
>  
>  #endif
Andrew Halaney March 21, 2023, 10:19 p.m. UTC | #6 
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