Re: [net-next v4 4/5] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski <lukma@denx.de>]

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

Hi Simon,

> > It is unclear why hwentry, which is a pointer, is being cast to an
> > integer and then back to a pointer. I see pointer arithmetic, but
> > that can operate on pointers just as well as integers, without
> > making assumptions about how wide pointers are with respect to
> > longs.
> > 
> > And in any case, can't the types be used to directly access the
> > offsets needed like this?
> > 
> > 	atable = fep->hwentry.mtip_table64b_entry;
> > 
> > 	*read_lo = readl(&atable[index].lo);
> > 	*read_hi = readl(&atable[index].hi);
> >   
> 
> The code as is seems to be OK.
> 
> The (atable) memory structure is as follows:
> 
> 1. You can store 2048 MAC addresses (2x32 bit each).
> 
> 2. Memory from point 1 is addressed as follows:
> 	2.1 -> from MAC address the CRC8 is calculated (0x00 - 0xFF).
> 	This is the 'index' in the original code.
> 	2.2 -> as it may happen that for two different MAC address the
> 	same CRC8 is calculated (i.e. 'index' is the same), each
> 	'index' can store 8 entries for MAC addresses (and it is
> 	searched in a linear way if needed).
> 
> IMHO, the index above shall be multiplied by 8.

I've double check it and it turned out that you were right :-)

The following code:

struct addr_table64b_entry *atable_base =
fep->hwentry->mtip_table64b_entry;

*read_lo = readl(&atable_base[index].lo);
*read_hi = readl(&atable_base[index].hi);

Is more readable than the current code.

The same would be used for atable_write()

I will change it for v5.

> 
> > Also, and perhaps more importantly, readl expects to be passed
> > a pointer to __iomem. But the appropriate annotations seem
> > to be missing (forcing them with a cast is not advisable here IMHO).
> >   
> 
> I think that the code below:
> unsigned long atable_base = (unsigned long)fep->hwentry;
> 
> could be replaced with
> void __iomem *atable_base = fep->hwentry;
> 
> and the (index << 3) with (index * ATABLE_ENTRY_PER_SLOT)




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]