[RFC,2/2] mfd: add Renesas RPC-IF driver

Commit Message

Sergei Shtylyov May 30, 2019, 8:03 p.m. UTC

Add the MFD driver for Renesas RPC-IF which registers either the SPI or
HyperFLash  device depending on the contents of the device tree subnode.
It also provides the absract "back end" device APIs that can be used by
the "front end" SPI/MTD drivers to talk to the real hardware.

Based on the original patch by Mason Yang <[e-mail]>.

Signed-off-by: Sergei Shtylyov <[e-mail]>

---
 drivers/mfd/Kconfig        |   10 
 drivers/mfd/Makefile       |    1 
 drivers/mfd/rpc-if.c       |  535 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/rpc-if.h |   85 +++++++
 4 files changed, 631 insertions(+)

Comments

Lee Jones June 12, 2019, 9:05 a.m. UTC | #1

On Thu, 30 May 2019, Sergei Shtylyov wrote:

> Add the MFD driver for Renesas RPC-IF which registers either the SPI or
> HyperFLash  device depending on the contents of the device tree subnode.
> It also provides the absract "back end" device APIs that can be used by
> the "front end" SPI/MTD drivers to talk to the real hardware.
> 
> Based on the original patch by Mason Yang <[e-mail]>.
> 
> Signed-off-by: Sergei Shtylyov <[e-mail]>
> 
> ---
>  drivers/mfd/Kconfig        |   10 
>  drivers/mfd/Makefile       |    1 
>  drivers/mfd/rpc-if.c       |  535 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/rpc-if.h |   85 +++++++
>  4 files changed, 631 insertions(+)
> 
> Index: mfd/drivers/mfd/Kconfig
> ===================================================================
> --- mfd.orig/drivers/mfd/Kconfig
> +++ mfd/drivers/mfd/Kconfig
> @@ -1002,6 +1002,16 @@ config MFD_RDC321X
>  	  southbridge which provides access to GPIOs and Watchdog using the
>  	  southbridge PCI device configuration space.
>  
> +config MFD_RPCIF
> +	tristate "Renesas RPC-IF driver"
> +	select MFD_CORE
> +	select REGMAP_MMIO
> +	depends on ARCH_RENESAS
> +	help
> +	  This supports Renesas R-Car Gen3 RPC-IF which provides either SPI
> +	  host or HyperFlash. You'll have to select individual components
> +	  under the corresponding menu.
> +
>  config MFD_RT5033
>  	tristate "Richtek RT5033 Power Management IC"
>  	depends on I2C
> Index: mfd/drivers/mfd/Makefile
> ===================================================================
> --- mfd.orig/drivers/mfd/Makefile
> +++ mfd/drivers/mfd/Makefile
> @@ -184,6 +184,7 @@ obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+
>  obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
>  obj-$(CONFIG_LPC_ICH)		+= lpc_ich.o
>  obj-$(CONFIG_MFD_RDC321X)	+= rdc321x-southbridge.o
> +obj-$(CONFIG_MFD_RPCIF)		+= rpc-if.o
>  obj-$(CONFIG_MFD_JANZ_CMODIO)	+= janz-cmodio.o
>  obj-$(CONFIG_MFD_JZ4740_ADC)	+= jz4740-adc.o
>  obj-$(CONFIG_MFD_TPS6586X)	+= tps6586x.o
> Index: mfd/drivers/mfd/rpc-if.c
> ===================================================================
> --- /dev/null
> +++ mfd/drivers/mfd/rpc-if.c
> @@ -0,0 +1,535 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RPC-IF MFD driver
> + *
> + * Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
> + * Copyright (C) 2019 Macronix International Co., Ltd.
> + * Copyright (C) 2019 Cogent Embedded, Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/rpc-if.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>

Alphabetical.

> +#include <asm/unaligned.h>
> +
> +#define RPCIF_CMNCR		0x0000	// R/W

No C++ comments.

> +#define RPCIF_CMNCR_MD		BIT(31)
> +#define RPCIF_CMNCR_SFDE	BIT(24) // undocumented bit but must be set
> +#define RPCIF_CMNCR_MOIIO3(val)	(((val) & 0x3) << 22)
> +#define RPCIF_CMNCR_MOIIO2(val)	(((val) & 0x3) << 20)
> +#define RPCIF_CMNCR_MOIIO1(val)	(((val) & 0x3) << 18)
> +#define RPCIF_CMNCR_MOIIO0(val)	(((val) & 0x3) << 16)
> +#define RPCIF_CMNCR_MOIIO_HIZ	(RPCIF_CMNCR_MOIIO0(3) | \
> +				 RPCIF_CMNCR_MOIIO1(3) | \
> +				 RPCIF_CMNCR_MOIIO2(3) | RPCIF_CMNCR_MOIIO3(3))
> +#define RPCIF_CMNCR_IO3FV(val)	(((val) & 0x3) << 14) // undocumented
> +#define RPCIF_CMNCR_IO2FV(val)	(((val) & 0x3) << 12) // undocumented
> +#define RPCIF_CMNCR_IO0FV(val)	(((val) & 0x3) << 8)
> +#define RPCIF_CMNCR_IOFV_HIZ	(RPCIF_CMNCR_IO0FV(3) | RPCIF_CMNCR_IO2FV(3) | \
> +				 RPCIF_CMNCR_IO3FV(3))
> +#define RPCIF_CMNCR_BSZ(val)	(((val) & 0x3) << 0)
> +
> +#define RPCIF_SSLDR		0x0004	// R/W
> +#define RPCIF_SSLDR_SPNDL(d)	(((d) & 0x7) << 16)
> +#define RPCIF_SSLDR_SLNDL(d)	(((d) & 0x7) << 8)
> +#define RPCIF_SSLDR_SCKDL(d)	(((d) & 0x7) << 0)
> +
> +#define RPCIF_DRCR		0x000C	// R/W
> +#define RPCIF_DRCR_SSLN		BIT(24)
> +#define RPCIF_DRCR_RBURST(v)	((((v) - 1) & 0x1F) << 16)
> +#define RPCIF_DRCR_RCF		BIT(9)
> +#define RPCIF_DRCR_RBE		BIT(8)
> +#define RPCIF_DRCR_SSLE		BIT(0)
> +
> +#define RPCIF_DRCMR		0x0010	// R/W
> +#define RPCIF_DRCMR_CMD(c)	(((c) & 0xFF) << 16)
> +#define RPCIF_DRCMR_OCMD(c)	(((c) & 0xFF) << 0)
> +
> +#define RPCIF_DREAR		0x0014	// R/W
> +#define RPCIF_DREAR_EAV(c)	(((c) & 0xf) << 16)
> +#define RPCIF_DREAR_EAC(c)	(((c) & 0x7) << 0)
> +
> +#define RPCIF_DROPR		0x0018	// R/W
> +
> +#define RPCIF_DRENR		0x001C	// R/W
> +#define RPCIF_DRENR_CDB(o)	(u32)((((o) & 0x3) << 30))
> +#define RPCIF_DRENR_OCDB(o)	(((o) & 0x3) << 28)
> +#define RPCIF_DRENR_ADB(o)	(((o) & 0x3) << 24)
> +#define RPCIF_DRENR_OPDB(o)	(((o) & 0x3) << 20)
> +#define RPCIF_DRENR_DRDB(o)	(((o) & 0x3) << 16)
> +#define RPCIF_DRENR_DME		BIT(15)
> +#define RPCIF_DRENR_CDE		BIT(14)
> +#define RPCIF_DRENR_OCDE	BIT(12)
> +#define RPCIF_DRENR_ADE(v)	(((v) & 0xF) << 8)
> +#define RPCIF_DRENR_OPDE(v)	(((v) & 0xF) << 4)
> +
> +#define RPCIF_SMCR		0x0020	// R/W
> +#define RPCIF_SMCR_SSLKP	BIT(8)
> +#define RPCIF_SMCR_SPIRE	BIT(2)
> +#define RPCIF_SMCR_SPIWE	BIT(1)
> +#define RPCIF_SMCR_SPIE		BIT(0)
> +
> +#define RPCIF_SMCMR		0x0024	// R/W
> +#define RPCIF_SMCMR_CMD(c)	(((c) & 0xFF) << 16)
> +#define RPCIF_SMCMR_OCMD(c)	(((c) & 0xFF) << 0)
> +
> +#define RPCIF_SMADR		0x0028	// R/W
> +#define RPCIF_SMOPR		0x002C	// R/W
> +#define RPCIF_SMOPR_OPD3(o)	(((o) & 0xFF) << 24)
> +#define RPCIF_SMOPR_OPD2(o)	(((o) & 0xFF) << 16)
> +#define RPCIF_SMOPR_OPD1(o)	(((o) & 0xFF) << 8)
> +#define RPCIF_SMOPR_OPD0(o)	(((o) & 0xFF) << 0)
> +
> +#define RPCIF_SMENR		0x0030	// R/W
> +#define RPCIF_SMENR_CDB(o)	(((o) & 0x3) << 30)
> +#define RPCIF_SMENR_OCDB(o)	(((o) & 0x3) << 28)
> +#define RPCIF_SMENR_ADB(o)	(((o) & 0x3) << 24)
> +#define RPCIF_SMENR_OPDB(o)	(((o) & 0x3) << 20)
> +#define RPCIF_SMENR_SPIDB(o)	(((o) & 0x3) << 16)
> +#define RPCIF_SMENR_DME		BIT(15)
> +#define RPCIF_SMENR_CDE		BIT(14)
> +#define RPCIF_SMENR_OCDE	BIT(12)
> +#define RPCIF_SMENR_ADE(v)	(((v) & 0xF) << 8)
> +#define RPCIF_SMENR_OPDE(v)	(((v) & 0xF) << 4)
> +#define RPCIF_SMENR_SPIDE(v)	(((v) & 0xF) << 0)
> +
> +#define RPCIF_SMRDR0		0x0038	// R
> +#define RPCIF_SMRDR1		0x003C	// R
> +#define RPCIF_SMWDR0		0x0040	// W
> +#define RPCIF_SMWDR1		0x0044	// W
> +
> +#define RPCIF_CMNSR		0x0048	// R
> +#define RPCIF_CMNSR_SSLF	BIT(1)
> +#define RPCIF_CMNSR_TEND	BIT(0)
> +
> +#define RPCIF_DRDMCR		0x0058	// R/W
> +#define RPCIF_DMDMCR_DMCYC(v)	((((v) - 1) & 0x1F) << 0)
> +
> +#define RPCIF_DRDRENR		0x005C	// R/W
> +#define RPCIF_DRDRENR_HYPE(v)	(((v) & 0x7) << 12)
> +#define RPCIF_DRDRENR_ADDRE	BIT(8)
> +#define RPCIF_DRDRENR_OPDRE	BIT(4)
> +#define RPCIF_DRDRENR_DRDRE	BIT(0)
> +
> +#define RPCIF_SMDMCR		0x0060	// R/W
> +#define RPCIF_SMDMCR_DMCYC(v)	((((v) - 1) & 0x1F) << 0)
> +
> +#define RPCIF_SMDRENR		0x0064	// R/W
> +#define RPCIF_SMDRENR_HYPE(v)	(((v) & 0x7) << 12)
> +#define RPCIF_SMDRENR_ADDRE	BIT(8)
> +#define RPCIF_SMDRENR_OPDRE	BIT(4)
> +#define RPCIF_SMDRENR_SPIDRE	BIT(0)
> +
> +#define RPCIF_PHYCNT		0x007C	// R/W
> +#define RPCIF_PHYCNT_CAL	BIT(31)
> +#define RPCIF_PHYCNT_OCTA_AA	BIT(22)
> +#define RPCIF_PHYCNT_OCTA_SA	BIT(23)
> +#define RPCIF_PHYCNT_EXDS	BIT(21)
> +#define RPCIF_PHYCNT_OCT	BIT(20)
> +#define RPCIF_PHYCNT_STRTIM(v)	(((v) & 0x7) << 15)
> +#define RPCIF_PHYCNT_WBUF2	BIT(4)
> +#define RPCIF_PHYCNT_WBUF	BIT(2)
> +#define RPCIF_PHYCNT_PHYMEM(v)	(((v) & 0x3) << 0)
> +
> +#define RPCIF_PHYOFFSET1	0x0080	// R/W
> +#define RPCIF_PHYOFFSET1_DDRTMG(v) (((v) & 0x3) << 28)
> +#define RPCIF_PHYOFFSET2	0x0084	// R/W
> +#define RPCIF_PHYOFFSET2_OCTTMG(v) (((v) & 0x7) << 8)
> +
> +#define RPCIF_DIRMAP_SIZE	0x4000000

Can you shift this lot out to a header file please.

> +void rpcif_enable_rpm(struct rpcif *rpc)
> +{
> +	pm_runtime_enable(rpc->dev);
> +}
> +EXPORT_SYMBOL(rpcif_enable_rpm);
> +
> +void rpcif_disable_rpm(struct rpcif *rpc)
> +{
> +	pm_runtime_disable(rpc->dev);
> +}
> +EXPORT_SYMBOL(rpcif_disable_rpm);

Where are you exporting these out to?

Why are you seemingly unnecessarily abstracting out the pm_* API?

> +void rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
> +{
> +	pm_runtime_get_sync(rpc->dev);
> +
> +	/*
> +	 * NOTE: The 0x260 are undocumented bits, but they must be set.
> +	 *	 RPCIF_PHYCNT_STRTIM is strobe timing adjustment bit,
> +	 *	 0x0 : the delay is biggest,
> +	 *	 0x1 : the delay is 2nd biggest,
> +	 *	 On H3 ES1.x, the value should be 0, while on others,
> +	 *	 the value should be 6.
> +	 */
> +	regmap_write(rpc->regmap, RPCIF_PHYCNT,
> +		     RPCIF_PHYCNT_CAL | RPCIF_PHYCNT_STRTIM(6) |
> +		     RPCIF_PHYCNT_PHYMEM(hyperflash ? 3 : 0) | 0x260);

At least #define it, even it it's not documented.

> +	/*
> +	 * NOTE: The 0x1511144 are undocumented bits, but they must be set
> +	 *       for RPCIF_PHYOFFSET1.
> +	 *	 The 0x31 are undocumented bits, but they must be set
> +	 *	 for RPCIF_PHYOFFSET2.
> +	 */
> +	regmap_write(rpc->regmap, RPCIF_PHYOFFSET1,
> +		     RPCIF_PHYOFFSET1_DDRTMG(3) | 0x1511144);
> +	regmap_write(rpc->regmap, RPCIF_PHYOFFSET2, 0x31 |
> +		     RPCIF_PHYOFFSET2_OCTTMG(4));

No magic numbers.  Please #define them all.

> +	regmap_write(rpc->regmap, RPCIF_SSLDR, RPCIF_SSLDR_SPNDL(7) |
> +		     RPCIF_SSLDR_SLNDL(7) | RPCIF_SSLDR_SCKDL(7));
> +	regmap_write(rpc->regmap, RPCIF_CMNCR, RPCIF_CMNCR_MD |
> +		     RPCIF_CMNCR_SFDE | RPCIF_CMNCR_MOIIO_HIZ |
> +		     RPCIF_CMNCR_IOFV_HIZ |
> +		     RPCIF_CMNCR_BSZ(hyperflash ? 1 : 0));
> +
> +	pm_runtime_put(rpc->dev);
> +}
> +EXPORT_SYMBOL(rpcif_hw_init);
> +
> +static int wait_msg_xfer_end(struct rpcif *rpc)
> +{
> +	u32 sts;
> +
> +	return regmap_read_poll_timeout(rpc->regmap, RPCIF_CMNSR, sts,
> +					sts & RPCIF_CMNSR_TEND, 0,

Aren't you using sts undefined here?

> +					USEC_PER_SEC);
> +}
> +
> +static u8 rpcif_bits_set(u32 nbytes)
> +{
> +	nbytes = clamp(nbytes, 1U, 4U);
> +
> +	return GENMASK(3, 4 - nbytes);
> +}


Looking at all this code from here ...

> +void rpcif_prepare(struct rpcif *rpc, const struct rpcif_op *op, u64 *offs,
> +		   size_t *len)
> +{
> +	rpc->enable = 0;
> +	rpc->command = 0;
> +	rpc->xferlen = 0;
> +	rpc->address = 0;
> +	rpc->ddr = 0;
> +
> +	if (op->cmd.buswidth) {
> +		rpc->enable  |= RPCIF_SMENR_CDE |
> +				RPCIF_SMENR_CDB(ilog2(op->cmd.buswidth));
> +		rpc->command |= RPCIF_SMCMR_CMD(op->cmd.opcode);
> +		if (op->cmd.ddr)
> +			rpc->ddr |= RPCIF_SMDRENR_HYPE(0x5);
> +	}
> +	if (op->ocmd.buswidth) {
> +		rpc->enable  |= RPCIF_SMENR_OCDE |
> +				RPCIF_SMENR_OCDB(ilog2(op->ocmd.buswidth));
> +		rpc->command |= RPCIF_SMCMR_OCMD(op->ocmd.opcode);
> +	}
> +
> +	if (op->addr.buswidth) {
> +		rpc->enable |= RPCIF_SMENR_ADB(ilog2(op->addr.buswidth));
> +		if (op->addr.nbytes == 4)
> +			rpc->enable |= RPCIF_SMENR_ADE(0xf);
> +		else
> +			rpc->enable |= RPCIF_SMENR_ADE(0x7);
> +		if (op->addr.ddr)
> +			rpc->ddr |= RPCIF_SMDRENR_ADDRE;
> +
> +		if (offs && len)
> +			rpc->address = *offs;
> +		else
> +			rpc->address = op->addr.val;
> +	}
> +
> +	if (op->dummy.buswidth) {
> +		rpc->enable |= RPCIF_SMENR_DME;
> +		rpc->dummy   = RPCIF_SMDMCR_DMCYC(op->dummy.nbytes * 8 /
> +						  op->dummy.buswidth);
> +	}
> +
> +	if (op->option.buswidth) {
> +		rpc->enable |= RPCIF_SMENR_OPDE(
> +					rpcif_bits_set(op->option.nbytes)) |
> +			       RPCIF_SMENR_OPDB(ilog2(op->option.buswidth));
> +		if (op->option.ddr)
> +			rpc->ddr |= RPCIF_SMDRENR_OPDRE;
> +		rpc->option = op->option.val;
> +	}
> +
> +	if (op->data.buswidth || (offs && len)) {
> +		switch (op->data.dir) {
> +		case RPCIF_DATA_IN:
> +			rpc->smcr = RPCIF_SMCR_SPIRE;
> +			break;
> +		case RPCIF_DATA_OUT:
> +			rpc->smcr = RPCIF_SMCR_SPIWE;
> +			break;
> +		default:
> +			break;
> +		}
> +		if (op->data.ddr)
> +			rpc->ddr |= RPCIF_SMDRENR_SPIDRE;
> +
> +		if (offs && len) {
> +			rpc->enable |= RPCIF_SMENR_SPIDE(rpcif_bits_set(*len)) |
> +				       RPCIF_SMENR_SPIDB(
> +						ilog2(op->data.buswidth));
> +			rpc->xferlen = *len;
> +		} else {
> +			rpc->enable |=
> +				RPCIF_SMENR_SPIDE(
> +					rpcif_bits_set(op->data.nbytes)) |
> +				RPCIF_SMENR_SPIDB(ilog2(op->data.buswidth));
> +			rpc->xferlen = op->data.nbytes;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL(rpcif_prepare);
> +
> +int rpcif_io_xfer(struct rpcif *rpc, const void *tx_buf, void *rx_buf)
> +{
> +	u32 smenr, smcr, data, pos = 0;
> +	int ret = 0;
> +
> +	pm_runtime_get_sync(rpc->dev);
> +
> +	regmap_update_bits(rpc->regmap, RPCIF_CMNCR, RPCIF_CMNCR_MD,
> +			   RPCIF_CMNCR_MD);
> +	regmap_write(rpc->regmap, RPCIF_SMCMR, rpc->command);
> +	regmap_write(rpc->regmap, RPCIF_SMDMCR, rpc->dummy);
> +	regmap_write(rpc->regmap, RPCIF_SMADR, rpc->address);
> +	regmap_write(rpc->regmap, RPCIF_SMDRENR, rpc->ddr);
> +	smenr = rpc->enable;
> +
> +	if (tx_buf) {
> +		while (pos < rpc->xferlen) {
> +			u32 nbytes = rpc->xferlen - pos;
> +
> +			regmap_write(rpc->regmap, RPCIF_SMWDR0,
> +				     get_unaligned((u32 *)(tx_buf + pos)));
> +
> +			smcr = rpc->smcr | RPCIF_SMCR_SPIE;
> +
> +			if (nbytes > 4) {
> +				nbytes = 4;
> +				smcr |= RPCIF_SMCR_SSLKP;
> +			}
> +
> +			regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
> +			regmap_write(rpc->regmap, RPCIF_SMCR, smcr);
> +			ret = wait_msg_xfer_end(rpc);
> +			if (ret)
> +				goto err_out;
> +
> +			pos += nbytes;
> +			smenr = rpc->enable &
> +				~RPCIF_SMENR_CDE & ~RPCIF_SMENR_ADE(0xf);
> +		}
> +	} else if (rx_buf) {
> +		/*
> +		 * RPC-IF spoils the data for the commands without an address
> +		 * phase (like RDID) in the manual mode, so we'll have to work
> +		 * around this issue by using the external address space read
> +		 * mode instead.
> +		 */
> +		if (!(smenr & RPCIF_SMENR_ADE(0xf)) && rpc->dirmap) {
> +			regmap_update_bits(rpc->regmap, RPCIF_CMNCR,
> +					   RPCIF_CMNCR_MD, 0);
> +			regmap_write(rpc->regmap, RPCIF_DRCR,
> +				     RPCIF_DRCR_RBURST(32) | RPCIF_DRCR_RBE);
> +			regmap_write(rpc->regmap, RPCIF_DREAR,
> +				     RPCIF_DREAR_EAC(1));
> +			regmap_write(rpc->regmap, RPCIF_DRCMR, rpc->command);
> +			regmap_write(rpc->regmap, RPCIF_DRDMCR, rpc->dummy);
> +			regmap_write(rpc->regmap, RPCIF_DROPR, 0);
> +			regmap_write(rpc->regmap, RPCIF_DRENR,
> +				     smenr & ~RPCIF_SMENR_SPIDE(0xf));
> +			memcpy_fromio(rx_buf, rpc->dirmap, rpc->xferlen);
> +			regmap_write(rpc->regmap, RPCIF_DRCR, RPCIF_DRCR_RCF);
> +		} else {
> +			while (pos < rpc->xferlen) {
> +				u32 nbytes = rpc->xferlen - pos;
> +
> +				if (nbytes > 4)
> +					nbytes = 4;
> +
> +				regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
> +				regmap_write(rpc->regmap, RPCIF_SMCR,
> +					     rpc->smcr	| RPCIF_SMCR_SPIE);
> +				ret = wait_msg_xfer_end(rpc);
> +				if (ret)
> +					goto err_out;
> +
> +				regmap_read(rpc->regmap, RPCIF_SMRDR0, &data);
> +				memcpy(rx_buf + pos, &data, nbytes);
> +				pos += nbytes;
> +
> +				regmap_write(rpc->regmap, RPCIF_SMADR,
> +					     rpc->address + pos);
> +			}
> +		}
> +	} else {
> +		regmap_write(rpc->regmap, RPCIF_SMENR, rpc->enable);
> +		regmap_write(rpc->regmap, RPCIF_SMCR,
> +			     rpc->smcr | RPCIF_SMCR_SPIE);
> +		ret = wait_msg_xfer_end(rpc);
> +		if (ret)
> +			goto err_out;
> +	}
> +
> +exit:
> +	pm_runtime_put(rpc->dev);
> +	return ret;
> +
> +err_out:
> +	ret = reset_control_reset(rpc->rstc);
> +	goto exit;
> +}
> +EXPORT_SYMBOL(rpcif_io_xfer);
> +
> +ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf)
> +{
> +	loff_t from = offs & (RPCIF_DIRMAP_SIZE - 1);
> +	size_t size = RPCIF_DIRMAP_SIZE - from;
> +	int rc;
> +
> +	if (len > size)
> +		len = size;
> +
> +	rc = pm_runtime_get_sync(rpc->dev);
> +
> +	regmap_update_bits(rpc->regmap, RPCIF_CMNCR, RPCIF_CMNCR_MD, 0);
> +	regmap_write(rpc->regmap, RPCIF_DRCR,
> +		     RPCIF_DRCR_RBURST(32) | RPCIF_DRCR_RBE);
> +
> +	regmap_write(rpc->regmap, RPCIF_DRCMR, rpc->command);
> +	regmap_write(rpc->regmap, RPCIF_DREAR,
> +		     RPCIF_DREAR_EAV(offs >> 25) | RPCIF_DREAR_EAC(1));
> +	regmap_write(rpc->regmap, RPCIF_DROPR, rpc->option);
> +	regmap_write(rpc->regmap, RPCIF_DRENR,
> +		     rpc->enable & ~RPCIF_SMENR_SPIDE(0xF));
> +	regmap_write(rpc->regmap, RPCIF_DRDMCR, rpc->dummy);
> +	regmap_write(rpc->regmap, RPCIF_DRDRENR, rpc->ddr);
> +
> +	memcpy_fromio(buf, rpc->dirmap + from, len);
> +
> +	pm_runtime_put(rpc->dev);
> +
> +	return len;
> +}
> +EXPORT_SYMBOL(rpcif_dirmap_read);

... to here.

Not sure what all this is, but it looks like it doesn't have anything
to do with MFD.  I suggest you move it to somewhere more appropriate.

> +static const struct mfd_cell rpcif_hf_ctlr = {
> +	.name = "rpcif-hyperflash",
> +};
> +
> +static const struct mfd_cell rpcif_spi_ctlr = {
> +	.name = "rpcif-spi",
> +};

This looks like a very tenuous use of the MFD API.

I suggest that this isn't actually an MFD at all.

> +static const struct regmap_range rpcif_volatile_ranges[] = {
> +	regmap_reg_range(RPCIF_SMRDR0, RPCIF_SMRDR1),
> +	regmap_reg_range(RPCIF_SMWDR0, RPCIF_SMWDR1),
> +	regmap_reg_range(RPCIF_CMNSR, RPCIF_CMNSR),
> +};
> +
> +static const struct regmap_access_table rpcif_volatile_table = {
> +	.yes_ranges	= rpcif_volatile_ranges,
> +	.n_yes_ranges	= ARRAY_SIZE(rpcif_volatile_ranges),
> +};
> +
> +static const struct regmap_config rpcif_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +	.max_register = RPCIF_PHYOFFSET2,
> +	.volatile_table = &rpcif_volatile_table,
> +};
> +
> +static int rpcif_probe(struct platform_device *pdev)
> +{
> +	struct device_node *flash;
> +	const struct mfd_cell *cell;
> +	struct resource *res;
> +	void __iomem *base;
> +	struct rpcif *rpc;
> +
> +	flash = of_get_next_child(pdev->dev.of_node, NULL);
> +	if (!flash) {
> +		dev_warn(&pdev->dev, "no flash node found\n");
> +		return -ENODEV;
> +	}
> +
> +	if (of_device_is_compatible(flash, "jedec,spi-nor")) {
> +		cell = &rpcif_spi_ctlr;
> +	} else if (of_device_is_compatible(flash, "cfi-flash")) {
> +		cell = &rpcif_hf_ctlr;
> +	} else {
> +		dev_warn(&pdev->dev, "unknown flash type\n");
> +		return -ENODEV;
> +	}

Why not let DT choose which device to probe?

> +	rpc = devm_kzalloc(&pdev->dev, sizeof(*rpc), GFP_KERNEL);
> +	if (!rpc)
> +		return -ENOMEM;
> +
> +	rpc->dev = &pdev->dev;
> +
> +	rpc->clk_rpcif = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(rpc->clk_rpcif))
> +		return PTR_ERR(rpc->clk_rpcif);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
> +	base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	rpc->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> +					    &rpcif_regmap_config);
> +	if (IS_ERR(rpc->regmap)) {
> +		dev_err(&pdev->dev,
> +			"failed to init regmap for rpcif, error %ld\n",
> +			PTR_ERR(rpc->regmap));
> +		return	PTR_ERR(rpc->regmap);
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirmap");
> +	rpc->dirmap = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(rpc->dirmap))
> +		rpc->dirmap = NULL;
> +
> +	rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +	if (IS_ERR(rpc->rstc))
> +		return PTR_ERR(rpc->rstc);
> +
> +	platform_set_drvdata(pdev, rpc);
> +
> +	return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0, NULL);
> +}
> +
> +static const struct of_device_id rpcif_of_match[] = {
> +	{ .compatible = "renesas,rcar-gen3-rpcif", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, rpcif_of_match);
> +
> +static struct platform_driver rpcif_driver = {
> +	.probe = rpcif_probe,
> +	.driver = {
> +		.name =	"rpcif",
> +		.of_match_table = rpcif_of_match,
> +	},
> +};
> +module_platform_driver(rpcif_driver);
> +
> +MODULE_DESCRIPTION("Renesas RPC-IF MFD driver");
> +MODULE_LICENSE("GPL v2");
> Index: mfd/include/linux/mfd/rpc-if.h
> ===================================================================
> --- /dev/null
> +++ mfd/include/linux/mfd/rpc-if.h
> @@ -0,0 +1,85 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
> + * Copyright (C) 2019 Macronix International Co., Ltd.
> + *
> + * R-Car Gen3 RPC-IF MFD driver
> + *
> + * Author:
> + *	Mason Yang <[e-mail]>
> + */
> +
> +#ifndef __MFD_RPC_IF_H
> +#define __MFD_RPC_IF_H
> +
> +#include <linux/types.h>
> +
> +enum rpcif_data_dir {
> +	RPCIF_NO_DATA,
> +	RPCIF_DATA_IN,
> +	RPCIF_DATA_OUT,
> +};
> +
> +struct	rpcif_op {
> +	struct {
> +		u8 buswidth;
> +		u8 opcode;
> +		bool ddr;
> +	} cmd, ocmd;
> +
> +	struct {
> +		u8 nbytes;
> +		u8 buswidth;
> +		bool ddr;
> +		u64 val;
> +	} addr;
> +
> +	struct {
> +		u8 nbytes;
> +		u8 buswidth;
> +	} dummy;
> +
> +	struct {
> +		u8 nbytes;
> +		u8 buswidth;
> +		bool ddr;
> +		u32 val;
> +	} option;
> +
> +	struct {
> +		u8 buswidth;
> +		enum rpcif_data_dir dir;
> +		unsigned int nbytes;
> +		bool ddr;
> +		union {
> +			void *in;
> +			const void *out;
> +		} buf;
> +	} data;
> +};
> +
> +struct	rpcif {
> +	struct device *dev;
> +	struct clk *clk_rpcif;
> +	void __iomem *dirmap;
> +	struct regmap *regmap;
> +	struct reset_control *rstc;
> +	u32 enable;
> +	u32 command;
> +	u32 address;
> +	u32 dummy;
> +	u32 option;
> +	u32 ddr;
> +	u32 smcr;
> +	u32 xferlen;
> +};
> +
> +void rpcif_enable_rpm(struct rpcif *rpc);
> +void rpcif_disable_rpm(struct rpcif *rpc);
> +void rpcif_hw_init(struct rpcif *rpc, bool hyperflash);
> +void rpcif_prepare(struct rpcif *rpc, const struct rpcif_op *op, u64 *offs,
> +		   size_t *len);
> +int rpcif_io_xfer(struct rpcif *rpc, const void *tx_buf, void *rx_buf);
> +ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf);
> +
> +#endif // __MFD_RPC_IF_H

Sergei Shtylyov June 14, 2019, 6:49 p.m. UTC | #2

Hello!

On 06/12/2019 12:05 PM, Lee Jones wrote:

>> Add the MFD driver for Renesas RPC-IF which registers either the SPI or
>> HyperFLash  device depending on the contents of the device tree subnode.
>> It also provides the absract "back end" device APIs that can be used by
>> the "front end" SPI/MTD drivers to talk to the real hardware.
>>
>> Based on the original patch by Mason Yang <[e-mail]>.
>>
>> Signed-off-by: Sergei Shtylyov <[e-mail]>

[...]
>> Index: mfd/drivers/mfd/rpc-if.c
>> ===================================================================
>> --- /dev/null
>> +++ mfd/drivers/mfd/rpc-if.c
>> @@ -0,0 +1,535 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Renesas RPC-IF MFD driver
>> + *
>> + * Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
>> + * Copyright (C) 2019 Macronix International Co., Ltd.
>> + * Copyright (C) 2019 Cogent Embedded, Inc.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/rpc-if.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
> 
> Alphabetical.

  I thought they were all sorted them but one #include escaped! :-)

>> +#include <asm/unaligned.h>
>> +
>> +#define RPCIF_CMNCR		0x0000	// R/W
> 
> No C++ comments.

   Well, with the appearance of the SPDX IDs, we thought that //'s will become
acceptable as well -- and indeed Mark Brown has requested consistent C++ comments
following the SPDX line for my SPI "sub-driver"...
   But anyway doesn't seem to be codified yet, so I'll have to fix that, grr... :-(

[...]
>> +#define RPCIF_DIRMAP_SIZE	0x4000000
> 
> Can you shift this lot out to a header file please.

   You mean all register #defne's? Why? I'm not intending to use them outside
this file.

>> +void rpcif_enable_rpm(struct rpcif *rpc)
>> +{
>> +	pm_runtime_enable(rpc->dev);
>> +}
>> +EXPORT_SYMBOL(rpcif_enable_rpm);
>> +
>> +void rpcif_disable_rpm(struct rpcif *rpc)
>> +{
>> +	pm_runtime_disable(rpc->dev);
>> +}
>> +EXPORT_SYMBOL(rpcif_disable_rpm);
> 
> Where are you exporting these out to?

   The "sub-drivers" (SPI/HyperFlash-to-RPC translation drivers).

> Why are you seemingly unnecessarily abstracting out the pm_* API?

   With RPM being otherwise controlled by this driver, I thought that should be
consistent.

>> +void rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
>> +{
>> +	pm_runtime_get_sync(rpc->dev);
>> +
>> +	/*
>> +	 * NOTE: The 0x260 are undocumented bits, but they must be set.
>> +	 *	 RPCIF_PHYCNT_STRTIM is strobe timing adjustment bit,
>> +	 *	 0x0 : the delay is biggest,
>> +	 *	 0x1 : the delay is 2nd biggest,
>> +	 *	 On H3 ES1.x, the value should be 0, while on others,
>> +	 *	 the value should be 6.
>> +	 */
>> +	regmap_write(rpc->regmap, RPCIF_PHYCNT,
>> +		     RPCIF_PHYCNT_CAL | RPCIF_PHYCNT_STRTIM(6) |
>> +		     RPCIF_PHYCNT_PHYMEM(hyperflash ? 3 : 0) | 0x260);
> 
> At least #define it, even it it's not documented.

   Do you have an idea how to name such #define?

>> +	/*
>> +	 * NOTE: The 0x1511144 are undocumented bits, but they must be set
>> +	 *       for RPCIF_PHYOFFSET1.
>> +	 *	 The 0x31 are undocumented bits, but they must be set
>> +	 *	 for RPCIF_PHYOFFSET2.
>> +	 */
>> +	regmap_write(rpc->regmap, RPCIF_PHYOFFSET1,
>> +		     RPCIF_PHYOFFSET1_DDRTMG(3) | 0x1511144);
>> +	regmap_write(rpc->regmap, RPCIF_PHYOFFSET2, 0x31 |
>> +		     RPCIF_PHYOFFSET2_OCTTMG(4));
> 
> No magic numbers.  Please #define them all.

   what about aming?

[...]
>> +static int wait_msg_xfer_end(struct rpcif *rpc)
>> +{
>> +	u32 sts;
>> +
>> +	return regmap_read_poll_timeout(rpc->regmap, RPCIF_CMNSR, sts,
>> +					sts & RPCIF_CMNSR_TEND, 0,
> 
> Aren't you using sts undefined here?

   No, the macro reads 'sts' from the register first.

[...]
> Looking at all this code from here ...
> 
>> +void rpcif_prepare(struct rpcif *rpc, const struct rpcif_op *op, u64 *offs,
>> +		   size_t *len)
>> +{
>> +	rpc->enable = 0;
>> +	rpc->command = 0;
>> +	rpc->xferlen = 0;
>> +	rpc->address = 0;
>> +	rpc->ddr = 0;
>> +
>> +	if (op->cmd.buswidth) {
>> +		rpc->enable  |= RPCIF_SMENR_CDE |
>> +				RPCIF_SMENR_CDB(ilog2(op->cmd.buswidth));
>> +		rpc->command |= RPCIF_SMCMR_CMD(op->cmd.opcode);
>> +		if (op->cmd.ddr)
>> +			rpc->ddr |= RPCIF_SMDRENR_HYPE(0x5);
>> +	}
>> +	if (op->ocmd.buswidth) {
>> +		rpc->enable  |= RPCIF_SMENR_OCDE |
>> +				RPCIF_SMENR_OCDB(ilog2(op->ocmd.buswidth));
>> +		rpc->command |= RPCIF_SMCMR_OCMD(op->ocmd.opcode);
>> +	}
>> +
>> +	if (op->addr.buswidth) {
>> +		rpc->enable |= RPCIF_SMENR_ADB(ilog2(op->addr.buswidth));
>> +		if (op->addr.nbytes == 4)
>> +			rpc->enable |= RPCIF_SMENR_ADE(0xf);
>> +		else
>> +			rpc->enable |= RPCIF_SMENR_ADE(0x7);
>> +		if (op->addr.ddr)
>> +			rpc->ddr |= RPCIF_SMDRENR_ADDRE;
>> +
>> +		if (offs && len)
>> +			rpc->address = *offs;
>> +		else
>> +			rpc->address = op->addr.val;
>> +	}
>> +
>> +	if (op->dummy.buswidth) {
>> +		rpc->enable |= RPCIF_SMENR_DME;
>> +		rpc->dummy   = RPCIF_SMDMCR_DMCYC(op->dummy.nbytes * 8 /
>> +						  op->dummy.buswidth);
>> +	}
>> +
>> +	if (op->option.buswidth) {
>> +		rpc->enable |= RPCIF_SMENR_OPDE(
>> +					rpcif_bits_set(op->option.nbytes)) |
>> +			       RPCIF_SMENR_OPDB(ilog2(op->option.buswidth));
>> +		if (op->option.ddr)
>> +			rpc->ddr |= RPCIF_SMDRENR_OPDRE;
>> +		rpc->option = op->option.val;
>> +	}
>> +
>> +	if (op->data.buswidth || (offs && len)) {
>> +		switch (op->data.dir) {
>> +		case RPCIF_DATA_IN:
>> +			rpc->smcr = RPCIF_SMCR_SPIRE;
>> +			break;
>> +		case RPCIF_DATA_OUT:
>> +			rpc->smcr = RPCIF_SMCR_SPIWE;
>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +		if (op->data.ddr)
>> +			rpc->ddr |= RPCIF_SMDRENR_SPIDRE;
>> +
>> +		if (offs && len) {
>> +			rpc->enable |= RPCIF_SMENR_SPIDE(rpcif_bits_set(*len)) |
>> +				       RPCIF_SMENR_SPIDB(
>> +						ilog2(op->data.buswidth));
>> +			rpc->xferlen = *len;
>> +		} else {
>> +			rpc->enable |=
>> +				RPCIF_SMENR_SPIDE(
>> +					rpcif_bits_set(op->data.nbytes)) |
>> +				RPCIF_SMENR_SPIDB(ilog2(op->data.buswidth));
>> +			rpc->xferlen = op->data.nbytes;
>> +		}
>> +	}
>> +}
>> +EXPORT_SYMBOL(rpcif_prepare);
>> +
>> +int rpcif_io_xfer(struct rpcif *rpc, const void *tx_buf, void *rx_buf)
>> +{
>> +	u32 smenr, smcr, data, pos = 0;
>> +	int ret = 0;
>> +
>> +	pm_runtime_get_sync(rpc->dev);
>> +
>> +	regmap_update_bits(rpc->regmap, RPCIF_CMNCR, RPCIF_CMNCR_MD,
>> +			   RPCIF_CMNCR_MD);
>> +	regmap_write(rpc->regmap, RPCIF_SMCMR, rpc->command);
>> +	regmap_write(rpc->regmap, RPCIF_SMDMCR, rpc->dummy);
>> +	regmap_write(rpc->regmap, RPCIF_SMADR, rpc->address);
>> +	regmap_write(rpc->regmap, RPCIF_SMDRENR, rpc->ddr);
>> +	smenr = rpc->enable;
>> +
>> +	if (tx_buf) {
>> +		while (pos < rpc->xferlen) {
>> +			u32 nbytes = rpc->xferlen - pos;
>> +
>> +			regmap_write(rpc->regmap, RPCIF_SMWDR0,
>> +				     get_unaligned((u32 *)(tx_buf + pos)));
>> +
>> +			smcr = rpc->smcr | RPCIF_SMCR_SPIE;
>> +
>> +			if (nbytes > 4) {
>> +				nbytes = 4;
>> +				smcr |= RPCIF_SMCR_SSLKP;
>> +			}
>> +
>> +			regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
>> +			regmap_write(rpc->regmap, RPCIF_SMCR, smcr);
>> +			ret = wait_msg_xfer_end(rpc);
>> +			if (ret)
>> +				goto err_out;
>> +
>> +			pos += nbytes;
>> +			smenr = rpc->enable &
>> +				~RPCIF_SMENR_CDE & ~RPCIF_SMENR_ADE(0xf);
>> +		}
>> +	} else if (rx_buf) {
>> +		/*
>> +		 * RPC-IF spoils the data for the commands without an address
>> +		 * phase (like RDID) in the manual mode, so we'll have to work
>> +		 * around this issue by using the external address space read
>> +		 * mode instead.
>> +		 */
>> +		if (!(smenr & RPCIF_SMENR_ADE(0xf)) && rpc->dirmap) {
>> +			regmap_update_bits(rpc->regmap, RPCIF_CMNCR,
>> +					   RPCIF_CMNCR_MD, 0);
>> +			regmap_write(rpc->regmap, RPCIF_DRCR,
>> +				     RPCIF_DRCR_RBURST(32) | RPCIF_DRCR_RBE);
>> +			regmap_write(rpc->regmap, RPCIF_DREAR,
>> +				     RPCIF_DREAR_EAC(1));
>> +			regmap_write(rpc->regmap, RPCIF_DRCMR, rpc->command);
>> +			regmap_write(rpc->regmap, RPCIF_DRDMCR, rpc->dummy);
>> +			regmap_write(rpc->regmap, RPCIF_DROPR, 0);
>> +			regmap_write(rpc->regmap, RPCIF_DRENR,
>> +				     smenr & ~RPCIF_SMENR_SPIDE(0xf));
>> +			memcpy_fromio(rx_buf, rpc->dirmap, rpc->xferlen);
>> +			regmap_write(rpc->regmap, RPCIF_DRCR, RPCIF_DRCR_RCF);
>> +		} else {
>> +			while (pos < rpc->xferlen) {
>> +				u32 nbytes = rpc->xferlen - pos;
>> +
>> +				if (nbytes > 4)
>> +					nbytes = 4;
>> +
>> +				regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
>> +				regmap_write(rpc->regmap, RPCIF_SMCR,
>> +					     rpc->smcr	| RPCIF_SMCR_SPIE);
>> +				ret = wait_msg_xfer_end(rpc);
>> +				if (ret)
>> +					goto err_out;
>> +
>> +				regmap_read(rpc->regmap, RPCIF_SMRDR0, &data);
>> +				memcpy(rx_buf + pos, &data, nbytes);
>> +				pos += nbytes;
>> +
>> +				regmap_write(rpc->regmap, RPCIF_SMADR,
>> +					     rpc->address + pos);
>> +			}
>> +		}
>> +	} else {
>> +		regmap_write(rpc->regmap, RPCIF_SMENR, rpc->enable);
>> +		regmap_write(rpc->regmap, RPCIF_SMCR,
>> +			     rpc->smcr | RPCIF_SMCR_SPIE);
>> +		ret = wait_msg_xfer_end(rpc);
>> +		if (ret)
>> +			goto err_out;
>> +	}
>> +
>> +exit:
>> +	pm_runtime_put(rpc->dev);
>> +	return ret;
>> +
>> +err_out:
>> +	ret = reset_control_reset(rpc->rstc);
>> +	goto exit;
>> +}
>> +EXPORT_SYMBOL(rpcif_io_xfer);
>> +
>> +ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf)
>> +{
>> +	loff_t from = offs & (RPCIF_DIRMAP_SIZE - 1);
>> +	size_t size = RPCIF_DIRMAP_SIZE - from;
>> +	int rc;
>> +
>> +	if (len > size)
>> +		len = size;
>> +
>> +	rc = pm_runtime_get_sync(rpc->dev);
>> +
>> +	regmap_update_bits(rpc->regmap, RPCIF_CMNCR, RPCIF_CMNCR_MD, 0);
>> +	regmap_write(rpc->regmap, RPCIF_DRCR,
>> +		     RPCIF_DRCR_RBURST(32) | RPCIF_DRCR_RBE);
>> +
>> +	regmap_write(rpc->regmap, RPCIF_DRCMR, rpc->command);
>> +	regmap_write(rpc->regmap, RPCIF_DREAR,
>> +		     RPCIF_DREAR_EAV(offs >> 25) | RPCIF_DREAR_EAC(1));
>> +	regmap_write(rpc->regmap, RPCIF_DROPR, rpc->option);
>> +	regmap_write(rpc->regmap, RPCIF_DRENR,
>> +		     rpc->enable & ~RPCIF_SMENR_SPIDE(0xF));
>> +	regmap_write(rpc->regmap, RPCIF_DRDMCR, rpc->dummy);
>> +	regmap_write(rpc->regmap, RPCIF_DRDRENR, rpc->ddr);
>> +
>> +	memcpy_fromio(buf, rpc->dirmap + from, len);
>> +
>> +	pm_runtime_put(rpc->dev);
>> +
>> +	return len;
>> +}
>> +EXPORT_SYMBOL(rpcif_dirmap_read);
> 
> ... to here.
> 
> Not sure what all this is, but it looks like it doesn't have anything

   This is an abstracted "back end" API for the "front end" MTD/SPI drivers.
Only this code talks to real RPC-IF hardware. Probably needs some kernel-doc...

> to do with MFD.  I suggest you move it to somewhere more appropriate.

   Like where?

>> +static const struct mfd_cell rpcif_hf_ctlr = {
>> +	.name = "rpcif-hyperflash",
>> +};
>> +
>> +static const struct mfd_cell rpcif_spi_ctlr = {
>> +	.name = "rpcif-spi",
>> +};
> 
> This looks like a very tenuous use of the MFD API.
> 
> I suggest that this isn't actually an MFD at all.

   The same hardware supports 2 different physical interfaces, hence
the drivers have to comply to 2 different driver frameworks... sounded
like MFD to me. :-)

[...]
>> +static int rpcif_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *flash;
>> +	const struct mfd_cell *cell;
>> +	struct resource *res;
>> +	void __iomem *base;
>> +	struct rpcif *rpc;
>> +
>> +	flash = of_get_next_child(pdev->dev.of_node, NULL);
>> +	if (!flash) {
>> +		dev_warn(&pdev->dev, "no flash node found\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (of_device_is_compatible(flash, "jedec,spi-nor")) {
>> +		cell = &rpcif_spi_ctlr;
>> +	} else if (of_device_is_compatible(flash, "cfi-flash")) {
>> +		cell = &rpcif_hf_ctlr;
>> +	} else {
>> +		dev_warn(&pdev->dev, "unknown flash type\n");
>> +		return -ENODEV;
>> +	}
> 
> Why not let DT choose which device to probe?

   It's the DT that decides here. How else would you imagine that?
It's the same hardware, just the different physical busses that it
commands...

[...]

MBR, Sergei

Lee Jones June 17, 2019, 9:30 a.m. UTC | #3

On Fri, 14 Jun 2019, Sergei Shtylyov wrote:
> On 06/12/2019 12:05 PM, Lee Jones wrote:
> 
> >> Add the MFD driver for Renesas RPC-IF which registers either the SPI or
> >> HyperFLash  device depending on the contents of the device tree subnode.
> >> It also provides the absract "back end" device APIs that can be used by
> >> the "front end" SPI/MTD drivers to talk to the real hardware.
> >>
> >> Based on the original patch by Mason Yang <[e-mail]>.
> >>
> >> Signed-off-by: Sergei Shtylyov <[e-mail]>
> 
> [...]
> >> Index: mfd/drivers/mfd/rpc-if.c
> >> ===================================================================
> >> --- /dev/null
> >> +++ mfd/drivers/mfd/rpc-if.c
> >> @@ -0,0 +1,535 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Renesas RPC-IF MFD driver
> >> + *
> >> + * Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
> >> + * Copyright (C) 2019 Macronix International Co., Ltd.
> >> + * Copyright (C) 2019 Cogent Embedded, Inc.
> >> + */
> >> +
> >> +#include <linux/clk.h>
> >> +#include <linux/io.h>
> >> +#include <linux/mfd/core.h>
> >> +#include <linux/mfd/rpc-if.h>
> >> +#include <linux/module.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/of.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/reset.h>
> > 
> > Alphabetical.
> 
>   I thought they were all sorted them but one #include escaped! :-)
> 
> >> +#include <asm/unaligned.h>
> >> +
> >> +#define RPCIF_CMNCR		0x0000	// R/W
> > 
> > No C++ comments.
> 
>    Well, with the appearance of the SPDX IDs, we thought that //'s will become
> acceptable as well -- and indeed Mark Brown has requested consistent C++ comments
> following the SPDX line for my SPI "sub-driver"...
>    But anyway doesn't seem to be codified yet, so I'll have to fix that, grr... :-(

He means in the header comment only.  This does not open the flood
gates for C++ comments throughout the kernel.

> [...]
> >> +#define RPCIF_DIRMAP_SIZE	0x4000000
> > 
> > Can you shift this lot out to a header file please.
> 
>    You mean all register #defne's? Why? I'm not intending to use them outside
> this file.

Because its 10's of lines of cruft.

People won't want to wade through that to get to real functional C
code every time they open up this file.

You already have a header file, please use it.

> >> +void rpcif_enable_rpm(struct rpcif *rpc)
> >> +{
> >> +	pm_runtime_enable(rpc->dev);
> >> +}
> >> +EXPORT_SYMBOL(rpcif_enable_rpm);
> >> +
> >> +void rpcif_disable_rpm(struct rpcif *rpc)
> >> +{
> >> +	pm_runtime_disable(rpc->dev);
> >> +}
> >> +EXPORT_SYMBOL(rpcif_disable_rpm);
> > 
> > Where are you exporting these out to?
> 
>    The "sub-drivers" (SPI/HyperFlash-to-RPC translation drivers).
> 
> > Why are you seemingly unnecessarily abstracting out the pm_* API?
> 
>    With RPM being otherwise controlled by this driver, I thought that should be
> consistent.

Just use the pm_runtime_*() API directly.

> >> +void rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
> >> +{
> >> +	pm_runtime_get_sync(rpc->dev);
> >> +
> >> +	/*
> >> +	 * NOTE: The 0x260 are undocumented bits, but they must be set.
> >> +	 *	 RPCIF_PHYCNT_STRTIM is strobe timing adjustment bit,
> >> +	 *	 0x0 : the delay is biggest,
> >> +	 *	 0x1 : the delay is 2nd biggest,
> >> +	 *	 On H3 ES1.x, the value should be 0, while on others,
> >> +	 *	 the value should be 6.
> >> +	 */
> >> +	regmap_write(rpc->regmap, RPCIF_PHYCNT,
> >> +		     RPCIF_PHYCNT_CAL | RPCIF_PHYCNT_STRTIM(6) |
> >> +		     RPCIF_PHYCNT_PHYMEM(hyperflash ? 3 : 0) | 0x260);
> > 
> > At least #define it, even it it's not documented.
> 
>    Do you have an idea how to name such #define?

How did you find out that they must be set?

How did you find out the value?

What happens if they are not set?

> >> +	/*
> >> +	 * NOTE: The 0x1511144 are undocumented bits, but they must be set
> >> +	 *       for RPCIF_PHYOFFSET1.
> >> +	 *	 The 0x31 are undocumented bits, but they must be set
> >> +	 *	 for RPCIF_PHYOFFSET2.
> >> +	 */
> >> +	regmap_write(rpc->regmap, RPCIF_PHYOFFSET1,
> >> +		     RPCIF_PHYOFFSET1_DDRTMG(3) | 0x1511144);
> >> +	regmap_write(rpc->regmap, RPCIF_PHYOFFSET2, 0x31 |
> >> +		     RPCIF_PHYOFFSET2_OCTTMG(4));
> > 
> > No magic numbers.  Please #define them all.
> 
>    what about aming?

I'd be happy to help, but I need some more information (see above).

> [...]
> >> +static int wait_msg_xfer_end(struct rpcif *rpc)
> >> +{
> >> +	u32 sts;
> >> +
> >> +	return regmap_read_poll_timeout(rpc->regmap, RPCIF_CMNSR, sts,
> >> +					sts & RPCIF_CMNSR_TEND, 0,
> > 
> > Aren't you using sts undefined here?
> 
>    No, the macro reads 'sts' from the register first.

That's confusing and ugly.

Please re-write it to the code is clear and easy to read/maintain.

> [...]
> > Looking at all this code from here ...
> > 
> >> +void rpcif_prepare(struct rpcif *rpc, const struct rpcif_op *op, u64 *offs,
> >> +		   size_t *len)
> >> +{
> >> +	rpc->enable = 0;
> >> +	rpc->command = 0;
> >> +	rpc->xferlen = 0;
> >> +	rpc->address = 0;
> >> +	rpc->ddr = 0;
> >> +
> >> +	if (op->cmd.buswidth) {
> >> +		rpc->enable  |= RPCIF_SMENR_CDE |
> >> +				RPCIF_SMENR_CDB(ilog2(op->cmd.buswidth));
> >> +		rpc->command |= RPCIF_SMCMR_CMD(op->cmd.opcode);
> >> +		if (op->cmd.ddr)
> >> +			rpc->ddr |= RPCIF_SMDRENR_HYPE(0x5);
> >> +	}
> >> +	if (op->ocmd.buswidth) {
> >> +		rpc->enable  |= RPCIF_SMENR_OCDE |
> >> +				RPCIF_SMENR_OCDB(ilog2(op->ocmd.buswidth));
> >> +		rpc->command |= RPCIF_SMCMR_OCMD(op->ocmd.opcode);
> >> +	}
> >> +
> >> +	if (op->addr.buswidth) {
> >> +		rpc->enable |= RPCIF_SMENR_ADB(ilog2(op->addr.buswidth));
> >> +		if (op->addr.nbytes == 4)
> >> +			rpc->enable |= RPCIF_SMENR_ADE(0xf);
> >> +		else
> >> +			rpc->enable |= RPCIF_SMENR_ADE(0x7);
> >> +		if (op->addr.ddr)
> >> +			rpc->ddr |= RPCIF_SMDRENR_ADDRE;
> >> +
> >> +		if (offs && len)
> >> +			rpc->address = *offs;
> >> +		else
> >> +			rpc->address = op->addr.val;
> >> +	}
> >> +
> >> +	if (op->dummy.buswidth) {
> >> +		rpc->enable |= RPCIF_SMENR_DME;
> >> +		rpc->dummy   = RPCIF_SMDMCR_DMCYC(op->dummy.nbytes * 8 /
> >> +						  op->dummy.buswidth);
> >> +	}
> >> +
> >> +	if (op->option.buswidth) {
> >> +		rpc->enable |= RPCIF_SMENR_OPDE(
> >> +					rpcif_bits_set(op->option.nbytes)) |
> >> +			       RPCIF_SMENR_OPDB(ilog2(op->option.buswidth));
> >> +		if (op->option.ddr)
> >> +			rpc->ddr |= RPCIF_SMDRENR_OPDRE;
> >> +		rpc->option = op->option.val;
> >> +	}
> >> +
> >> +	if (op->data.buswidth || (offs && len)) {
> >> +		switch (op->data.dir) {
> >> +		case RPCIF_DATA_IN:
> >> +			rpc->smcr = RPCIF_SMCR_SPIRE;
> >> +			break;
> >> +		case RPCIF_DATA_OUT:
> >> +			rpc->smcr = RPCIF_SMCR_SPIWE;
> >> +			break;
> >> +		default:
> >> +			break;
> >> +		}
> >> +		if (op->data.ddr)
> >> +			rpc->ddr |= RPCIF_SMDRENR_SPIDRE;
> >> +
> >> +		if (offs && len) {
> >> +			rpc->enable |= RPCIF_SMENR_SPIDE(rpcif_bits_set(*len)) |
> >> +				       RPCIF_SMENR_SPIDB(
> >> +						ilog2(op->data.buswidth));
> >> +			rpc->xferlen = *len;
> >> +		} else {
> >> +			rpc->enable |=
> >> +				RPCIF_SMENR_SPIDE(
> >> +					rpcif_bits_set(op->data.nbytes)) |
> >> +				RPCIF_SMENR_SPIDB(ilog2(op->data.buswidth));
> >> +			rpc->xferlen = op->data.nbytes;
> >> +		}
> >> +	}
> >> +}
> >> +EXPORT_SYMBOL(rpcif_prepare);
> >> +
> >> +int rpcif_io_xfer(struct rpcif *rpc, const void *tx_buf, void *rx_buf)
> >> +{
> >> +	u32 smenr, smcr, data, pos = 0;
> >> +	int ret = 0;
> >> +
> >> +	pm_runtime_get_sync(rpc->dev);
> >> +
> >> +	regmap_update_bits(rpc->regmap, RPCIF_CMNCR, RPCIF_CMNCR_MD,
> >> +			   RPCIF_CMNCR_MD);
> >> +	regmap_write(rpc->regmap, RPCIF_SMCMR, rpc->command);
> >> +	regmap_write(rpc->regmap, RPCIF_SMDMCR, rpc->dummy);
> >> +	regmap_write(rpc->regmap, RPCIF_SMADR, rpc->address);
> >> +	regmap_write(rpc->regmap, RPCIF_SMDRENR, rpc->ddr);
> >> +	smenr = rpc->enable;
> >> +
> >> +	if (tx_buf) {
> >> +		while (pos < rpc->xferlen) {
> >> +			u32 nbytes = rpc->xferlen - pos;
> >> +
> >> +			regmap_write(rpc->regmap, RPCIF_SMWDR0,
> >> +				     get_unaligned((u32 *)(tx_buf + pos)));
> >> +
> >> +			smcr = rpc->smcr | RPCIF_SMCR_SPIE;
> >> +
> >> +			if (nbytes > 4) {
> >> +				nbytes = 4;
> >> +				smcr |= RPCIF_SMCR_SSLKP;
> >> +			}
> >> +
> >> +			regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
> >> +			regmap_write(rpc->regmap, RPCIF_SMCR, smcr);
> >> +			ret = wait_msg_xfer_end(rpc);
> >> +			if (ret)
> >> +				goto err_out;
> >> +
> >> +			pos += nbytes;
> >> +			smenr = rpc->enable &
> >> +				~RPCIF_SMENR_CDE & ~RPCIF_SMENR_ADE(0xf);
> >> +		}
> >> +	} else if (rx_buf) {
> >> +		/*
> >> +		 * RPC-IF spoils the data for the commands without an address
> >> +		 * phase (like RDID) in the manual mode, so we'll have to work
> >> +		 * around this issue by using the external address space read
> >> +		 * mode instead.
> >> +		 */
> >> +		if (!(smenr & RPCIF_SMENR_ADE(0xf)) && rpc->dirmap) {
> >> +			regmap_update_bits(rpc->regmap, RPCIF_CMNCR,
> >> +					   RPCIF_CMNCR_MD, 0);
> >> +			regmap_write(rpc->regmap, RPCIF_DRCR,
> >> +				     RPCIF_DRCR_RBURST(32) | RPCIF_DRCR_RBE);
> >> +			regmap_write(rpc->regmap, RPCIF_DREAR,
> >> +				     RPCIF_DREAR_EAC(1));
> >> +			regmap_write(rpc->regmap, RPCIF_DRCMR, rpc->command);
> >> +			regmap_write(rpc->regmap, RPCIF_DRDMCR, rpc->dummy);
> >> +			regmap_write(rpc->regmap, RPCIF_DROPR, 0);
> >> +			regmap_write(rpc->regmap, RPCIF_DRENR,
> >> +				     smenr & ~RPCIF_SMENR_SPIDE(0xf));
> >> +			memcpy_fromio(rx_buf, rpc->dirmap, rpc->xferlen);
> >> +			regmap_write(rpc->regmap, RPCIF_DRCR, RPCIF_DRCR_RCF);
> >> +		} else {
> >> +			while (pos < rpc->xferlen) {
> >> +				u32 nbytes = rpc->xferlen - pos;
> >> +
> >> +				if (nbytes > 4)
> >> +					nbytes = 4;
> >> +
> >> +				regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
> >> +				regmap_write(rpc->regmap, RPCIF_SMCR,
> >> +					     rpc->smcr	| RPCIF_SMCR_SPIE);
> >> +				ret = wait_msg_xfer_end(rpc);
> >> +				if (ret)
> >> +					goto err_out;
> >> +
> >> +				regmap_read(rpc->regmap, RPCIF_SMRDR0, &data);
> >> +				memcpy(rx_buf + pos, &data, nbytes);
> >> +				pos += nbytes;
> >> +
> >> +				regmap_write(rpc->regmap, RPCIF_SMADR,
> >> +					     rpc->address + pos);
> >> +			}
> >> +		}
> >> +	} else {
> >> +		regmap_write(rpc->regmap, RPCIF_SMENR, rpc->enable);
> >> +		regmap_write(rpc->regmap, RPCIF_SMCR,
> >> +			     rpc->smcr | RPCIF_SMCR_SPIE);
> >> +		ret = wait_msg_xfer_end(rpc);
> >> +		if (ret)
> >> +			goto err_out;
> >> +	}
> >> +
> >> +exit:
> >> +	pm_runtime_put(rpc->dev);
> >> +	return ret;
> >> +
> >> +err_out:
> >> +	ret = reset_control_reset(rpc->rstc);
> >> +	goto exit;
> >> +}
> >> +EXPORT_SYMBOL(rpcif_io_xfer);
> >> +
> >> +ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf)
> >> +{
> >> +	loff_t from = offs & (RPCIF_DIRMAP_SIZE - 1);
> >> +	size_t size = RPCIF_DIRMAP_SIZE - from;
> >> +	int rc;
> >> +
> >> +	if (len > size)
> >> +		len = size;
> >> +
> >> +	rc = pm_runtime_get_sync(rpc->dev);
> >> +
> >> +	regmap_update_bits(rpc->regmap, RPCIF_CMNCR, RPCIF_CMNCR_MD, 0);
> >> +	regmap_write(rpc->regmap, RPCIF_DRCR,
> >> +		     RPCIF_DRCR_RBURST(32) | RPCIF_DRCR_RBE);
> >> +
> >> +	regmap_write(rpc->regmap, RPCIF_DRCMR, rpc->command);
> >> +	regmap_write(rpc->regmap, RPCIF_DREAR,
> >> +		     RPCIF_DREAR_EAV(offs >> 25) | RPCIF_DREAR_EAC(1));
> >> +	regmap_write(rpc->regmap, RPCIF_DROPR, rpc->option);
> >> +	regmap_write(rpc->regmap, RPCIF_DRENR,
> >> +		     rpc->enable & ~RPCIF_SMENR_SPIDE(0xF));
> >> +	regmap_write(rpc->regmap, RPCIF_DRDMCR, rpc->dummy);
> >> +	regmap_write(rpc->regmap, RPCIF_DRDRENR, rpc->ddr);
> >> +
> >> +	memcpy_fromio(buf, rpc->dirmap + from, len);
> >> +
> >> +	pm_runtime_put(rpc->dev);
> >> +
> >> +	return len;
> >> +}
> >> +EXPORT_SYMBOL(rpcif_dirmap_read);
> > 
> > ... to here.
> > 
> > Not sure what all this is, but it looks like it doesn't have anything
> 
>    This is an abstracted "back end" API for the "front end" MTD/SPI drivers.
> Only this code talks to real RPC-IF hardware. Probably needs some kernel-doc...

I suggest this needs moving out to a suitable subsystem.

Possibly MTD.

> > to do with MFD.  I suggest you move it to somewhere more appropriate.
> 
>    Like where?

MTD?

> >> +static const struct mfd_cell rpcif_hf_ctlr = {
> >> +	.name = "rpcif-hyperflash",
> >> +};
> >> +
> >> +static const struct mfd_cell rpcif_spi_ctlr = {
> >> +	.name = "rpcif-spi",
> >> +};
> > 
> > This looks like a very tenuous use of the MFD API.
> > 
> > I suggest that this isn't actually an MFD at all.
> 
>    The same hardware supports 2 different physical interfaces, hence
> the drivers have to comply to 2 different driver frameworks... sounded
> like MFD to me. :-)

Not to me.

This appears to be some kind of 'mode selector' for an MTD device.

Lots of drivers have multiple ways to control them - they are not all
MFDs.

> [...]
> >> +static int rpcif_probe(struct platform_device *pdev)
> >> +{
> >> +	struct device_node *flash;
> >> +	const struct mfd_cell *cell;
> >> +	struct resource *res;
> >> +	void __iomem *base;
> >> +	struct rpcif *rpc;
> >> +
> >> +	flash = of_get_next_child(pdev->dev.of_node, NULL);
> >> +	if (!flash) {
> >> +		dev_warn(&pdev->dev, "no flash node found\n");
> >> +		return -ENODEV;
> >> +	}
> >> +
> >> +	if (of_device_is_compatible(flash, "jedec,spi-nor")) {
> >> +		cell = &rpcif_spi_ctlr;
> >> +	} else if (of_device_is_compatible(flash, "cfi-flash")) {
> >> +		cell = &rpcif_hf_ctlr;
> >> +	} else {
> >> +		dev_warn(&pdev->dev, "unknown flash type\n");
> >> +		return -ENODEV;
> >> +	}
> > 
> > Why not let DT choose which device to probe?
> 
>    It's the DT that decides here. How else would you imagine that?
> It's the same hardware, just the different physical busses that it
> commands...

DT is not deciding here.  This driver is deciding based on the
information contained in the tables - very different thing.

Why not just let the OF framework probe the correct device i.e. let it
parse the 2 compatible strings and let it match the correct driver for
the device?

Geert Uytterhoeven June 18, 2019, 8:45 a.m. UTC | #4

Hi Lee,

Thanks for your comments!

On Mon, Jun 17, 2019 at 11:31 AM Lee Jones <[e-mail]> wrote:
> On Fri, 14 Jun 2019, Sergei Shtylyov wrote:
> > On 06/12/2019 12:05 PM, Lee Jones wrote:
> > >> +static const struct mfd_cell rpcif_hf_ctlr = {
> > >> +  .name = "rpcif-hyperflash",
> > >> +};
> > >> +
> > >> +static const struct mfd_cell rpcif_spi_ctlr = {
> > >> +  .name = "rpcif-spi",
> > >> +};
> > >
> > > This looks like a very tenuous use of the MFD API.
> > >
> > > I suggest that this isn't actually an MFD at all.
> >
> >    The same hardware supports 2 different physical interfaces, hence
> > the drivers have to comply to 2 different driver frameworks... sounded
> > like MFD to me. :-)
>
> Not to me.
>
> This appears to be some kind of 'mode selector' for an MTD device.

... for either an SPI or MTD device.

> Lots of drivers have multiple ways to control them - they are not all
> MFDs.

So where to reside the common part? drivers/platform/renesas/?

> > [...]
> > >> +static int rpcif_probe(struct platform_device *pdev)
> > >> +{
> > >> +  struct device_node *flash;
> > >> +  const struct mfd_cell *cell;
> > >> +  struct resource *res;
> > >> +  void __iomem *base;
> > >> +  struct rpcif *rpc;
> > >> +
> > >> +  flash = of_get_next_child(pdev->dev.of_node, NULL);
> > >> +  if (!flash) {
> > >> +          dev_warn(&pdev->dev, "no flash node found\n");
> > >> +          return -ENODEV;
> > >> +  }
> > >> +
> > >> +  if (of_device_is_compatible(flash, "jedec,spi-nor")) {
> > >> +          cell = &rpcif_spi_ctlr;
> > >> +  } else if (of_device_is_compatible(flash, "cfi-flash")) {
> > >> +          cell = &rpcif_hf_ctlr;
> > >> +  } else {
> > >> +          dev_warn(&pdev->dev, "unknown flash type\n");
> > >> +          return -ENODEV;
> > >> +  }
> > >
> > > Why not let DT choose which device to probe?
> >
> >    It's the DT that decides here. How else would you imagine that?
> > It's the same hardware, just the different physical busses that it
> > commands...
>
> DT is not deciding here.  This driver is deciding based on the
> information contained in the tables - very different thing.
>
> Why not just let the OF framework probe the correct device i.e. let it
> parse the 2 compatible strings and let it match the correct driver for
> the device?

The OF framework matches against the RPC-IF node, which is a single
hardware type, hence has a fixed compatible value.
The mode depends on the subnode in DT, which is something the OF
framework doesn't match against, so the driver itself has to check the
subnode's compatible value.
DT describes hardware, not software (Linux subsystem boundary) policy.

I think you could have two drivers (SPI and MFD) each matching against
the same compatible value, with .probe() functions returning -ENODEV
if the subnode doesn't have the appropriate compatible value.
However, (1) I don't know how well that would play with module
autoloading based on of_device_id, and (2) that still leaves the question
where to put the shared code.

Thanks!

Gr{oetje,eeting}s,

                        Geert

Lee Jones June 18, 2019, 9:19 a.m. UTC | #5

On Tue, 18 Jun 2019, Geert Uytterhoeven wrote:
> On Mon, Jun 17, 2019 at 11:31 AM Lee Jones <[e-mail]> wrote:
> > On Fri, 14 Jun 2019, Sergei Shtylyov wrote:
> > > On 06/12/2019 12:05 PM, Lee Jones wrote:
> > > >> +static const struct mfd_cell rpcif_hf_ctlr = {
> > > >> +  .name = "rpcif-hyperflash",
> > > >> +};
> > > >> +
> > > >> +static const struct mfd_cell rpcif_spi_ctlr = {
> > > >> +  .name = "rpcif-spi",
> > > >> +};
> > > >
> > > > This looks like a very tenuous use of the MFD API.
> > > >
> > > > I suggest that this isn't actually an MFD at all.
> > >
> > >    The same hardware supports 2 different physical interfaces, hence
> > > the drivers have to comply to 2 different driver frameworks... sounded
> > > like MFD to me. :-)
> >
> > Not to me.
> >
> > This appears to be some kind of 'mode selector' for an MTD device.
> 
> ... for either an SPI or MTD device.

Okay, so I think I misunderstood the device.  I was under the
impression that it was a flash memory device where the only difference
was the interface by which it is controlled?

> > Lots of drivers have multiple ways to control them - they are not all
> > MFDs.
> 
> So where to reside the common part? drivers/platform/renesas/?

That does not make sense, since this is not a platform controller.

> > > [...]
> > > >> +static int rpcif_probe(struct platform_device *pdev)
> > > >> +{
> > > >> +  struct device_node *flash;
> > > >> +  const struct mfd_cell *cell;
> > > >> +  struct resource *res;
> > > >> +  void __iomem *base;
> > > >> +  struct rpcif *rpc;
> > > >> +
> > > >> +  flash = of_get_next_child(pdev->dev.of_node, NULL);
> > > >> +  if (!flash) {
> > > >> +          dev_warn(&pdev->dev, "no flash node found\n");
> > > >> +          return -ENODEV;
> > > >> +  }
> > > >> +
> > > >> +  if (of_device_is_compatible(flash, "jedec,spi-nor")) {
> > > >> +          cell = &rpcif_spi_ctlr;
> > > >> +  } else if (of_device_is_compatible(flash, "cfi-flash")) {
> > > >> +          cell = &rpcif_hf_ctlr;
> > > >> +  } else {
> > > >> +          dev_warn(&pdev->dev, "unknown flash type\n");
> > > >> +          return -ENODEV;
> > > >> +  }
> > > >
> > > > Why not let DT choose which device to probe?
> > >
> > >    It's the DT that decides here. How else would you imagine that?
> > > It's the same hardware, just the different physical busses that it
> > > commands...
> >
> > DT is not deciding here.  This driver is deciding based on the
> > information contained in the tables - very different thing.
> >
> > Why not just let the OF framework probe the correct device i.e. let it
> > parse the 2 compatible strings and let it match the correct driver for
> > the device?
> 
> The OF framework matches against the RPC-IF node, which is a single
> hardware type, hence has a fixed compatible value.
> The mode depends on the subnode in DT, which is something the OF
> framework doesn't match against, so the driver itself has to check the
> subnode's compatible value.

I can see how it has been implemented.

It is that which I was questioning.

> DT describes hardware, not software (Linux subsystem boundary) policy.

So is an RPC-IF a real hardware device.  Can you share the datasheet?

> I think you could have two drivers (SPI and MFD) each matching against
> the same compatible value, with .probe() functions returning -ENODEV

No, don't do that.

> if the subnode doesn't have the appropriate compatible value.
> However, (1) I don't know how well that would play with module
> autoloading based on of_device_id, and (2) that still leaves the question
> where to put the shared code.

Other than the SPI driver in this set (which I have now looked at),
what else uses the MFD "back-end"?

Geert Uytterhoeven June 18, 2019, 9:33 a.m. UTC | #6

Hi Lee,

On Tue, Jun 18, 2019 at 11:20 AM Lee Jones <[e-mail]> wrote:
> On Tue, 18 Jun 2019, Geert Uytterhoeven wrote:
> > On Mon, Jun 17, 2019 at 11:31 AM Lee Jones <[e-mail]> wrote:
> > > On Fri, 14 Jun 2019, Sergei Shtylyov wrote:
> > > > On 06/12/2019 12:05 PM, Lee Jones wrote:
> > > > >> +static const struct mfd_cell rpcif_hf_ctlr = {
> > > > >> +  .name = "rpcif-hyperflash",
> > > > >> +};
> > > > >> +
> > > > >> +static const struct mfd_cell rpcif_spi_ctlr = {
> > > > >> +  .name = "rpcif-spi",
> > > > >> +};
> > > > >
> > > > > This looks like a very tenuous use of the MFD API.
> > > > >
> > > > > I suggest that this isn't actually an MFD at all.
> > > >
> > > >    The same hardware supports 2 different physical interfaces, hence
> > > > the drivers have to comply to 2 different driver frameworks... sounded
> > > > like MFD to me. :-)
> > >
> > > Not to me.
> > >
> > > This appears to be some kind of 'mode selector' for an MTD device.
> >
> > ... for either an SPI or MTD device.
>
> Okay, so I think I misunderstood the device.  I was under the
> impression that it was a flash memory device where the only difference
> was the interface by which it is controlled?
>
> > > Lots of drivers have multiple ways to control them - they are not all
> > > MFDs.
> >
> > So where to reside the common part? drivers/platform/renesas/?
>
> That does not make sense, since this is not a platform controller.
>
> > > > [...]
> > > > >> +static int rpcif_probe(struct platform_device *pdev)
> > > > >> +{
> > > > >> +  struct device_node *flash;
> > > > >> +  const struct mfd_cell *cell;
> > > > >> +  struct resource *res;
> > > > >> +  void __iomem *base;
> > > > >> +  struct rpcif *rpc;
> > > > >> +
> > > > >> +  flash = of_get_next_child(pdev->dev.of_node, NULL);
> > > > >> +  if (!flash) {
> > > > >> +          dev_warn(&pdev->dev, "no flash node found\n");
> > > > >> +          return -ENODEV;
> > > > >> +  }
> > > > >> +
> > > > >> +  if (of_device_is_compatible(flash, "jedec,spi-nor")) {
> > > > >> +          cell = &rpcif_spi_ctlr;
> > > > >> +  } else if (of_device_is_compatible(flash, "cfi-flash")) {
> > > > >> +          cell = &rpcif_hf_ctlr;
> > > > >> +  } else {
> > > > >> +          dev_warn(&pdev->dev, "unknown flash type\n");
> > > > >> +          return -ENODEV;
> > > > >> +  }
> > > > >
> > > > > Why not let DT choose which device to probe?
> > > >
> > > >    It's the DT that decides here. How else would you imagine that?
> > > > It's the same hardware, just the different physical busses that it
> > > > commands...
> > >
> > > DT is not deciding here.  This driver is deciding based on the
> > > information contained in the tables - very different thing.
> > >
> > > Why not just let the OF framework probe the correct device i.e. let it
> > > parse the 2 compatible strings and let it match the correct driver for
> > > the device?
> >
> > The OF framework matches against the RPC-IF node, which is a single
> > hardware type, hence has a fixed compatible value.
> > The mode depends on the subnode in DT, which is something the OF
> > framework doesn't match against, so the driver itself has to check the
> > subnode's compatible value.
>
> I can see how it has been implemented.
>
> It is that which I was questioning.
>
> > DT describes hardware, not software (Linux subsystem boundary) policy.
>
> So is an RPC-IF a real hardware device.  Can you share the datasheet?

Unfortunately the datasheet for the R-Car Gen3 and RZ/G2 SoCs is
not yet public.

However, a very similar hardware block is present in the RZ/A2M SoC.
Please see Chapter 20 ("SPI Multi I/O Bus Controller") of the "RZ/A2M Group
User’s Manual: Hardware", which you can download from
https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rza/rza2m.html#documents

Thanks!

Gr{oetje,eeting}s,

                        Geert

Lee Jones June 18, 2019, 9:57 a.m. UTC | #7

On Tue, 18 Jun 2019, Geert Uytterhoeven wrote:
> On Tue, Jun 18, 2019 at 11:20 AM Lee Jones <[e-mail]> wrote:
> > On Tue, 18 Jun 2019, Geert Uytterhoeven wrote:
> > > On Mon, Jun 17, 2019 at 11:31 AM Lee Jones <[e-mail]> wrote:
> > > > On Fri, 14 Jun 2019, Sergei Shtylyov wrote:
> > > > > On 06/12/2019 12:05 PM, Lee Jones wrote:
> > > > > >> +static const struct mfd_cell rpcif_hf_ctlr = {
> > > > > >> +  .name = "rpcif-hyperflash",
> > > > > >> +};
> > > > > >> +
> > > > > >> +static const struct mfd_cell rpcif_spi_ctlr = {
> > > > > >> +  .name = "rpcif-spi",
> > > > > >> +};
> > > > > >
> > > > > > This looks like a very tenuous use of the MFD API.
> > > > > >
> > > > > > I suggest that this isn't actually an MFD at all.
> > > > >
> > > > >    The same hardware supports 2 different physical interfaces, hence
> > > > > the drivers have to comply to 2 different driver frameworks... sounded
> > > > > like MFD to me. :-)
> > > >
> > > > Not to me.
> > > >
> > > > This appears to be some kind of 'mode selector' for an MTD device.
> > >
> > > ... for either an SPI or MTD device.
> >
> > Okay, so I think I misunderstood the device.  I was under the
> > impression that it was a flash memory device where the only difference
> > was the interface by which it is controlled?
> >
> > > > Lots of drivers have multiple ways to control them - they are not all
> > > > MFDs.
> > >
> > > So where to reside the common part? drivers/platform/renesas/?
> >
> > That does not make sense, since this is not a platform controller.
> >
> > > > > [...]
> > > > > >> +static int rpcif_probe(struct platform_device *pdev)
> > > > > >> +{
> > > > > >> +  struct device_node *flash;
> > > > > >> +  const struct mfd_cell *cell;
> > > > > >> +  struct resource *res;
> > > > > >> +  void __iomem *base;
> > > > > >> +  struct rpcif *rpc;
> > > > > >> +
> > > > > >> +  flash = of_get_next_child(pdev->dev.of_node, NULL);
> > > > > >> +  if (!flash) {
> > > > > >> +          dev_warn(&pdev->dev, "no flash node found\n");
> > > > > >> +          return -ENODEV;
> > > > > >> +  }
> > > > > >> +
> > > > > >> +  if (of_device_is_compatible(flash, "jedec,spi-nor")) {
> > > > > >> +          cell = &rpcif_spi_ctlr;
> > > > > >> +  } else if (of_device_is_compatible(flash, "cfi-flash")) {
> > > > > >> +          cell = &rpcif_hf_ctlr;
> > > > > >> +  } else {
> > > > > >> +          dev_warn(&pdev->dev, "unknown flash type\n");
> > > > > >> +          return -ENODEV;
> > > > > >> +  }
> > > > > >
> > > > > > Why not let DT choose which device to probe?
> > > > >
> > > > >    It's the DT that decides here. How else would you imagine that?
> > > > > It's the same hardware, just the different physical busses that it
> > > > > commands...
> > > >
> > > > DT is not deciding here.  This driver is deciding based on the
> > > > information contained in the tables - very different thing.
> > > >
> > > > Why not just let the OF framework probe the correct device i.e. let it
> > > > parse the 2 compatible strings and let it match the correct driver for
> > > > the device?
> > >
> > > The OF framework matches against the RPC-IF node, which is a single
> > > hardware type, hence has a fixed compatible value.
> > > The mode depends on the subnode in DT, which is something the OF
> > > framework doesn't match against, so the driver itself has to check the
> > > subnode's compatible value.
> >
> > I can see how it has been implemented.
> >
> > It is that which I was questioning.
> >
> > > DT describes hardware, not software (Linux subsystem boundary) policy.
> >
> > So is an RPC-IF a real hardware device.  Can you share the datasheet?
> 
> Unfortunately the datasheet for the R-Car Gen3 and RZ/G2 SoCs is
> not yet public.

When will it be public?

Do you have access to it?

Maybe someone who does can help with the magic number definitions.

> However, a very similar hardware block is present in the RZ/A2M SoC.
> Please see Chapter 20 ("SPI Multi I/O Bus Controller") of the "RZ/A2M Group
> User’s Manual: Hardware", which you can download from
> https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rza/rza2m.html#documents

  "The SPI multi I/O bus controller enables the direct connection of
   serial flash, OctaFlashTM, XccelaTM flash, or HyperFlashTM memory
   devices to this LSI chip.

   This module allows the connected serial flash, OctaFlashTM, XccelaTM
   flash, or HyperFlashTM memory devices to be accessed by reading the
   external address space, or using Manual mode to transmit and receive
   data."

Looks like a flash device to me.

Can the SPI portion be used to connect generic SPI devices?

Geert Uytterhoeven June 18, 2019, 11:12 a.m. UTC | #8

Hi Lee,

On Tue, Jun 18, 2019 at 11:57 AM Lee Jones <[e-mail]> wrote:
> On Tue, 18 Jun 2019, Geert Uytterhoeven wrote:
> > On Tue, Jun 18, 2019 at 11:20 AM Lee Jones <[e-mail]> wrote:
> > > So is an RPC-IF a real hardware device.  Can you share the datasheet?
> >
> > Unfortunately the datasheet for the R-Car Gen3 and RZ/G2 SoCs is
> > not yet public.
>
> When will it be public?

Dunno. RZ/G1 documentation became public a few months after the SoC
release.

> Do you have access to it?

Yes I do.

> > However, a very similar hardware block is present in the RZ/A2M SoC.
> > Please see Chapter 20 ("SPI Multi I/O Bus Controller") of the "RZ/A2M Group
> > User’s Manual: Hardware", which you can download from
> > https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rza/rza2m.html#documents
>
>   "The SPI multi I/O bus controller enables the direct connection of
>    serial flash, OctaFlashTM, XccelaTM flash, or HyperFlashTM memory
>    devices to this LSI chip.
>
>    This module allows the connected serial flash, OctaFlashTM, XccelaTM
>    flash, or HyperFlashTM memory devices to be accessed by reading the
>    external address space, or using Manual mode to transmit and receive
>    data."
>
> Looks like a flash device to me.

The external address space is a small window.

> Can the SPI portion be used to connect generic SPI devices?

I'll defer that to the people who worked on the driver...

Gr{oetje,eeting}s,

                        Geert

Lee Jones June 18, 2019, 11:34 a.m. UTC | #9

On Tue, 18 Jun 2019, Geert Uytterhoeven wrote:

> Hi Lee,
> 
> On Tue, Jun 18, 2019 at 11:57 AM Lee Jones <[e-mail]> wrote:
> > On Tue, 18 Jun 2019, Geert Uytterhoeven wrote:
> > > On Tue, Jun 18, 2019 at 11:20 AM Lee Jones <[e-mail]> wrote:
> > > > So is an RPC-IF a real hardware device.  Can you share the datasheet?
> > >
> > > Unfortunately the datasheet for the R-Car Gen3 and RZ/G2 SoCs is
> > > not yet public.
> >
> > When will it be public?
> 
> Dunno. RZ/G1 documentation became public a few months after the SoC
> release.
> 
> > Do you have access to it?
> 
> Yes I do.

Great.  Maybe you can help Sergei with his 'undocumented bits' issue.

> > > However, a very similar hardware block is present in the RZ/A2M SoC.
> > > Please see Chapter 20 ("SPI Multi I/O Bus Controller") of the "RZ/A2M Group
> > > User’s Manual: Hardware", which you can download from
> > > https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rza/rza2m.html#documents
> >
> >   "The SPI multi I/O bus controller enables the direct connection of
> >    serial flash, OctaFlashTM, XccelaTM flash, or HyperFlashTM memory
> >    devices to this LSI chip.
> >
> >    This module allows the connected serial flash, OctaFlashTM, XccelaTM
> >    flash, or HyperFlashTM memory devices to be accessed by reading the
> >    external address space, or using Manual mode to transmit and receive
> >    data."
> >
> > Looks like a flash device to me.
> 
> The external address space is a small window.
> 
> > Can the SPI portion be used to connect generic SPI devices?
> 
> I'll defer that to the people who worked on the driver...

...

Sergei Shtylyov June 18, 2019, 6:30 p.m. UTC | #10

Hello!

On 06/18/2019 02:34 PM, Lee Jones wrote:

>>>>> So is an RPC-IF a real hardware device.  Can you share the datasheet?
>>>>
>>>> Unfortunately the datasheet for the R-Car Gen3 and RZ/G2 SoCs is
>>>> not yet public.
>>>
>>> When will it be public?
>>
>> Dunno. RZ/G1 documentation became public a few months after the SoC
>> release.
>>
>>> Do you have access to it?
>>
>> Yes I do.
> 
> Great.  Maybe you can help Sergei with his 'undocumented bits' issue.

   You're an optimist. :-)
   I think I have the same gen3 manual v1.50 as Geert. It won't help as the
bits that constitute the magic numbers in the driver are not described (not
even the default values are listed for the most of them).

>>>> However, a very similar hardware block is present in the RZ/A2M SoC.
>>>> Please see Chapter 20 ("SPI Multi I/O Bus Controller") of the "RZ/A2M Group
>>>> User’s Manual: Hardware", which you can download from
>>>> https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rza/rza2m.html#documents
>>>
>>>   "The SPI multi I/O bus controller enables the direct connection of
>>>    serial flash, OctaFlashTM, XccelaTM flash, or HyperFlashTM memory
>>>    devices to this LSI chip.
>>>
>>>    This module allows the connected serial flash, OctaFlashTM, XccelaTM
>>>    flash, or HyperFlashTM memory devices to be accessed by reading the
>>>    external address space, or using Manual mode to transmit and receive
>>>    data."
>>>
>>> Looks like a flash device to me.
>>
>> The external address space is a small window.

   Yeah, it only provides 64 MiB window into the flash chip.

>>> Can the SPI portion be used to connect generic SPI devices?
>>
>> I'll defer that to the people who worked on the driver...

   Yes. Or at least it should, looking at the driver code...

MBR, Sergei

Sergei Shtylyov June 18, 2019, 7:26 p.m. UTC | #11

On 06/18/2019 12:57 PM, Lee Jones wrote:

[...]
>>>>>>>> +static int rpcif_probe(struct platform_device *pdev)
>>>>>>>> +{
>>>>>>>> +  struct device_node *flash;
>>>>>>>> +  const struct mfd_cell *cell;
>>>>>>>> +  struct resource *res;
>>>>>>>> +  void __iomem *base;
>>>>>>>> +  struct rpcif *rpc;
>>>>>>>> +
>>>>>>>> +  flash = of_get_next_child(pdev->dev.of_node, NULL);
>>>>>>>> +  if (!flash) {
>>>>>>>> +          dev_warn(&pdev->dev, "no flash node found\n");
>>>>>>>> +          return -ENODEV;
>>>>>>>> +  }
>>>>>>>> +
>>>>>>>> +  if (of_device_is_compatible(flash, "jedec,spi-nor")) {
>>>>>>>> +          cell = &rpcif_spi_ctlr;
>>>>>>>> +  } else if (of_device_is_compatible(flash, "cfi-flash")) {
>>>>>>>> +          cell = &rpcif_hf_ctlr;
>>>>>>>> +  } else {
>>>>>>>> +          dev_warn(&pdev->dev, "unknown flash type\n");
>>>>>>>> +          return -ENODEV;
>>>>>>>> +  }
>>>>>>>
>>>>>>> Why not let DT choose which device to probe?
>>>>>>
>>>>>>    It's the DT that decides here. How else would you imagine that?
>>>>>> It's the same hardware, just the different physical busses that it
>>>>>> commands...
>>>>>
>>>>> DT is not deciding here.  This driver is deciding based on the
>>>>> information contained in the tables - very different thing.
>>>>>
>>>>> Why not just let the OF framework probe the correct device i.e. let it
>>>>> parse the 2 compatible strings and let it match the correct driver for
>>>>> the device?
>>>>
>>>> The OF framework matches against the RPC-IF node, which is a single
>>>> hardware type, hence has a fixed compatible value.
>>>> The mode depends on the subnode in DT, which is something the OF
>>>> framework doesn't match against, so the driver itself has to check the
>>>> subnode's compatible value.
>>>
>>> I can see how it has been implemented.
>>>
>>> It is that which I was questioning.
>>>
>>>> DT describes hardware, not software (Linux subsystem boundary) policy.
>>>
>>> So is an RPC-IF a real hardware device.  Can you share the datasheet?
>>
>> Unfortunately the datasheet for the R-Car Gen3 and RZ/G2 SoCs is
>> not yet public.
> 
> When will it be public?

   Ask Renesas. :-)

> Do you have access to it?

   We do...

> Maybe someone who does can help with the magic number definitions.

   Not very likely. :-)

>> However, a very similar hardware block is present in the RZ/A2M SoC.
>> Please see Chapter 20 ("SPI Multi I/O Bus Controller") of the "RZ/A2M Group
>> User’s Manual: Hardware", which you can download from
>> https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rza/rza2m.html#documents

   Also, there's RZ/A1 where a previous version of this hardware seems to be used, see chapter 17
(SPI Multi I/O Bus Controller) of the RZ/A1H/M manual, downloadable from:
https://www.renesas.com/us/en/products/microcontrollers-microprocessors/rz/rza/rza1h.html#documents

>   "The SPI multi I/O bus controller enables the direct connection of
>    serial flash, OctaFlashTM, XccelaTM flash, or HyperFlashTM memory
>    devices to this LSI chip.> 
>    This module allows the connected serial flash, OctaFlashTM, XccelaTM
>    flash, or HyperFlashTM memory devices to be accessed by reading the
>    external address space, or using Manual mode to transmit and receive
>    data."

   For contrast, RZ/A1 didn't yet support OctaFlash and HyperFlash, and R-Car gen3 RPC-IF doesn't
support Xccella yet...

> Looks like a flash device to me.

   More like a multi-protocol flash controller, with the real flash chip connected
to it.

MBR, Sergei

Lee Jones June 19, 2019, 6:16 a.m. UTC | #12

On Tue, 18 Jun 2019, Sergei Shtylyov wrote:

> On 06/18/2019 12:57 PM, Lee Jones wrote:
> 
> [...]
> >>>>>>>> +static int rpcif_probe(struct platform_device *pdev)
> >>>>>>>> +{
> >>>>>>>> +  struct device_node *flash;
> >>>>>>>> +  const struct mfd_cell *cell;
> >>>>>>>> +  struct resource *res;
> >>>>>>>> +  void __iomem *base;
> >>>>>>>> +  struct rpcif *rpc;
> >>>>>>>> +
> >>>>>>>> +  flash = of_get_next_child(pdev->dev.of_node, NULL);
> >>>>>>>> +  if (!flash) {
> >>>>>>>> +          dev_warn(&pdev->dev, "no flash node found\n");
> >>>>>>>> +          return -ENODEV;
> >>>>>>>> +  }
> >>>>>>>> +
> >>>>>>>> +  if (of_device_is_compatible(flash, "jedec,spi-nor")) {
> >>>>>>>> +          cell = &rpcif_spi_ctlr;
> >>>>>>>> +  } else if (of_device_is_compatible(flash, "cfi-flash")) {
> >>>>>>>> +          cell = &rpcif_hf_ctlr;
> >>>>>>>> +  } else {
> >>>>>>>> +          dev_warn(&pdev->dev, "unknown flash type\n");
> >>>>>>>> +          return -ENODEV;
> >>>>>>>> +  }
> >>>>>>>
> >>>>>>> Why not let DT choose which device to probe?
> >>>>>>
> >>>>>>    It's the DT that decides here. How else would you imagine that?
> >>>>>> It's the same hardware, just the different physical busses that it
> >>>>>> commands...
> >>>>>
> >>>>> DT is not deciding here.  This driver is deciding based on the
> >>>>> information contained in the tables - very different thing.
> >>>>>
> >>>>> Why not just let the OF framework probe the correct device i.e. let it
> >>>>> parse the 2 compatible strings and let it match the correct driver for
> >>>>> the device?
> >>>>
> >>>> The OF framework matches against the RPC-IF node, which is a single
> >>>> hardware type, hence has a fixed compatible value.
> >>>> The mode depends on the subnode in DT, which is something the OF
> >>>> framework doesn't match against, so the driver itself has to check the
> >>>> subnode's compatible value.
> >>>
> >>> I can see how it has been implemented.
> >>>
> >>> It is that which I was questioning.
> >>>
> >>>> DT describes hardware, not software (Linux subsystem boundary) policy.
> >>>
> >>> So is an RPC-IF a real hardware device.  Can you share the datasheet?
> >>
> >> Unfortunately the datasheet for the R-Car Gen3 and RZ/G2 SoCs is
> >> not yet public.
> > 
> > When will it be public?
> 
>    Ask Renesas. :-)
> 
> > Do you have access to it?
> 
>    We do...
> 
> > Maybe someone who does can help with the magic number definitions.
> 
>    Not very likely. :-)
> 
> >> However, a very similar hardware block is present in the RZ/A2M SoC.
> >> Please see Chapter 20 ("SPI Multi I/O Bus Controller") of the "RZ/A2M Group
> >> User’s Manual: Hardware", which you can download from
> >> https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rza/rza2m.html#documents
> 
>    Also, there's RZ/A1 where a previous version of this hardware seems to be used, see chapter 17
> (SPI Multi I/O Bus Controller) of the RZ/A1H/M manual, downloadable from:
> https://www.renesas.com/us/en/products/microcontrollers-microprocessors/rz/rza/rza1h.html#documents
> 
> >   "The SPI multi I/O bus controller enables the direct connection of
> >    serial flash, OctaFlashTM, XccelaTM flash, or HyperFlashTM memory
> >    devices to this LSI chip.> 
> >    This module allows the connected serial flash, OctaFlashTM, XccelaTM
> >    flash, or HyperFlashTM memory devices to be accessed by reading the
> >    external address space, or using Manual mode to transmit and receive
> >    data."
> 
>    For contrast, RZ/A1 didn't yet support OctaFlash and HyperFlash, and R-Car gen3 RPC-IF doesn't
> support Xccella yet...
> 
> > Looks like a flash device to me.
> 
>    More like a multi-protocol flash controller, with the real flash chip connected
> to it.

Right, this has been my point from the start.

It's a flash controller.  Sure, you can access it in different ways,
but it's still *just* a flash controller and thus not a true MFD.

Surely this whole thing, including the shared portion should live in
one of the memory related subsystems?

This is not the first device people have tried to shove in MFD, that
is really *just* an <X> device, able to be controlled via different
protocols.

MFD is for registering child devices of chips which offer genuine
cross-subsystem functionality.  It is not designed for mode selecting,
or as a place to shove shared code just because a better location
doesn't appear to exist.

Also, ramming it into drivers/platform/<vendor> is not correct either,
since this is not a platform controller driver either.

Lee Jones June 19, 2019, 6:18 a.m. UTC | #13

On Tue, 18 Jun 2019, Sergei Shtylyov wrote:

> Hello!
> 
> On 06/18/2019 02:34 PM, Lee Jones wrote:
> 
> >>>>> So is an RPC-IF a real hardware device.  Can you share the datasheet?
> >>>>
> >>>> Unfortunately the datasheet for the R-Car Gen3 and RZ/G2 SoCs is
> >>>> not yet public.
> >>>
> >>> When will it be public?
> >>
> >> Dunno. RZ/G1 documentation became public a few months after the SoC
> >> release.
> >>
> >>> Do you have access to it?
> >>
> >> Yes I do.
> > 
> > Great.  Maybe you can help Sergei with his 'undocumented bits' issue.
> 
>    You're an optimist. :-)
>    I think I have the same gen3 manual v1.50 as Geert. It won't help as the
> bits that constitute the magic numbers in the driver are not described (not
> even the default values are listed for the most of them).

Then how did you get hold of them?

> >>>> However, a very similar hardware block is present in the RZ/A2M SoC.
> >>>> Please see Chapter 20 ("SPI Multi I/O Bus Controller") of the "RZ/A2M Group
> >>>> User’s Manual: Hardware", which you can download from
> >>>> https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rza/rza2m.html#documents
> >>>
> >>>   "The SPI multi I/O bus controller enables the direct connection of
> >>>    serial flash, OctaFlashTM, XccelaTM flash, or HyperFlashTM memory
> >>>    devices to this LSI chip.
> >>>
> >>>    This module allows the connected serial flash, OctaFlashTM, XccelaTM
> >>>    flash, or HyperFlashTM memory devices to be accessed by reading the
> >>>    external address space, or using Manual mode to transmit and receive
> >>>    data."
> >>>
> >>> Looks like a flash device to me.
> >>
> >> The external address space is a small window.
> 
>    Yeah, it only provides 64 MiB window into the flash chip.
> 
> >>> Can the SPI portion be used to connect generic SPI devices?
> >>
> >> I'll defer that to the people who worked on the driver...
> 
>    Yes. Or at least it should, looking at the driver code...

Judging by that response, I'm guessing that this is unused/untested.

Mason Yang June 19, 2019, 6:58 a.m. UTC | #14

Hi Jones, 

> > 
> > > Looks like a flash device to me.
> > 
> >    More like a multi-protocol flash controller, with the real flash 
chip connected
> > to it.
> 
> Right, this has been my point from the start.
> 
> It's a flash controller.  Sure, you can access it in different ways,
> but it's still *just* a flash controller and thus not a true MFD.
> 
> Surely this whole thing, including the shared portion should live in
> one of the memory related subsystems?
> 
> This is not the first device people have tried to shove in MFD, that
> is really *just* an <X> device, able to be controlled via different
> protocols.
> 
> MFD is for registering child devices of chips which offer genuine
> cross-subsystem functionality.  It is not designed for mode selecting,
> or as a place to shove shared code just because a better location
> doesn't appear to exist.
> 
> Also, ramming it into drivers/platform/<vendor> is not correct either,
> since this is not a platform controller driver either.


I will patch RPC-IF back to SPI only and 
rebase onto previous patches as bellow:

> "Mark Brown" <[e-mail]> 
> 2019/02/12 下午 10:22
> 
> To
> 
> "Mason Yang" <[e-mail]>, 
> 
> cc
> 
> "Sergei Shtylyov" <[e-mail]>, "Mark Brown" 
> <[e-mail]>, [e-mail], [e-mail], linux-
> [e-mail], [e-mail], 
[e-mail], 
> [e-mail], "Geert Uytterhoeven" <geert
> [e-mail]>, [e-mail], 
[e-mail],
> "Simon Horman" <[e-mail]>, [e-mail], 
[e-mail]
> 
> Subject
> 
> Applied "spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver" to 
the spi tree
> 
> The patch
> 
>    spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver
> 
> has been applied to the spi tree at
> 
>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 
> 
> All being well this means that it will be integrated into the linux-next
> tree (usually sometime in the next 24 hours) and sent to Linus during
> the next merge window (or sooner if it is a bug fix), however if
> problems are discovered then the patch may be dropped or reverted. 

thanks & best regards,
Mason


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

Lee Jones June 19, 2019, 7:54 a.m. UTC | #15

On Wed, 19 Jun 2019, [e-mail] wrote:
> > > > Looks like a flash device to me.
> > > 
> > >    More like a multi-protocol flash controller, with the real flash 
> chip connected
> > > to it.
> > 
> > Right, this has been my point from the start.
> > 
> > It's a flash controller.  Sure, you can access it in different ways,
> > but it's still *just* a flash controller and thus not a true MFD.
> > 
> > Surely this whole thing, including the shared portion should live in
> > one of the memory related subsystems?
> > 
> > This is not the first device people have tried to shove in MFD, that
> > is really *just* an <X> device, able to be controlled via different
> > protocols.
> > 
> > MFD is for registering child devices of chips which offer genuine
> > cross-subsystem functionality.  It is not designed for mode selecting,
> > or as a place to shove shared code just because a better location
> > doesn't appear to exist.
> > 
> > Also, ramming it into drivers/platform/<vendor> is not correct either,
> > since this is not a platform controller driver either.
> 
> 
> I will patch RPC-IF back to SPI only and 
> rebase onto previous patches as bellow:

This sounds more like the easy way out, rather than the right thing to
do.  Just because this isn't an MFD, doesn't mean it's not suitable
for inclusion into the kernel.  Take a look at drivers/memory/Kconfig,
and see if any of those devices sound familiar.

Sergei Shtylyov June 19, 2019, 4:45 p.m. UTC | #16

Hello!

On 06/18/2019 12:19 PM, Lee Jones wrote:

[...]
>>>>>> +static int rpcif_probe(struct platform_device *pdev)
>>>>>> +{
>>>>>> +  struct device_node *flash;
>>>>>> +  const struct mfd_cell *cell;
>>>>>> +  struct resource *res;
>>>>>> +  void __iomem *base;
>>>>>> +  struct rpcif *rpc;
>>>>>> +
>>>>>> +  flash = of_get_next_child(pdev->dev.of_node, NULL);
>>>>>> +  if (!flash) {
>>>>>> +          dev_warn(&pdev->dev, "no flash node found\n");
>>>>>> +          return -ENODEV;
>>>>>> +  }
>>>>>> +
>>>>>> +  if (of_device_is_compatible(flash, "jedec,spi-nor")) {
>>>>>> +          cell = &rpcif_spi_ctlr;
>>>>>> +  } else if (of_device_is_compatible(flash, "cfi-flash")) {
>>>>>> +          cell = &rpcif_hf_ctlr;
>>>>>> +  } else {
>>>>>> +          dev_warn(&pdev->dev, "unknown flash type\n");
>>>>>> +          return -ENODEV;
>>>>>> +  }
>>>>>
>>>>> Why not let DT choose which device to probe?
>>>>
>>>>    It's the DT that decides here. How else would you imagine that?
>>>> It's the same hardware, just the different physical busses that it
>>>> commands...
>>>
>>> DT is not deciding here.  This driver is deciding based on the
>>> information contained in the tables - very different thing.
>>>
>>> Why not just let the OF framework probe the correct device i.e. let it
>>> parse the 2 compatible strings and let it match the correct driver for
>>> the device?

   How do you imagine that? We typically declare SoC devices in the <soc>.dtsi
files so we'd have to override the "compatible" prop in the <board>.dts files?
Or we'd just skip that prop in the <soc>.dtsi and specify it only in a <board>.dts.
Seems quite ugly... 

>> The OF framework matches against the RPC-IF node, which is a single
>> hardware type, hence has a fixed compatible value.
>> The mode depends on the subnode in DT, which is something the OF
>> framework doesn't match against, so the driver itself has to check the
>> subnode's compatible value.
> 
> I can see how it has been implemented.
> 
> It is that which I was questioning.
> 
>> DT describes hardware, not software (Linux subsystem boundary) policy.
> 
> So is an RPC-IF a real hardware device.  Can you share the datasheet?
> 
>> I think you could have two drivers (SPI and MFD) each matching against

   s/MFD/MTD/?

>> the same compatible value, with .probe() functions returning -ENODEV
> 
> No, don't do that.
> 
>> if the subnode doesn't have the appropriate compatible value.
>> However, (1) I don't know how well that would play with module
>> autoloading based on of_device_id, and (2) that still leaves the question
>> where to put the shared code.
> 
> Other than the SPI driver in this set (which I have now looked at),
> what else uses the MFD "back-end"?

   drivers/mtd/hyperbus/. See the (still in-flight) patch set at:

http://lists.infradead.org/pipermail/linux-mtd/2019-June/089873.html

MBR, Sergei

Sergei Shtylyov June 19, 2019, 4:55 p.m. UTC | #17

On 06/19/2019 09:18 AM, Lee Jones wrote:

>>>>>>> So is an RPC-IF a real hardware device.  Can you share the datasheet?
>>>>>>
>>>>>> Unfortunately the datasheet for the R-Car Gen3 and RZ/G2 SoCs is
>>>>>> not yet public.
>>>>>
>>>>> When will it be public?
>>>>
>>>> Dunno. RZ/G1 documentation became public a few months after the SoC
>>>> release.
>>>>
>>>>> Do you have access to it?
>>>>
>>>> Yes I do.
>>>
>>> Great.  Maybe you can help Sergei with his 'undocumented bits' issue.
>>
>>    You're an optimist. :-)
>>    I think I have the same gen3 manual v1.50 as Geert. It won't help as the
>> bits that constitute the magic numbers in the driver are not described (not
>> even the default values are listed for the most of them).
> 
> Then how did you get hold of them?

   Mason did, not me. And they were copied from the bootloader, IIRC.

>>>>>> However, a very similar hardware block is present in the RZ/A2M SoC.
>>>>>> Please see Chapter 20 ("SPI Multi I/O Bus Controller") of the "RZ/A2M Group
>>>>>> User’s Manual: Hardware", which you can download from
>>>>>> https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rza/rza2m.html#documents
>>>>>
>>>>>   "The SPI multi I/O bus controller enables the direct connection of
>>>>>    serial flash, OctaFlashTM, XccelaTM flash, or HyperFlashTM memory
>>>>>    devices to this LSI chip.
>>>>>
>>>>>    This module allows the connected serial flash, OctaFlashTM, XccelaTM
>>>>>    flash, or HyperFlashTM memory devices to be accessed by reading the
>>>>>    external address space, or using Manual mode to transmit and receive
>>>>>    data."
>>>>>
>>>>> Looks like a flash device to me.
>>>>
>>>> The external address space is a small window.
>>
>>    Yeah, it only provides 64 MiB window into the flash chip.
>>
>>>>> Can the SPI portion be used to connect generic SPI devices?
>>>>
>>>> I'll defer that to the people who worked on the driver...
>>
>>    Yes. Or at least it should, looking at the driver code...
> 
> Judging by that response, I'm guessing that this is unused/untested.

   I certainly haven't tested it, perhaps Mason did?

MBR, Sergei

Sergei Shtylyov June 19, 2019, 7:11 p.m. UTC | #18

On 06/19/2019 10:54 AM, Lee Jones wrote:

>>>>> Looks like a flash device to me.
>>>>
>>>>    More like a multi-protocol flash controller, with the real flash 
>>>> chip connected to it.
>>>
>>> Right, this has been my point from the start.
>>>
>>> It's a flash controller.  Sure, you can access it in different ways,

   No, the software access will be the same, just the initial controler
setup will be somewhat different depending on the flash type used...

>>> but it's still *just* a flash controller and thus not a true MFD.

   Also a SPI controller when a SPI bus is used.

>>> Surely this whole thing, including the shared portion should live in
>>> one of the memory related subsystems?
>>>
>>> This is not the first device people have tried to shove in MFD, that
>>> is really *just* an <X> device, able to be controlled via different
>>> protocols.

   You somehow still mix the (master) controller and (slave) device,
it seems...

>>> MFD is for registering child devices of chips which offer genuine
>>> cross-subsystem functionality.  It is not designed for mode selecting,
>>> or as a place to shove shared code just because a better location
>>> doesn't appear to exist.

   OK, fair enough...

>>> Also, ramming it into drivers/platform/<vendor> is not correct either,
>>> since this is not a platform controller driver either.

>> I will patch RPC-IF back to SPI only and 
>> rebase onto previous patches as bellow:
> 
> This sounds more like the easy way out, rather than the right thing to
> do.  Just because this isn't an MFD, doesn't mean it's not suitable
> for inclusion into the kernel.  Take a look at drivers/memory/Kconfig,
> and see if any of those devices sound familiar.

   TI AEMIF sounded familiar, I have some DaVinci/DA8xx background.
Trying to wrap my head into (missing?) API in drivers/memory/...

MBR, Sergei

Sergei Shtylyov June 20, 2019, 6:45 p.m. UTC | #19

On 06/17/2019 12:30 PM, Lee Jones wrote:

>>>> Add the MFD driver for Renesas RPC-IF which registers either the SPI or
>>>> HyperFLash  device depending on the contents of the device tree subnode.
>>>> It also provides the absract "back end" device APIs that can be used by
>>>> the "front end" SPI/MTD drivers to talk to the real hardware.
>>>>
>>>> Based on the original patch by Mason Yang <[e-mail]>.
>>>>
>>>> Signed-off-by: Sergei Shtylyov <[e-mail]>
>>
>> [...]
>>>> Index: mfd/drivers/mfd/rpc-if.c
>>>> ===================================================================
>>>> --- /dev/null
>>>> +++ mfd/drivers/mfd/rpc-if.c
>>>> @@ -0,0 +1,535 @@
[...]
>>>> +#define RPCIF_DIRMAP_SIZE	0x4000000
>>>
>>> Can you shift this lot out to a header file please.
>>
>>    You mean all register #defne's? Why? I'm not intending to use them outside
>> this file.
> 
> Because its 10's of lines of cruft.

   Thank you! :-)

> People won't want to wade through that to get to real functional C
> code every time they open up this file.

   This is how the most drivers are written.

> You already have a header file, please use it.

   Headers are for public things. I've encapsulated the h/w assess into
the MFD driver, the client code doesn't have to see all the gory hardware
details... IOW, I don't agree to this request.

>>>> +void rpcif_enable_rpm(struct rpcif *rpc)
>>>> +{
>>>> +	pm_runtime_enable(rpc->dev);
>>>> +}
>>>> +EXPORT_SYMBOL(rpcif_enable_rpm);
>>>> +
>>>> +void rpcif_disable_rpm(struct rpcif *rpc)
>>>> +{
>>>> +	pm_runtime_disable(rpc->dev);
>>>> +}
>>>> +EXPORT_SYMBOL(rpcif_disable_rpm);
>>>
>>> Where are you exporting these out to?
>>
>>    The "sub-drivers" (SPI/HyperFlash-to-RPC translation drivers).
>>
>>> Why are you seemingly unnecessarily abstracting out the pm_* API?
>>
>>    With RPM being otherwise controlled by this driver, I thought that should be
>> consistent.
> 
> Just use the pm_runtime_*() API directly.

   This would go against the driver encapsulation as well, I would leave it
as is...

>>>> +void rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
>>>> +{
>>>> +	pm_runtime_get_sync(rpc->dev);
>>>> +
>>>> +	/*
>>>> +	 * NOTE: The 0x260 are undocumented bits, but they must be set.
>>>> +	 *	 RPCIF_PHYCNT_STRTIM is strobe timing adjustment bit,
>>>> +	 *	 0x0 : the delay is biggest,
>>>> +	 *	 0x1 : the delay is 2nd biggest,
>>>> +	 *	 On H3 ES1.x, the value should be 0, while on others,
>>>> +	 *	 the value should be 6.
>>>> +	 */
>>>> +	regmap_write(rpc->regmap, RPCIF_PHYCNT,
>>>> +		     RPCIF_PHYCNT_CAL | RPCIF_PHYCNT_STRTIM(6) |
>>>> +		     RPCIF_PHYCNT_PHYMEM(hyperflash ? 3 : 0) | 0x260);
>>>
>>> At least #define it, even it it's not documented.
>>
>>    Do you have an idea how to name such #define?
> 
> How did you find out that they must be set?

   Mason lifted all this "magic" from the bootloader's driver.

> How did you find out the value?

> What happens if they are not set?

   Don't know, maybe Mason does. :-)

[...]
>>>> +static int wait_msg_xfer_end(struct rpcif *rpc)
>>>> +{
>>>> +	u32 sts;
>>>> +
>>>> +	return regmap_read_poll_timeout(rpc->regmap, RPCIF_CMNSR, sts,
>>>> +					sts & RPCIF_CMNSR_TEND, 0,
>>>
>>> Aren't you using sts undefined here?
>>
>>    No, the macro reads 'sts' from the register first.
> 
> That's confusing and ugly.
> 
> Please re-write it to the code is clear and easy to read/maintain.

   OK, I will look into this...

[...]

>>> Looking at all this code from here ...

[...]

>>> ... to here.
>>>
>>> Not sure what all this is, but it looks like it doesn't have anything
>>
>>    This is an abstracted "back end" API for the "front end" MTD/SPI drivers.
>> Only this code talks to real RPC-IF hardware. Probably needs some kernel-doc...
> 
> I suggest this needs moving out to a suitable subsystem.
> 
> Possibly MTD.
> 
>>> to do with MFD.  I suggest you move it to somewhere more appropriate.
>>
>>    Like where?
> 
> MTD?

   Well, the new idea is /drivers/memory/, right?

>>>> +static const struct mfd_cell rpcif_hf_ctlr = {
>>>> +	.name = "rpcif-hyperflash",
>>>> +};
>>>> +
>>>> +static const struct mfd_cell rpcif_spi_ctlr = {
>>>> +	.name = "rpcif-spi",
>>>> +};
>>>
>>> This looks like a very tenuous use of the MFD API.
>>>
>>> I suggest that this isn't actually an MFD at all.
>>
>>    The same hardware supports 2 different physical interfaces, hence
>> the drivers have to comply to 2 different driver frameworks... sounded
>> like MFD to me. :-)
> 
> Not to me.
> 
> This appears to be some kind of 'mode selector' for an MTD device.
> 
> Lots of drivers have multiple ways to control them - they are not all
> MFDs.

   OK, sounds like drivers/mfd/ are for a single device having multiple
distinct subdevices, right?

>> [...]
>>>> +static int rpcif_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct device_node *flash;
>>>> +	const struct mfd_cell *cell;
>>>> +	struct resource *res;
>>>> +	void __iomem *base;
>>>> +	struct rpcif *rpc;
>>>> +
>>>> +	flash = of_get_next_child(pdev->dev.of_node, NULL);
>>>> +	if (!flash) {
>>>> +		dev_warn(&pdev->dev, "no flash node found\n");
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	if (of_device_is_compatible(flash, "jedec,spi-nor")) {
>>>> +		cell = &rpcif_spi_ctlr;
>>>> +	} else if (of_device_is_compatible(flash, "cfi-flash")) {
>>>> +		cell = &rpcif_hf_ctlr;
>>>> +	} else {
>>>> +		dev_warn(&pdev->dev, "unknown flash type\n");
>>>> +		return -ENODEV;
>>>> +	}
>>>
>>> Why not let DT choose which device to probe?
>>
>>    It's the DT that decides here. How else would you imagine that?
>> It's the same hardware, just the different physical busses that it
>> commands...
> 
> DT is not deciding here.  This driver is deciding based on the
> information contained in the tables - very different thing.

   Well, both are done in software. :-)

> Why not just let the OF framework probe the correct device i.e. let it
> parse the 2 compatible strings and let it match the correct driver for
> the device?

   That doesn't go well with the DT spec, I'm afraid. And much code would be
duplicated in case of 2 independent drivers...

MBR, Sergei

Geert Uytterhoeven June 21, 2019, 11:02 a.m. UTC | #20

Hi Sergei, Lee,

On Thu, Jun 20, 2019 at 8:46 PM Sergei Shtylyov
<[e-mail]> wrote:
> On 06/17/2019 12:30 PM, Lee Jones wrote:
> >>>> Add the MFD driver for Renesas RPC-IF which registers either the SPI or
> >>>> HyperFLash  device depending on the contents of the device tree subnode.
> >>>> It also provides the absract "back end" device APIs that can be used by
> >>>> the "front end" SPI/MTD drivers to talk to the real hardware.
> >>>>
> >>>> Based on the original patch by Mason Yang <[e-mail]>.
> >>>>
> >>>> Signed-off-by: Sergei Shtylyov <[e-mail]>
> >>
> >> [...]
> >>>> Index: mfd/drivers/mfd/rpc-if.c
> >>>> ===================================================================
> >>>> --- /dev/null
> >>>> +++ mfd/drivers/mfd/rpc-if.c
> >>>> @@ -0,0 +1,535 @@
> [...]
> >>>> +#define RPCIF_DIRMAP_SIZE 0x4000000
> >>>
> >>> Can you shift this lot out to a header file please.
> >>
> >>    You mean all register #defne's? Why? I'm not intending to use them outside
> >> this file.
> >
> > Because its 10's of lines of cruft.
>
>    Thank you! :-)
>
> > People won't want to wade through that to get to real functional C
> > code every time they open up this file.
>
>    This is how the most drivers are written.
>
> > You already have a header file, please use it.
>
>    Headers are for public things. I've encapsulated the h/w assess into
> the MFD driver, the client code doesn't have to see all the gory hardware
> details... IOW, I don't agree to this request.

+1

> >>>> +static int wait_msg_xfer_end(struct rpcif *rpc)
> >>>> +{
> >>>> +  u32 sts;
> >>>> +
> >>>> +  return regmap_read_poll_timeout(rpc->regmap, RPCIF_CMNSR, sts,
> >>>> +                                  sts & RPCIF_CMNSR_TEND, 0,
> >>>
> >>> Aren't you using sts undefined here?
> >>
> >>    No, the macro reads 'sts' from the register first.
> >
> > That's confusing and ugly.
> >
> > Please re-write it to the code is clear and easy to read/maintain.

How to rewrite?
This is exactly how the various *_poll_timeout*() helpers are intended
to be used.

 * @val: Unsigned integer variable to read the value into

See also include/linux/iopoll.h.

Thanks!

Gr{oetje,eeting}s,

                        Geert

Lee Jones June 24, 2019, 1:20 p.m. UTC | #21

On Fri, 21 Jun 2019, Geert Uytterhoeven wrote:

> Hi Sergei, Lee,
> 
> On Thu, Jun 20, 2019 at 8:46 PM Sergei Shtylyov
> <[e-mail]> wrote:
> > On 06/17/2019 12:30 PM, Lee Jones wrote:
> > >>>> Add the MFD driver for Renesas RPC-IF which registers either the SPI or
> > >>>> HyperFLash  device depending on the contents of the device tree subnode.
> > >>>> It also provides the absract "back end" device APIs that can be used by
> > >>>> the "front end" SPI/MTD drivers to talk to the real hardware.
> > >>>>
> > >>>> Based on the original patch by Mason Yang <[e-mail]>.
> > >>>>
> > >>>> Signed-off-by: Sergei Shtylyov <[e-mail]>
> > >>
> > >> [...]
> > >>>> Index: mfd/drivers/mfd/rpc-if.c
> > >>>> ===================================================================
> > >>>> --- /dev/null
> > >>>> +++ mfd/drivers/mfd/rpc-if.c
> > >>>> @@ -0,0 +1,535 @@
> > [...]
> > >>>> +#define RPCIF_DIRMAP_SIZE 0x4000000
> > >>>
> > >>> Can you shift this lot out to a header file please.
> > >>
> > >>    You mean all register #defne's? Why? I'm not intending to use them outside
> > >> this file.
> > >
> > > Because its 10's of lines of cruft.
> >
> >    Thank you! :-)
> >
> > > People won't want to wade through that to get to real functional C
> > > code every time they open up this file.
> >
> >    This is how the most drivers are written.
> >
> > > You already have a header file, please use it.
> >
> >    Headers are for public things. I've encapsulated the h/w assess into
> > the MFD driver, the client code doesn't have to see all the gory hardware
> > details... IOW, I don't agree to this request.
> 
> +1

Header files aren't only for sharing.  Plenty of source files have
their own headers for storing defines which are of little use to the
reader.

Keeping 125 lines of defines at the top of a source file is pretty
ugly and only border-line unsociable.  If you had many more, I'd be
more insistent.

> > >>>> +static int wait_msg_xfer_end(struct rpcif *rpc)
> > >>>> +{
> > >>>> +  u32 sts;
> > >>>> +
> > >>>> +  return regmap_read_poll_timeout(rpc->regmap, RPCIF_CMNSR, sts,
> > >>>> +                                  sts & RPCIF_CMNSR_TEND, 0,
> > >>>
> > >>> Aren't you using sts undefined here?
> > >>
> > >>    No, the macro reads 'sts' from the register first.
> > >
> > > That's confusing and ugly.
> > >
> > > Please re-write it to the code is clear and easy to read/maintain.
> 
> How to rewrite?
> This is exactly how the various *_poll_timeout*() helpers are intended
> to be used.
> 
>  * @val: Unsigned integer variable to read the value into
> 
> See also include/linux/iopoll.h.

Yuck!  What a horrible way to write a function.

Well if this is how the API is meant to called then I guess it is not
you I am taking exception to.

Mason Yang June 26, 2019, 1:06 a.m. UTC | #22

Hello, 

> Re: [PATCH RFC 2/2] mfd: add Renesas RPC-IF driver
> 
> On 06/19/2019 10:54 AM, Lee Jones wrote:
> 
> >>>>> Looks like a flash device to me.
> >>>>
> >>>>    More like a multi-protocol flash controller, with the real flash 

> >>>> chip connected to it.
> >>>
> >>> Right, this has been my point from the start.
> >>>
> >>> It's a flash controller.  Sure, you can access it in different ways,
> 
>    No, the software access will be the same, just the initial controler
> setup will be somewhat different depending on the flash type used...
> 
> >>> but it's still *just* a flash controller and thus not a true MFD.
> 
>    Also a SPI controller when a SPI bus is used.


Cypress has announced the inclusion of Cypress’ high-bandwidth 
HyperBus™ 8-bit serial memory interface into the new eXpanded SPI (xSPI) 
electrical interface standard from the JEDEC Solid State Technology 
Association 

for detail, please goes to
https://www.cypress.com/news/cypress-hyperbus-memory-interface-instant-applications-incorporated-jedec-xspi-electrical 


As mentioned before that HyperFlash is a kind of standard NOR Flash with 
high-speed SPI interface.

thanks & best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

Patch

Index: mfd/drivers/mfd/Kconfig
===================================================================
--- mfd.orig/drivers/mfd/Kconfig
+++ mfd/drivers/mfd/Kconfig
@@ -1002,6 +1002,16 @@  config MFD_RDC321X
 	  southbridge which provides access to GPIOs and Watchdog using the
 	  southbridge PCI device configuration space.
 
+config MFD_RPCIF
+	tristate "Renesas RPC-IF driver"
+	select MFD_CORE
+	select REGMAP_MMIO
+	depends on ARCH_RENESAS
+	help
+	  This supports Renesas R-Car Gen3 RPC-IF which provides either SPI
+	  host or HyperFlash. You'll have to select individual components
+	  under the corresponding menu.
+
 config MFD_RT5033
 	tristate "Richtek RT5033 Power Management IC"
 	depends on I2C
Index: mfd/drivers/mfd/Makefile
===================================================================
--- mfd.orig/drivers/mfd/Makefile
+++ mfd/drivers/mfd/Makefile
@@ -184,6 +184,7 @@  obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+
 obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
 obj-$(CONFIG_LPC_ICH)		+= lpc_ich.o
 obj-$(CONFIG_MFD_RDC321X)	+= rdc321x-southbridge.o
+obj-$(CONFIG_MFD_RPCIF)		+= rpc-if.o
 obj-$(CONFIG_MFD_JANZ_CMODIO)	+= janz-cmodio.o
 obj-$(CONFIG_MFD_JZ4740_ADC)	+= jz4740-adc.o
 obj-$(CONFIG_MFD_TPS6586X)	+= tps6586x.o
Index: mfd/drivers/mfd/rpc-if.c
===================================================================
--- /dev/null
+++ mfd/drivers/mfd/rpc-if.c
@@ -0,0 +1,535 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RPC-IF MFD driver
+ *
+ * Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
+ * Copyright (C) 2019 Macronix International Co., Ltd.
+ * Copyright (C) 2019 Cogent Embedded, Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/rpc-if.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#include <asm/unaligned.h>
+
+#define RPCIF_CMNCR		0x0000	// R/W
+#define RPCIF_CMNCR_MD		BIT(31)
+#define RPCIF_CMNCR_SFDE	BIT(24) // undocumented bit but must be set
+#define RPCIF_CMNCR_MOIIO3(val)	(((val) & 0x3) << 22)
+#define RPCIF_CMNCR_MOIIO2(val)	(((val) & 0x3) << 20)
+#define RPCIF_CMNCR_MOIIO1(val)	(((val) & 0x3) << 18)
+#define RPCIF_CMNCR_MOIIO0(val)	(((val) & 0x3) << 16)
+#define RPCIF_CMNCR_MOIIO_HIZ	(RPCIF_CMNCR_MOIIO0(3) | \
+				 RPCIF_CMNCR_MOIIO1(3) | \
+				 RPCIF_CMNCR_MOIIO2(3) | RPCIF_CMNCR_MOIIO3(3))
+#define RPCIF_CMNCR_IO3FV(val)	(((val) & 0x3) << 14) // undocumented
+#define RPCIF_CMNCR_IO2FV(val)	(((val) & 0x3) << 12) // undocumented
+#define RPCIF_CMNCR_IO0FV(val)	(((val) & 0x3) << 8)
+#define RPCIF_CMNCR_IOFV_HIZ	(RPCIF_CMNCR_IO0FV(3) | RPCIF_CMNCR_IO2FV(3) | \
+				 RPCIF_CMNCR_IO3FV(3))
+#define RPCIF_CMNCR_BSZ(val)	(((val) & 0x3) << 0)
+
+#define RPCIF_SSLDR		0x0004	// R/W
+#define RPCIF_SSLDR_SPNDL(d)	(((d) & 0x7) << 16)
+#define RPCIF_SSLDR_SLNDL(d)	(((d) & 0x7) << 8)
+#define RPCIF_SSLDR_SCKDL(d)	(((d) & 0x7) << 0)
+
+#define RPCIF_DRCR		0x000C	// R/W
+#define RPCIF_DRCR_SSLN		BIT(24)
+#define RPCIF_DRCR_RBURST(v)	((((v) - 1) & 0x1F) << 16)
+#define RPCIF_DRCR_RCF		BIT(9)
+#define RPCIF_DRCR_RBE		BIT(8)
+#define RPCIF_DRCR_SSLE		BIT(0)
+
+#define RPCIF_DRCMR		0x0010	// R/W
+#define RPCIF_DRCMR_CMD(c)	(((c) & 0xFF) << 16)
+#define RPCIF_DRCMR_OCMD(c)	(((c) & 0xFF) << 0)
+
+#define RPCIF_DREAR		0x0014	// R/W
+#define RPCIF_DREAR_EAV(c)	(((c) & 0xf) << 16)
+#define RPCIF_DREAR_EAC(c)	(((c) & 0x7) << 0)
+
+#define RPCIF_DROPR		0x0018	// R/W
+
+#define RPCIF_DRENR		0x001C	// R/W
+#define RPCIF_DRENR_CDB(o)	(u32)((((o) & 0x3) << 30))
+#define RPCIF_DRENR_OCDB(o)	(((o) & 0x3) << 28)
+#define RPCIF_DRENR_ADB(o)	(((o) & 0x3) << 24)
+#define RPCIF_DRENR_OPDB(o)	(((o) & 0x3) << 20)
+#define RPCIF_DRENR_DRDB(o)	(((o) & 0x3) << 16)
+#define RPCIF_DRENR_DME		BIT(15)
+#define RPCIF_DRENR_CDE		BIT(14)
+#define RPCIF_DRENR_OCDE	BIT(12)
+#define RPCIF_DRENR_ADE(v)	(((v) & 0xF) << 8)
+#define RPCIF_DRENR_OPDE(v)	(((v) & 0xF) << 4)
+
+#define RPCIF_SMCR		0x0020	// R/W
+#define RPCIF_SMCR_SSLKP	BIT(8)
+#define RPCIF_SMCR_SPIRE	BIT(2)
+#define RPCIF_SMCR_SPIWE	BIT(1)
+#define RPCIF_SMCR_SPIE		BIT(0)
+
+#define RPCIF_SMCMR		0x0024	// R/W
+#define RPCIF_SMCMR_CMD(c)	(((c) & 0xFF) << 16)
+#define RPCIF_SMCMR_OCMD(c)	(((c) & 0xFF) << 0)
+
+#define RPCIF_SMADR		0x0028	// R/W
+#define RPCIF_SMOPR		0x002C	// R/W
+#define RPCIF_SMOPR_OPD3(o)	(((o) & 0xFF) << 24)
+#define RPCIF_SMOPR_OPD2(o)	(((o) & 0xFF) << 16)
+#define RPCIF_SMOPR_OPD1(o)	(((o) & 0xFF) << 8)
+#define RPCIF_SMOPR_OPD0(o)	(((o) & 0xFF) << 0)
+
+#define RPCIF_SMENR		0x0030	// R/W
+#define RPCIF_SMENR_CDB(o)	(((o) & 0x3) << 30)
+#define RPCIF_SMENR_OCDB(o)	(((o) & 0x3) << 28)
+#define RPCIF_SMENR_ADB(o)	(((o) & 0x3) << 24)
+#define RPCIF_SMENR_OPDB(o)	(((o) & 0x3) << 20)
+#define RPCIF_SMENR_SPIDB(o)	(((o) & 0x3) << 16)
+#define RPCIF_SMENR_DME		BIT(15)
+#define RPCIF_SMENR_CDE		BIT(14)
+#define RPCIF_SMENR_OCDE	BIT(12)
+#define RPCIF_SMENR_ADE(v)	(((v) & 0xF) << 8)
+#define RPCIF_SMENR_OPDE(v)	(((v) & 0xF) << 4)
+#define RPCIF_SMENR_SPIDE(v)	(((v) & 0xF) << 0)
+
+#define RPCIF_SMRDR0		0x0038	// R
+#define RPCIF_SMRDR1		0x003C	// R
+#define RPCIF_SMWDR0		0x0040	// W
+#define RPCIF_SMWDR1		0x0044	// W
+
+#define RPCIF_CMNSR		0x0048	// R
+#define RPCIF_CMNSR_SSLF	BIT(1)
+#define RPCIF_CMNSR_TEND	BIT(0)
+
+#define RPCIF_DRDMCR		0x0058	// R/W
+#define RPCIF_DMDMCR_DMCYC(v)	((((v) - 1) & 0x1F) << 0)
+
+#define RPCIF_DRDRENR		0x005C	// R/W
+#define RPCIF_DRDRENR_HYPE(v)	(((v) & 0x7) << 12)
+#define RPCIF_DRDRENR_ADDRE	BIT(8)
+#define RPCIF_DRDRENR_OPDRE	BIT(4)
+#define RPCIF_DRDRENR_DRDRE	BIT(0)
+
+#define RPCIF_SMDMCR		0x0060	// R/W
+#define RPCIF_SMDMCR_DMCYC(v)	((((v) - 1) & 0x1F) << 0)
+
+#define RPCIF_SMDRENR		0x0064	// R/W
+#define RPCIF_SMDRENR_HYPE(v)	(((v) & 0x7) << 12)
+#define RPCIF_SMDRENR_ADDRE	BIT(8)
+#define RPCIF_SMDRENR_OPDRE	BIT(4)
+#define RPCIF_SMDRENR_SPIDRE	BIT(0)
+
+#define RPCIF_PHYCNT		0x007C	// R/W
+#define RPCIF_PHYCNT_CAL	BIT(31)
+#define RPCIF_PHYCNT_OCTA_AA	BIT(22)
+#define RPCIF_PHYCNT_OCTA_SA	BIT(23)
+#define RPCIF_PHYCNT_EXDS	BIT(21)
+#define RPCIF_PHYCNT_OCT	BIT(20)
+#define RPCIF_PHYCNT_STRTIM(v)	(((v) & 0x7) << 15)
+#define RPCIF_PHYCNT_WBUF2	BIT(4)
+#define RPCIF_PHYCNT_WBUF	BIT(2)
+#define RPCIF_PHYCNT_PHYMEM(v)	(((v) & 0x3) << 0)
+
+#define RPCIF_PHYOFFSET1	0x0080	// R/W
+#define RPCIF_PHYOFFSET1_DDRTMG(v) (((v) & 0x3) << 28)
+#define RPCIF_PHYOFFSET2	0x0084	// R/W
+#define RPCIF_PHYOFFSET2_OCTTMG(v) (((v) & 0x7) << 8)
+
+#define RPCIF_DIRMAP_SIZE	0x4000000
+
+void rpcif_enable_rpm(struct rpcif *rpc)
+{
+	pm_runtime_enable(rpc->dev);
+}
+EXPORT_SYMBOL(rpcif_enable_rpm);
+
+void rpcif_disable_rpm(struct rpcif *rpc)
+{
+	pm_runtime_disable(rpc->dev);
+}
+EXPORT_SYMBOL(rpcif_disable_rpm);
+
+void rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
+{
+	pm_runtime_get_sync(rpc->dev);
+
+	/*
+	 * NOTE: The 0x260 are undocumented bits, but they must be set.
+	 *	 RPCIF_PHYCNT_STRTIM is strobe timing adjustment bit,
+	 *	 0x0 : the delay is biggest,
+	 *	 0x1 : the delay is 2nd biggest,
+	 *	 On H3 ES1.x, the value should be 0, while on others,
+	 *	 the value should be 6.
+	 */
+	regmap_write(rpc->regmap, RPCIF_PHYCNT,
+		     RPCIF_PHYCNT_CAL | RPCIF_PHYCNT_STRTIM(6) |
+		     RPCIF_PHYCNT_PHYMEM(hyperflash ? 3 : 0) | 0x260);
+
+	/*
+	 * NOTE: The 0x1511144 are undocumented bits, but they must be set
+	 *       for RPCIF_PHYOFFSET1.
+	 *	 The 0x31 are undocumented bits, but they must be set
+	 *	 for RPCIF_PHYOFFSET2.
+	 */
+	regmap_write(rpc->regmap, RPCIF_PHYOFFSET1,
+		     RPCIF_PHYOFFSET1_DDRTMG(3) | 0x1511144);
+	regmap_write(rpc->regmap, RPCIF_PHYOFFSET2, 0x31 |
+		     RPCIF_PHYOFFSET2_OCTTMG(4));
+
+	regmap_write(rpc->regmap, RPCIF_SSLDR, RPCIF_SSLDR_SPNDL(7) |
+		     RPCIF_SSLDR_SLNDL(7) | RPCIF_SSLDR_SCKDL(7));
+	regmap_write(rpc->regmap, RPCIF_CMNCR, RPCIF_CMNCR_MD |
+		     RPCIF_CMNCR_SFDE | RPCIF_CMNCR_MOIIO_HIZ |
+		     RPCIF_CMNCR_IOFV_HIZ |
+		     RPCIF_CMNCR_BSZ(hyperflash ? 1 : 0));
+
+	pm_runtime_put(rpc->dev);
+}
+EXPORT_SYMBOL(rpcif_hw_init);
+
+static int wait_msg_xfer_end(struct rpcif *rpc)
+{
+	u32 sts;
+
+	return regmap_read_poll_timeout(rpc->regmap, RPCIF_CMNSR, sts,
+					sts & RPCIF_CMNSR_TEND, 0,
+					USEC_PER_SEC);
+}
+
+static u8 rpcif_bits_set(u32 nbytes)
+{
+	nbytes = clamp(nbytes, 1U, 4U);
+
+	return GENMASK(3, 4 - nbytes);
+}
+
+void rpcif_prepare(struct rpcif *rpc, const struct rpcif_op *op, u64 *offs,
+		   size_t *len)
+{
+	rpc->enable = 0;
+	rpc->command = 0;
+	rpc->xferlen = 0;
+	rpc->address = 0;
+	rpc->ddr = 0;
+
+	if (op->cmd.buswidth) {
+		rpc->enable  |= RPCIF_SMENR_CDE |
+				RPCIF_SMENR_CDB(ilog2(op->cmd.buswidth));
+		rpc->command |= RPCIF_SMCMR_CMD(op->cmd.opcode);
+		if (op->cmd.ddr)
+			rpc->ddr |= RPCIF_SMDRENR_HYPE(0x5);
+	}
+	if (op->ocmd.buswidth) {
+		rpc->enable  |= RPCIF_SMENR_OCDE |
+				RPCIF_SMENR_OCDB(ilog2(op->ocmd.buswidth));
+		rpc->command |= RPCIF_SMCMR_OCMD(op->ocmd.opcode);
+	}
+
+	if (op->addr.buswidth) {
+		rpc->enable |= RPCIF_SMENR_ADB(ilog2(op->addr.buswidth));
+		if (op->addr.nbytes == 4)
+			rpc->enable |= RPCIF_SMENR_ADE(0xf);
+		else
+			rpc->enable |= RPCIF_SMENR_ADE(0x7);
+		if (op->addr.ddr)
+			rpc->ddr |= RPCIF_SMDRENR_ADDRE;
+
+		if (offs && len)
+			rpc->address = *offs;
+		else
+			rpc->address = op->addr.val;
+	}
+
+	if (op->dummy.buswidth) {
+		rpc->enable |= RPCIF_SMENR_DME;
+		rpc->dummy   = RPCIF_SMDMCR_DMCYC(op->dummy.nbytes * 8 /
+						  op->dummy.buswidth);
+	}
+
+	if (op->option.buswidth) {
+		rpc->enable |= RPCIF_SMENR_OPDE(
+					rpcif_bits_set(op->option.nbytes)) |
+			       RPCIF_SMENR_OPDB(ilog2(op->option.buswidth));
+		if (op->option.ddr)
+			rpc->ddr |= RPCIF_SMDRENR_OPDRE;
+		rpc->option = op->option.val;
+	}
+
+	if (op->data.buswidth || (offs && len)) {
+		switch (op->data.dir) {
+		case RPCIF_DATA_IN:
+			rpc->smcr = RPCIF_SMCR_SPIRE;
+			break;
+		case RPCIF_DATA_OUT:
+			rpc->smcr = RPCIF_SMCR_SPIWE;
+			break;
+		default:
+			break;
+		}
+		if (op->data.ddr)
+			rpc->ddr |= RPCIF_SMDRENR_SPIDRE;
+
+		if (offs && len) {
+			rpc->enable |= RPCIF_SMENR_SPIDE(rpcif_bits_set(*len)) |
+				       RPCIF_SMENR_SPIDB(
+						ilog2(op->data.buswidth));
+			rpc->xferlen = *len;
+		} else {
+			rpc->enable |=
+				RPCIF_SMENR_SPIDE(
+					rpcif_bits_set(op->data.nbytes)) |
+				RPCIF_SMENR_SPIDB(ilog2(op->data.buswidth));
+			rpc->xferlen = op->data.nbytes;
+		}
+	}
+}
+EXPORT_SYMBOL(rpcif_prepare);
+
+int rpcif_io_xfer(struct rpcif *rpc, const void *tx_buf, void *rx_buf)
+{
+	u32 smenr, smcr, data, pos = 0;
+	int ret = 0;
+
+	pm_runtime_get_sync(rpc->dev);
+
+	regmap_update_bits(rpc->regmap, RPCIF_CMNCR, RPCIF_CMNCR_MD,
+			   RPCIF_CMNCR_MD);
+	regmap_write(rpc->regmap, RPCIF_SMCMR, rpc->command);
+	regmap_write(rpc->regmap, RPCIF_SMDMCR, rpc->dummy);
+	regmap_write(rpc->regmap, RPCIF_SMADR, rpc->address);
+	regmap_write(rpc->regmap, RPCIF_SMDRENR, rpc->ddr);
+	smenr = rpc->enable;
+
+	if (tx_buf) {
+		while (pos < rpc->xferlen) {
+			u32 nbytes = rpc->xferlen - pos;
+
+			regmap_write(rpc->regmap, RPCIF_SMWDR0,
+				     get_unaligned((u32 *)(tx_buf + pos)));
+
+			smcr = rpc->smcr | RPCIF_SMCR_SPIE;
+
+			if (nbytes > 4) {
+				nbytes = 4;
+				smcr |= RPCIF_SMCR_SSLKP;
+			}
+
+			regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
+			regmap_write(rpc->regmap, RPCIF_SMCR, smcr);
+			ret = wait_msg_xfer_end(rpc);
+			if (ret)
+				goto err_out;
+
+			pos += nbytes;
+			smenr = rpc->enable &
+				~RPCIF_SMENR_CDE & ~RPCIF_SMENR_ADE(0xf);
+		}
+	} else if (rx_buf) {
+		/*
+		 * RPC-IF spoils the data for the commands without an address
+		 * phase (like RDID) in the manual mode, so we'll have to work
+		 * around this issue by using the external address space read
+		 * mode instead.
+		 */
+		if (!(smenr & RPCIF_SMENR_ADE(0xf)) && rpc->dirmap) {
+			regmap_update_bits(rpc->regmap, RPCIF_CMNCR,
+					   RPCIF_CMNCR_MD, 0);
+			regmap_write(rpc->regmap, RPCIF_DRCR,
+				     RPCIF_DRCR_RBURST(32) | RPCIF_DRCR_RBE);
+			regmap_write(rpc->regmap, RPCIF_DREAR,
+				     RPCIF_DREAR_EAC(1));
+			regmap_write(rpc->regmap, RPCIF_DRCMR, rpc->command);
+			regmap_write(rpc->regmap, RPCIF_DRDMCR, rpc->dummy);
+			regmap_write(rpc->regmap, RPCIF_DROPR, 0);
+			regmap_write(rpc->regmap, RPCIF_DRENR,
+				     smenr & ~RPCIF_SMENR_SPIDE(0xf));
+			memcpy_fromio(rx_buf, rpc->dirmap, rpc->xferlen);
+			regmap_write(rpc->regmap, RPCIF_DRCR, RPCIF_DRCR_RCF);
+		} else {
+			while (pos < rpc->xferlen) {
+				u32 nbytes = rpc->xferlen - pos;
+
+				if (nbytes > 4)
+					nbytes = 4;
+
+				regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
+				regmap_write(rpc->regmap, RPCIF_SMCR,
+					     rpc->smcr	| RPCIF_SMCR_SPIE);
+				ret = wait_msg_xfer_end(rpc);
+				if (ret)
+					goto err_out;
+
+				regmap_read(rpc->regmap, RPCIF_SMRDR0, &data);
+				memcpy(rx_buf + pos, &data, nbytes);
+				pos += nbytes;
+
+				regmap_write(rpc->regmap, RPCIF_SMADR,
+					     rpc->address + pos);
+			}
+		}
+	} else {
+		regmap_write(rpc->regmap, RPCIF_SMENR, rpc->enable);
+		regmap_write(rpc->regmap, RPCIF_SMCR,
+			     rpc->smcr | RPCIF_SMCR_SPIE);
+		ret = wait_msg_xfer_end(rpc);
+		if (ret)
+			goto err_out;
+	}
+
+exit:
+	pm_runtime_put(rpc->dev);
+	return ret;
+
+err_out:
+	ret = reset_control_reset(rpc->rstc);
+	goto exit;
+}
+EXPORT_SYMBOL(rpcif_io_xfer);
+
+ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf)
+{
+	loff_t from = offs & (RPCIF_DIRMAP_SIZE - 1);
+	size_t size = RPCIF_DIRMAP_SIZE - from;
+	int rc;
+
+	if (len > size)
+		len = size;
+
+	rc = pm_runtime_get_sync(rpc->dev);
+
+	regmap_update_bits(rpc->regmap, RPCIF_CMNCR, RPCIF_CMNCR_MD, 0);
+	regmap_write(rpc->regmap, RPCIF_DRCR,
+		     RPCIF_DRCR_RBURST(32) | RPCIF_DRCR_RBE);
+
+	regmap_write(rpc->regmap, RPCIF_DRCMR, rpc->command);
+	regmap_write(rpc->regmap, RPCIF_DREAR,
+		     RPCIF_DREAR_EAV(offs >> 25) | RPCIF_DREAR_EAC(1));
+	regmap_write(rpc->regmap, RPCIF_DROPR, rpc->option);
+	regmap_write(rpc->regmap, RPCIF_DRENR,
+		     rpc->enable & ~RPCIF_SMENR_SPIDE(0xF));
+	regmap_write(rpc->regmap, RPCIF_DRDMCR, rpc->dummy);
+	regmap_write(rpc->regmap, RPCIF_DRDRENR, rpc->ddr);
+
+	memcpy_fromio(buf, rpc->dirmap + from, len);
+
+	pm_runtime_put(rpc->dev);
+
+	return len;
+}
+EXPORT_SYMBOL(rpcif_dirmap_read);
+
+static const struct mfd_cell rpcif_hf_ctlr = {
+	.name = "rpcif-hyperflash",
+};
+
+static const struct mfd_cell rpcif_spi_ctlr = {
+	.name = "rpcif-spi",
+};
+
+static const struct regmap_range rpcif_volatile_ranges[] = {
+	regmap_reg_range(RPCIF_SMRDR0, RPCIF_SMRDR1),
+	regmap_reg_range(RPCIF_SMWDR0, RPCIF_SMWDR1),
+	regmap_reg_range(RPCIF_CMNSR, RPCIF_CMNSR),
+};
+
+static const struct regmap_access_table rpcif_volatile_table = {
+	.yes_ranges	= rpcif_volatile_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(rpcif_volatile_ranges),
+};
+
+static const struct regmap_config rpcif_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+	.max_register = RPCIF_PHYOFFSET2,
+	.volatile_table = &rpcif_volatile_table,
+};
+
+static int rpcif_probe(struct platform_device *pdev)
+{
+	struct device_node *flash;
+	const struct mfd_cell *cell;
+	struct resource *res;
+	void __iomem *base;
+	struct rpcif *rpc;
+
+	flash = of_get_next_child(pdev->dev.of_node, NULL);
+	if (!flash) {
+		dev_warn(&pdev->dev, "no flash node found\n");
+		return -ENODEV;
+	}
+
+	if (of_device_is_compatible(flash, "jedec,spi-nor")) {
+		cell = &rpcif_spi_ctlr;
+	} else if (of_device_is_compatible(flash, "cfi-flash")) {
+		cell = &rpcif_hf_ctlr;
+	} else {
+		dev_warn(&pdev->dev, "unknown flash type\n");
+		return -ENODEV;
+	}
+
+	rpc = devm_kzalloc(&pdev->dev, sizeof(*rpc), GFP_KERNEL);
+	if (!rpc)
+		return -ENOMEM;
+
+	rpc->dev = &pdev->dev;
+
+	rpc->clk_rpcif = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(rpc->clk_rpcif))
+		return PTR_ERR(rpc->clk_rpcif);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	rpc->regmap = devm_regmap_init_mmio(&pdev->dev, base,
+					    &rpcif_regmap_config);
+	if (IS_ERR(rpc->regmap)) {
+		dev_err(&pdev->dev,
+			"failed to init regmap for rpcif, error %ld\n",
+			PTR_ERR(rpc->regmap));
+		return	PTR_ERR(rpc->regmap);
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirmap");
+	rpc->dirmap = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(rpc->dirmap))
+		rpc->dirmap = NULL;
+
+	rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+	if (IS_ERR(rpc->rstc))
+		return PTR_ERR(rpc->rstc);
+
+	platform_set_drvdata(pdev, rpc);
+
+	return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0, NULL);
+}
+
+static const struct of_device_id rpcif_of_match[] = {
+	{ .compatible = "renesas,rcar-gen3-rpcif", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rpcif_of_match);
+
+static struct platform_driver rpcif_driver = {
+	.probe = rpcif_probe,
+	.driver = {
+		.name =	"rpcif",
+		.of_match_table = rpcif_of_match,
+	},
+};
+module_platform_driver(rpcif_driver);
+
+MODULE_DESCRIPTION("Renesas RPC-IF MFD driver");
+MODULE_LICENSE("GPL v2");
Index: mfd/include/linux/mfd/rpc-if.h
===================================================================
--- /dev/null
+++ mfd/include/linux/mfd/rpc-if.h
@@ -0,0 +1,85 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
+ * Copyright (C) 2019 Macronix International Co., Ltd.
+ *
+ * R-Car Gen3 RPC-IF MFD driver
+ *
+ * Author:
+ *	Mason Yang <[e-mail]>
+ */
+
+#ifndef __MFD_RPC_IF_H
+#define __MFD_RPC_IF_H
+
+#include <linux/types.h>
+
+enum rpcif_data_dir {
+	RPCIF_NO_DATA,
+	RPCIF_DATA_IN,
+	RPCIF_DATA_OUT,
+};
+
+struct	rpcif_op {
+	struct {
+		u8 buswidth;
+		u8 opcode;
+		bool ddr;
+	} cmd, ocmd;
+
+	struct {
+		u8 nbytes;
+		u8 buswidth;
+		bool ddr;
+		u64 val;
+	} addr;
+
+	struct {
+		u8 nbytes;
+		u8 buswidth;
+	} dummy;
+
+	struct {
+		u8 nbytes;
+		u8 buswidth;
+		bool ddr;
+		u32 val;
+	} option;
+
+	struct {
+		u8 buswidth;
+		enum rpcif_data_dir dir;
+		unsigned int nbytes;
+		bool ddr;
+		union {
+			void *in;
+			const void *out;
+		} buf;
+	} data;
+};
+
+struct	rpcif {
+	struct device *dev;
+	struct clk *clk_rpcif;
+	void __iomem *dirmap;
+	struct regmap *regmap;
+	struct reset_control *rstc;
+	u32 enable;
+	u32 command;
+	u32 address;
+	u32 dummy;
+	u32 option;
+	u32 ddr;
+	u32 smcr;
+	u32 xferlen;
+};
+
+void rpcif_enable_rpm(struct rpcif *rpc);
+void rpcif_disable_rpm(struct rpcif *rpc);
+void rpcif_hw_init(struct rpcif *rpc, bool hyperflash);
+void rpcif_prepare(struct rpcif *rpc, const struct rpcif_op *op, u64 *offs,
+		   size_t *len);
+int rpcif_io_xfer(struct rpcif *rpc, const void *tx_buf, void *rx_buf);
+ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf);
+
+#endif // __MFD_RPC_IF_H