by Andrew Halaney

[permalink] [raw]

Subject: Re: [PATCH net-next v2 09/12] net: stmmac: Add EMAC3 variant of dwmac4

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

2023-03-22 03:10:05

by Bjorn Andersson

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/12] Add EMAC3 support for sa8540p-ride

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/[email protected]/
> >
> > 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
>