| Message ID | 20230124074706.13383-1-wsa+renesas@sang-engineering.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: sh-msiof: Enforce fixed DTDL for R-Car H3 | expand |
Hi Wolfram, Thanks for your patch! On Tue, Jan 24, 2023 at 8:47 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > Documentation says only DTDL of 200 is allowed for this SoC. Do you have a pointer to that? > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Tested with MSIOF0 on a Salvator-XS with R-Car H3 ES2.0 by creating a > loopback with a wire. > > drivers/spi/spi-sh-msiof.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c > index 9bca3d076f05..609f48ec84dd 100644 > --- a/drivers/spi/spi-sh-msiof.c > +++ b/drivers/spi/spi-sh-msiof.c > @@ -30,12 +30,15 @@ > > #include <asm/unaligned.h> > > +#define SH_MSIOF_FLAG_FIXED_DTDL_200 BIT(0) We already have "renesas,dtdl" to configure this from DT. Iff this is really needed, perhaps it should be added to r8a77951.dtsi? I suspect this is a leftover in the BSP from attempts to get MSIOF working on R-Car H3 ES1.0 (which it never did for me, as CLK starts and stops too soon, compared to MOSI/MISO). On R-Car H3 ES2.0, everything works fine, without touching DTDL. 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 Geert, > > Documentation says only DTDL of 200 is allowed for this SoC. > > Do you have a pointer to that? Yes: Gen3 docs Rev.2.30 from Aug 2021, Section 59.2.1, Bits 22-20: "[R-Car H3, R-Car H3-N] 010: 2-clock-cycle delay Other than above: Setting prohibited" > We already have "renesas,dtdl" to configure this from DT. > Iff this is really needed, perhaps it should be added to r8a77951.dtsi? I have to disagree here. The docs say that other values are prohibited. IMO the driver should take care of valid values then. We should not rely on user provided input. > I suspect this is a leftover in the BSP from attempts to get MSIOF > working on R-Car H3 ES1.0 (which it never did for me, as CLK starts > and stops too soon, compared to MOSI/MISO). > On R-Car H3 ES2.0, everything works fine, without touching DTDL. The BSP originally has this patch for ES3 only. I extended to ES2 as well because that is what the docs say. Happy hacking, Wolfram
Hi Wolfram, On Thu, Jan 26, 2023 at 2:07 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > > Documentation says only DTDL of 200 is allowed for this SoC. > > > > Do you have a pointer to that? > > Yes: Gen3 docs Rev.2.30 from Aug 2021, Section 59.2.1, Bits 22-20: > > "[R-Car H3, R-Car H3-N] > 010: 2-clock-cycle delay > Other than above: Setting prohibited" Oops, I looked at the 59.2.4, which describes the receive equivalent, and which does not have this limitations. > > We already have "renesas,dtdl" to configure this from DT. > > Iff this is really needed, perhaps it should be added to r8a77951.dtsi? > > I have to disagree here. The docs say that other values are prohibited. > IMO the driver should take care of valid values then. We should not rely > on user provided input. Then we should make sure the user cannot override to an invalid value through "renesas,dtdl" either? > > I suspect this is a leftover in the BSP from attempts to get MSIOF > > working on R-Car H3 ES1.0 (which it never did for me, as CLK starts > > and stops too soon, compared to MOSI/MISO). > > On R-Car H3 ES2.0, everything works fine, without touching DTDL. > > The BSP originally has this patch for ES3 only. I extended to ES2 as > well because that is what the docs say. This limitation appeared in Hardware User's Manual rev. 1.50, which is also the first version to document R-Car H3-N. So I wouldn't be surprised if this applies to R-Car H3(-N) ES3.0 only. Remember that rev.0.53 was the switching point from R-Car H3 ES1.0 to ES2.0. To be clarified with Renesas? 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
> > I have to disagree here. The docs say that other values are prohibited. > > IMO the driver should take care of valid values then. We should not rely > > on user provided input. > > Then we should make sure the user cannot override to an invalid value > through "renesas,dtdl" either? We do. The new flag is checked after sh_msiof_spi_parse_dt(), so any user input will be overwritten with the only value allowed. > To be clarified with Renesas? Frankly, I don't think it is worth the hazzle and just stick to the latest docs. Yes, they may be inaccurate for ES2.0 but what is the downside? Will it break things or is this just a little overhead?
Hi Wolfram, On Thu, Jan 26, 2023 at 2:40 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > > I have to disagree here. The docs say that other values are prohibited. > > > IMO the driver should take care of valid values then. We should not rely > > > on user provided input. > > > > Then we should make sure the user cannot override to an invalid value > > through "renesas,dtdl" either? > > We do. The new flag is checked after sh_msiof_spi_parse_dt(), so any > user input will be overwritten with the only value allowed. OK> > > To be clarified with Renesas? > > Frankly, I don't think it is worth the hazzle and just stick to the > latest docs. Yes, they may be inaccurate for ES2.0 but what is the > downside? Will it break things or is this just a little overhead? I don't know. You have to try it with a real SPI device, and/or look at the SPI bus signals with a logic analyzer. 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 Wolfram, On Thu, Jan 26, 2023 at 2:40 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > > I have to disagree here. The docs say that other values are prohibited. > > > IMO the driver should take care of valid values then. We should not rely > > > on user provided input. > > > > Then we should make sure the user cannot override to an invalid value > > through "renesas,dtdl" either? > > We do. The new flag is checked after sh_msiof_spi_parse_dt(), so any > user input will be overwritten with the only value allowed. OK. > > To be clarified with Renesas? > > Frankly, I don't think it is worth the hazzle and just stick to the > latest docs. Yes, they may be inaccurate for ES2.0 but what is the > downside? Will it break things or is this just a little overhead? Given the recent clarification from Renesas that this applies to all revisions of R-Car H3 (ES1.0, ES2.0 and ES3.0): 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
Hello Wolfram-san, > From: Wolfram Sang, Sent: Tuesday, January 24, 2023 4:47 PM > > Documentation says only DTDL of 200 is allowed for this SoC. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thank you for the patch! Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Best regards, Yoshihiro Shimoda > --- > > Tested with MSIOF0 on a Salvator-XS with R-Car H3 ES2.0 by creating a > loopback with a wire. > > drivers/spi/spi-sh-msiof.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c > index 9bca3d076f05..609f48ec84dd 100644 > --- a/drivers/spi/spi-sh-msiof.c > +++ b/drivers/spi/spi-sh-msiof.c > @@ -30,12 +30,15 @@ > > #include <asm/unaligned.h> > > +#define SH_MSIOF_FLAG_FIXED_DTDL_200 BIT(0) > + > struct sh_msiof_chipdata { > u32 bits_per_word_mask; > u16 tx_fifo_size; > u16 rx_fifo_size; > u16 ctlr_flags; > u16 min_div_pow; > + u32 flags; > }; > > struct sh_msiof_spi_priv { > @@ -1073,6 +1076,16 @@ static const struct sh_msiof_chipdata rcar_gen3_data = { > .min_div_pow = 1, > }; > > +static const struct sh_msiof_chipdata rcar_r8a7795_data = { > + .bits_per_word_mask = SPI_BPW_MASK(8) | SPI_BPW_MASK(16) | > + SPI_BPW_MASK(24) | SPI_BPW_MASK(32), > + .tx_fifo_size = 64, > + .rx_fifo_size = 64, > + .ctlr_flags = SPI_CONTROLLER_MUST_TX, > + .min_div_pow = 1, > + .flags = SH_MSIOF_FLAG_FIXED_DTDL_200, > +}; > + > static const struct of_device_id sh_msiof_match[] = { > { .compatible = "renesas,sh-mobile-msiof", .data = &sh_data }, > { .compatible = "renesas,msiof-r8a7743", .data = &rcar_gen2_data }, > @@ -1083,6 +1096,7 @@ static const struct of_device_id sh_msiof_match[] = { > { .compatible = "renesas,msiof-r8a7793", .data = &rcar_gen2_data }, > { .compatible = "renesas,msiof-r8a7794", .data = &rcar_gen2_data }, > { .compatible = "renesas,rcar-gen2-msiof", .data = &rcar_gen2_data }, > + { .compatible = "renesas,msiof-r8a7795", .data = &rcar_r8a7795_data }, > { .compatible = "renesas,msiof-r8a7796", .data = &rcar_gen3_data }, > { .compatible = "renesas,rcar-gen3-msiof", .data = &rcar_gen3_data }, > { .compatible = "renesas,rcar-gen4-msiof", .data = &rcar_gen3_data }, > @@ -1280,6 +1294,9 @@ static int sh_msiof_spi_probe(struct platform_device *pdev) > return -ENXIO; > } > > + if (chipdata->flags & SH_MSIOF_FLAG_FIXED_DTDL_200) > + info->dtdl = 200; > + > if (info->mode == MSIOF_SPI_SLAVE) > ctlr = spi_alloc_slave(&pdev->dev, > sizeof(struct sh_msiof_spi_priv)); > -- > 2.30.2
diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c index 9bca3d076f05..609f48ec84dd 100644 --- a/drivers/spi/spi-sh-msiof.c +++ b/drivers/spi/spi-sh-msiof.c @@ -30,12 +30,15 @@ #include <asm/unaligned.h> +#define SH_MSIOF_FLAG_FIXED_DTDL_200 BIT(0) + struct sh_msiof_chipdata { u32 bits_per_word_mask; u16 tx_fifo_size; u16 rx_fifo_size; u16 ctlr_flags; u16 min_div_pow; + u32 flags; }; struct sh_msiof_spi_priv { @@ -1073,6 +1076,16 @@ static const struct sh_msiof_chipdata rcar_gen3_data = { .min_div_pow = 1, }; +static const struct sh_msiof_chipdata rcar_r8a7795_data = { + .bits_per_word_mask = SPI_BPW_MASK(8) | SPI_BPW_MASK(16) | + SPI_BPW_MASK(24) | SPI_BPW_MASK(32), + .tx_fifo_size = 64, + .rx_fifo_size = 64, + .ctlr_flags = SPI_CONTROLLER_MUST_TX, + .min_div_pow = 1, + .flags = SH_MSIOF_FLAG_FIXED_DTDL_200, +}; + static const struct of_device_id sh_msiof_match[] = { { .compatible = "renesas,sh-mobile-msiof", .data = &sh_data }, { .compatible = "renesas,msiof-r8a7743", .data = &rcar_gen2_data }, @@ -1083,6 +1096,7 @@ static const struct of_device_id sh_msiof_match[] = { { .compatible = "renesas,msiof-r8a7793", .data = &rcar_gen2_data }, { .compatible = "renesas,msiof-r8a7794", .data = &rcar_gen2_data }, { .compatible = "renesas,rcar-gen2-msiof", .data = &rcar_gen2_data }, + { .compatible = "renesas,msiof-r8a7795", .data = &rcar_r8a7795_data }, { .compatible = "renesas,msiof-r8a7796", .data = &rcar_gen3_data }, { .compatible = "renesas,rcar-gen3-msiof", .data = &rcar_gen3_data }, { .compatible = "renesas,rcar-gen4-msiof", .data = &rcar_gen3_data }, @@ -1280,6 +1294,9 @@ static int sh_msiof_spi_probe(struct platform_device *pdev) return -ENXIO; } + if (chipdata->flags & SH_MSIOF_FLAG_FIXED_DTDL_200) + info->dtdl = 200; + if (info->mode == MSIOF_SPI_SLAVE) ctlr = spi_alloc_slave(&pdev->dev, sizeof(struct sh_msiof_spi_priv));
Documentation says only DTDL of 200 is allowed for this SoC. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- Tested with MSIOF0 on a Salvator-XS with R-Car H3 ES2.0 by creating a loopback with a wire. drivers/spi/spi-sh-msiof.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)