| Message ID | 20230331214549.756660-1-ahalaney@redhat.com |
|---|---|
| Headers | show |
| Series | Add EMAC3 support for sa8540p-ride | expand |
On Fri, 31 Mar 2023 17:06:13 -0500 Andrew Halaney wrote:
> As promised: https://lore.kernel.org/netdev/20230331215804.783439-1-ahalaney@redhat.com/T/#t
Patch 12 never made it to netdev or lore :(
On Fri, Mar 31, 2023 at 04:45:46PM -0500, Andrew Halaney wrote: > Some platforms have dwmac4 implementations that have a different > address space layout than the default, resulting in the need to define > their own DMA/MTL offsets. > > Extend the functions to allow a platform driver to indicate what its > addresses are, overriding the defaults. > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > --- > > This patch (and the prior patch) are replacements for > https://lore.kernel.org/netdev/20230320204153.21736840@kernel.org/ > as was requested. Hopefully I was understanding the intent correctly :) > > I'm pretty sure further refinement will be requested for this one, but > it is the best I could come up with myself! Specifically some of the > naming, dealing with spacing in some older spots of dwmac4, > where the addresses should live in the structure hierarchy, etc are > things I would not be surprised to have to rework if this is still > preferred over the wrapper approach. > > Changes since v2: > * New, replacing old wrapper approach > > drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 91 ++++++++-- > .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 36 ++-- > .../net/ethernet/stmicro/stmmac/dwmac4_dma.c | 157 ++++++++++-------- > .../net/ethernet/stmicro/stmmac/dwmac4_dma.h | 51 +++--- > .../net/ethernet/stmicro/stmmac/dwmac4_lib.c | 67 +++++--- > include/linux/stmmac.h | 19 +++ > 6 files changed, 279 insertions(+), 142 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > index ccd49346d3b3..a0c0ee1dc13f 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > @@ -336,14 +336,23 @@ enum power_event { > > #define MTL_CHAN_BASE_ADDR 0x00000d00 > #define MTL_CHAN_BASE_OFFSET 0x40 > -#define MTL_CHANX_BASE_ADDR(x) (MTL_CHAN_BASE_ADDR + \ > - (x * MTL_CHAN_BASE_OFFSET)) > - > -#define MTL_CHAN_TX_OP_MODE(x) MTL_CHANX_BASE_ADDR(x) > -#define MTL_CHAN_TX_DEBUG(x) (MTL_CHANX_BASE_ADDR(x) + 0x8) > -#define MTL_CHAN_INT_CTRL(x) (MTL_CHANX_BASE_ADDR(x) + 0x2c) > -#define MTL_CHAN_RX_OP_MODE(x) (MTL_CHANX_BASE_ADDR(x) + 0x30) > -#define MTL_CHAN_RX_DEBUG(x) (MTL_CHANX_BASE_ADDR(x) + 0x38) > +#define MTL_CHANX_BASE_ADDR(addrs, x) \ > +({ \ > + const struct dwmac4_addrs *__addrs = addrs; \ > + const u32 __x = x; \ > + u32 __addr; \ > + if (__addrs) \ > + __addr = __addrs->mtl_chan + (__x * __addrs->mtl_chan_offset); \ > + else \ > + __addr = MTL_CHAN_BASE_ADDR + (__x * MTL_CHAN_BASE_OFFSET); \ > + __addr; \ > +}) Could this and similar macros added by this patch be functions?
On Sat, Apr 01, 2023 at 05:06:21PM +0200, Simon Horman wrote: > On Fri, Mar 31, 2023 at 04:45:45PM -0500, Andrew Halaney wrote: > > Passing stmmac_priv to some of the callbacks allows hwif implementations > > to grab some data that platforms can customize. Adjust the callbacks > > accordingly in preparation of such a platform customization. > > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > ... > > > #define stmmac_reset(__priv, __args...) \ > > @@ -223,59 +240,59 @@ struct stmmac_dma_ops { > > #define stmmac_dma_init(__priv, __args...) \ > > stmmac_do_void_callback(__priv, dma, init, __args) > > #define stmmac_init_chan(__priv, __args...) \ > > - stmmac_do_void_callback(__priv, dma, init_chan, __args) > > + stmmac_do_void_callback(__priv, dma, init_chan, __priv, __args) > > Hi Andrew, > > Rather than maintaining these macros can we just get rid of them? > I'd be surprised if things aren't nicer with functions in their place [1]. > > f.e., we now have (__priv, ..., __priv, ...) due to a generalisation > that seems to take a lot more than it gives. > > [1] https://lore.kernel.org/linux-arm-kernel/ZBst1SzcIS4j+t46@corigine.com/ > Thanks for the pointer. I think that makes sense, I'll take that approach for these functions (and maybe in a follow-up series I'll tackle all of them just because the lack of consistency will eat me up). Sorry for the delay, had some issues around the house that became urgent. Thanks, Andrew
On Sat, Apr 01, 2023 at 04:58:59PM +0200, Simon Horman wrote: > On Fri, Mar 31, 2023 at 04:45:46PM -0500, Andrew Halaney wrote: > > Some platforms have dwmac4 implementations that have a different > > address space layout than the default, resulting in the need to define > > their own DMA/MTL offsets. > > > > Extend the functions to allow a platform driver to indicate what its > > addresses are, overriding the defaults. > > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > --- > > > > This patch (and the prior patch) are replacements for > > https://lore.kernel.org/netdev/20230320204153.21736840@kernel.org/ > > as was requested. Hopefully I was understanding the intent correctly :) > > > > I'm pretty sure further refinement will be requested for this one, but > > it is the best I could come up with myself! Specifically some of the > > naming, dealing with spacing in some older spots of dwmac4, > > where the addresses should live in the structure hierarchy, etc are > > things I would not be surprised to have to rework if this is still > > preferred over the wrapper approach. > > > > Changes since v2: > > * New, replacing old wrapper approach > > > > drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 91 ++++++++-- > > .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 36 ++-- > > .../net/ethernet/stmicro/stmmac/dwmac4_dma.c | 157 ++++++++++-------- > > .../net/ethernet/stmicro/stmmac/dwmac4_dma.h | 51 +++--- > > .../net/ethernet/stmicro/stmmac/dwmac4_lib.c | 67 +++++--- > > include/linux/stmmac.h | 19 +++ > > 6 files changed, 279 insertions(+), 142 deletions(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > > index ccd49346d3b3..a0c0ee1dc13f 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > > @@ -336,14 +336,23 @@ enum power_event { > > > > #define MTL_CHAN_BASE_ADDR 0x00000d00 > > #define MTL_CHAN_BASE_OFFSET 0x40 > > -#define MTL_CHANX_BASE_ADDR(x) (MTL_CHAN_BASE_ADDR + \ > > - (x * MTL_CHAN_BASE_OFFSET)) > > - > > -#define MTL_CHAN_TX_OP_MODE(x) MTL_CHANX_BASE_ADDR(x) > > -#define MTL_CHAN_TX_DEBUG(x) (MTL_CHANX_BASE_ADDR(x) + 0x8) > > -#define MTL_CHAN_INT_CTRL(x) (MTL_CHANX_BASE_ADDR(x) + 0x2c) > > -#define MTL_CHAN_RX_OP_MODE(x) (MTL_CHANX_BASE_ADDR(x) + 0x30) > > -#define MTL_CHAN_RX_DEBUG(x) (MTL_CHANX_BASE_ADDR(x) + 0x38) > > +#define MTL_CHANX_BASE_ADDR(addrs, x) \ > > +({ \ > > + const struct dwmac4_addrs *__addrs = addrs; \ > > + const u32 __x = x; \ > > + u32 __addr; \ > > + if (__addrs) \ > > + __addr = __addrs->mtl_chan + (__x * __addrs->mtl_chan_offset); \ > > + else \ > > + __addr = MTL_CHAN_BASE_ADDR + (__x * MTL_CHAN_BASE_OFFSET); \ > > + __addr; \ > > +}) > > Could this and similar macros added by this patch be functions? > From my pov a benefit would be slightly more type safety. > And as a bonus there wouldn't be any need to handle aliasing of input. > Sure, to be honest I'll be much more comfortable coding that up anyways. I don't do a ton of macro programming and had to refamiliarize myself of the pitfalls that comes with it when doing this. Thanks, Andrew
On Fri, Apr 07, 2023 at 12:34:53PM -0500, Andrew Halaney wrote: > On Sat, Apr 01, 2023 at 05:06:21PM +0200, Simon Horman wrote: > > On Fri, Mar 31, 2023 at 04:45:45PM -0500, Andrew Halaney wrote: > > > Passing stmmac_priv to some of the callbacks allows hwif implementations > > > to grab some data that platforms can customize. Adjust the callbacks > > > accordingly in preparation of such a platform customization. > > > > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > > > ... > > > > > #define stmmac_reset(__priv, __args...) \ > > > @@ -223,59 +240,59 @@ struct stmmac_dma_ops { > > > #define stmmac_dma_init(__priv, __args...) \ > > > stmmac_do_void_callback(__priv, dma, init, __args) > > > #define stmmac_init_chan(__priv, __args...) \ > > > - stmmac_do_void_callback(__priv, dma, init_chan, __args) > > > + stmmac_do_void_callback(__priv, dma, init_chan, __priv, __args) > > > > Hi Andrew, > > > > Rather than maintaining these macros can we just get rid of them? > > I'd be surprised if things aren't nicer with functions in their place [1]. > > > > f.e., we now have (__priv, ..., __priv, ...) due to a generalisation > > that seems to take a lot more than it gives. > > > > [1] https://lore.kernel.org/linux-arm-kernel/ZBst1SzcIS4j+t46@corigine.com/ > > > > Thanks for the pointer. I think that makes sense, I'll take that > approach for these functions (and maybe in a follow-up series I'll > tackle all of them just because the lack of consistency will eat me up). > I tried taking this approach for a spin, and I'm not so sure about it now! 1. Implementing the functions as static inline requires us to know about stmmac_priv, but that's getting into circular dependency land 2. You could define them in hwif.c, but then they're not inlined 3. There's still a good bit of boilerplate that's repeated all over with the approach. Ignoring 1 above, you get something like this: static inline int stmmac_init_chan(struct stmmac_priv *priv, void __iomem *ioaddr, struct stmmac_dma_cfg *dma_cfg, u32 chan) { if (priv->hw->dma && priv->hw->dma->init_chan) { priv->hw->dma->init_chan(priv, ioaddr, dma_cfg, chan); return 0; } return -EINVAL; } that is then repeated for every function... which is making me actually appreciate the macros some for reducing boilerplate. Am I suffering from a case of holiday brain, and 1-3 above are silly points with obvious answers, or do they make you reconsider continuing with the current approach in hwif.h? Thanks, Andrew
On Mon, Apr 10, 2023 at 04:24:22PM -0500, Andrew Halaney wrote: > On Fri, Apr 07, 2023 at 12:34:53PM -0500, Andrew Halaney wrote: > > On Sat, Apr 01, 2023 at 05:06:21PM +0200, Simon Horman wrote: > > > On Fri, Mar 31, 2023 at 04:45:45PM -0500, Andrew Halaney wrote: > > > > Passing stmmac_priv to some of the callbacks allows hwif implementations > > > > to grab some data that platforms can customize. Adjust the callbacks > > > > accordingly in preparation of such a platform customization. > > > > > > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > > > > > ... > > > > > > > #define stmmac_reset(__priv, __args...) \ > > > > @@ -223,59 +240,59 @@ struct stmmac_dma_ops { > > > > #define stmmac_dma_init(__priv, __args...) \ > > > > stmmac_do_void_callback(__priv, dma, init, __args) > > > > #define stmmac_init_chan(__priv, __args...) \ > > > > - stmmac_do_void_callback(__priv, dma, init_chan, __args) > > > > + stmmac_do_void_callback(__priv, dma, init_chan, __priv, __args) > > > > > > Hi Andrew, > > > > > > Rather than maintaining these macros can we just get rid of them? > > > I'd be surprised if things aren't nicer with functions in their place [1]. > > > > > > f.e., we now have (__priv, ..., __priv, ...) due to a generalisation > > > that seems to take a lot more than it gives. > > > > > > [1] https://lore.kernel.org/linux-arm-kernel/ZBst1SzcIS4j+t46@corigine.com/ > > > > > > > Thanks for the pointer. I think that makes sense, I'll take that > > approach for these functions (and maybe in a follow-up series I'll > > tackle all of them just because the lack of consistency will eat me up). > > > > I tried taking this approach for a spin, and I'm not so sure about it > now! > > 1. Implementing the functions as static inline requires us to know > about stmmac_priv, but that's getting into circular dependency land > 2. You could define them in hwif.c, but then they're not inlined > 3. There's still a good bit of boilerplate that's repeated all over > with the approach. Ignoring 1 above, you get something like this: > > static inline int stmmac_init_chan(struct stmmac_priv *priv, > void __iomem *ioaddr, > struct stmmac_dma_cfg *dma_cfg, u32 chan) > { > if (priv->hw->dma && priv->hw->dma->init_chan) { > priv->hw->dma->init_chan(priv, ioaddr, dma_cfg, chan); > return 0; > } > return -EINVAL; > } > > that is then repeated for every function... which is making me actually > appreciate the macros some for reducing boilerplate. > > Am I suffering from a case of holiday brain, and 1-3 above are silly > points with obvious answers, or do they make you reconsider continuing > with the current approach in hwif.h? I'm about to embark to the holiday brain zone. But before I do I wanted to acknowledge your concerns and that, yes, it may be easier said than done.