| Message ID | 20211025205631.21151-8-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add SPI Multi I/O Bus Controller support for RZ/G2L | expand |
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
Hi Krzysztof, Thank you for the review. On Tue, Oct 26, 2021 at 3:47 PM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > 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. > Agree with the above. > Is there any reason why memory/renesas-rpc-if.c cannot do SW and HW init? > I'll investigate this to avoid the above issues. Cheers, Prabhakar > Best regards, > Krzysztof
Hi Prabhakar, > +#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} */ Nice detailed research, thanks! Minor nit: Keep the sorting alphabetical: D3, E3, V3M. > +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); > +} Still magic values here. Don't you have them explained in your Gen3 documentation? It is tables 62.16 and 62.17 in my versions. Other than these, looks good. Thanks, Wolfram
Hi Wolfram, Thank you for the review. On Tue, Nov 2, 2021 at 11:48 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > Hi Prabhakar, > > > +#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} */ > > Nice detailed research, thanks! Minor nit: Keep the sorting > alphabetical: D3, E3, V3M. > > > +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); > > +} > > Still magic values here. Don't you have them explained in your Gen3 > documentation? It is tables 62.16 and 62.17 in my versions. > Oops I missed that, does the below look good? #define RPCIF_PHYADD_ADD_MD 0x00 #define RPCIF_PHYADD_ADD_RDLSEL 0x22 #define RPCIF_PHYADD_ADD_FDLSEL 0x24 #define RPCIF_PHYADD_ADD_RDLMON 0x26 #define RPCIF_PHYADD_ADD_FDLMON 0x28 #define RPCIF_PHYADD_ACCEN BIT(31) #define RPCIF_PHYADD_RW BIT(30) #define RPCIF_PHYADD_ADD(v) (v & 0x3f) #define RPCIF_MD_PHYREGEN_VAL 0xa539 #define RPCIF_MD_PHYREGEN(v) ((v & 0xffff) << 16) #define RPCIF_RDLSEL_QSPI0DLTAPSEL(v) (v & 0x1f) #define RPCIF_RDLSEL_QSPI0DLSETEN(v) ((v & 0x1) << 7) #define RPCIF_RDLSEL_QSPI1DLTAPSEL(v) ((v & 0x1f) << 8) #define RPCIF_RDLSEL_QSPI1DLSETEN(v) ((v & 0x1) << 15) #define RPCIF_FDLSEL_QSPI0DLTAPSEL(v) (v & 0x1f) #define RPCIF_FDLSEL_QSPI0DLSETEN(v) ((v & 0x1) << 7) #define RPCIF_FDLSEL_QSPI1DLTAPSEL(v) ((v & 0x1f) << 8) #define RPCIF_FDLSEL_QSPI1DLSETEN(v) ((v & 0x1) << 15) > + regmap_write(rpc->regmap, RPCIF_PHYWR, 0x00000030); > + regmap_write(rpc->regmap, RPCIF_PHYADD, 0x80000032); > For the above do you have any suggestions? As I couldn't find any details about it or shall I just go with magic numbers for now? > Other than these, looks good. > thanks, once we agree upon above I shall re-spin v3. Cheers, Prabhakar > Thanks, > > Wolfram
Hi Prabhakar, > Oops I missed that, does the below look good? Yes, only a minor nit. > > #define RPCIF_PHYADD_ADD_MD 0x00 > #define RPCIF_PHYADD_ADD_RDLSEL 0x22 > #define RPCIF_PHYADD_ADD_FDLSEL 0x24 > #define RPCIF_PHYADD_ADD_RDLMON 0x26 > #define RPCIF_PHYADD_ADD_FDLMON 0x28 > > #define RPCIF_PHYADD_ACCEN BIT(31) > #define RPCIF_PHYADD_RW BIT(30) Maybe we could leave this because we don't use it? You decide. > > + regmap_write(rpc->regmap, RPCIF_PHYWR, 0x00000030); > > + regmap_write(rpc->regmap, RPCIF_PHYADD, 0x80000032); > > > For the above do you have any suggestions? As I couldn't find any > details about it or shall I just go with magic numbers for now? Ack. I couldn't find docs about these as well. I suggest to add a comment where this value came from. We can ask the BSP and/or HW team for details and update this pair incrementally. > thanks, once we agree upon above I shall re-spin v3. Cool, looking forward to it! Happy hacking, Wolfram
Hi Wolfram, On Wed, Nov 3, 2021 at 8:41 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > Hi Prabhakar, > > > Oops I missed that, does the below look good? > > Yes, only a minor nit. > > > > > #define RPCIF_PHYADD_ADD_MD 0x00 > > #define RPCIF_PHYADD_ADD_RDLSEL 0x22 > > #define RPCIF_PHYADD_ADD_FDLSEL 0x24 > > #define RPCIF_PHYADD_ADD_RDLMON 0x26 > > #define RPCIF_PHYADD_ADD_FDLMON 0x28 > > > > #define RPCIF_PHYADD_ACCEN BIT(31) > > #define RPCIF_PHYADD_RW BIT(30) > > Maybe we could leave this because we don't use it? You decide. > Agreed will drop RPCIF_PHYADD_RW macro. > > > + regmap_write(rpc->regmap, RPCIF_PHYWR, 0x00000030); > > > + regmap_write(rpc->regmap, RPCIF_PHYADD, 0x80000032); > > > > > For the above do you have any suggestions? As I couldn't find any > > details about it or shall I just go with magic numbers for now? > > Ack. I couldn't find docs about these as well. I suggest to add a > comment where this value came from. We can ask the BSP and/or HW team > for details and update this pair incrementally. > Thanks, I'll add a comment stating the values have come from the RZ/G2L HW manual. Cheers, Prabhakar > > thanks, once we agree upon above I shall re-spin v3. > > Cool, looking forward to it! > > Happy hacking, > > Wolfram >
On Mon, Oct 25, 2021 at 09:56:31PM +0100, 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> After some internal investigations we found out that we have to live with these magic numbers. We shouldn't mix documentation there. So, this patch is fine as is. The minor nit I will fix incrementally because I will work on this file soon as well. Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
On Mon, 25 Oct 2021 21:56:31 +0100, 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. > > > [...] Applied, thanks! [7/7] memory: renesas-rpc-if: Add support for RZ/G2L commit: b04cc0d912eb80d3c438b11d96ca847c3e77e8ab Best regards,
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; hyperbus->hbdev.map.size = hyperbus->rpc.size; hyperbus->hbdev.map.virt = hyperbus->rpc.dirmap; diff --git a/drivers/spi/spi-rpc-if.c b/drivers/spi/spi-rpc-if.c index 83796a4ead34..fe82f3575df4 100644 --- a/drivers/spi/spi-rpc-if.c +++ b/drivers/spi/spi-rpc-if.c @@ -156,7 +156,9 @@ static int rpcif_spi_probe(struct platform_device *pdev) ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_TX_QUAD | SPI_RX_QUAD; ctlr->flags = SPI_CONTROLLER_HALF_DUPLEX; - rpcif_hw_init(rpc, false); + error = rpcif_hw_init(rpc, false); + if (error) + return error; error = spi_register_controller(ctlr); if (error) { diff --git a/include/memory/renesas-rpc-if.h b/include/memory/renesas-rpc-if.h index 77c694a19149..7c93f5177532 100644 --- a/include/memory/renesas-rpc-if.h +++ b/include/memory/renesas-rpc-if.h @@ -57,6 +57,11 @@ struct rpcif_op { } data; }; +enum rpcif_type { + RPCIF_RCAR_GEN3, + RPCIF_RZ_G2L, +}; + struct rpcif { struct device *dev; void __iomem *base; @@ -64,6 +69,7 @@ struct rpcif { struct regmap *regmap; struct reset_control *rstc; size_t size; + enum rpcif_type type; enum rpcif_data_dir dir; u8 bus_size; void *buffer; @@ -78,7 +84,7 @@ struct rpcif { }; int rpcif_sw_init(struct rpcif *rpc, struct device *dev); -void rpcif_hw_init(struct rpcif *rpc, bool hyperflash); +int 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_manual_xfer(struct rpcif *rpc);