Re: [PATCH RFT 0/2/2] mtd: hyperbus: add Renesas RPC-IF driver

[Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>]

Hello!

On 02/07/2020 03:59 PM, Behme Dirk (CM/ESO2) wrote:

>> Add the HyperFLash driver for the Renesas RPC-IF.  It's the "front end"
>> driver using the "back end" APIs in the main driver to talk to the real
>> hardware.
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
[...]
>> Index: linux/drivers/mtd/hyperbus/rpc-if.c
>> ===================================================================
>> --- /dev/null
>> +++ linux/drivers/mtd/hyperbus/rpc-if.c
>> @@ -0,0 +1,162 @@
[...]
>> +static u16 rpcif_hb_read16(struct hyperbus_device *hbdev, unsigned long addr)
>> +{
>> +    struct rpcif_hyperbus *hyperbus =
>> +        container_of(hbdev, struct rpcif_hyperbus, hbdev);
>> +    struct rpcif_op op = rpcif_op_tmpl;
>> +    map_word data;
>> +
>> +    op.cmd.opcode = 0xC0;
>> +    op.addr.val = addr >> 1;
>> +    op.dummy.buswidth = 1;
>> +    op.dummy.ncycles = 15;
>> +    op.data.dir = RPCIF_DATA_IN;
>> +    op.data.nbytes = 2;
>> +    op.data.buf.in = &data;
>> +    rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); // ?
>> +    rpcif_io_xfer(&hyperbus->rpc);
>> +
>> +    return be16_to_cpu(data.x[0]);
>> +}
>> +
>> +static void rpcif_hb_write16(struct hyperbus_device *hbdev, unsigned long addr,
>> +                 u16 data)
>> +{
>> +    struct rpcif_hyperbus *hyperbus =
>> +        container_of(hbdev, struct rpcif_hyperbus, hbdev);
>> +    struct rpcif_op op = rpcif_op_tmpl;
>> +
>> +    op.cmd.opcode = 0x40;
>> +    op.addr.val = addr >> 1;
>> +    op.data.dir = RPCIF_DATA_OUT;
>> +    op.data.nbytes = 2;
>> +    op.data.buf.out = &data;
>> +    cpu_to_be16s(&data);
> 
> 
> 
> Testing this, I found that writing data to the Hyperflash results in swapped _data_ in Hyperflash due to this cpu_to_be16s() conversion:
> 
> 02 01 04 03 06 05 08 07 ...
> 
> Breaking the usage of the data written for other users, i.e. the boot loaders.
> 
> On the other hand, dropping this cpu_to_be16s() (and be16_to_cpu() in the read16 above) makes the probing to fail completely.
> 
> The topic seems to be that rpcif_hb_write16() handles command _and_ data, and the commands seem to need the conversion.

   The HyperBus spec says the register space is always big-endian but the again
HypoerFlash doesn't have the register space...

> As mentioned, the first idea, dropping the conversion and adding some debug output in the driver [1] results in failed probe [2]. Successful probing of the unmodified driver  results in [3], then.
> 
> Seems I need some advice: Why is this conversion for successful probe required?
> Why is the first 'QRY' returned by the device not detected by cfi_qry_mode_on()?

   "QRY" is in the MSBs?

> Is the any possibility to drop the conversion _and_ make the driver probe successful? Or do we need to split the path the commands and the data are routed? If so, how?

   I've found some interesting options under the CFI advanced config options,
e.g. "Flash cmd/query data swapping" having MTD_CFI_BE_BYTE_SWAP value in this
item. With this variant chosen, I don't need any byte swapping in the driver
any more... and the QRY signature is read correctly on the very 1st try.

> Many questions ;)

   Hopefully an answer was found.

> Best regards
> 
> Dirk
> 
> 
> [1] Dropping be16_to_cpu() & cpu_to_be16s() and adding some debug output:
> 
> diff --git a/drivers/mtd/chips/cfi_util.c b/drivers/mtd/chips/cfi_util.c
> index 6f16552cd59f..e5dd8dd3b594 100644
> --- a/drivers/mtd/chips/cfi_util.c
> +++ b/drivers/mtd/chips/cfi_util.c
> @@ -239,9 +239,13 @@ int __xipram cfi_qry_present(struct map_info *map, __u32 base,
>  }
>  EXPORT_SYMBOL_GPL(cfi_qry_present);
> 
> +static unsigned int count = 1;
> +
>  int __xipram cfi_qry_mode_on(uint32_t base, struct map_info *map,
>                              struct cfi_private *cfi)
>  {
> +       pr_err("cfi_qry_mode_on() called #%i\n", count++);
> +
>         cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL);
>         cfi_send_gen_cmd(0x98, 0x55, base, map, cfi, cfi->device_type, NULL);
>         if (cfi_qry_present(map, base, cfi))
> @@ -273,6 +277,9 @@ int __xipram cfi_qry_mode_on(uint32_t base, struct map_info *map,
>         if (cfi_qry_present(map, base, cfi))
>                 return 1;
>         /* QRY not found */
> +
> +       pr_err("cfi_qry_mode_on() failed\n");
> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(cfi_qry_mode_on);
> diff --git a/drivers/mtd/hyperbus/rpc-if.c b/drivers/mtd/hyperbus/rpc-if.c
> index a66a5080b482..bb83a8f3f3bc 100644
> --- a/drivers/mtd/hyperbus/rpc-if.c
> +++ b/drivers/mtd/hyperbus/rpc-if.c
> @@ -60,7 +60,11 @@ static u16 rpcif_hb_read16(struct hyperbus_device *hbdev, unsigned long addr)
>         rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); // ?
>         rpcif_io_xfer(&hyperbus->rpc);
> 
> -       return be16_to_cpu(data.x[0]);
> +       pr_err("read:  a: 0x%08lX  d: 0x%04X %c %c\n", addr, (unsigned short)data.x[0],
> +              (unsigned char)((data.x[0] >> 8) & 0xFF),
> +              (unsigned char)data.x[0]);
> +
> +       return data.x[0];
>  }
> 
>  static void rpcif_hb_write16(struct hyperbus_device *hbdev, unsigned long addr,
> @@ -75,7 +79,7 @@ static void rpcif_hb_write16(struct hyperbus_device *hbdev, unsigned long addr,
>         op.data.dir = RPCIF_DATA_OUT;
>         op.data.nbytes = 2;
>         op.data.buf.out = &data;
> -       cpu_to_be16s(&data);
> +       pr_err("write: a: 0x%08lX  d: 0x%04X\n", addr, data);
>         rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); // ?
>         rpcif_io_xfer(&hyperbus->rpc);
>  }
> 
> 
> [2] Probe fails without be16_to_cpu/cpu_to_be16s:
> 
> cfi_qry_mode_on() called #1
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x000000AA  d: 0x9898
> read:  a: 0x00000020  d: 0x5100 Q
> read:  a: 0x00000022  d: 0x5200 R
> read:  a: 0x00000024  d: 0x5900 Y
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000000  d: 0xFFFF
> write: a: 0x000000AA  d: 0x9898
> read:  a: 0x00000020  d: 0x5100 Q
> read:  a: 0x00000022  d: 0x5200 R
> read:  a: 0x00000024  d: 0x5900 Y
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000AAA  d: 0x9898
> read:  a: 0x00000020  d: 0x5100 Q
> read:  a: 0x00000022  d: 0x5200 R
> read:  a: 0x00000024  d: 0x5900 Y
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x0000AAAA  d: 0xAAAA
> write: a: 0x00005554  d: 0x5555
> write: a: 0x0000AAAA  d: 0x9898
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000AAA  d: 0xAAAA
> write: a: 0x00000554  d: 0x5555
> write: a: 0x00000AAA  d: 0x9898
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> cfi_qry_mode_on() failed
> cfi_qry_mode_on() called #2
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000154  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000000  d: 0xFFFF
> write: a: 0x00000154  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00001554  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00015554  d: 0xAAAA
> write: a: 0x0000AAAA  d: 0x5555
> write: a: 0x00015554  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00001554  d: 0xAAAA
> write: a: 0x00000AAA  d: 0x5555
> write: a: 0x00001554  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> cfi_qry_mode_on() failed
> cfi_qry_mode_on() called #3
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x000002A8  d: 0x9898
> read:  a: 0x00000080  d: 0xF358 ¦ X
> read:  a: 0x00000088  d: 0x0057  W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000000  d: 0xFFFF
> write: a: 0x000002A8  d: 0x9898
> read:  a: 0x00000080  d: 0xF358 ¦ X
> read:  a: 0x00000088  d: 0x0057  W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00002AA8  d: 0x9898
> read:  a: 0x00000080  d: 0xF358 ¦ X
> read:  a: 0x00000088  d: 0x0057  W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x0002AAA8  d: 0xAAAA
> write: a: 0x00015554  d: 0x5555
> write: a: 0x0002AAA8  d: 0x9898
> read:  a: 0x00000080  d: 0xF358 ¦ X
> read:  a: 0x00000088  d: 0x0057  W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00002AA8  d: 0xAAAA
> write: a: 0x00001554  d: 0x5555
> write: a: 0x00002AA8  d: 0x9898
> read:  a: 0x00000080  d: 0xF358 ¦ X
> read:  a: 0x00000088  d: 0x0057  W
> read:  a: 0x00000090  d: 0x0000
> cfi_qry_mode_on() failed
> cfi_qry_mode_on() called #4
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x000000AA  d: 0x0098
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00000000  d: 0x00FF
> write: a: 0x000000AA  d: 0x0098
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00000AAA  d: 0x0098
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x0000AAAA  d: 0x00AA
> write: a: 0x00005554  d: 0x0055
> write: a: 0x0000AAAA  d: 0x0098
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00000AAA  d: 0x00AA
> write: a: 0x00000554  d: 0x0055
> write: a: 0x00000AAA  d: 0x0098
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> cfi_qry_mode_on() failed
> cfi_qry_mode_on() called #5
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00000154  d: 0x0098
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00000000  d: 0x00FF
> write: a: 0x00000154  d: 0x0098
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00001554  d: 0x0098
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00015554  d: 0x00AA
> write: a: 0x0000AAAA  d: 0x0055
> write: a: 0x00015554  d: 0x0098
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0x00F0
> write: a: 0x00001554  d: 0x00AA
> write: a: 0x00000AAA  d: 0x0055
> write: a: 0x00001554  d: 0x0098
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> cfi_qry_mode_on() failed
> rpc-if-hyperflash rpc-if-hyperflash: probing of hyperbus device failed
> rpc-if-hyperflash rpc-if-hyperflash: failed to register device
> 
> 
> 
> [3] Probe success WITH be16_to_cpu/cpu_to_be16s:
> 
> cfi_qry_mode_on() called #1
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x000000AA  d: 0x9898
> read:  a: 0x00000020  d: 0x0051  Q
> read:  a: 0x00000022  d: 0x0052  R
> read:  a: 0x00000024  d: 0x0059  Y
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000000  d: 0xFFFF
> write: a: 0x000000AA  d: 0x9898
> read:  a: 0x00000020  d: 0x0051  Q
> read:  a: 0x00000022  d: 0x0052  R
> read:  a: 0x00000024  d: 0x0059  Y
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000AAA  d: 0x9898
> read:  a: 0x00000020  d: 0x0051  Q
> read:  a: 0x00000022  d: 0x0052  R
> read:  a: 0x00000024  d: 0x0059  Y
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x0000AAAA  d: 0xAAAA
> write: a: 0x00005554  d: 0x5555
> write: a: 0x0000AAAA  d: 0x9898
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000AAA  d: 0xAAAA
> write: a: 0x00000554  d: 0x5555
> write: a: 0x00000AAA  d: 0x9898
> read:  a: 0x00000020  d: 0x0000
> read:  a: 0x00000022  d: 0x0000
> read:  a: 0x00000024  d: 0x0000
> cfi_qry_mode_on() failed
> cfi_qry_mode_on() called #2
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000154  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000000  d: 0xFFFF
> write: a: 0x00000154  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00001554  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00015554  d: 0xAAAA
> write: a: 0x0000AAAA  d: 0x5555
> write: a: 0x00015554  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00001554  d: 0xAAAA
> write: a: 0x00000AAA  d: 0x5555
> write: a: 0x00001554  d: 0x9898
> read:  a: 0x00000040  d: 0x0000
> read:  a: 0x00000044  d: 0x0000
> read:  a: 0x00000048  d: 0x0000
> cfi_qry_mode_on() failed
> cfi_qry_mode_on() called #3
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x000002A8  d: 0x9898
> read:  a: 0x00000080  d: 0x58F3 X ¦
> read:  a: 0x00000088  d: 0x5700 W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00000000  d: 0xFFFF
> write: a: 0x000002A8  d: 0x9898
> read:  a: 0x00000080  d: 0x58F3 X ¦
> read:  a: 0x00000088  d: 0x5700 W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00002AA8  d: 0x9898
> read:  a: 0x00000080  d: 0x58F3 X ¦
> read:  a: 0x00000088  d: 0x5700 W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x0002AAA8  d: 0xAAAA
> write: a: 0x00015554  d: 0x5555
> write: a: 0x0002AAA8  d: 0x9898
> read:  a: 0x00000080  d: 0x58F3 X ¦
> read:  a: 0x00000088  d: 0x5700 W
> read:  a: 0x00000090  d: 0x0000
> write: a: 0x00000000  d: 0xF0F0
> write: a: 0x00002AA8  d: 0xAAAA
> write: a: 0x00001554  d: 0x5555
> write: a: 0x00002AA8  d: 0x9898
> read:  a: 0x00000080  d: 0x58F3 X ¦
> read:  a: 0x00000088  d: 0x5700 W
> read:  a: 0x00000090  d: 0x0000
> cfi_qry_mode_on() failed
> cfi_qry_mode_on() called #4
> write: a: 0x00000000  d: 0xF000
> write: a: 0x000000AA  d: 0x9800
> read:  a: 0x00000020  d: 0x0051  Q
> read:  a: 0x00000022  d: 0x0052  R
> read:  a: 0x00000024  d: 0x0059  Y
> read:  a: 0x00000058  d: 0x0001
> read:  a: 0x00000020  d: 0x0051  Q
> read:  a: 0x00000022  d: 0x0052  R
> read:  a: 0x00000024  d: 0x0059  Y
> read:  a: 0x00000026  d: 0x0002
> read:  a: 0x00000028  d: 0x0000
> read:  a: 0x0000002A  d: 0x0040  @
> read:  a: 0x0000002C  d: 0x0000
> read:  a: 0x0000002E  d: 0x0000
> read:  a: 0x00000030  d: 0x0000
> read:  a: 0x00000032  d: 0x0000
> read:  a: 0x00000034  d: 0x0000
> read:  a: 0x00000036  d: 0x0017
> read:  a: 0x00000038  d: 0x0019
> read:  a: 0x0000003A  d: 0x0000
> read:  a: 0x0000003C  d: 0x0000
> read:  a: 0x0000003E  d: 0x0009
> read:  a: 0x00000040  d: 0x0009
> read:  a: 0x00000042  d: 0x000A
> 
> read:  a: 0x00000044  d: 0x0012
> read:  a: 0x00000046  d: 0x0002
> read:  a: 0x00000048  d: 0x0002
> read:  a: 0x0000004A  d: 0x0002
> read:  a: 0x0000004C  d: 0x0002
> read:  a: 0x0000004E  d: 0x001A
> read:  a: 0x00000050  d: 0x0000
> read:  a: 0x00000052  d: 0x0000
> read:  a: 0x00000054  d: 0x0009
> read:  a: 0x00000056  d: 0x0000
> read:  a: 0x00000058  d: 0x0001
> read:  a: 0x0000005A  d: 0x00FF  ¦
> read:  a: 0x0000005C  d: 0x0000
> read:  a: 0x0000005E  d: 0x0000
> read:  a: 0x00000060  d: 0x0004
> write: a: 0x00000000  d: 0xF000
> write: a: 0x00000AAA  d: 0xAA00
> write: a: 0x00000554  d: 0x5500
> write: a: 0x00000AAA  d: 0x9000
> read:  a: 0x00000000  d: 0x0001
> read:  a: 0x00000002  d: 0x007E  ~
> read:  a: 0x0000001C  d: 0x0070  p
> read:  a: 0x0000001E  d: 0x0000
> write: a: 0x00000000  d: 0xF000
> write: a: 0x00000000  d: 0xFF00
> cfi_qry_mode_on() called #5
> write: a: 0x00000000  d: 0xF000
> write: a: 0x000000AA  d: 0x9800
> read:  a: 0x00000020  d: 0x0051  Q
> read:  a: 0x00000022  d: 0x0052  R
> read:  a: 0x00000024  d: 0x0059  Y
> read:  a: 0x00000080  d: 0x0050  P
> read:  a: 0x00000082  d: 0x0052  R
> read:  a: 0x00000084  d: 0x0049  I
> read:  a: 0x00000086  d: 0x0031  1
> read:  a: 0x00000088  d: 0x0035  5
> read:  a: 0x0000008A  d: 0x001C
> read:  a: 0x0000008C  d: 0x0002
> read:  a: 0x0000008E  d: 0x0001
> read:  a: 0x00000090  d: 0x0000
> read:  a: 0x00000092  d: 0x0008
> read:  a: 0x00000094  d: 0x0000
> read:  a: 0x00000096  d: 0x0001
> read:  a: 0x00000098  d: 0x0000
> read:  a: 0x0000009A  d: 0x0000
> read:  a: 0x0000009C  d: 0x0000
> read:  a: 0x0000009E  d: 0x0000
> write: a: 0x00000000  d: 0xF000
> write: a: 0x00000000  d: 0xFF00
> => success
> 
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/