Re: [PATCH v3] memory: renesas-rpc-if: Fix PHYCNT.STRTIM setting

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Jan 26, 2023 at 6:41 PM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
> According to the datasheets, the Strobe Timing Adjustment bit (STRTIM)
> setting is different on R-Car SoCs, i.e.
>
> R-Car M3 ES1.*  : STRTIM[2:0] is set to 0x6
> other R-Car Gen3: STRTIM[2:0] is set to 0x7
> other R-Car Gen4: STRTIM[3:0] is set to 0xf
>
> To fix this issue, a DT match data was added to specify the setting
> for special use cases.
>
> Signed-off-by: Cong Dang <cong.dang.xn@xxxxxxxxxxx>
> Signed-off-by: Hai Pham  <hai.pham.ud@xxxxxxxxxxx>
> [wsa: rebased, restructured, added Gen4 support]
> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> Changes since v2:
> * dropped support for H3 ES1 (we don't support it upstream anymore)
> * M3-W ES1 now handled via 'compatible' (Thanks, Geert!)
> * -> dropped soc_device_match()
> * removed old rpcif_type entry in struct rpcif (Thanks, Prabhakar)

Thanks for the update!

> --- a/drivers/memory/renesas-rpc-if.c
> +++ b/drivers/memory/renesas-rpc-if.c

> @@ -276,9 +296,9 @@ int rpcif_sw_init(struct rpcif *rpc, struct device *dev)
>         rpc->dirmap = devm_ioremap_resource(&pdev->dev, res);
>         if (IS_ERR(rpc->dirmap))
>                 return PTR_ERR(rpc->dirmap);
> -       rpc->size = resource_size(res);
>
> -       rpc->type = (uintptr_t)of_device_get_match_data(dev);
> +       rpc->size = resource_size(res);

Nit: No need to move the initialization of rpc->size.

> +       rpc->info = of_device_get_match_data(dev);
>         rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>
>         return PTR_ERR_OR_ZERO(rpc->rstc);

> @@ -321,12 +341,10 @@ int rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
>         /* DMA Transfer is not supported */
>         regmap_update_bits(rpc->regmap, RPCIF_PHYCNT, RPCIF_PHYCNT_HS, 0);
>
> -       if (rpc->type == RPCIF_RCAR_GEN3)
> -               regmap_update_bits(rpc->regmap, RPCIF_PHYCNT,
> -                                  RPCIF_PHYCNT_STRTIM(7), RPCIF_PHYCNT_STRTIM(7));
> -       else if (rpc->type == RPCIF_RCAR_GEN4)
> -               regmap_update_bits(rpc->regmap, RPCIF_PHYCNT,
> -                                  RPCIF_PHYCNT_STRTIM(15), RPCIF_PHYCNT_STRTIM(15));
> +       regmap_update_bits(rpc->regmap, RPCIF_PHYCNT,
> +                          /* create mask with all affected bits set */
> +                          RPCIF_PHYCNT_STRTIM(BIT(fls(rpc->info->strtim)) - 1),

Note that this relies on the strtim value being 4 or higher.
As this value is not user-configurable, but fixed in the driver,
it's probably OK.

> +                          RPCIF_PHYCNT_STRTIM(rpc->info->strtim));
>
>         regmap_update_bits(rpc->regmap, RPCIF_PHYOFFSET1, RPCIF_PHYOFFSET1_DDRTMG(3),
>                            RPCIF_PHYOFFSET1_DDRTMG(3));

Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds