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

[Rangoju, Raju]

On 5/1/2025 4:08 AM, Vadim Fedorenko wrote:
> On 28/04/2025 16:02, 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.
> >
> > Signed-off-by: Raju Rangoju <Raju.Rangoju@xxxxxxx>
> > ---
> > Changes since v1:
> > - add the xgbe_ prefix to new functions
> >
> >   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..765f20b24722 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 xgbe_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 xgbe_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;
>
> Please, follow reverse Xmass tree ordering

Sure, will address it in v3. Thanks.

> > +
> > +    mmd_address = xgbe_get_mmd_address(pdata, mmd_reg);
> > +
> > +    xgbe_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 = xgbe_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);
> > +    xgbe_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 = xgbe_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 = xgbe_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