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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 25/04/2025 09:49, Lukasz Majewski wrote:
> Hi Krzysztof, Jakub
> 
>> On 25/04/2025 08:05, Lukasz Majewski wrote:
>>> Hi Krzysztof, Jakub,
>>>   
>>>> On 25/04/2025 03:11, Jakub Kicinski wrote:  
>>>>> On Wed, 23 Apr 2025 09:29:08 +0200 Lukasz Majewski wrote:    
>>>>>> This patch series provides support for More Than IP L2 switch
>>>>>> embedded in the imx287 SoC.
>>>>>>
>>>>>> This is a two port switch (placed between uDMA[01] and
>>>>>> MAC-NET[01]), which can be used for offloading the network
>>>>>> traffic.
>>>>>>
>>>>>> It can be used interchangeably with current FEC driver - to be
>>>>>> more specific: one can use either of it, depending on the
>>>>>> requirements.
>>>>>>
>>>>>> The biggest difference is the usage of DMA - when FEC is used,
>>>>>> separate DMAs are available for each ENET-MAC block.
>>>>>> However, with switch enabled - only the DMA0 is used to
>>>>>> send/receive data to/form switch (and then switch sends them to
>>>>>> respecitive ports).    
>>>>>
>>>>> Lots of sparse warnings and build issues here, at least on x86.
>>>>>
>>>>> Could you make sure it's clean with an allmodconfig config, 
>>>>> something like:
>>>>>
>>>>> make C=1 W=1 drivers/net/ethernet/freescale/mtipsw/     
>>>>
>>>> ... and W=1 with clang as well.
>>>>  
>>>
>>> The sparse warnings are because of struct switch_t casting and
>>> register  
>>
>> clang W=1 fails on errors, so it is not only sparse:
>>
>> error: cast to smaller integer type 'uint' (aka 'unsigned int') from
>> 'struct cbd_t *' [-Werror,-Wpointer-to-int-cast]
>>
>> You probably wanted there kenel_ulong_t.
> 
> This I did not catch earlier (probably because of my testing on
> imx287). Thanks for spotting it.
> 
>>
>>> access with this paradigm (as it is done with other drivers).  
>>
>> I don't understand. I see code like:
>>
>> 	struct switch_t *fecp = fep->hwp;
>>
>> But this is not a cast - the same types.
> 
> For example:
> 
> The warning:
> 
> mtipl2sw.c:208:30: warning: incorrect type in argument 1 (different
> address spaces) mtipl2sw.c:208:30:    expected void const volatile
> [noderef] __iomem *addr mtipl2sw.c:208:30:    got unsigned int *
> 
> corresponds to:
>  info->maclo = readl(&fecp->ESW_LREC0);   [*]
> 
> where:
> 
> struct switch_t {
>         u32 ESW_REVISION;
>         u32 ESW_SCRATCH;
> 	...
>         /*from 0x420-0x4FC*/
>         u32 esw_reserved9[57];
>         /*0xFC0DC500---0xFC0DC508*/
>         u32 ESW_LREC0;
>         u32 ESW_LREC1;
>         u32 ESW_LSR;
> };
> 
> 
> The 'u32' type seems to be valid here as this register is 32 bit wide.

It is not about size, but IOMEM annotation and pointer/non-pointer.


> 
> To fix the sparse warnings - I think that I will replace [*] with:
> 
> info->maclo = readl((u32 __iomem *)&fecp->ESW_LREC0);

I don't understand why are you reading address of ESW_LREC0. This is
MMIO, right? So you are supposes to read base + offset (where base is a
proper iomem pointer).


Best regards,
Krzysztof