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

[Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>]

On 02/07/2020 10:31 PM, Dirk Behme 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
                                                                          ^^^ then

>> 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?
> 
> 
> Well, even if we have swapping enabled and with this it's in the LSBs, it's not detected in the first run. See the first 5 traces in [3] below.
> 
> 
>>> 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.
> 
> 
> Yes, but ;)
> 
> I tried MTD_CFI_BE_BYTE_SWAP config option, too. Enabling that and dropping cpu_to_be16s()/be16_to_cpu() in the driver result in a successful probe. And
> /dev/mtdx afterwards. That's the good news.
> 
> But, the bad news:
> 
> Trying a write (dd to /dev/mtdx) hanged and never returned. In contrast to the

   Not for me:

root@192.168.2.11:~# dd if=jffs2.img of=/dev/mtd11                              
random: crng init done                                                          
2666+1 records in                                                               
2666+1 records out                                                              
1365320 bytes (1.4 MB) copied, 33.0917 seconds, 41.3 kB/s                       

> solution with the cpu_to_be16s()/be16_to_cpu() in the driver, which wrote nicely to the Hyperflash, but swapped.

   Something's wrong at your end...

> Best regards
> 
> Dirk

MBR, Sergei

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