Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 7 Apr 2023 12:36:03 -0500
From:   Andrew Halaney <ahalaney@redhat.com>
To:     Simon Horman <simon.horman@corigine.com>
Cc:     linux-kernel@vger.kernel.org, agross@kernel.org,
        andersson@kernel.org, konrad.dybcio@linaro.org,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, vkoul@kernel.org,
        bhupesh.sharma@linaro.org, wens@csie.org, jernej.skrabec@gmail.com,
        samuel@sholland.org, mturquette@baylibre.com,
        peppe.cavallaro@st.com, alexandre.torgue@foss.st.com,
        joabreu@synopsys.com, mcoquelin.stm32@gmail.com,
        richardcochran@gmail.com, linux@armlinux.org.uk, veekhee@apple.com,
        tee.min.tan@linux.intel.com, mohammad.athari.ismail@intel.com,
        jonathanh@nvidia.com, ruppala@nvidia.com, bmasney@redhat.com,
        andrey.konovalov@linaro.org, linux-arm-msm@vger.kernel.org,
        netdev@vger.kernel.org, devicetree@vger.kernel.org,
        linux-stm32@st-md-mailman.stormreply.com,
        linux-arm-kernel@lists.infradead.org, ncai@quicinc.com,
        jsuraj@qti.qualcomm.com, hisunil@quicinc.com, echanude@redhat.com
Subject: Re: [PATCH net-next v3 09/12] net: stmmac: dwmac4: Allow platforms
 to specify some DMA/MTL offsets
Message-ID: <20230407173603.lyj5fjox23uhn2gb@halaney-x13s>
References: <20230331214549.756660-1-ahalaney@redhat.com>
 <20230331214549.756660-10-ahalaney@redhat.com>
 <ZChGswjgAOkT0jvY@corigine.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZChGswjgAOkT0jvY@corigine.com>
Precedence: bulk

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