| Message ID | 20191203034519.5640-4-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | spi: Add Renesas SPIBSC controller | expand |
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 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, 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, > > 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
diff --git a/drivers/clk/renesas/r7s9210-cpg-mssr.c b/drivers/clk/renesas/r7s9210-cpg-mssr.c index 14093503c085..153d3a49eee0 100644 --- 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), 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 */ +}; + /* 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,
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> --- drivers/clk/renesas/r7s9210-cpg-mssr.c | 9 +++++++++ 1 file changed, 9 insertions(+)