lkml.org 
[lkml]   [2021]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH net-next 5/6] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC
    On Wed, Oct 13, 2021 at 10:05:21AM +0000, Alvin Šipraga wrote:
    > On 10/13/21 11:55 AM, DENG Qingfang wrote:
    > > On Tue, Oct 12, 2021 at 02:35:54PM +0200, Alvin Šipraga wrote:
    > >> +/* Port mapping macros
    > >> + *
    > >> + * PORT_NUM_x2y: map a port number from domain x to domain y
    > >> + * PORT_MASK_x2y: map a port mask from domain x to domain y
    > >> + *
    > >> + * L = logical port domain, i.e. dsa_port.index
    > >> + * P = physical port domain, used by the Realtek ASIC for port indexing;
    > >> + * for ports with internal PHYs, this is also the PHY index
    > >> + * E = extension port domain, used by the Realtek ASIC for managing EXT ports
    > >> + *
    > >> + * The terminology is borrowed from the vendor driver. The extension port domain
    > >> + * is mostly used to navigate the labyrinthine layout of EXT port configuration
    > >> + * registers and is not considered intuitive by the author.
    > >> + *
    > >> + * Unless a function is accessing chip registers, it should be using the logical
    > >> + * port domain. Moreover, function arguments for port numbers and port masks
    > >> + * must always be in the logical domain. The conversion must be done as close as
    > >> + * possible to the register access to avoid chaos.
    > >> + *
    > >> + * The mappings vary between chips in the family supported by this driver. Here
    > >> + * is an example of the mapping for the RTL8365MB-VC:
    > >> + *
    > >> + * L | P | E | remark
    > >> + * ---+---+---+--------
    > >> + * 0 | 0 | | user port
    > >> + * 1 | 1 | | user port
    > >> + * 2 | 2 | | user port
    > >> + * 3 | 3 | | user port
    > >> + * 4 | 6 | 1 | extension (CPU) port
    > >> + *
    > >> + * NOTE: Currently this is hardcoded for the RTL8365MB-VC. This will probably
    > >> + * require a rework when adding support for other chips.
    > >> + */
    > >> +#define CPU_PORT_LOGICAL_NUM 4
    > >> +#define CPU_PORT_LOGICAL_MASK BIT(CPU_PORT_LOGICAL_NUM)
    > >> +#define CPU_PORT_PHYSICAL_NUM 6
    > >> +#define CPU_PORT_PHYSICAL_MASK BIT(CPU_PORT_PHYSICAL_NUM)
    > >> +#define CPU_PORT_EXTENSION_NUM 1
    > >> +
    > >> +static u32 rtl8365mb_port_num_l2p(u32 port)
    > >> +{
    > >> + return port == CPU_PORT_LOGICAL_NUM ? CPU_PORT_PHYSICAL_NUM : port;
    > >> +}
    > >> +
    > >> +static u32 rtl8365mb_port_mask_l2p(u32 mask)
    > >> +{
    > >> + u32 phys_mask = mask & ~CPU_PORT_LOGICAL_MASK;
    > >> +
    > >> + if (mask & CPU_PORT_LOGICAL_MASK)
    > >> + phys_mask |= CPU_PORT_PHYSICAL_MASK;
    > >> +
    > >> + return phys_mask;
    > >> +}
    > >> +
    > >> +static u32 rtl8365mb_port_mask_p2l(u32 phys_mask)
    > >> +{
    > >> + u32 mask = phys_mask & ~CPU_PORT_PHYSICAL_MASK;
    > >> +
    > >> + if (phys_mask & CPU_PORT_PHYSICAL_MASK)
    > >> + mask |= CPU_PORT_LOGICAL_MASK;
    > >> +
    > >> + return mask;
    > >> +}
    > >> +
    > >> +#define PORT_NUM_L2P(_p) (rtl8365mb_port_num_l2p(_p))
    > >> +#define PORT_NUM_L2E(_p) (CPU_PORT_EXTENSION_NUM)
    > >> +#define PORT_MASK_L2P(_m) (rtl8365mb_port_mask_l2p(_m))
    > >> +#define PORT_MASK_P2L(_m) (rtl8365mb_port_mask_p2l(_m))
    > >
    > > The whole port mapping thing can be avoided if you just use port 6 as the CPU
    > > port.
    >
    > Andrew also suggested this, but the discontinuity in port IDs seems to
    > be an invitation for trouble. Here is an example of a series of
    > functions from dsa.h:
    >
    > static inline struct dsa_port *dsa_to_port(struct dsa_switch *ds, int p)
    > {
    > struct dsa_switch_tree *dst = ds->dst;
    > struct dsa_port *dp;
    >
    > list_for_each_entry(dp, &dst->ports, list)
    > if (dp->ds == ds && dp->index == p)
    > return dp;
    >
    > return NULL;
    > }
    >
    > static inline bool dsa_is_user_port(struct dsa_switch *ds, int p)
    > {
    > return dsa_to_port(ds, p)->type == DSA_PORT_TYPE_USER;
    > }
    >
    > static inline u32 dsa_user_ports(struct dsa_switch *ds)
    > {
    > u32 mask = 0;
    > int p;
    >
    > for (p = 0; p < ds->num_ports; p++)
    > if (dsa_is_user_port(ds, p))
    > mask |= BIT(p);
    >
    > return mask;
    > }
    >
    > My reading of dsa_user_ports() is that the port IDs run from 0 to
    > (ds->num_ports - 1). If num_ports is 5 (4 user ports and 1 CPU port, as
    > in my case), but the CPU is port 6, will we not dereference NULL when
    > calling dsa_is_user_port(ds, 4)?
    >
    > >
    > >> +
    > >> +/* Chip-specific data and limits */
    >

    No, have you actually tried it? Discontinuities should be absolutely
    fine, see dsa_switch_touch_ports(), a struct dsa_port is created for
    every port number up to ds->num_ports, the ones absent from DT will
    simply remain as DSA_PORT_TYPE_UNUSED.

    \
     
     \ /
      Last update: 2021-10-13 12:11    [W:6.292 / U:0.632 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site