[PATCH v2] mtd: hyperbus: add Renesas RPC-IF driver

[Vignesh Raghavendra]

On 9/19/20 10:07 PM, Sergei Shtylyov wrote:
> Hello!
> 
> On 09/14/2020 04:09 PM, Vignesh Raghavendra 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 at cogentembedded.com>
>>
[...]
>>> +#include <linux/err.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mtd/hyperbus.h>
>>> +#include <linux/mtd/mtd.h>
>>> +#include <linux/mux/consumer.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/types.h>
>>> +
>>> +#include <memory/renesas-rpc-if.h>
>>> +
>>> +/* FIXME: How to drop this? */
>>> +#ifndef CONFIG_MTD_CFI_BE_BYTE_SWAP
>>> +#error Enable config "Flash cmd/query data swapping (BIG_ENDIAN_BYTE)"
>>> +#endif
>>
>> select MTD_CFI_BE_BYTE_SWAP in Kconfig does not help?
> 
>    Alas, it doesn't work!
> 

As I corrected in another reply, why not use "depends on" like:

 config RPCIF_HYPERBUS
        tristate "Renesas RPC-IF HyperBus driver"
        depends on RENESAS_RPCIF
-       select MTD_CFI_ADV_OPTIONS
+       depends on MTD_CFI_BE_BYTE_SWAP || COMPILE_TEST
        help
          This option includes Renesas RPC-IF HyperFlash support.

>>> +
>>> + struct	rpcif_hyperbus {
>>
>> WARNING: please, no spaces at the start of a line
>> #73: FILE: drivers/mtd/hyperbus/rpc-if.c:25:
>> + struct^Irpcif_hyperbus {$
> 
>    Ugh, must be a stray space... :-/
> 
> [...]
>>> +static const struct rpcif_op rpcif_op_tmpl = {
>>> +	.cmd = {
>>> +		.buswidth = 8,
>>> +		.ddr = true,
>>> +	},
>>> +	.ocmd = {
>>> +		.buswidth = 8,
>>> +		.ddr = true,
>>> +	},
>>> +	.addr = {
>>> +		.nbytes = 1,
>>> +		.buswidth = 8,
>>> +		.ddr = true,
>>> +	},
>>> +	.data = {
>>> +		.buswidth = 8,
>>> +		.ddr = true,
>>> +	},
>>> +};
>>> +
>>> +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 = 0x80;
>>
>> Here and elsewhere what does 0x80 represent?
> 
>    This is bits 40..47 of the HyperBus command/address packet: thus 0x80 means
> memory read.
> 
>> Maybe using a self
>> describing macro would be helpful when assigning values to op.cmd.opcode?
> 
>    It probably would but it apparently needs to be placed in your HyperBus header...

That's fine, Patches are welcome...

> 
>>> +	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;
>>
>> This code block seems like a good candidate to be converted to a macro/
>> template to avoid repetition. Template can initializes few values remain
> 
>    I'm already using a common template...
> 

Yes, but I see there is scope to extend further as I pointed out below..

>> common across the driver and override others based on parameters passed.
>> You could probably have one for write op and one for read op, if needed.
> 
>    OK, I'll look into doing that...
> 
>> [...]
>>
>> Regards
>> Vignesh
> 
> MBR, Sergei
>