Re: [PATCH net-next 1/5] amd-xgbe: reorganize the code of XPCS access

[Rangoju, Raju]

On 4/11/2025 2:03 PM, Larysa Zaremba wrote:
> On Tue, Apr 08, 2025 at 11:49:57PM +0530, Raju Rangoju wrote:
> > The xgbe_{read/write}_mmd_regs_v* functions have common code which can
> > be moved to helper functions. Add new helper functions to calculate the
> > mmd_address for v1/v2 of xpcs access.
>
> Overall seems reasonable, but the new functions are missing the xgbe_ prefix,
> contrary to other in this file.

Thank you for your observation. We have additional patches in 
development that follow this path, and I'll take care of this in the 
future patches that follow.

> > Signed-off-by: Raju Rangoju <Raju.Rangoju@xxxxxxx>
> > ---
> >   drivers/net/ethernet/amd/xgbe/xgbe-dev.c | 63 ++++++++++--------------
> >   1 file changed, 27 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> > index b51a3666dddb..ae82dc3ac460 100644
> > --- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> > +++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> > @@ -1041,18 +1041,17 @@ static int xgbe_set_gpio(struct xgbe_prv_data *pdata, unsigned int gpio)
> >   	return 0;
> >   }
> >   
> > -static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
> > -				 int mmd_reg)
> > +static unsigned int get_mmd_address(struct xgbe_prv_data *pdata, int mmd_reg)
> >   {
> > -	unsigned long flags;
> > -	unsigned int mmd_address, index, offset;
> > -	int mmd_data;
> > -
> > -	if (mmd_reg & XGBE_ADDR_C45)
> > -		mmd_address = mmd_reg & ~XGBE_ADDR_C45;
> > -	else
> > -		mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff);
> > +	return (mmd_reg & XGBE_ADDR_C45) ?
> > +		mmd_reg & ~XGBE_ADDR_C45 :
> > +		(pdata->mdio_mmd << 16) | (mmd_reg & 0xffff);
> > +}
> >   
> > +static void get_pcs_index_and_offset(struct xgbe_prv_data *pdata,
> > +				     unsigned int mmd_address,
> > +				     unsigned int *index, unsigned int *offset)
> > +{
> >   	/* The PCS registers are accessed using mmio. The underlying
> >   	 * management interface uses indirect addressing to access the MMD
> >   	 * register sets. This requires accessing of the PCS register in two
> > @@ -1063,8 +1062,20 @@ static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
> >   	 * offset 1 bit and reading 16 bits of data.
> >   	 */
> >   	mmd_address <<= 1;
> > -	index = mmd_address & ~pdata->xpcs_window_mask;
> > -	offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask);
> > +	*index = mmd_address & ~pdata->xpcs_window_mask;
> > +	*offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask);
> > +}
> > +
> > +static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
> > +				 int mmd_reg)
> > +{
> > +	unsigned long flags;
> > +	unsigned int mmd_address, index, offset;
> > +	int mmd_data;
> > +
> > +	mmd_address = get_mmd_address(pdata, mmd_reg);
> > +
> > +	get_pcs_index_and_offset(pdata, mmd_address, &index, &offset);
> >   
> >   	spin_lock_irqsave(&pdata->xpcs_lock, flags);
> >   	XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index);
> > @@ -1080,23 +1091,9 @@ static void xgbe_write_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
> >   	unsigned long flags;
> >   	unsigned int mmd_address, index, offset;
> >   
> > -	if (mmd_reg & XGBE_ADDR_C45)
> > -		mmd_address = mmd_reg & ~XGBE_ADDR_C45;
> > -	else
> > -		mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff);
> > +	mmd_address = get_mmd_address(pdata, mmd_reg);
> >   
> > -	/* The PCS registers are accessed using mmio. The underlying
> > -	 * management interface uses indirect addressing to access the MMD
> > -	 * register sets. This requires accessing of the PCS register in two
> > -	 * phases, an address phase and a data phase.
> > -	 *
> > -	 * The mmio interface is based on 16-bit offsets and values. All
> > -	 * register offsets must therefore be adjusted by left shifting the
> > -	 * offset 1 bit and writing 16 bits of data.
> > -	 */
> > -	mmd_address <<= 1;
> > -	index = mmd_address & ~pdata->xpcs_window_mask;
> > -	offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask);
> > +	get_pcs_index_and_offset(pdata, mmd_address, &index, &offset);
> >   
> >   	spin_lock_irqsave(&pdata->xpcs_lock, flags);
> >   	XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index);
> > @@ -1111,10 +1108,7 @@ static int xgbe_read_mmd_regs_v1(struct xgbe_prv_data *pdata, int prtad,
> >   	unsigned int mmd_address;
> >   	int mmd_data;
> >   
> > -	if (mmd_reg & XGBE_ADDR_C45)
> > -		mmd_address = mmd_reg & ~XGBE_ADDR_C45;
> > -	else
> > -		mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff);
> > +	mmd_address = get_mmd_address(pdata, mmd_reg);
> >   
> >   	/* The PCS registers are accessed using mmio. The underlying APB3
> >   	 * management interface uses indirect addressing to access the MMD
> > @@ -1139,10 +1133,7 @@ static void xgbe_write_mmd_regs_v1(struct xgbe_prv_data *pdata, int prtad,
> >   	unsigned int mmd_address;
> >   	unsigned long flags;
> >   
> > -	if (mmd_reg & XGBE_ADDR_C45)
> > -		mmd_address = mmd_reg & ~XGBE_ADDR_C45;
> > -	else
> > -		mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff);
> > +	mmd_address = get_mmd_address(pdata, mmd_reg);
> >   
> >   	/* The PCS registers are accessed using mmio. The underlying APB3
> >   	 * management interface uses indirect addressing to access the MMD
> > --
> > 2.34.1