Re: [PATCH v2 7/7] memory: renesas-rpc-if: Add support for RZ/G2L

From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
To: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	Rob Herring <robh+dt@kernel.org>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Richard Weinberger <richard@nod.at>,
	Mark Brown <broonie@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Wolfram Sang <wsa+renesas@sang-engineering.com>,
	Sergei Shtylyov <sergei.shtylyov@gmail.com>
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org,
	Prabhakar <prabhakar.csengg@gmail.com>,
	Biju Das <biju.das.jz@bp.renesas.com>
Subject: Re: [PATCH v2 7/7] memory: renesas-rpc-if: Add support for RZ/G2L
Date: Tue, 26 Oct 2021 16:46:58 +0200	[thread overview]
Message-ID: <3739744b-0a10-6d6b-8d1c-9c82b6afe0b3@canonical.com> (raw)
In-Reply-To: <20211025205631.21151-8-prabhakar.mahadev-lad.rj@bp.renesas.com>

On 25/10/2021 22:56, Lad Prabhakar wrote:
> SPI Multi I/O Bus Controller on RZ/G2L SoC is almost identical to
> the RPC-IF interface found on R-Car Gen3 SoC's.
> 
> This patch adds a new compatible string for the RZ/G2L family so
> that the timing values on RZ/G2L can be adjusted.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v1->v2:
>  * Updated macros as suggested by Wolfram
> ---
>  drivers/memory/renesas-rpc-if.c | 72 ++++++++++++++++++++++++++++-----
>  drivers/mtd/hyperbus/rpc-if.c   |  4 +-
>  drivers/spi/spi-rpc-if.c        |  4 +-
>  include/memory/renesas-rpc-if.h |  8 +++-
>  4 files changed, 75 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c
> index 0c56decc91f2..8c51145c0f5c 100644
> --- a/drivers/memory/renesas-rpc-if.c
> +++ b/drivers/memory/renesas-rpc-if.c
> @@ -12,6 +12,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
>  
> @@ -27,8 +28,8 @@
>  #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_IO3FV(val)	(((val) & 0x3) << 14) /* documented for RZ/G2L */
> +#define RPCIF_CMNCR_IO2FV(val)	(((val) & 0x3) << 12) /* documented for RZ/G2L */
>  #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))
> @@ -126,6 +127,9 @@
>  #define RPCIF_SMDRENR_OPDRE	BIT(4)
>  #define RPCIF_SMDRENR_SPIDRE	BIT(0)
>  
> +#define RPCIF_PHYADD		0x0070	/* R/W available on R-Car E3/D3/V3M and RZ/G2{E,L} */
> +#define RPCIF_PHYWR		0x0074	/* R/W available on R-Car E3/D3/V3M and RZ/G2{E,L} */
> +
>  #define RPCIF_PHYCNT		0x007C	/* R/W */
>  #define RPCIF_PHYCNT_CAL	BIT(31)
>  #define RPCIF_PHYCNT_OCTA(v)	(((v) & 0x3) << 22)
> @@ -133,10 +137,12 @@
>  #define RPCIF_PHYCNT_OCT	BIT(20)
>  #define RPCIF_PHYCNT_DDRCAL	BIT(19)
>  #define RPCIF_PHYCNT_HS		BIT(18)
> -#define RPCIF_PHYCNT_STRTIM(v)	(((v) & 0x7) << 15)
> +#define RPCIF_PHYCNT_CKSEL(v)	(((v) & 0x3) << 16) /* valid only for RZ/G2L */
> +#define RPCIF_PHYCNT_STRTIM(v)	(((v) & 0x7) << 15) /* valid for R-Car and RZ/G2{E,H,M,N} */
>  #define RPCIF_PHYCNT_WBUF2	BIT(4)
>  #define RPCIF_PHYCNT_WBUF	BIT(2)
>  #define RPCIF_PHYCNT_PHYMEM(v)	(((v) & 0x3) << 0)
> +#define RPCIF_PHYCNT_PHYMEM_MASK GENMASK(1, 0)
>  
>  #define RPCIF_PHYOFFSET1	0x0080	/* R/W */
>  #define RPCIF_PHYOFFSET1_DDRTMG(v) (((v) & 0x3) << 28)
> @@ -244,18 +250,46 @@ int rpcif_sw_init(struct rpcif *rpc, struct device *dev)
>  		return PTR_ERR(rpc->dirmap);
>  	rpc->size = resource_size(res);
>  
> +	rpc->type = (enum rpcif_type)of_device_get_match_data(dev);
>  	rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>  
>  	return PTR_ERR_OR_ZERO(rpc->rstc);
>  }
>  EXPORT_SYMBOL(rpcif_sw_init);
>  
> -void rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
> +static void rpcif_rzg2l_timing_adjust_sdr(struct rpcif *rpc)
> +{
> +	u32 data;
> +
> +	regmap_write(rpc->regmap, RPCIF_PHYWR, 0xa5390000);
> +	regmap_write(rpc->regmap, RPCIF_PHYADD, 0x80000000);
> +	regmap_write(rpc->regmap, RPCIF_PHYWR, 0x00008080);
> +	regmap_write(rpc->regmap, RPCIF_PHYADD, 0x80000022);
> +	regmap_write(rpc->regmap, RPCIF_PHYWR, 0x00008080);
> +	regmap_write(rpc->regmap, RPCIF_PHYADD, 0x80000024);
> +
> +	regmap_read(rpc->regmap, RPCIF_PHYCNT, &data);
> +	regmap_write(rpc->regmap, RPCIF_PHYCNT, data | RPCIF_PHYCNT_CKSEL(3));
> +	regmap_write(rpc->regmap, RPCIF_PHYWR, 0x00000030);
> +	regmap_write(rpc->regmap, RPCIF_PHYADD, 0x80000032);
> +}
> +
> +int rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
>  {
>  	u32 dummy;
>  
>  	pm_runtime_get_sync(rpc->dev);
>  
> +	if (rpc->type == RPCIF_RZ_G2L) {
> +		int ret;
> +
> +		ret = reset_control_reset(rpc->rstc);
> +		if (ret)
> +			return ret;
> +		usleep_range(200, 300);
> +		rpcif_rzg2l_timing_adjust_sdr(rpc);
> +	}
> +
>  	/*
>  	 * NOTE: The 0x260 are undocumented bits, but they must be set.
>  	 *	 RPCIF_PHYCNT_STRTIM is strobe timing adjustment bits,
> @@ -264,8 +298,15 @@ void rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
>  	 *	 On H3 ES1.x, the value should be 0, while on others,
>  	 *	 the value should be 7.
>  	 */
> -	regmap_write(rpc->regmap, RPCIF_PHYCNT, RPCIF_PHYCNT_STRTIM(7) |
> -		     RPCIF_PHYCNT_PHYMEM(hyperflash ? 3 : 0) | 0x260);
> +	if (rpc->type == RPCIF_RCAR_GEN3) {
> +		regmap_write(rpc->regmap, RPCIF_PHYCNT, RPCIF_PHYCNT_STRTIM(7) |
> +			     RPCIF_PHYCNT_PHYMEM(hyperflash ? 3 : 0) | 0x260);
> +	} else {
> +		regmap_read(rpc->regmap, RPCIF_PHYCNT, &dummy);
> +		dummy &= ~RPCIF_PHYCNT_PHYMEM_MASK;
> +		dummy |= RPCIF_PHYCNT_PHYMEM(hyperflash ? 3 : 0) | 0x260;
> +		regmap_write(rpc->regmap, RPCIF_PHYCNT, dummy);
> +	}
>  
>  	/*
>  	 * NOTE: The 0x1511144 are undocumented bits, but they must be set
> @@ -282,9 +323,17 @@ void rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
>  		regmap_update_bits(rpc->regmap, RPCIF_PHYINT,
>  				   RPCIF_PHYINT_WPVAL, 0);
>  
> -	regmap_write(rpc->regmap, RPCIF_CMNCR, RPCIF_CMNCR_SFDE |
> -		     RPCIF_CMNCR_MOIIO_HIZ | RPCIF_CMNCR_IOFV_HIZ |
> -		     RPCIF_CMNCR_BSZ(hyperflash ? 1 : 0));
> +	if (rpc->type == RPCIF_RCAR_GEN3)
> +		regmap_write(rpc->regmap, RPCIF_CMNCR, RPCIF_CMNCR_SFDE |
> +			     RPCIF_CMNCR_MOIIO_HIZ | RPCIF_CMNCR_IOFV_HIZ |
> +			     RPCIF_CMNCR_BSZ(hyperflash ? 1 : 0));
> +	else
> +		regmap_write(rpc->regmap, RPCIF_CMNCR, RPCIF_CMNCR_SFDE |
> +			     RPCIF_CMNCR_MOIIO3(1) | RPCIF_CMNCR_MOIIO2(1) |
> +			     RPCIF_CMNCR_MOIIO1(1) | RPCIF_CMNCR_MOIIO0(1) |
> +			     RPCIF_CMNCR_IO3FV(2) | RPCIF_CMNCR_IO2FV(2) |
> +			     RPCIF_CMNCR_IO0FV(2) | RPCIF_CMNCR_BSZ(hyperflash ? 1 : 0));
> +
>  	/* Set RCF after BSZ update */
>  	regmap_write(rpc->regmap, RPCIF_DRCR, RPCIF_DRCR_RCF);
>  	/* Dummy read according to spec */
> @@ -295,6 +344,8 @@ void rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
>  	pm_runtime_put(rpc->dev);
>  
>  	rpc->bus_size = hyperflash ? 2 : 1;
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(rpcif_hw_init);
>  
> @@ -657,7 +708,8 @@ static int rpcif_remove(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id rpcif_of_match[] = {
> -	{ .compatible = "renesas,rcar-gen3-rpc-if", },
> +	{ .compatible = "renesas,rcar-gen3-rpc-if", .data = (void *)RPCIF_RCAR_GEN3 },
> +	{ .compatible = "renesas,rzg2l-rpc-if", .data = (void *)RPCIF_RZ_G2L },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, rpcif_of_match);
> diff --git a/drivers/mtd/hyperbus/rpc-if.c b/drivers/mtd/hyperbus/rpc-if.c
> index 367b0d72bf62..40bca89268c3 100644
> --- a/drivers/mtd/hyperbus/rpc-if.c
> +++ b/drivers/mtd/hyperbus/rpc-if.c
> @@ -132,7 +132,9 @@ static int rpcif_hb_probe(struct platform_device *pdev)
>  
>  	rpcif_enable_rpm(&hyperbus->rpc);
>  
> -	rpcif_hw_init(&hyperbus->rpc, true);
> +	error = rpcif_hw_init(&hyperbus->rpc, true);
> +	if (error)
> +		return error;
>  

Not related to this patch, but the concept used here looks fragile. The
child driver calls also rpcif_sw_init() and ignores the error code. What
happens in case of rpcif_sw_init() failure or child probe deferral?
Since the SW and HW init is called in context of child device, the
parent won't do anything. Then, second bind of child device (manual or
because of deferral) will fail on devm_reset_control_get_exclusive()
with -EBUSY.

Initializing parent's resources should be rather done from parent's
context (so renesas-rpc-if.c) to handle properly deferred probe and
other failures. Doing it from a child, breaks encapsulation and
separation of devices.

Is there any reason why memory/renesas-rpc-if.c cannot do SW and HW init?

Best regards,
Krzysztof