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

["Rangoju, Raju" <raju.rangoju@amd.com>]

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@amd.com>
>> ---
>> 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
>