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

[Sergei Shtylyov]

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>
> 
> Mails to this ID is bouncing which meant I could not ask status of
> dependencies.

   I no longer work for Cogent Embedded (this also means that I've lost
the hardware access). The mail kept working for several months after my
layoff though...

>> ---
>> This patch is against the 'mtd/next' branch of the MTD 'linux.git' repo.
>> Requires  the RPC-IF main driver patch in order to build/work:
>>
>> [1] https://patchwork.kernel.org/patch/11283127/
>>
> [...]
>> --- /dev/null
>> +++ linux/drivers/mtd/hyperbus/rpc-if.c
>> @@ -0,0 +1,165 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Linux driver for RPC-IF HyperFlash
>> + *
>> + * Copyright (C) 2019-2020 Cogent Embedded, Inc.
>> + */
>> +
>> +#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!

>> +
>> + 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...

>> +	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...

> 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