| Message ID | 20211025205631.21151-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| 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
On 25/10/2021 22:56, Lad Prabhakar wrote: > Hi All, > > This patch series adds a couple of fixes for rpc-if driver and > adds support for RZ/G2L SoC, where the SPI Multi I/O Bus Controller > is identical to the RPC-IF block found on R-Car Gen3 SoC's. > > Cheers, > Prabhakar > > Changes for v2: > * Rebased the patches on linux-next > * Split patch 5 from v1 > * Included RB tags > * Fixed review comments pointed by Wolfram > > v1: > https://patchwork.kernel.org/project/linux-renesas-soc/cover/ > 20210928140721.8805-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ > Patches look good but you sent them too late for this cycle. I'll take the memory controller parts after the merge window. Best regards, Krzysztof
On Mon, 25 Oct 2021 21:56:24 +0100, Lad Prabhakar wrote: > This patch series adds a couple of fixes for rpc-if driver and > adds support for RZ/G2L SoC, where the SPI Multi I/O Bus Controller > is identical to the RPC-IF block found on R-Car Gen3 SoC's. > > Cheers, > Prabhakar > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [3/7] spi: spi-rpc-if: Check return value of rpcif_sw_init() commit: 0b0a281ed7001d4c4f4c47bdc84680c4997761ca 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. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Hi Prabhakar, CC seebe On Mon, Oct 25, 2021 at 10:57 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Make sure we return error in case devm_ioremap_resource() fails for dirmap > resource. > > Fixes: ca7d8b980b67 ("memory: add Renesas RPC-IF driver") > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thanks for your patch! > --- a/drivers/memory/renesas-rpc-if.c > +++ b/drivers/memory/renesas-rpc-if.c > @@ -243,7 +243,7 @@ int rpcif_sw_init(struct rpcif *rpc, struct device *dev) > 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; > + return PTR_ERR(rpc->dirmap); IIRC, it was intentional to make the dirmap optional (because the device can be used without and/or because some variants on other SoCs lack it?). Unfortunately this is not reflected in the DT bindings (yet?). All code using the dirmap does check if rpc->dirmap is valid first. > rpc->size = resource_size(res); > > rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Wed, Oct 27, 2021 at 9:17 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Oct 25, 2021 at 10:57 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Make sure we return error in case devm_ioremap_resource() fails for dirmap > > resource. > > > > Fixes: ca7d8b980b67 ("memory: add Renesas RPC-IF driver") > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Thanks for your patch! > > > --- a/drivers/memory/renesas-rpc-if.c > > +++ b/drivers/memory/renesas-rpc-if.c > > @@ -243,7 +243,7 @@ int rpcif_sw_init(struct rpcif *rpc, struct device *dev) > > 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; > > + return PTR_ERR(rpc->dirmap); > > IIRC, it was intentional to make the dirmap optional (because the > device can be used without and/or because some variants on other SoCs > lack it?). Unfortunately this is not reflected in the DT bindings > (yet?). All code using the dirmap does check if rpc->dirmap is > valid first. > > > rpc->size = resource_size(res); Of course this will crash if the dirmap is not present, so for now it's better to just bail out. Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, Oct 25, 2021 at 10:57 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > RPCIF_DIRMAP_SIZE may differ on various SoC's. Instead of using > RPCIF_DIRMAP_SIZE macro use resource size to get dirmap size > which is already part of struct rpcif. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
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:29 +0100, Lad Prabhakar wrote: > Make sure we return error in case devm_ioremap_resource() fails for dirmap > resource. > > Applied, thanks! [5/7] memory: renesas-rpc-if: Return error in case devm_ioremap_resource() fails commit: 818fdfa89baac77a8df5a2c30f4fb798cc937aa0 Best regards,
On Mon, 25 Oct 2021 21:56:30 +0100, Lad Prabhakar wrote: > RPCIF_DIRMAP_SIZE may differ on various SoC's. Instead of using > RPCIF_DIRMAP_SIZE macro use resource size to get dirmap size > which is already part of struct rpcif. > > Applied, thanks! [6/7] memory: renesas-rpc-if: Drop usage of RPCIF_DIRMAP_SIZE macro commit: 5da9b59b23d8112709034a07338e03dcc65fa11f Best regards,
On 25/10/2021 22:56, Lad Prabhakar wrote: > Hi All, > > This patch series adds a couple of fixes for rpc-if driver and > adds support for RZ/G2L SoC, where the SPI Multi I/O Bus Controller > is identical to the RPC-IF block found on R-Car Gen3 SoC's. > > Cheers, > Prabhakar > > Changes for v2: > * Rebased the patches on linux-next > * Split patch 5 from v1 > * Included RB tags > * Fixed review comments pointed by Wolfram > > v1: > https://patchwork.kernel.org/project/linux-renesas-soc/cover/ > 20210928140721.8805-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ > > Lad Prabhakar (7): > dt-bindings: memory: renesas,rpc-if: Add support for the R9A07G044 > dt-bindings: memory: renesas,rpc-if: Add optional interrupts property > spi: spi-rpc-if: Check return value of rpcif_sw_init() > mtd: hyperbus: rpc-if: Check return value of rpcif_sw_init() > memory: renesas-rpc-if: Return error in case devm_ioremap_resource() > fails > memory: renesas-rpc-if: Drop usage of RPCIF_DIRMAP_SIZE macro > memory: renesas-rpc-if: Add support for RZ/G2L > Applied parts 1, 2, 5 and 6. I think 7 is going to have a new version due to Wolfram's comments? Best regards, Krzysztof
Hi Krzysztof, On Tue, Nov 16, 2021 at 10:33 AM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 25/10/2021 22:56, Lad Prabhakar wrote: > > Hi All, > > > > This patch series adds a couple of fixes for rpc-if driver and > > adds support for RZ/G2L SoC, where the SPI Multi I/O Bus Controller > > is identical to the RPC-IF block found on R-Car Gen3 SoC's. > > > > Cheers, > > Prabhakar > > > > Changes for v2: > > * Rebased the patches on linux-next > > * Split patch 5 from v1 > > * Included RB tags > > * Fixed review comments pointed by Wolfram > > > > v1: > > https://patchwork.kernel.org/project/linux-renesas-soc/cover/ > > 20210928140721.8805-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ > > > > Lad Prabhakar (7): > > dt-bindings: memory: renesas,rpc-if: Add support for the R9A07G044 > > dt-bindings: memory: renesas,rpc-if: Add optional interrupts property > > spi: spi-rpc-if: Check return value of rpcif_sw_init() > > mtd: hyperbus: rpc-if: Check return value of rpcif_sw_init() > > memory: renesas-rpc-if: Return error in case devm_ioremap_resource() > > fails > > memory: renesas-rpc-if: Drop usage of RPCIF_DIRMAP_SIZE macro > > memory: renesas-rpc-if: Add support for RZ/G2L > > > > Applied parts 1, 2, 5 and 6. I think 7 is going to have a new version > due to Wolfram's comments? > Thank you for queuing up the patches, wrt patch 7/7 this can also be picked up, after the internal discussion it was clear that we cannot use the R-car hw manual for RZ/G2L (we will have to live with magic values). Wolfram has agreed on this and has already Acked patch 7/7. Cheers, Prabhakar > > Best regards, > Krzysztof
On 16/11/2021 11:40, Lad, Prabhakar wrote: > Hi Krzysztof, > > On Tue, Nov 16, 2021 at 10:33 AM Krzysztof Kozlowski > <krzysztof.kozlowski@canonical.com> wrote: >> >> On 25/10/2021 22:56, Lad Prabhakar wrote: >>> Hi All, >>> >>> This patch series adds a couple of fixes for rpc-if driver and >>> adds support for RZ/G2L SoC, where the SPI Multi I/O Bus Controller >>> is identical to the RPC-IF block found on R-Car Gen3 SoC's. >>> >>> Cheers, >>> Prabhakar >>> >>> Changes for v2: >>> * Rebased the patches on linux-next >>> * Split patch 5 from v1 >>> * Included RB tags >>> * Fixed review comments pointed by Wolfram >>> >>> v1: >>> https://patchwork.kernel.org/project/linux-renesas-soc/cover/ >>> 20210928140721.8805-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ >>> >>> Lad Prabhakar (7): >>> dt-bindings: memory: renesas,rpc-if: Add support for the R9A07G044 >>> dt-bindings: memory: renesas,rpc-if: Add optional interrupts property >>> spi: spi-rpc-if: Check return value of rpcif_sw_init() >>> mtd: hyperbus: rpc-if: Check return value of rpcif_sw_init() >>> memory: renesas-rpc-if: Return error in case devm_ioremap_resource() >>> fails >>> memory: renesas-rpc-if: Drop usage of RPCIF_DIRMAP_SIZE macro >>> memory: renesas-rpc-if: Add support for RZ/G2L >>> >> >> Applied parts 1, 2, 5 and 6. I think 7 is going to have a new version >> due to Wolfram's comments? >> > Thank you for queuing up the patches, wrt patch 7/7 this can also be > picked up, after the internal discussion it was clear that we cannot > use the R-car hw manual for RZ/G2L (we will have to live with magic > values). Wolfram has agreed on this and has already Acked patch 7/7. > OK, applied now. Thanks! Best regards, Krzysztof
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,
Hi All, This patch series adds a couple of fixes for rpc-if driver and adds support for RZ/G2L SoC, where the SPI Multi I/O Bus Controller is identical to the RPC-IF block found on R-Car Gen3 SoC's. Cheers, Prabhakar Changes for v2: * Rebased the patches on linux-next * Split patch 5 from v1 * Included RB tags * Fixed review comments pointed by Wolfram v1: https://patchwork.kernel.org/project/linux-renesas-soc/cover/ 20210928140721.8805-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ Lad Prabhakar (7): dt-bindings: memory: renesas,rpc-if: Add support for the R9A07G044 dt-bindings: memory: renesas,rpc-if: Add optional interrupts property spi: spi-rpc-if: Check return value of rpcif_sw_init() mtd: hyperbus: rpc-if: Check return value of rpcif_sw_init() memory: renesas-rpc-if: Return error in case devm_ioremap_resource() fails memory: renesas-rpc-if: Drop usage of RPCIF_DIRMAP_SIZE macro memory: renesas-rpc-if: Add support for RZ/G2L .../memory-controllers/renesas,rpc-if.yaml | 54 +++++++++---- drivers/memory/renesas-rpc-if.c | 80 +++++++++++++++---- drivers/mtd/hyperbus/rpc-if.c | 8 +- drivers/spi/spi-rpc-if.c | 8 +- include/memory/renesas-rpc-if.h | 8 +- 5 files changed, 124 insertions(+), 34 deletions(-)