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

["Behme Dirk (CM/ESO2)" <dirk.behme@de.bosch.com>]

On 14.09.2020 15:09, Vignesh Raghavendra wrote:
> Hi Sergei,
> 
> On 5/14/20 2:13 AM, Sergei Shtylyov 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@cogentembedded.com>
>>
> 
> Mails to this ID is bouncing which meant I could not ask status of
> dependencies.
> 
>> ---
>> 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://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11283127%2F&amp;data=02%7C01%7Cdirk.behme%40de.bosch.com%7C67d24357a6644232518008d858af6cd0%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C637356857751715684&amp;sdata=XDR3Nu15LcC3G5C4HZgb9UH8eNGWDpuxupf6zoXcM%2Fc%3D&amp;reserved=0
>>
> [...]
>> --- /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?


If I remember correctly 'select MTD_CFI_BE_BYTE_SWAP' in Kconfig doesn't 
seem to work. Therefore the FIXME. I can't remember exactly the root 
cause for that any more, but maybe it was the 'bool' type of 
MTD_CFI_BE_BYTE_SWAP?

If anybody has a nice solution for that, it would be welcome :)

Best regards

Dirk


>> +
>> + 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 {$
> 
> 
> 
>> +	struct rpcif rpc;
>> +	struct hyperbus_ctlr ctlr;
>> +	struct hyperbus_device hbdev;
>> +};
>> +
>> +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? Maybe using a self
> describing macro would be helpful when assigning values to op.cmd.opcode?
> 
>> +	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
> 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.
> 
> [...]
> 
> Regards
> Vignesh
> 

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