Re: [PATCH 6/6] soc: fsl: qe: fix sparse warnings for ucc_slow.c

[Rasmus Villemoes]

On 12/03/2020 23.28, Li Yang wrote:
> Fixes the following sparse warnings:
> 
[snip]
> 
> Also removed the unneccessary clearing for kzalloc'ed structure.

Please don't mix that in the same patch, do it in a preparatory patch.
That makes reviewing much easier.

>  
>  	/* Get PRAM base */
>  	uccs->us_pram_offset =
> @@ -231,24 +224,24 @@ int ucc_slow_init(struct ucc_slow_info * us_info, struct ucc_slow_private ** ucc
>  		/* clear bd buffer */
>  		qe_iowrite32be(0, &bd->buf);
>  		/* set bd status and length */
> -		qe_iowrite32be(0, (u32 *)bd);
> +		qe_iowrite32be(0, (u32 __iomem *)bd);

It's cleaner to do two qe_iowrite16be to &bd->status and &bd->length,
that avoids the casting altogether.

>  		bd++;
>  	}
>  	/* for last BD set Wrap bit */
>  	qe_iowrite32be(0, &bd->buf);
> -	qe_iowrite32be(cpu_to_be32(T_W), (u32 *)bd);
> +	qe_iowrite32be(T_W, (u32 __iomem *)bd);

Yeah, and this is why. Who can actually keep track of where that bit
ends up being set with that casting going on. Please use
qe_iowrite16be() with an appropriately modified constant to the
appropriate field instead of these games.

And if the hardware doesn't support 16 bit writes, the definition of
struct qe_bd is wrong and should have a single __be32 status_length
field, with appropriate accessors defined.

>  	/* Init Rx bds */
>  	bd = uccs->rx_bd = qe_muram_addr(uccs->rx_base_offset);
>  	for (i = 0; i < us_info->rx_bd_ring_len - 1; i++) {
>  		/* set bd status and length */
> -		qe_iowrite32be(0, (u32 *)bd);
> +		qe_iowrite32be(0, (u32 __iomem *)bd);

Same.

>  		/* clear bd buffer */
>  		qe_iowrite32be(0, &bd->buf);
>  		bd++;
>  	}
>  	/* for last BD set Wrap bit */
> -	qe_iowrite32be(cpu_to_be32(R_W), (u32 *)bd);
> +	qe_iowrite32be(R_W, (u32 __iomem *)bd);

Same.

>  	qe_iowrite32be(0, &bd->buf);
>  
>  	/* Set GUMR (For more details see the hardware spec.). */
> @@ -273,8 +266,8 @@ int ucc_slow_init(struct ucc_slow_info * us_info, struct ucc_slow_private ** ucc
>  	qe_iowrite32be(gumr, &us_regs->gumr_h);
>  
>  	/* gumr_l */
> -	gumr = us_info->tdcr | us_info->rdcr | us_info->tenc | us_info->renc |
> -		us_info->diag | us_info->mode;
> +	gumr = (u32)us_info->tdcr | (u32)us_info->rdcr | (u32)us_info->tenc |
> +	       (u32)us_info->renc | (u32)us_info->diag | (u32)us_info->mode;

Are the tdcr, rdcr, tenc, renc fields actually set anywhere (the same
for the diag and mode, but word-grepping for those give way too many
false positives)? They seem to be a somewhat pointless split out of the
bitfields of gumr_l, and not populated anywhere?. That's not directly
related to this patch, of course, but getting rid of them first (if they
are indeed completely unused) might make the sparse cleanup a little
simpler.

Rasmus