lkml.org 
[lkml]   [2020]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 6/6] soc: fsl: qe: fix sparse warnings for ucc_slow.c
From
Date
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

\
 
 \ /
  Last update: 2020-03-16 22:09    [W:0.079 / U:12.840 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site