| Message ID | 20160825190518.20764-1-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Chris, On Thu, Aug 25, 2016 at 9:05 PM, Chris Brandt <chris.brandt@renesas.com> wrote: > Instead of hard coding EXTAL only, check if EXTAL was specified. If not, > then assume the USB clock is used as the main system clock. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> What's the rationale behind this change? According to the schematics, all of Genmai, RSK, and GR-Peach have both EXTAL and USB_X1 populated. So they can only use EXTAL, both with and without your patch. But both Genmai and RSK have a DIP switch to control MD_CLK, and select the clock to use, while GR-Peach has MD_CLK hardwired to GND. > --- > drivers/clk/renesas/clk-rz.c | 30 +++++++++++++++++++++++------- > 1 file changed, 23 insertions(+), 7 deletions(-) > > diff --git a/drivers/clk/renesas/clk-rz.c b/drivers/clk/renesas/clk-rz.c > index f6312c6..466b9fc 100644 > --- a/drivers/clk/renesas/clk-rz.c > +++ b/drivers/clk/renesas/clk-rz.c > @@ -37,13 +37,29 @@ rz_cpg_register_clock(struct device_node *np, struct rz_cpg *cpg, const char *na > static const unsigned frqcr_tab[4] = { 3, 2, 0, 1 }; > > if (strcmp(name, "pll") == 0) { > - /* FIXME: cpg_mode should be read from GPIO. But no GPIO support yet */ > - unsigned cpg_mode = 0; /* hardcoded to EXTAL for now */ > - const char *parent_name = of_clk_get_parent_name(np, cpg_mode); > - > - mult = cpg_mode ? (32 / 4) : 30; > - > - return clk_register_fixed_factor(NULL, name, parent_name, 0, mult, 1); > + u32 freq = 0; > + struct device_node *np; > + > + /* If a clock-frequency for extal was specified, assume EXTAL boot */ 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 Aug 29, 2016, Geert Uytterhoeven wrote: >> Instead of hard coding EXTAL only, check if EXTAL was specified. If >> not, then assume the USB clock is used as the main system clock. >> >> Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > > What's the rationale behind this change? I'm thinking about beyond Renesas boards. People are making RZ/A1 boards with just the USB clock to save on BOM costs since 384MHz vs 400MHz is not enough of a drop to care about. And of course, having someone make this kind of selection in the DT is better than forcing them to hack the clock file. Chris
Hi Chris, On Mon, Aug 29, 2016 at 4:53 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Aug 29, 2016, Geert Uytterhoeven wrote: >>> Instead of hard coding EXTAL only, check if EXTAL was specified. If >>> not, then assume the USB clock is used as the main system clock. >>> >>> Signed-off-by: Chris Brandt <chris.brandt@renesas.com> >> >> What's the rationale behind this change? > > I'm thinking about beyond Renesas boards. People are making RZ/A1 boards with just the USB clock to save on BOM costs since 384MHz vs 400MHz is not enough of a drop to care about. > And of course, having someone make this kind of selection in the DT is better than forcing them to hack the clock file. We really need to get the GPIO driver upstream, and add support for reading the MD_* pins. 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 Uytterhoeven,
On Aug 29, 2016, Geert Uytterhoeven wrote:
> We really need to get the GPIO driver upstream, and add support for reading the MD_* pins.
OK, we can just read the mode pin instead of looking at the DT.
In setup-r8a7779.c, they use ioremap to read the mode and then pass the mode into r8a7779_clocks_init() in the clock file.
Looks like setup-rcar-gen2.c does the same thing.
So, can we do the same in setup-r7s72100.c and clk-rz.c? No GPIO driver is needed and it would match the other platforms are doing.
Chris
Hi Chris, On Mon, Aug 29, 2016 at 5:32 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Aug 29, 2016, Geert Uytterhoeven wrote: >> We really need to get the GPIO driver upstream, and add support for reading the MD_* pins. > > OK, we can just read the mode pin instead of looking at the DT. > > In setup-r8a7779.c, they use ioremap to read the mode and then pass the mode into r8a7779_clocks_init() in the clock file. > Looks like setup-rcar-gen2.c does the same thing. > > So, can we do the same in setup-r7s72100.c and clk-rz.c? No GPIO driver is needed and it would match the other platforms are doing. Please don't: we're trying to get rid of reading the mode pins (a) in setup-*.c and passing them to the clock driver, (b) without a proper driver. Until you have a proper GPIO driver, I think the best solution is to ioremap() the hardcoded registers in clk-rz.c, and reading the mode pins. 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 Mon, Aug 29, 2016, Geert Uytterhoeven wrote: > Until you have a proper GPIO driver, I think the best solution is to ioremap() the > hardcoded registers in clk-rz.c, and reading the mode pins. OK. Thank you. Chris
Hello. On 08/25/2016 10:05 PM, Chris Brandt wrote: > Instead of hard coding EXTAL only, check if EXTAL was specified. If not, > then assume the USB clock is used as the main system clock. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > drivers/clk/renesas/clk-rz.c | 30 +++++++++++++++++++++++------- > 1 file changed, 23 insertions(+), 7 deletions(-) > > diff --git a/drivers/clk/renesas/clk-rz.c b/drivers/clk/renesas/clk-rz.c > index f6312c6..466b9fc 100644 > --- a/drivers/clk/renesas/clk-rz.c > +++ b/drivers/clk/renesas/clk-rz.c > @@ -37,13 +37,29 @@ rz_cpg_register_clock(struct device_node *np, struct rz_cpg *cpg, const char *na > static const unsigned frqcr_tab[4] = { 3, 2, 0, 1 }; > > if (strcmp(name, "pll") == 0) { > - /* FIXME: cpg_mode should be read from GPIO. But no GPIO support yet */ > - unsigned cpg_mode = 0; /* hardcoded to EXTAL for now */ > - const char *parent_name = of_clk_get_parent_name(np, cpg_mode); > - > - mult = cpg_mode ? (32 / 4) : 30; > - > - return clk_register_fixed_factor(NULL, name, parent_name, 0, mult, 1); > + u32 freq = 0; > + struct device_node *np; > + > + /* If a clock-frequency for extal was specified, assume EXTAL boot */ > + np = of_find_node_by_name(NULL, "extal"); > + if( np ) { if (np) { Please run your patches thru scripts/checkpatrch.pl. > + of_property_read_u32(np, "clock-frequency", &freq); > + if( freq ) if (freq) > + return clk_register_fixed_factor(NULL, "pll", "extal", > + 0, 30, 1); > + } > + > + /* Must be USB clock boot */ > + np = of_find_node_by_name(NULL, "usb_x1"); "usb_x1" looks like a board specific name too much. Previously on R-Car we had the USB_EXTAL pin, maybe that one would be better? > + if( np ) { if (np) { > + of_property_read_u32(np, "clock-frequency", &freq); > + if( freq ) if (freq) > + return clk_register_fixed_factor(NULL, "pll", "usb_x1", > + 0, (32 / 4), 1); The inner parens not needed. [...] MBR, Sergei
On Mon, Aug 29, 2016 at 8:39 PM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: >> + np = of_find_node_by_name(NULL, "usb_x1"); > > > "usb_x1" looks like a board specific name too much. Previously on R-Car > we had the USB_EXTAL pin, maybe that one would be better? See Documentation/devicetree/bindings/clock/renesas,rz-cpg-clocks.txt 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
diff --git a/drivers/clk/renesas/clk-rz.c b/drivers/clk/renesas/clk-rz.c index f6312c6..466b9fc 100644 --- a/drivers/clk/renesas/clk-rz.c +++ b/drivers/clk/renesas/clk-rz.c @@ -37,13 +37,29 @@ rz_cpg_register_clock(struct device_node *np, struct rz_cpg *cpg, const char *na static const unsigned frqcr_tab[4] = { 3, 2, 0, 1 }; if (strcmp(name, "pll") == 0) { - /* FIXME: cpg_mode should be read from GPIO. But no GPIO support yet */ - unsigned cpg_mode = 0; /* hardcoded to EXTAL for now */ - const char *parent_name = of_clk_get_parent_name(np, cpg_mode); - - mult = cpg_mode ? (32 / 4) : 30; - - return clk_register_fixed_factor(NULL, name, parent_name, 0, mult, 1); + u32 freq = 0; + struct device_node *np; + + /* If a clock-frequency for extal was specified, assume EXTAL boot */ + np = of_find_node_by_name(NULL, "extal"); + if( np ) { + of_property_read_u32(np, "clock-frequency", &freq); + if( freq ) + return clk_register_fixed_factor(NULL, "pll", "extal", + 0, 30, 1); + } + + /* Must be USB clock boot */ + np = of_find_node_by_name(NULL, "usb_x1"); + if( np ) { + of_property_read_u32(np, "clock-frequency", &freq); + if( freq ) + return clk_register_fixed_factor(NULL, "pll", "usb_x1", + 0, (32 / 4), 1); + } + + /* No clock frequency set in DT */ + BUG(); } /* If mapping regs failed, skip non-pll clocks. System will boot anyhow */
Instead of hard coding EXTAL only, check if EXTAL was specified. If not, then assume the USB clock is used as the main system clock. Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- drivers/clk/renesas/clk-rz.c | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-)