Re: [PATCH v5 3/6] net: dsa: microchip: ksz8795: move register offsets and shifts to separate struct

[Michael Grzeschik <mgr@pengutronix.de>]

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

Gentle Ping. Did you find time to look into my other patches of the
series. I really would like to send the next version.

Thanks!

On Mon, Dec 07, 2020 at 10:44:15PM +0100, Michael Grzeschik wrote:
>On Mon, Dec 07, 2020 at 08:02:57PM +0000, Tristram.Ha@microchip.com wrote:
>>>In order to get this driver used with other switches the functions need
>>>to use different offsets and register shifts. This patch changes the
>>>direct use of the register defines to register description structures,
>>>which can be set depending on the chips register layout.
>>>
>>>Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>>
>>>---
>>>v1 -> v4: - extracted this change from bigger previous patch
>>>v4 -> v5: - added missing variables in ksz8_r_vlan_entries
>>>          - moved shifts, masks and registers to arrays indexed by enums
>>>          - using unsigned types where possible
>>>---
>>> drivers/net/dsa/microchip/ksz8.h        |  69 +++++++
>>> drivers/net/dsa/microchip/ksz8795.c     | 261 +++++++++++++++++-------
>>> drivers/net/dsa/microchip/ksz8795_reg.h |  85 --------
>>> 3 files changed, 253 insertions(+), 162 deletions(-)
>>> create mode 100644 drivers/net/dsa/microchip/ksz8.h
>>
>>Sorry for not respond to these patches sooner.
>>
>>There are 3 older KSZ switch families: KSZ8863/73, KSZ8895/64, and KSZ8795/94.
>>The newer KSZ8795 is not considered an upgrade for KSZ8895, so some of
>>these switch registers are moved around and some features are dropped.
>>
>>It is best to have one driver to support all 3 switches, but some operations are
>>Incompatible so it may be better to keep the drivers separate for now.
>>
>>For basic operations those issues may not occur so it seems simple to have
>>one driver handling all 3 switches.  I will come up with a list of those
>>incompatibilities.
>
>Look into the next patch. I handled many special cases for the ksz8863
>in the "net: dsa: microchip: ksz8795: add support for ksz88xx chips".
>These cases, including the VLAN, Tagging ... are handled by checking if
>the feautre IS_88X3 is set. This can be extended to other types as well.
>
>My first version of the patches was an RFC series that was mentioning
>that it is based on your RFC series for the ksz8895.
>
>8863 RFC: https://patchwork.ozlabs.org/project/netdev/cover/20190508211330.19328-1-m.grzeschik@pengutronix.de/
>
>8895 RFC: https://patchwork.ozlabs.org/patch/822712/
>
>I remember, that I was reading the datasheets of all three chips,
>8895, 8863 and 8795. After the 8795 series was mainline, the
>obvious next step was to get the 8863 into the 8795 code. The result
>is this series.
>
>So the obvious question is, how far does your 8895 series differ
>from the 8863 switches?
>
>>The tail tag format of KSZ8863 is different from KSZ8895 and KSZ8795, but
>>because of the DSA driver implementation that issue never comes up.
>
>Right. In the first four series I kept an extra tail tag patch. But
>after cleaning up I figured that the Implementation matched the
>one for the KSZ9893. Therefor I reused the tag code.
>
>>>-static void ksz8_from_vlan(u16 vlan, u8 *fid, u8 *member, u8 *valid)
>>>+static void ksz8_from_vlan(struct ksz_device *dev, u32 vlan, u8 *fid,
>>>+                          u8 *member, u8 *valid)
>>> {
>>>-       *fid = vlan & VLAN_TABLE_FID;
>>>-       *member = (vlan & VLAN_TABLE_MEMBERSHIP) >>
>>>VLAN_TABLE_MEMBERSHIP_S;
>>>-       *valid = !!(vlan & VLAN_TABLE_VALID);
>>>+       struct ksz8 *ksz8 = dev->priv;
>>>+       const u32 *masks = ksz8->masks;
>>>+       const u8 *shifts = ksz8->shifts;
>>>+
>>>+       *fid = vlan & masks[VLAN_TABLE_FID];
>>>+       *member = (vlan & masks[VLAN_TABLE_MEMBERSHIP]) >>
>>>+                       shifts[VLAN_TABLE_MEMBERSHIP_S];
>>>+       *valid = !!(vlan & masks[VLAN_TABLE_VALID]);
>>> }
>>>
>>>-static void ksz8_to_vlan(u8 fid, u8 member, u8 valid, u16 *vlan)
>>>+static void ksz8_to_vlan(struct ksz_device *dev, u8 fid, u8 member, u8
>>>valid,
>>>+                        u32 *vlan)
>>> {
>>>+       struct ksz8 *ksz8 = dev->priv;
>>>+       const u32 *masks = ksz8->masks;
>>>+       const u8 *shifts = ksz8->shifts;
>>>+
>>>        *vlan = fid;
>>>-       *vlan |= (u16)member << VLAN_TABLE_MEMBERSHIP_S;
>>>+       *vlan |= (u16)member << shifts[VLAN_TABLE_MEMBERSHIP_S];
>>>        if (valid)
>>>-               *vlan |= VLAN_TABLE_VALID;
>>>+               *vlan |= masks[VLAN_TABLE_VALID];
>>> }
>>>
>>> static void ksz8_r_vlan_entries(struct ksz_device *dev, u16 addr)
>>> {
>>>+       struct ksz8 *ksz8 = dev->priv;
>>>+       const u8 *shifts = ksz8->shifts;
>>>        u64 data;
>>>        int i;
>>>
>>>@@ -418,7 +509,7 @@ static void ksz8_r_vlan_entries(struct ksz_device
>>>*dev, u16 addr)
>>>        addr *= dev->phy_port_cnt;
>>>        for (i = 0; i < dev->phy_port_cnt; i++) {
>>>                dev->vlan_cache[addr + i].table[0] = (u16)data;
>>>-               data >>= VLAN_TABLE_S;
>>>+               data >>= shifts[VLAN_TABLE];
>>>        }
>>> }
>>>
>>>@@ -454,6 +545,8 @@ static void ksz8_w_vlan_table(struct ksz_device *dev,
>>>u16 vid, u32 vlan)
>>>
>>
>>The VLAN table operation in KSZ8863 is completely different from KSZ8795.
>>
>>>-/**
>>>- * VLAN_TABLE_FID                      00-007F007F-007F007F
>>>- * VLAN_TABLE_MEMBERSHIP               00-0F800F80-0F800F80
>>>- * VLAN_TABLE_VALID                    00-10001000-10001000
>>>- */
>>>-
>>>-#define VLAN_TABLE_FID                 0x007F
>>>-#define VLAN_TABLE_MEMBERSHIP          0x0F80
>>>-#define VLAN_TABLE_VALID               0x1000
>>>-
>>>-#define VLAN_TABLE_MEMBERSHIP_S                7
>>>-#define VLAN_TABLE_S                   16
>>>-
>>
>>In KSZ8795 you can use all 4096 VLAN id.  Each entry in the table contains
>>4 VID.  The FID is actually used for lookup and there is a limit, so you need
>>to convert VID to FID when programming the VLAN table.
>>
>>The only effect of using FID is the same MAC address can be used in
>>different VLANs.
>>
>>In KSZ8863 there are only 16 entries in the VLAN table.  Just like static
>>MAC table each entry is programmed using an index.  The entry contains
>>the VID, which does not have any relationship with the index unlike in
>>KSZ8795.
>>
>>The number of FID is also limited to 16.  So the maximum VLAN used is 16.
>>
>>> /**
>>>  * MIB_COUNTER_VALUE                   00-00000000-3FFFFFFF
>>>  * MIB_TOTAL_BYTES                     00-0000000F-FFFFFFFF
>>>@@ -956,9 +874,6 @@
>>>  * MIB_COUNTER_OVERFLOW                        00-00000040-00000000
>>>  */
>>>
>>>-#define MIB_COUNTER_OVERFLOW           BIT(6)
>>>-#define MIB_COUNTER_VALID              BIT(5)
>>>-
>>> #define MIB_COUNTER_VALUE              0x3FFFFFFF
>>
>>The MIB counters are also a little different in KSZ8863 and KSZ8795.
>>KSZ8863 may have smaller total bytes.  In normal operation this issue may
>>not come up.
>
>As mentioned before, these cases are handled differently for both
>types of drivers.
>
>-- 
>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 |



-- 
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 --]