| Message ID | 20191203034519.5640-1-chris.brandt@renesas.com |
|---|---|
| Headers | show |
| Series | spi: Add Renesas SPIBSC controller | expand |
On Mon, Dec 02, 2019 at 10:45:17PM -0500, Chris Brandt wrote: > +config SPI_SPIBSC > + tristate "Renesas SPI Multi I/O Bus Controller" > + depends on ARCH_R7S72100 || ARCH_R7S9210 I'm not seeing any build dependency here, please add an || COMPILE_TEST for build coverage. > +++ b/drivers/spi/spi-spibsc.c > @@ -0,0 +1,609 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * SPI Bus Space Controller (SPIBSC) bus driver Please make the entire comment block here a C++ one so things look more intentional. > +static void spibsc_write(struct spibsc_priv *sbsc, int reg, u32 val) > +{ > + iowrite32(val, sbsc->base + reg); > +} > +static void spibsc_write8(struct spibsc_priv *sbsc, int reg, u8 val) Blank likes between functions, please see coding-style.rst. Looking at a bunch of the stuff here it looks like you could benefit from regmap, it's got lots of debug infrastructure. > + if (tx) > + pr_debug("spibsc: send data: "); > + else > + pr_debug("spibsc: recv data: "); dev_dbg() if you're going to do tis. > + > + for (i = 0; i < len; ) { > + sprintf(line_buffer + line_index, " %02X", buf[i]); snprintf()! > +static int spibsc_transfer_one_message(struct spi_controller *master, > + struct spi_message *msg) > +{ > + struct spibsc_priv *sbsc = spi_controller_get_devdata(master); > + struct spi_transfer *t, *t_last; > + u8 tx_data[MAX_CMD_LEN]; > + int tx_only; > + u8 tx_len; > + int ret; > + > + t_last = list_last_entry(&msg->transfers, struct spi_transfer, > + transfer_list); > + /* defaults */ > + ret = 0; > + sbsc->last_xfer = 0; > + tx_only = 1; > + > + /* Analyze the messages */ > + t = list_first_entry(&msg->transfers, struct spi_transfer, > + transfer_list); > + if (t->rx_buf) { > + dev_dbg(sbsc->dev, "Cannot Rx without Tx first!\n"); > + return -EIO; These errors should probably be -EINVAL, you're failing on validation here. > + } > + list_for_each_entry(t, &msg->transfers, transfer_list) { Blank line here please as well. > + if (spi->bits_per_word != 8) { > + dev_err(sbsc->dev, "bits_per_word must be 8\n"); > + return -EIO; > + } The core will validate this for you. > + master->num_chipselect = 1; > + master->mode_bits = SPI_CPOL | SPI_CPHA; > + master->setup = spibsc_setup; > + master->transfer_one_message = spibsc_transfer_one_message; Set bits_per_word_mask here. > + dev_info(&pdev->dev, "probed\n"); > + Remove this, it's just noise. > +static int spibsc_remove(struct platform_device *pdev) > +{ > + struct spibsc_priv *sbsc = dev_get_drvdata(&pdev->dev); > + > + pm_runtime_put(&pdev->dev); > + pm_runtime_disable(&pdev->dev); There seems to be no purpose in the runtime PM code in this driver, there's no PM operations of any kind and the driver holds a runtime PM reference for the entire lifetime of the device. > + spi_unregister_controller(sbsc->master); You registered the controller with devm_, there's no need to unregister it and if you do you need to use a matching devm_ unregiser.
Hello Mark, On Tue, Dec 3, 2019, Mark Brown wrote: > On Mon, Dec 02, 2019 at 10:45:17PM -0500, Chris Brandt wrote: > > > +config SPI_SPIBSC > > + tristate "Renesas SPI Multi I/O Bus Controller" > > + depends on ARCH_R7S72100 || ARCH_R7S9210 > > I'm not seeing any build dependency here, please add an || COMPILE_TEST for > build coverage. (snip) Thank you for your review! I have no complaints about your comments so I will make the changes and send out a v2. Chris
Hi Chris, On Tue, Dec 3, 2019 at 4:46 AM Chris Brandt <chris.brandt@renesas.com> wrote: > Add a driver for the SPIBSC controller in Renesas SoC devices. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Thanks for your patch! > --- /dev/null > +++ b/drivers/spi/spi-spibsc.c > @@ -0,0 +1,609 @@ > +// SPDX-License-Identifier: GPL-2.0 GPL-2.0-only > +static void spibsc_print_data(struct spibsc_priv *sbsc, u8 tx, const u8 *buf, > + int len) > +{ > +#if defined(DEBUG_PRINT_DATA) > + char line_buffer[3*16+1]; > + int i, line_index = 0; > + > + if (tx) > + pr_debug("spibsc: send data: "); > + else > + pr_debug("spibsc: recv data: "); > + > + for (i = 0; i < len; ) { > + sprintf(line_buffer + line_index, " %02X", buf[i]); > + line_index += 3; > + i++; > + if (i % 16 == 0) { > + pr_debug(line_buffer); > + line_index = 0; > + } > + } > + if (i % 16 != 0) > + pr_debug(line_buffer); > + else > + pr_debug("\n"); > +#endif print_hex_dump_debug()? > +} > + > +static int spibsc_wait_trans_completion(struct spibsc_priv *sbsc) > +{ > + int t = 256 * 100000; > + > + while (t--) { > + if (spibsc_read(sbsc, CMNSR) & CMNSR_TEND) > + return 0; > + ndelay(1); > + } So this may busy loop for up to 25.6 ms? Oops... Can you use the interrupt (SPIHF) instead, signalling a completion? > + > + dev_err(sbsc->dev, "Timeout waiting for TEND\n"); > + return -ETIMEDOUT; > +} > + /* Use 'Option Data' for 3rd-6th bytes */ > + switch (tx_len) { > + case 6: > + dropr |= DROPR_OPD0(tx_data[5]); > + opde |= (1 << 0); Both checkpatch and gcc tell you to add fallthrough coments. > + case 5: > + dropr |= DROPR_OPD1(tx_data[4]); > + opde |= (1 << 1); > + case 4: > + dropr |= DROPR_OPD2(tx_data[3]); > + opde |= (1 << 2); > + case 3: > + dropr |= DROPR_OPD3(tx_data[2]); > + opde |= (1 << 3); > + drenr |= DRENR_OPDE(opde); > + } > +static int spibsc_transfer_one_message(struct spi_controller *master, > + struct spi_message *msg) > +{ > + struct spibsc_priv *sbsc = spi_controller_get_devdata(master); > + struct spi_transfer *t, *t_last; > + u8 tx_data[MAX_CMD_LEN]; > + int tx_only; > + u8 tx_len; > + int ret; > + > + t_last = list_last_entry(&msg->transfers, struct spi_transfer, > + transfer_list); > + /* defaults */ > + ret = 0; > + sbsc->last_xfer = 0; > + tx_only = 1; > + > + /* Analyze the messages */ > + t = list_first_entry(&msg->transfers, struct spi_transfer, > + transfer_list); > + if (t->rx_buf) { > + dev_dbg(sbsc->dev, "Cannot Rx without Tx first!\n"); > + return -EIO; > + } > + list_for_each_entry(t, &msg->transfers, transfer_list) { > + if (t->rx_buf) { > + tx_only = 0; > + if (t != t_last) { > + dev_dbg(sbsc->dev, "RX transaction is not the last transaction!\n"); > + return -EIO; > + } > + } > + if (t->cs_change) { > + dev_err(sbsc->dev, "cs_change not supported"); > + return -EIO; > + } > + } If you would do the checks above in .prepare_message() (like e.g. rspi does)... > + > + /* Tx Only (SPI Mode is used) */ > + if (tx_only == 1) { > + > + dev_dbg(sbsc->dev, "%s: TX only\n", __func__); > + > + /* Initialize for SPI Mode */ > + spibsc_write(sbsc, CMNCR, CMNCR_INIT); > + > + /* Send messages */ > + list_for_each_entry(t, &msg->transfers, transfer_list) { > + > + if (t == t_last) > + sbsc->last_xfer = 1; > + > + ret = spibsc_send_data(sbsc, t->tx_buf, t->len); > + if (ret) > + break; > + > + msg->actual_length += t->len; > + } > + > + /* Done */ > + msg->status = ret; > + spi_finalize_current_message(master); > + return ret; > + } > + > + /* Tx, then RX (Data Read Mode is used) */ > + dev_dbg(sbsc->dev, "%s: Tx then Rx\n", __func__); > + > + /* Buffer up the transmit portion (cmd + addr) so we can send it all at > + * once > + */ > + tx_len = 0; > + list_for_each_entry(t, &msg->transfers, transfer_list) { > + if (t->tx_buf) { > + if ((tx_len + t->len) > sizeof(tx_data)) { > + dev_dbg(sbsc->dev, "Command too big (%d)\n", > + tx_len + t->len); > + return -EIO; > + } > + memcpy(tx_data + tx_len, t->tx_buf, t->len); > + tx_len += t->len; > + } > + > + if (t->rx_buf) > + ret = spibsc_send_recv_data(sbsc, tx_data, tx_len, > + t->rx_buf, t->len); > + > + msg->actual_length += t->len; > + } > + > + msg->status = ret; > + spi_finalize_current_message(master); ... can't you just use .transfer_one(), instead of duplicating the logic from spi_transfer_one_message()? Or is there some special detail that's not obvious to the casual reviewer? > +static const struct of_device_id of_spibsc_match[] = { > + { .compatible = "renesas,spibsc"}, > + { .compatible = "renesas,r7s72100-spibsc", .data = (void *)HAS_SPBCR}, > + { .compatible = "renesas,r7s9210-spibsc"}, Do you need to match against all 3 in the driver? Does SPIBSC work on RZ/A1 when not setting HAS_SPBCR? If not, the fallback to "renesas,spibsc" is not valid for RZ/A1. > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, of_spibsc_match); > + > +static struct platform_driver spibsc_driver = { > + .probe = spibsc_probe, > + .remove = spibsc_remove, > + .driver = { > + .name = "spibsc", > + .owner = THIS_MODULE, > + .of_match_table = of_spibsc_match, > + }, > +}; > +module_platform_driver(spibsc_driver); > + > +MODULE_DESCRIPTION("Renesas SPIBSC SPI Flash driver"); > +MODULE_LICENSE("GPL v2"); > +MODULE_AUTHOR("Chris Brandt"); > -- > 2.23.0 > -- 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 Mark, On Tue, Dec 3, 2019 at 3:19 PM Mark Brown <broonie@kernel.org> wrote: > On Mon, Dec 02, 2019 at 10:45:17PM -0500, Chris Brandt wrote: > > +static int spibsc_remove(struct platform_device *pdev) > > +{ > > + struct spibsc_priv *sbsc = dev_get_drvdata(&pdev->dev); > > + > > + pm_runtime_put(&pdev->dev); > > + pm_runtime_disable(&pdev->dev); > > There seems to be no purpose in the runtime PM code in this > driver, there's no PM operations of any kind and the driver holds > a runtime PM reference for the entire lifetime of the device. It matters for the clock domain (assumed the module clock is not always marked as a critical clock). Gr{oetje,eeting}s, Geert
Hi Chris, On Tue, Dec 3, 2019 at 4:46 AM Chris Brandt <chris.brandt@renesas.com> wrote: > Allow critical clocks to be specified in the Device Tree. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Thanks for your patch! Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Notes: 1. Assumed we really need this, 2. Minor nit below. > --- a/drivers/clk/renesas/clk-mstp.c > +++ b/drivers/clk/renesas/clk-mstp.c > @@ -187,6 +187,7 @@ static void __init cpg_mstp_clocks_init(struct device_node *np) > const char *idxname; > struct clk **clks; > unsigned int i; > + unsigned long flags; = 0 here... > > group = kzalloc(struct_size(group, clks, MSTP_MAX_CLOCKS), GFP_KERNEL); > if (!group) > @@ -239,8 +240,11 @@ static void __init cpg_mstp_clocks_init(struct device_node *np) > continue; > } > > + flags = 0; ... instead of here? > + of_clk_detect_critical(np, i, &flags); > + > clks[clkidx] = cpg_mstp_clock_register(name, parent_name, > - clkidx, group); > + clkidx, group, flags); > if (!IS_ERR(clks[clkidx])) { > group->data.clk_num = max(group->data.clk_num, > clkidx + 1); Gr{oetje,eeting}s, Geert
Hi Chris, On Tue, Dec 3, 2019 at 4:46 AM Chris Brandt <chris.brandt@renesas.com> wrote: > The SPIBSC-0 clock is marked as critical because for XIP systems, this > is the SPI flash controller it will boot from and the kernel will also > be running from so it cannot be turned off. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Thanks for your patch! > --- a/arch/arm/boot/dts/r7s72100.dtsi > +++ b/arch/arm/boot/dts/r7s72100.dtsi > @@ -101,6 +101,26 @@ > #size-cells = <1>; > ranges; > > + spibsc0: spi@3fefa000 { > + compatible = "renesas,r7s72100-spibsc", "renesas,spibsc"; > + reg = <0x3fefa000 0x100>, <0x18000000 0x4000000>; The second region conflicts with the XIP flash@18000000 in arch/arm/boot/dts/r7s72100-gr-peach.dts. Yes, I know it is the same device ;-) > + clocks = <&mstp9_clks R7S72100_CLK_SPIBSC0>; > + power-domains = <&cpg_clocks>; > + #address-cells = <1>; > + #size-cells = <0>; > + status = "disabled"; > + }; > + > + spibsc1: spi@3fefb000 { > + compatible = "renesas,r7s72100-spibsc", "renesas,spibsc"; > + reg = <0x3fefb000 0x100>, <0x1c000000 0x4000000>; > + clocks = <&mstp9_clks R7S72100_CLK_SPIBSC1>; > + power-domains = <&cpg_clocks>; > + #address-cells = <1>; > + #size-cells = <0>; > + status = "disabled"; > + }; > + > L2: cache-controller@3ffff000 { > compatible = "arm,pl310-cache"; > reg = <0x3ffff000 0x1000>; > @@ -467,11 +487,13 @@ > #clock-cells = <1>; > compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks"; > reg = <0xfcfe0438 4>; > - clocks = <&p0_clk>, <&p0_clk>, <&p0_clk>, <&p0_clk>; > + clocks = <&p0_clk>, <&p0_clk>, <&p0_clk>, <&p0_clk>, <&b_clk>, <&b_clk>; > clock-indices = < > R7S72100_CLK_I2C0 R7S72100_CLK_I2C1 R7S72100_CLK_I2C2 R7S72100_CLK_I2C3 > + R7S72100_CLK_SPIBSC0 R7S72100_CLK_SPIBSC1 > >; > - clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3"; > + clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "spibsc0", "spibsc1"; > + clock-critical = <4>; /* spibsc0 */ Iff we go this clock-critical route, I think this should be specified in the board-specific .dts instead of in the SoC-specific .dtsi. Gr{oetje,eeting}s, Geert
Hi Geert, Thank you for your review! On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > + unsigned long flags; > > = 0 here... > > > > + flags = 0; > > ... instead of here? That was my first thought too...and it was wrong. If of_clk_detect_critical does NOT detect a critical clock, it does not touch flags at all. And since it is a loop, what you get is after the first clock is marked as CRITICAL, all the remaining clocks also get marked CRITICAL. In this case, both spibsc0 and spibsc1 get marked critical. That's why I have to reset it for each loop. Chris
Hi Chris, On Tue, Dec 3, 2019 at 4:46 AM Chris Brandt <chris.brandt@renesas.com> wrote: > The SPIBSC clocks are marked as critical because for XIP systems, the > kernel will be running from QSPI flash and cannot be turned off. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Thanks for your patch! > --- a/drivers/clk/renesas/r7s9210-cpg-mssr.c > +++ b/drivers/clk/renesas/r7s9210-cpg-mssr.c > @@ -93,6 +93,7 @@ static const struct mssr_mod_clk r7s9210_mod_clks[] __initconst = { > DEF_MOD_STB("ether1", 64, R7S9210_CLK_B), > DEF_MOD_STB("ether0", 65, R7S9210_CLK_B), > > + DEF_MOD_STB("spibsc", 83, R7S9210_CLK_P1), OK. > DEF_MOD_STB("i2c3", 84, R7S9210_CLK_P1), > DEF_MOD_STB("i2c2", 85, R7S9210_CLK_P1), > DEF_MOD_STB("i2c1", 86, R7S9210_CLK_P1), > @@ -112,6 +113,10 @@ static const struct mssr_mod_clk r7s9210_mod_clks[] __initconst = { > DEF_MOD_STB("vdc6", 81, R7S9210_CLK_P1), > }; > > +static const unsigned int r7s9210_crit_mod_clks[] __initconst = { > + MOD_CLK_ID_10(83), /* SPIBSC */ This is only a critical clock if XIP is in use, right? Can we do better, and only mark it critical if we detect the FLASH is used in XIP mode? E.g. by using for_each_compatible_node(..., "mtd-rom"), and checking if any of the corresponding register blocks matches the SPIBSC FLASH memory window? > +}; > + > /* The clock dividers in the table vary based on DT and register settings */ > static void __init r7s9210_update_clk_table(struct clk *extal_clk, > void __iomem *base) > @@ -213,6 +218,10 @@ const struct cpg_mssr_info r7s9210_cpg_mssr_info __initconst = { > .num_mod_clks = ARRAY_SIZE(r7s9210_mod_clks), > .num_hw_mod_clks = 11 * 32, /* includes STBCR0 which doesn't exist */ > > + /* Critical Module Clocks */ > + .crit_mod_clks = r7s9210_crit_mod_clks, > + .num_crit_mod_clks = ARRAY_SIZE(r7s9210_crit_mod_clks), > + > /* Callbacks */ > .cpg_clk_register = rza2_cpg_clk_register, Gr{oetje,eeting}s, Geert
Hi Chris, On Tue, Dec 3, 2019 at 7:46 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > > + unsigned long flags; > > > > = 0 here... > > > > > > + flags = 0; > > > > ... instead of here? > > That was my first thought too...and it was wrong. > > If of_clk_detect_critical does NOT detect a critical clock, it does not > touch flags at all. > And since it is a loop, what you get is after the first clock is marked > as CRITICAL, all the remaining clocks also get marked CRITICAL. In this > case, both spibsc0 and spibsc1 get marked critical. That's why I have to > reset it for each loop. Thanks, I missed this is done inside a loop. Gr{oetje,eeting}s, Geert
Hi Geert, On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > + reg = <0x3fefa000 0x100>, <0x18000000 > > + 0x4000000>; > > The second region conflicts with the XIP flash@18000000 in > arch/arm/boot/dts/r7s72100-gr-peach.dts. > Yes, I know it is the same device ;-) Is that just an FYI?? Or do you have some suggestion?? > > + clock-critical = <4>; /* spibsc0 */ > > Iff we go this clock-critical route, I think this should be specified in the > board-specific .dts instead of in the SoC-specific .dtsi. OK, that's fine. It makes more sense to be in the .dts because it's a board design decision. I will remove it from the patch. The one 'tricky' thing is that the <4> is the index into the number of clocks. So in the Renesas BSP where it includes the VDC5 LCD controllers, clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "vdc50", "vdc51", "spibsc0", "spibsc1"; the <4> needs to become a <6>. Chris
Hi Chris, On Tue, Dec 3, 2019 at 4:46 AM Chris Brandt <chris.brandt@renesas.com> wrote: > Add SPIBSC Device support for RZ/A2. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Thanks for your patch! > --- a/arch/arm/boot/dts/r7s9210.dtsi > +++ b/arch/arm/boot/dts/r7s9210.dtsi > @@ -68,6 +68,16 @@ > cache-level = <2>; > }; > > + spibsc: spi@1f800000 { > + compatible = "renesas,r7s9210-spibsc", "renesas,spibsc"; > + reg = <0x1f800000 0x8c>, <0x20000000 0x10000000 >; Any specific reason you're using 0x8c, not 0x100? > + clocks = <&cpg CPG_MOD 83>; > + power-domains = <&cpg>; > + #address-cells = <1>; > + #size-cells = <0>; interrupts? > + status = "disabled"; > + }; > + > scif0: serial@e8007000 { > compatible = "renesas,scif-r7s9210"; > reg = <0xe8007000 0x18>; Gr{oetje,eeting}s, Geert
Hi Geert, On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > +static const unsigned int r7s9210_crit_mod_clks[] __initconst = { > > + MOD_CLK_ID_10(83), /* SPIBSC */ > > This is only a critical clock if XIP is in use, right? Correct. > Can we do better, and only mark it critical if we detect the FLASH is used in > XIP mode? > E.g. by using for_each_compatible_node(..., "mtd-rom"), and checking if any > of the corresponding register blocks matches the SPIBSC FLASH memory window? Well...technically...you don't need the "mtd-rom" partition when using the AXFS file system. But, we can make a rule that you have to use it regardless. Instead, I think it would be better if we could flag the clock as critical in a board-specific .dts like we're going to do with the RZ/A1 MSTP driver. Can we add something like "clock-critical = <83>;" to the cpg-mssr driver? What do you think? Chris
Hi Chris, CC Lee (for clock-critical) On Tue, Dec 3, 2019 at 7:58 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > > + reg = <0x3fefa000 0x100>, <0x18000000 > > > + 0x4000000>; > > > > The second region conflicts with the XIP flash@18000000 in > > arch/arm/boot/dts/r7s72100-gr-peach.dts. > > Yes, I know it is the same device ;-) > > Is that just an FYI?? Or do you have some suggestion?? Can the flash subnode be compatible with "mtd-rom", even if the parent node is kept disabled? > > > + clock-critical = <4>; /* spibsc0 */ > > > > Iff we go this clock-critical route, I think this should be specified in the > > board-specific .dts instead of in the SoC-specific .dtsi. > > OK, that's fine. It makes more sense to be in the .dts because it's a board > design decision. I will remove it from the patch. > > The one 'tricky' thing is that the <4> is the index into the number of clocks. > > So in the Renesas BSP where it includes the VDC5 LCD controllers, > > clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "vdc50", "vdc51", "spibsc0", "spibsc1"; > > the <4> needs to become a <6>. Unless you pass "clkidx" instead of "i" to of_clk_detect_critical() in [1], and use "clock-critical = <R7S72100_CLK_SPIBSC0>" in DT? Unfortunately the exact semantics of clock-critical were never documented. Lee? Thanks! [1] "[PATCH 1/6] clk: renesas: mstp: Add critical clock from device tree support" https://lore.kernel.org/linux-renesas-soc/20191203034519.5640-2-chris.brandt@renesas.com/ 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 Chris, On Tue, Dec 3, 2019 at 8:09 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > > +static const unsigned int r7s9210_crit_mod_clks[] __initconst = { > > > + MOD_CLK_ID_10(83), /* SPIBSC */ > > > > This is only a critical clock if XIP is in use, right? > > Correct. > > > > Can we do better, and only mark it critical if we detect the FLASH is used in > > XIP mode? > > E.g. by using for_each_compatible_node(..., "mtd-rom"), and checking if any > > of the corresponding register blocks matches the SPIBSC FLASH memory window? > > Well...technically...you don't need the "mtd-rom" partition when using the AXFS > file system. But, we can make a rule that you have to use it regardless. Just wondering, how does AXFS access the FLASH without it being mapped using "mtd-rom"? 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, On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > + reg = <0x1f800000 0x8c>, <0x20000000 > > + 0x10000000 >; > > Any specific reason you're using 0x8c, not 0x100? Because....I keep forgetting what is the latest 'correct' size: A. The exact size of the register range or B. The size rounded up to look nicer > > + clocks = <&cpg CPG_MOD 83>; > > + power-domains = <&cpg>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > interrupts? I guess I can put it in there just to correctly 'document the hardware' in the Device Tree file. Chris
Hi Geert, > > Well...technically...you don't need the "mtd-rom" partition when using > > the AXFS file system. But, we can make a rule that you have to use it > regardless. > > Just wondering, how does AXFS access the FLASH without it being mapped using > "mtd-rom"? ioreamp in the driver (the physical address gets passed as an argument). But, as the DAX people found out, ioremap in a file system drivers is a big no-no when you are trying to get a new file system into mainline. Nicolas (Pitre) also ran into this when he was adding XIP support to cramfs last year....hence mtd-rom is mandatory for that. I would like us to be able to flag a clock as critical without a specific use case to look for (ie, looking for mtd-rom). For RZ/A1, we're all set because we can do it in the board's .dst. Easy! I just wish it was that easy for RZ/A2 (using the newer renesas-cpg-mssr driver). Chris
Hi Chris, On Tue, Dec 3, 2019 at 11:38 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > > + reg = <0x1f800000 0x8c>, <0x20000000 > > > + 0x10000000 >; > > > > Any specific reason you're using 0x8c, not 0x100? > > Because....I keep forgetting what is the latest 'correct' size: > A. The exact size of the register range > or > B. The size rounded up to look nicer C. The size used by the on-chip address decoder providing the module's select signal? I doubt that's not a power of two ;-) Gr{oetje,eeting}s, Geert
On Tue, 03 Dec 2019, Geert Uytterhoeven wrote: > Hi Chris, > > CC Lee (for clock-critical) > > On Tue, Dec 3, 2019 at 7:58 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > > On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > > > + reg = <0x3fefa000 0x100>, <0x18000000 > > > > + 0x4000000>; > > > > > > The second region conflicts with the XIP flash@18000000 in > > > arch/arm/boot/dts/r7s72100-gr-peach.dts. > > > Yes, I know it is the same device ;-) > > > > Is that just an FYI?? Or do you have some suggestion?? > > Can the flash subnode be compatible with "mtd-rom", even if the parent node > is kept disabled? > > > > > + clock-critical = <4>; /* spibsc0 */ > > > > > > Iff we go this clock-critical route, I think this should be specified in the > > > board-specific .dts instead of in the SoC-specific .dtsi. > > > > OK, that's fine. It makes more sense to be in the .dts because it's a board > > design decision. I will remove it from the patch. > > > > The one 'tricky' thing is that the <4> is the index into the number of clocks. > > > > So in the Renesas BSP where it includes the VDC5 LCD controllers, > > > > clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "vdc50", "vdc51", "spibsc0", "spibsc1"; > > > > the <4> needs to become a <6>. > > Unless you pass "clkidx" instead of "i" to of_clk_detect_critical() in [1], > and use "clock-critical = <R7S72100_CLK_SPIBSC0>" in DT? > > Unfortunately the exact semantics of clock-critical were never documented. > Lee? /** * of_clk_detect_critical() - set CLK_IS_CRITICAL flag from Device Tree * @np: Device node pointer associated with clock provider * @index: clock index * @flags: pointer to top-level framework flags * * Detects if the clock-critical property exists and, if so, sets the * corresponding CLK_IS_CRITICAL flag. * * Do not use this function. It exists only for legacy Device Tree * bindings, such as the one-clock-per-node style that are outdated. * Those bindings typically put all clock data into .dts and the Linux * driver has no clock data, thus making it impossible to set this flag * correctly from the driver. Only those drivers may call * of_clk_detect_critical from their setup functions. * * Return: error code or zero on success */ If this meets the criteria, the API is pretty simple/self explanatory. What are you having trouble with? > Thanks! > > [1] "[PATCH 1/6] clk: renesas: mstp: Add critical clock from device > tree support" > https://lore.kernel.org/linux-renesas-soc/20191203034519.5640-2-chris.brandt@renesas.com/ > > > Gr{oetje,eeting}s, > > Geert >
Hi Lee, On Wed, Dec 4, 2019 at 9:38 AM Lee Jones <lee.jones@linaro.org> wrote: > On Tue, 03 Dec 2019, Geert Uytterhoeven wrote: > > On Tue, Dec 3, 2019 at 7:58 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > > > On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > > > > + reg = <0x3fefa000 0x100>, <0x18000000 > > > > > + 0x4000000>; > > > > > > > > The second region conflicts with the XIP flash@18000000 in > > > > arch/arm/boot/dts/r7s72100-gr-peach.dts. > > > > Yes, I know it is the same device ;-) > > > > > > Is that just an FYI?? Or do you have some suggestion?? > > > > Can the flash subnode be compatible with "mtd-rom", even if the parent node > > is kept disabled? > > > > > > > + clock-critical = <4>; /* spibsc0 */ > > > > > > > > Iff we go this clock-critical route, I think this should be specified in the > > > > board-specific .dts instead of in the SoC-specific .dtsi. > > > > > > OK, that's fine. It makes more sense to be in the .dts because it's a board > > > design decision. I will remove it from the patch. > > > > > > The one 'tricky' thing is that the <4> is the index into the number of clocks. > > > > > > So in the Renesas BSP where it includes the VDC5 LCD controllers, > > > > > > clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "vdc50", "vdc51", "spibsc0", "spibsc1"; > > > > > > the <4> needs to become a <6>. > > > > Unless you pass "clkidx" instead of "i" to of_clk_detect_critical() in [1], > > and use "clock-critical = <R7S72100_CLK_SPIBSC0>" in DT? > > > > Unfortunately the exact semantics of clock-critical were never documented. > > Lee? > > /** > * of_clk_detect_critical() - set CLK_IS_CRITICAL flag from Device Tree > * @np: Device node pointer associated with clock provider > * @index: clock index > * @flags: pointer to top-level framework flags > * > * Detects if the clock-critical property exists and, if so, sets the > * corresponding CLK_IS_CRITICAL flag. > * > * Do not use this function. It exists only for legacy Device Tree > * bindings, such as the one-clock-per-node style that are outdated. > * Those bindings typically put all clock data into .dts and the Linux > * driver has no clock data, thus making it impossible to set this flag > * correctly from the driver. Only those drivers may call > * of_clk_detect_critical from their setup functions. > * > * Return: error code or zero on success > */ > > If this meets the criteria, the API is pretty simple/self > explanatory. What are you having trouble with? What exactly is the "index" parameter? This value is matched against the values of the "clock-critical" (array) property, but it is described nowhere in the DT bindings. stih407-clock.dtsi seems to assume this value is an index into the clock-output-names array? Can we use it as a value of "clock-indices" instead? > > Thanks! > > > > [1] "[PATCH 1/6] clk: renesas: mstp: Add critical clock from device > > tree support" > > https://lore.kernel.org/linux-renesas-soc/20191203034519.5640-2-chris.brandt@renesas.com/ Gr{oetje,eeting}s, Geert
On Wed, 04 Dec 2019, Geert Uytterhoeven wrote: > Hi Lee, > > On Wed, Dec 4, 2019 at 9:38 AM Lee Jones <lee.jones@linaro.org> wrote: > > On Tue, 03 Dec 2019, Geert Uytterhoeven wrote: > > > On Tue, Dec 3, 2019 at 7:58 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > > > > On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > > > > > + reg = <0x3fefa000 0x100>, <0x18000000 > > > > > > + 0x4000000>; > > > > > > > > > > The second region conflicts with the XIP flash@18000000 in > > > > > arch/arm/boot/dts/r7s72100-gr-peach.dts. > > > > > Yes, I know it is the same device ;-) > > > > > > > > Is that just an FYI?? Or do you have some suggestion?? > > > > > > Can the flash subnode be compatible with "mtd-rom", even if the parent node > > > is kept disabled? > > > > > > > > > + clock-critical = <4>; /* spibsc0 */ > > > > > > > > > > Iff we go this clock-critical route, I think this should be specified in the > > > > > board-specific .dts instead of in the SoC-specific .dtsi. > > > > > > > > OK, that's fine. It makes more sense to be in the .dts because it's a board > > > > design decision. I will remove it from the patch. > > > > > > > > The one 'tricky' thing is that the <4> is the index into the number of clocks. > > > > > > > > So in the Renesas BSP where it includes the VDC5 LCD controllers, > > > > > > > > clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "vdc50", "vdc51", "spibsc0", "spibsc1"; > > > > > > > > the <4> needs to become a <6>. > > > > > > Unless you pass "clkidx" instead of "i" to of_clk_detect_critical() in [1], > > > and use "clock-critical = <R7S72100_CLK_SPIBSC0>" in DT? > > > > > > Unfortunately the exact semantics of clock-critical were never documented. > > > Lee? > > > > /** > > * of_clk_detect_critical() - set CLK_IS_CRITICAL flag from Device Tree > > * @np: Device node pointer associated with clock provider > > * @index: clock index > > * @flags: pointer to top-level framework flags > > * > > * Detects if the clock-critical property exists and, if so, sets the > > * corresponding CLK_IS_CRITICAL flag. > > * > > * Do not use this function. It exists only for legacy Device Tree > > * bindings, such as the one-clock-per-node style that are outdated. > > * Those bindings typically put all clock data into .dts and the Linux > > * driver has no clock data, thus making it impossible to set this flag > > * correctly from the driver. Only those drivers may call > > * of_clk_detect_critical from their setup functions. > > * > > * Return: error code or zero on success > > */ > > > > If this meets the criteria, the API is pretty simple/self > > explanatory. What are you having trouble with? > > What exactly is the "index" parameter? This value is matched against > the values of the "clock-critical" (array) property, but it is described > nowhere in the DT bindings. > stih407-clock.dtsi seems to assume this value is an index into the > clock-output-names array? > Can we use it as a value of "clock-indices" instead? of_clk_detect_critical(), the consumer of the device tree property 'clock-critical', is a helper. Neither deliver any auto-magical functionality. Simply providing an index into the property will not do anything useful. It's up to the vendor's clock driver to handle. The vendor driver can call of_clk_detect_critical() with *any* index it sees fit. If it matches a number listed in the 'clock-critical' array, the CLK_IS_CRITICAL flag is set in the flags pointed to by the 3rd function parameter. Take a look at some of the call sites in drivers/clk/st/* for further clarification. > > > Thanks! > > > > > > [1] "[PATCH 1/6] clk: renesas: mstp: Add critical clock from device > > > tree support" > > > https://lore.kernel.org/linux-renesas-soc/20191203034519.5640-2-chris.brandt@renesas.com/ > > Gr{oetje,eeting}s, > > Geert >
Hi Lee, Thank you. Hi Geert, On Wed, Dec 4, 2019, Lee Jones wrote: > The vendor driver can call of_clk_detect_critical() with *any* index it sees > fit. If it matches a number listed in the 'clock-critical' > array, the CLK_IS_CRITICAL flag is set in the flags pointed to by the 3rd > function parameter. Since this is the case, I'll change the driver code so we can use it how we prefer: ie, clock-critical = <R7S72100_CLK_SPIBSC0>; Chris
Hi Geert, On Wed, Dec 4, 2019, Geert Uytterhoeven wrote: > > Because....I keep forgetting what is the latest 'correct' size: > > A. The exact size of the register range or > > B. The size rounded up to look nicer > > C. The size used by the on-chip address decoder providing the module's > select signal? I doubt that's not a power of two ;-) Point taken :) I'll change it. Chris
On Tue, Dec 03, 2019 at 07:28:18PM +0100, Geert Uytterhoeven wrote: > On Tue, Dec 3, 2019 at 4:46 AM Chris Brandt <chris.brandt@renesas.com> wrote: > > +static const struct of_device_id of_spibsc_match[] = { > > + { .compatible = "renesas,spibsc"}, > > + { .compatible = "renesas,r7s72100-spibsc", .data = (void *)HAS_SPBCR}, > > + { .compatible = "renesas,r7s9210-spibsc"}, > Do you need to match against all 3 in the driver? > Does SPIBSC work on RZ/A1 when not setting HAS_SPBCR? > If not, the fallback to "renesas,spibsc" is not valid for RZ/A1. I do think it's useful to explicitly list all the compatibles in the driver, it documents the handling needed for each of the variants and it provides some robustness against DTs that neglect to list the fallback compatibles.
On Tue, Dec 03, 2019 at 07:29:45PM +0100, Geert Uytterhoeven wrote: > On Tue, Dec 3, 2019 at 3:19 PM Mark Brown <broonie@kernel.org> wrote: > > > + pm_runtime_put(&pdev->dev); > > > + pm_runtime_disable(&pdev->dev); > > There seems to be no purpose in the runtime PM code in this > > driver, there's no PM operations of any kind and the driver holds > > a runtime PM reference for the entire lifetime of the device. > It matters for the clock domain (assumed the module clock is not always > marked as a critical clock). That seems like a problem with what the clock domains are doing surely? The default is supposed to be that if runtime PM isn't enabled we get the behaviour the driver is manually implementing here. Besides, if this is actually impacting power management I'd expect us to be letting the IP idle rather than holding it powered up all the time.
Hi Mark, On Wed, Dec 4, 2019 at 12:25 PM Mark Brown <broonie@kernel.org> wrote: > On Tue, Dec 03, 2019 at 07:29:45PM +0100, Geert Uytterhoeven wrote: > > On Tue, Dec 3, 2019 at 3:19 PM Mark Brown <broonie@kernel.org> wrote: > > > > + pm_runtime_put(&pdev->dev); > > > > + pm_runtime_disable(&pdev->dev); > > > > There seems to be no purpose in the runtime PM code in this > > > driver, there's no PM operations of any kind and the driver holds > > > a runtime PM reference for the entire lifetime of the device. > > > It matters for the clock domain (assumed the module clock is not always > > marked as a critical clock). > > That seems like a problem with what the clock domains are doing > surely? The default is supposed to be that if runtime PM isn't > enabled we get the behaviour the driver is manually implementing Unfortunately not: if the driver doesn't implement Runtime PM, the default is to not do anything. Later, unused clocks will be disabled, and the device will stop functioning. > here. Besides, if this is actually impacting power management > I'd expect us to be letting the IP idle rather than holding it > powered up all the time. That would be better, definitely. However, that's just an optimization over holding it powered up all the time. Gr{oetje,eeting}s, Geert
Hello Mark, On Tue, Dec 3, 2019, Mark Brown wrote: > > +static void spibsc_write(struct spibsc_priv *sbsc, int reg, u32 val) > > +{ > > + iowrite32(val, sbsc->base + reg); > > +} > > +static void spibsc_write8(struct spibsc_priv *sbsc, int reg, u8 val) > > Looking at a bunch of the stuff here it looks like you could benefit from > regmap, it's got lots of debug infrastructure. Thank you for the suggestion, but I looked into using regmap, and there are a lot of drivers that use it, but I don't think it's going to work well for me. Regmap assumes that all the registers will be the same size. I have to have functions that write with different widths (8/16/32) for a reason. Chris
On Wed, Dec 04, 2019 at 03:51:48PM +0000, Chris Brandt wrote: > On Tue, Dec 3, 2019, Mark Brown wrote: > > Looking at a bunch of the stuff here it looks like you could benefit from > > regmap, it's got lots of debug infrastructure. > Thank you for the suggestion, but I looked into using regmap, and there > are a lot of drivers that use it, but I don't think it's going to work > well for me. > Regmap assumes that all the registers will be the same size. I have to > have functions that write with different widths (8/16/32) for a reason. You *can* have more than one regmap for a device, or if it really only is one or two registers open code accesses to just those registers.
Hi Geert, Thank you for your review and suggestions! On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > +static int spibsc_wait_trans_completion(struct spibsc_priv *sbsc) { > > + int t = 256 * 100000; > > + > > + while (t--) { > > + if (spibsc_read(sbsc, CMNSR) & CMNSR_TEND) > > + return 0; > > + ndelay(1); > > + } > > So this may busy loop for up to 25.6 ms? Oops... > > Can you use the interrupt (SPIHF) instead, signalling a completion? RZ/A1 doesn't have any interrupts. And I think the interrupts in RZ/A2 only work for HyperFlash (not QSPI). But, 25.6ms is way too long! I'll assume the slowest clock to be 1MHz and the max FIFO transfer of 4 bytes. That's 32us, so that's what I'll use. > > + if (t->cs_change) { > > + dev_err(sbsc->dev, "cs_change not supported"); > > + return -EIO; > > + } > > + } > > If you would do the checks above in .prepare_message() (like e.g. rspi > does)... OK, Thanks. :) > > + case 6: > > + dropr |= DROPR_OPD0(tx_data[5]); > > + opde |= (1 << 0); > > Both checkpatch and gcc tell you to add fallthrough coments. OK, fixed. > ... can't you just use .transfer_one(), instead of duplicating the logic from > spi_transfer_one_message()? > Or is there some special detail that's not obvious to the casual reviewer? I guess .transfer_one() could be used if I kept shoving more stuff into .prepare_message(). But, the screwy thing about this controller is that messages that are 'Tx only' are processed differently then 'Tx then Rx' messages and not like a traditional SPI controller. By implementing both conditions in .spi_transfer_one_message(), it makes that more clear for the next person in my opinion because you can see that sometimes we have to work with the entire message as a whole, not just individual pieces. > > +static const struct of_device_id of_spibsc_match[] = { > > + { .compatible = "renesas,spibsc"}, > > + { .compatible = "renesas,r7s72100-spibsc", .data = (void > *)HAS_SPBCR}, > > + { .compatible = "renesas,r7s9210-spibsc"}, > > Do you need to match against all 3 in the driver? > Does SPIBSC work on RZ/A1 when not setting HAS_SPBCR? > If not, the fallback to "renesas,spibsc" is not valid for RZ/A1. OK, I dropped "renesas,spibsc". I like having individual SoC names anyway. Chris