Re: [PATCH v5 4/6] net: dsa: microchip: ksz8795: add support for ksz88xx chips

[Michael Grzeschik <mgr@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3109 bytes --]

Hi Tristam!

On Wed, Dec 16, 2020 at 06:33:06AM +0000, Tristram.Ha@microchip.com wrote:
>>  static void ksz8_r_vlan_table(struct ksz_device *dev, u16 vid, u32 *vlan)
>>  {
>> -       int index;
>> -       u16 *data;
>> -       u16 addr;
>> +       u16 addr = vid / dev->phy_port_cnt;
>>         u64 buf;
>>
>> -       data = (u16 *)&buf;
>> -       addr = vid / dev->phy_port_cnt;
>> -       index = vid & 3;
>>         ksz8_r_table(dev, TABLE_VLAN, addr, &buf);
>> -       *vlan = data[index];
>> +       if (dev->features & IS_88X3) {
>> +               *vlan = (u32)buf;
>> +       } else {
>> +               u16 *data = (u16 *)&buf;
>> +
>> +               *vlan = data[vid & 3];
>> +       }
>>  }
>>
>>  static void ksz8_w_vlan_table(struct ksz_device *dev, u16 vid, u32 vlan)
>>  {
>> -       int index;
>> -       u16 *data;
>> -       u16 addr;
>> +       u16 addr = vid / dev->phy_port_cnt;
>>         u64 buf;
>>
>> -       data = (u16 *)&buf;
>> -       addr = vid / dev->phy_port_cnt;
>> -       index = vid & 3;
>>         ksz8_r_table(dev, TABLE_VLAN, addr, &buf);
>> -       data[index] = vlan;
>> +
>> +       if (dev->features & IS_88X3) {
>> +               buf = vlan;
>> +       } else {
>> +               u16 *data = (u16 *)&buf;
>> +
>> +               data[vid & 3] = vlan;
>> +       }
>> +
>>         dev->vlan_cache[vid].table[0] = vlan;
>>         ksz8_w_table(dev, TABLE_VLAN, addr, buf);
>>  }
>
>I am confused about how the addr is derived.
>
>In KSZ8795 vid is in range of 0-4095.  The addr is just (vid / 4) as there
>are 4 entries in one access.  The data are lined up in 16-bit boundary
>so that the VLAN information can be accessed using the array.
>
>For KSZ8895 the VLAN data are not lined up so the 64-bit variable
>needs to be shifted accordingly and masked.
>
>For KSZ8863 the addr is a hard value from 0 to 15.  The data buffer is just
>32-bit.  The vid value is contained in the entry.  You need to match that vid
>with the input vid to return the right information.
>
>You need a different VLAN read function to check if the VLAN is already
>programmed in the VLAN table by searching all 16 entries.
>
>For the VLAN write function you need to check if there is available space
>to add a new entry.  The VID range is still 0-4095, but the FID range is 0-15.

Thanks for the clarification. Its possible that I missed that when
porting this from your first RFC.

Regarding the drivers for ksz88{6,7}3, did you also plan to mainline
some work? Do you have some code that could be shared? If so I would
not need to implement everything again. In case you have some work
pending, please let me know. Than we could colaborate to get this
chips finaly mainline.

Thanks,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]