Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20210922091007.5516-1-wsa+renesas@sang-engineering.com>
In-Reply-To: <20210922091007.5516-1-wsa+renesas@sang-engineering.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Wed, 27 Apr 2022 12:22:43 +0200
Message-ID: <CAMuHMdWJZM=-Py=W+qKUfCQU=Y5=G6qdp9uubNv56o+5JztaGA@mail.gmail.com>
Subject: Re: [RFC PATCH] memory: renesas-rpc-if: Correct QSPI data transfer in
 Manual mode
To:     Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Wolfram,

On Wed, Sep 22, 2021 at 11:10 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> This patch fixes 2 problems:
> [1] The output warning logs and data loss when performing
> mount/umount then remount the device with jffs2 format.
> [2] The access width of SMWDR[0:1]/SMRDR[0:1] register is wrong.

Revisiting commit fff53a551db50f5e ("memory: renesas-rpc-if: Correct
QSPI data transfer in Manual mode") in  v5.16-rc1...

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

>  int rpcif_manual_xfer(struct rpcif *rpc)
>  {
> -       u32 smenr, smcr, pos = 0, max = 4;
> +       u32 smenr, smcr, pos = 0, max = rpc->bus_size == 2 ? 8 : 4;
>         int ret = 0;
>
> -       if (rpc->bus_size == 2)
> -               max = 8;
> -
>         pm_runtime_get_sync(rpc->dev);
>
>         regmap_update_bits(rpc->regmap, RPCIF_PHYCNT,
> @@ -378,37 +424,36 @@ int rpcif_manual_xfer(struct rpcif *rpc)
>         regmap_write(rpc->regmap, RPCIF_SMOPR, rpc->option);
>         regmap_write(rpc->regmap, RPCIF_SMDMCR, rpc->dummy);
>         regmap_write(rpc->regmap, RPCIF_SMDRENR, rpc->ddr);
> +       regmap_write(rpc->regmap, RPCIF_SMADR, rpc->smadr);
>         smenr = rpc->enable;
>
>         switch (rpc->dir) {
>         case RPCIF_DATA_OUT:
>                 while (pos < rpc->xferlen) {
> -                       u32 nbytes = rpc->xferlen - pos;
> -                       u32 data[2];
> +                       u32 bytes_left = rpc->xferlen - pos;
> +                       u32 nbytes, data[2];
>
>                         smcr = rpc->smcr | RPCIF_SMCR_SPIE;
> -                       if (nbytes > max) {
> -                               nbytes = max;
> +
> +                       /* nbytes may only be 1, 2, 4, or 8 */
> +                       nbytes = bytes_left >= max ? max : (1 << ilog2(bytes_left));
> +                       if (bytes_left > nbytes)
>                                 smcr |= RPCIF_SMCR_SSLKP;
> -                       }
> +
> +                       smenr |= RPCIF_SMENR_SPIDE(rpcif_bits_set(rpc, nbytes));
> +                       regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
>
>                         memcpy(data, rpc->buffer + pos, nbytes);
> -                       if (nbytes > 4) {
> +                       if (nbytes == 8) {
>                                 regmap_write(rpc->regmap, RPCIF_SMWDR1,
>                                              data[0]);
>                                 regmap_write(rpc->regmap, RPCIF_SMWDR0,
>                                              data[1]);
> -                       } else if (nbytes > 2) {
> +                       } else {
>                                 regmap_write(rpc->regmap, RPCIF_SMWDR0,
>                                              data[0]);
> -                       } else  {
> -                               regmap_write(rpc->regmap, RPCIF_SMWDR0,
> -                                            data[0] << 16);
>                         }
>
> -                       regmap_write(rpc->regmap, RPCIF_SMADR,
> -                                    rpc->smadr + pos);

Removing this implies SMADR is auto-incrementing for writes...

> -                       regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
>                         regmap_write(rpc->regmap, RPCIF_SMCR, smcr);
>                         ret = wait_msg_xfer_end(rpc);
>                         if (ret)
> @@ -448,14 +493,16 @@ int rpcif_manual_xfer(struct rpcif *rpc)
>                         break;
>                 }
>                 while (pos < rpc->xferlen) {
> -                       u32 nbytes = rpc->xferlen - pos;
> -                       u32 data[2];
> +                       u32 bytes_left = rpc->xferlen - pos;
> +                       u32 nbytes, data[2];
>
> -                       if (nbytes > max)
> -                               nbytes = max;
> +                       /* nbytes may only be 1, 2, 4, or 8 */
> +                       nbytes = bytes_left >= max ? max : (1 << ilog2(bytes_left));
>
>                         regmap_write(rpc->regmap, RPCIF_SMADR,
>                                      rpc->smadr + pos);

... while keeping this assumes SMADR is not auto-incrementing for
reads?

Figure 62.17 "Example of Data Transfer Setting Flow in Manual Operating
Mode" does show writing SMADR in each loop iteration.
I cannot find anything about auto-incrementing in the documentation,
except for Figure 62.28 "Write Buffer Usage Sequence", which does
not apply as Linux does not support the write buffer yet.

Given you tested this, and the BSP commit 0d37f69cacb33435 ("memory:
renesas-rpc-if: Correct QSPI data transfer in Manual mode") does the
same, I assume it's working fine?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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