[PATCH] clk: renesas: rz: Select EXTAL vs USB clock

[PATCH] clk: renesas: rz: Select EXTAL vs USB clock

[Chris Brandt]

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 ) {
+			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 */
-- 
2.9.2

Re: [PATCH] clk: renesas: rz: Select EXTAL vs USB clock

[Geert Uytterhoeven]

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

RE: [PATCH] clk: renesas: rz: Select EXTAL vs USB clock

[Chris Brandt]

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

RE: [PATCH] clk: renesas: rz: Select EXTAL vs USB clock

[Chris Brandt]

SGkgR2VlcnQsDQoNCk9uIEF1ZyAyOSwgMjAxNiwgR2VlcnQgVXl0dGVyaG9ldmVuIHdyb3RlOg0K
Pj4gSW5zdGVhZCBvZiBoYXJkIGNvZGluZyBFWFRBTCBvbmx5LCBjaGVjayBpZiBFWFRBTCB3YXMg
c3BlY2lmaWVkLiBJZiANCj4+IG5vdCwgdGhlbiBhc3N1bWUgdGhlIFVTQiBjbG9jayBpcyB1c2Vk
IGFzIHRoZSBtYWluIHN5c3RlbSBjbG9jay4NCj4+DQo+PiBTaWduZWQtb2ZmLWJ5OiBDaHJpcyBC
cmFuZHQgPGNocmlzLmJyYW5kdEByZW5lc2FzLmNvbT4NCj4NCj4gV2hhdCdzIHRoZSByYXRpb25h
bGUgYmVoaW5kIHRoaXMgY2hhbmdlPw0KDQoNCkknbSB0aGlua2luZyBhYm91dCBiZXlvbmQgUmVu
ZXNhcyBib2FyZHMuIFBlb3BsZSBhcmUgbWFraW5nIFJaL0ExIGJvYXJkcyB3aXRoIGp1c3QgdGhl
IFVTQiBjbG9jayB0byBzYXZlIG9uIEJPTSBjb3N0cyBzaW5jZSAzODRNSHogdnMgNDAwTUh6IGlz
IG5vdCBlbm91Z2ggb2YgYSBkcm9wIHRvIGNhcmUgYWJvdXQuDQpBbmQgb2YgY291cnNlLCBoYXZp
bmcgc29tZW9uZSBtYWtlIHRoaXMga2luZCBvZiBzZWxlY3Rpb24gaW4gdGhlIERUIGlzIGJldHRl
ciB0aGFuIGZvcmNpbmcgdGhlbSB0byBoYWNrIHRoZSBjbG9jayBmaWxlLg0KDQoNCg0KQ2hyaXMN
Cg==

Re: [PATCH] clk: renesas: rz: Select EXTAL vs USB clock

[Geert Uytterhoeven]

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

RE: [PATCH] clk: renesas: rz: Select EXTAL vs USB clock

[Chris Brandt]

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

RE: [PATCH] clk: renesas: rz: Select EXTAL vs USB clock

[Chris Brandt]

SGkgR2VlcnQgVXl0dGVyaG9ldmVuLA0KDQpPbiBBdWcgMjksIDIwMTYsIEdlZXJ0IFV5dHRlcmhv
ZXZlbiB3cm90ZToNCj4gV2UgcmVhbGx5IG5lZWQgdG8gZ2V0IHRoZSBHUElPIGRyaXZlciB1cHN0
cmVhbSwgYW5kIGFkZCBzdXBwb3J0IGZvciByZWFkaW5nIHRoZSBNRF8qIHBpbnMuDQoNCk9LLCB3
ZSBjYW4ganVzdCByZWFkIHRoZSBtb2RlIHBpbiBpbnN0ZWFkIG9mIGxvb2tpbmcgYXQgdGhlIERU
Lg0KDQpJbiBzZXR1cC1yOGE3Nzc5LmMsIHRoZXkgdXNlIGlvcmVtYXAgdG8gcmVhZCB0aGUgbW9k
ZSBhbmQgdGhlbiBwYXNzIHRoZSBtb2RlIGludG8gcjhhNzc3OV9jbG9ja3NfaW5pdCgpIGluIHRo
ZSBjbG9jayBmaWxlLg0KTG9va3MgbGlrZSBzZXR1cC1yY2FyLWdlbjIuYyBkb2VzIHRoZSBzYW1l
IHRoaW5nLg0KDQpTbywgY2FuIHdlIGRvIHRoZSBzYW1lIGluIHNldHVwLXI3czcyMTAwLmMgYW5k
IGNsay1yei5jPyBObyBHUElPIGRyaXZlciBpcyBuZWVkZWQgYW5kIGl0IHdvdWxkIG1hdGNoIHRo
ZSBvdGhlciBwbGF0Zm9ybXMgYXJlIGRvaW5nLg0KDQoNCkNocmlzDQo=

Re: [PATCH] clk: renesas: rz: Select EXTAL vs USB clock

[Geert Uytterhoeven]

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

RE: [PATCH] clk: renesas: rz: Select EXTAL vs USB clock

[Chris Brandt]

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

RE: [PATCH] clk: renesas: rz: Select EXTAL vs USB clock

[Chris Brandt]

SGkgR2VlcnQsDQoNCk9uIE1vbiwgQXVnIDI5LCAyMDE2LCBHZWVydCBVeXR0ZXJob2V2ZW4gd3Jv
dGU6DQoNCj4gVW50aWwgeW91IGhhdmUgYSBwcm9wZXIgR1BJTyBkcml2ZXIsIEkgdGhpbmsgdGhl
IGJlc3Qgc29sdXRpb24gaXMgdG8gaW9yZW1hcCgpIHRoZQ0KPiBoYXJkY29kZWQgcmVnaXN0ZXJz
IGluIGNsay1yei5jLCBhbmQgcmVhZGluZyB0aGUgbW9kZSBwaW5zLg0KDQpPSy4gVGhhbmsgeW91
Lg0KDQoNCkNocmlzDQoNCg==

Re: [PATCH] clk: renesas: rz: Select EXTAL vs USB clock

[Sergei Shtylyov]

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

Re: [PATCH] clk: renesas: rz: Select EXTAL vs USB clock

[Geert Uytterhoeven]

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

[PATCH v2] clk: renesas: rz: Select EXTAL vs USB clock

[Chris Brandt]

Check the MD_CLK pin to determine the current clock mode in order to set
the pll clock parent correctly.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
v2:
* Switched to reading MD_CLK pin to determine mode
---
 drivers/clk/renesas/clk-rz.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/renesas/clk-rz.c b/drivers/clk/renesas/clk-rz.c
index f6312c6..2fc8aae 100644
--- a/drivers/clk/renesas/clk-rz.c
+++ b/drivers/clk/renesas/clk-rz.c
@@ -25,6 +25,9 @@ struct rz_cpg {
 #define CPG_FRQCR	0x10
 #define CPG_FRQCR2	0x14
 
+#define PPR0 0xFCFE3200
+#define PIBC0 0xFCFE7000
+
 /* -----------------------------------------------------------------------------
  * Initialization
  */
@@ -37,10 +40,20 @@ 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);
-
+		unsigned int cpg_mode;
+		const char *parent_name;
+		void __iomem *ppr0, *pibc0;
+
+		/* MD_CLK is on P0_2 */
+		ppr0 = ioremap_nocache(PPR0, 2);
+		pibc0 = ioremap_nocache(PIBC0, 2);
+		BUG_ON(!ppr0 || !pibc0);
+		iowrite16(4, pibc0);	/* Enable input buffer */
+		cpg_mode = ioread16(ppr0) & 4;
+		iounmap(ppr0);
+		iounmap(pibc0);
+
+		parent_name = cpg_mode ? "usb_x1" : "extal";
 		mult = cpg_mode ? (32 / 4) : 30;
 
 		return clk_register_fixed_factor(NULL, name, parent_name, 0, mult, 1);
-- 
2.9.2

Re: [PATCH v2] clk: renesas: rz: Select EXTAL vs USB clock

[Simon Horman]

On Mon, Aug 29, 2016 at 11:13:58PM -0400, Chris Brandt wrote:
> Check the MD_CLK pin to determine the current clock mode in order to set
> the pll clock parent correctly.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> v2:
> * Switched to reading MD_CLK pin to determine mode

Thanks, I have queued this up.

Re: [PATCH v2] clk: renesas: rz: Select EXTAL vs USB clock

[Simon Horman]

On Tue, Aug 30, 2016 at 09:04:44AM +0200, Simon Horman wrote:
> On Mon, Aug 29, 2016 at 11:13:58PM -0400, Chris Brandt wrote:
> > Check the MD_CLK pin to determine the current clock mode in order to set
> > the pll clock parent correctly.
> > 
> > Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> > ---
> > v2:
> > * Switched to reading MD_CLK pin to determine mode
> 
> Thanks, I have queued this up.

Sorry, I hit reply to the wrong message
This patch is for Geert not me.

Re: [PATCH v2] clk: renesas: rz: Select EXTAL vs USB clock

[Geert Uytterhoeven]

Hi Chris,

On Tue, Aug 30, 2016 at 5:13 AM, Chris Brandt <chris.brandt@renesas.com> wrote:
> Check the MD_CLK pin to determine the current clock mode in order to set
> the pll clock parent correctly.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Thanks for your patch!

> ---
> v2:
> * Switched to reading MD_CLK pin to determine mode
> ---
>  drivers/clk/renesas/clk-rz.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/renesas/clk-rz.c b/drivers/clk/renesas/clk-rz.c
> index f6312c6..2fc8aae 100644
> --- a/drivers/clk/renesas/clk-rz.c
> +++ b/drivers/clk/renesas/clk-rz.c
> @@ -25,6 +25,9 @@ struct rz_cpg {
>  #define CPG_FRQCR      0x10
>  #define CPG_FRQCR2     0x14
>
> +#define PPR0 0xFCFE3200
> +#define PIBC0 0xFCFE7000
> +
>  /* -----------------------------------------------------------------------------
>   * Initialization
>   */
> @@ -37,10 +40,20 @@ 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);
> -
> +               unsigned int cpg_mode;
> +               const char *parent_name;
> +               void __iomem *ppr0, *pibc0;
> +
> +               /* MD_CLK is on P0_2 */
> +               ppr0 = ioremap_nocache(PPR0, 2);
> +               pibc0 = ioremap_nocache(PIBC0, 2);
> +               BUG_ON(!ppr0 || !pibc0);
> +               iowrite16(4, pibc0);    /* Enable input buffer */
> +               cpg_mode = ioread16(ppr0) & 4;
> +               iounmap(ppr0);
> +               iounmap(pibc0);
> +
> +               parent_name = cpg_mode ? "usb_x1" : "extal";

If you would use "cpg_mode = (ioread16(ppr0) >> 2) & 1;", the value of
cpg_mode would be in sync with Table 6.2. Then you can avoid relying on actual
clock names in DT, and keep

    parent_name = of_clk_get_parent_name(np, cpg_mode);

The bindings already dictate the parent clocks must match clock modes in the
datasheet, i.e. Table 6.2.

For easier maintenance, I would factor out reading the mode pins in a separate
function. When a proper GPIO driver is added, the function can be removed,
and its callsite updated.

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

RE: [PATCH v2] clk: renesas: rz: Select EXTAL vs USB clock

[Chris Brandt]

Hi Geert,

Thank you for the good review!


On Aug 30, 2016, Geert Uytterhoeven wrote:
> If you would use "cpg_mode = (ioread16(ppr0) >> 2) & 1;", the value of cpg_mode would
> be in sync with Table 6.2. Then you can avoid relying on actual clock names in DT, and
> keep
>
>    parent_name = of_clk_get_parent_name(np, cpg_mode);
>
> The bindings already dictate the parent clocks must match clock modes in the datasheet,
> i.e. Table 6.2.

Cool trick. I guess I don't get understand all the "DT Magic" yet.
I'll update the patch and resend.


> For easier maintenance, I would factor out reading the mode pins in a separate function.
> When a proper GPIO driver is added, the function can be removed, and its callsite updated.

OK. I see setup-rcar-gen2.c had a separate rcar_gen2_read_mode_pins() function.
Inside of clk-rz.c I'll make a rz_cpg_read_mode_pin() function.

Chris

RE: [PATCH v2] clk: renesas: rz: Select EXTAL vs USB clock

[Chris Brandt]

SGkgR2VlcnQsDQoNClRoYW5rIHlvdSBmb3IgdGhlIGdvb2QgcmV2aWV3IQ0KDQoNCk9uIEF1ZyAz
MCwgMjAxNiwgR2VlcnQgVXl0dGVyaG9ldmVuIHdyb3RlOg0KPiBJZiB5b3Ugd291bGQgdXNlICJj
cGdfbW9kZSA9IChpb3JlYWQxNihwcHIwKSA+PiAyKSAmIDE7IiwgdGhlIHZhbHVlIG9mIGNwZ19t
b2RlIHdvdWxkDQo+IGJlIGluIHN5bmMgd2l0aCBUYWJsZSA2LjIuIFRoZW4geW91IGNhbiBhdm9p
ZCByZWx5aW5nIG9uIGFjdHVhbCBjbG9jayBuYW1lcyBpbiBEVCwgYW5kDQo+IGtlZXANCj4NCj4g
ICAgcGFyZW50X25hbWUgPSBvZl9jbGtfZ2V0X3BhcmVudF9uYW1lKG5wLCBjcGdfbW9kZSk7DQo+
DQo+IFRoZSBiaW5kaW5ncyBhbHJlYWR5IGRpY3RhdGUgdGhlIHBhcmVudCBjbG9ja3MgbXVzdCBt
YXRjaCBjbG9jayBtb2RlcyBpbiB0aGUgZGF0YXNoZWV0LA0KPiBpLmUuIFRhYmxlIDYuMi4NCg0K
Q29vbCB0cmljay4gSSBndWVzcyBJIGRvbid0IGdldCB1bmRlcnN0YW5kIGFsbCB0aGUgIkRUIE1h
Z2ljIiB5ZXQuDQpJJ2xsIHVwZGF0ZSB0aGUgcGF0Y2ggYW5kIHJlc2VuZC4NCg0KDQo+IEZvciBl
YXNpZXIgbWFpbnRlbmFuY2UsIEkgd291bGQgZmFjdG9yIG91dCByZWFkaW5nIHRoZSBtb2RlIHBp
bnMgaW4gYSBzZXBhcmF0ZSBmdW5jdGlvbi4NCj4gV2hlbiBhIHByb3BlciBHUElPIGRyaXZlciBp
cyBhZGRlZCwgdGhlIGZ1bmN0aW9uIGNhbiBiZSByZW1vdmVkLCBhbmQgaXRzIGNhbGxzaXRlIHVw
ZGF0ZWQuDQoNCk9LLiBJIHNlZSBzZXR1cC1yY2FyLWdlbjIuYyBoYWQgYSBzZXBhcmF0ZSByY2Fy
X2dlbjJfcmVhZF9tb2RlX3BpbnMoKSBmdW5jdGlvbi4NCkluc2lkZSBvZiBjbGstcnouYyBJJ2xs
IG1ha2UgYSByel9jcGdfcmVhZF9tb2RlX3BpbigpIGZ1bmN0aW9uLg0KDQpDaHJpcw0K

[PATCH v3] clk: renesas: rz: Select EXTAL vs USB clock

[Chris Brandt]

Check the MD_CLK pin to determine the current clock mode in order to set
the pll clock parent correctly.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
v3:
* move reading GPIO port into separate function
v2:
* Switched to reading MD_CLK pin to determine mode
---
 drivers/clk/renesas/clk-rz.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/renesas/clk-rz.c b/drivers/clk/renesas/clk-rz.c
index f6312c6..29a8638 100644
--- a/drivers/clk/renesas/clk-rz.c
+++ b/drivers/clk/renesas/clk-rz.c
@@ -25,10 +25,34 @@ struct rz_cpg {
 #define CPG_FRQCR	0x10
 #define CPG_FRQCR2	0x14
 
+#define PPR0 0xFCFE3200
+#define PIBC0 0xFCFE7000
+
+#define MD_BOOT0(x) ((x >> 0) & 1)	/* P0_0 */
+#define MD_BOOT1(x) ((x >> 1) & 1)	/* P0_1 */
+#define MD_CLK(x)   ((x >> 2) & 1)	/* P0_2 */
+#define MD_CLKS(x)  ((x >> 3) & 1)	/* P0_3 */
+
 /* -----------------------------------------------------------------------------
  * Initialization
  */
 
+u16 rz_cpg_read_mode_pins(void)
+{
+	void __iomem *ppr0, *pibc0;
+	u16 modes;
+
+	ppr0 = ioremap_nocache(PPR0, 2);
+	pibc0 = ioremap_nocache(PIBC0, 2);
+	BUG_ON(!ppr0 || !pibc0);
+	iowrite16(4, pibc0);	/* enable input buffer */
+	modes = ioread16(ppr0);
+	iounmap(ppr0);
+	iounmap(pibc0);
+
+	return modes;
+}
+
 static struct clk * __init
 rz_cpg_register_clock(struct device_node *np, struct rz_cpg *cpg, const char *name)
 {
@@ -37,10 +61,11 @@ 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);
+		unsigned int cpg_mode;
+		const char *parent_name;
 
+		cpg_mode = MD_CLK(rz_cpg_read_mode_pins());
+		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);
-- 
2.9.2

Re: [PATCH v3] clk: renesas: rz: Select EXTAL vs USB clock

[Geert Uytterhoeven]

Hi Chris,

On Thu, Sep 1, 2016 at 3:10 PM, Chris Brandt <chris.brandt@renesas.com> wrote:
> Check the MD_CLK pin to determine the current clock mode in order to set
> the pll clock parent correctly.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Thanks for the update!

> ---
> v3:
> * move reading GPIO port into separate function
> v2:
> * Switched to reading MD_CLK pin to determine mode
> ---
>  drivers/clk/renesas/clk-rz.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/renesas/clk-rz.c b/drivers/clk/renesas/clk-rz.c
> index f6312c6..29a8638 100644
> --- a/drivers/clk/renesas/clk-rz.c
> +++ b/drivers/clk/renesas/clk-rz.c
> @@ -25,10 +25,34 @@ struct rz_cpg {
>  #define CPG_FRQCR      0x10
>  #define CPG_FRQCR2     0x14
>
> +#define PPR0 0xFCFE3200
> +#define PIBC0 0xFCFE7000
> +
> +#define MD_BOOT0(x) ((x >> 0) & 1)     /* P0_0 */
> +#define MD_BOOT1(x) ((x >> 1) & 1)     /* P0_1 */
> +#define MD_CLK(x)   ((x >> 2) & 1)     /* P0_2 */

Good idea to use a macro for that!
However, adding more (unused) macros may trigger more questions from reviewers,
cfr. below ;-)

> +#define MD_CLKS(x)  ((x >> 3) & 1)     /* P0_3 */
> +
>  /* -----------------------------------------------------------------------------
>   * Initialization
>   */
>
> +u16 rz_cpg_read_mode_pins(void)

static u16 __init rz_cpg_read_mode_pins(void)

Or do you intend to make this a global function?
There's also MD_BOOT2 at P7_0. To read that, you have to map and read a
different set of registers, though...

> +{
> +       void __iomem *ppr0, *pibc0;
> +       u16 modes;
> +
> +       ppr0 = ioremap_nocache(PPR0, 2);
> +       pibc0 = ioremap_nocache(PIBC0, 2);
> +       BUG_ON(!ppr0 || !pibc0);
> +       iowrite16(4, pibc0);    /* enable input buffer */
> +       modes = ioread16(ppr0);
> +       iounmap(ppr0);
> +       iounmap(pibc0);
> +
> +       return modes;
> +}
> +
>  static struct clk * __init
>  rz_cpg_register_clock(struct device_node *np, struct rz_cpg *cpg, const char *name)
>  {
> @@ -37,10 +61,11 @@ 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 */

I'd just assign "MD_CLK(rz_cpg_read_mode_pins())" to cpg_mode here...

> -               const char *parent_name = of_clk_get_parent_name(np, cpg_mode);
> +               unsigned int cpg_mode;
> +               const char *parent_name;
>
> +               cpg_mode = MD_CLK(rz_cpg_read_mode_pins());
> +               parent_name = of_clk_get_parent_name(np, cpg_mode);

... and not bother splitting the lines, as they don't break the
80-chars-per-line
limit.

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

RE: [PATCH v3] clk: renesas: rz: Select EXTAL vs USB clock

[Chris Brandt]

Hi Geert,

On 9/1/2016, Geert Uytterhoeven wrote:
> > +#define MD_BOOT0(x) ((x >> 0) & 1)     /* P0_0 */
> > +#define MD_BOOT1(x) ((x >> 1) & 1)     /* P0_1 */
> > +#define MD_CLK(x)   ((x >> 2) & 1)     /* P0_2 */
>
> Good idea to use a macro for that!
> However, adding more (unused) macros may trigger more questions
> from reviewers, cfr. below ;-)

OK, I can pull the other ones out. I'm OK with that.


> > +#define MD_CLKS(x)  ((x >> 3) & 1)     /* P0_3 */
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Initialization
> >   */
> >
> > +u16 rz_cpg_read_mode_pins(void)
>
> static u16 __init rz_cpg_read_mode_pins(void)
>
> Or do you intend to make this a global function?

Ooops. I'll make it static.



> There's also MD_BOOT2 at P7_0. To read that, you have to map and read
> a different set of registers, though...

Good point. So...going back to your first comment, there is no reason for making macros or discussing mode pins that you have no plan on dealing with.


> >         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 */
>
> I'd just assign "MD_CLK(rz_cpg_read_mode_pins())" to cpg_mode here...
>
> > -               const char *parent_name = of_clk_get_parent_name(np, cpg_mode);
> > +               unsigned int cpg_mode;
> > +               const char *parent_name;
> >
> > +               cpg_mode = MD_CLK(rz_cpg_read_mode_pins());
> > +               parent_name = of_clk_get_parent_name(np, cpg_mode);
>
>... and not bother splitting the lines, as they don't break the 80-chars-per-line limit.

But I get paid per line of code!!!   (just kidding)
I'll make the changes and resend.


Thanks for your review!


Chris

RE: [PATCH v3] clk: renesas: rz: Select EXTAL vs USB clock

[Chris Brandt]

SGkgR2VlcnQsDQoNCk9uIDkvMS8yMDE2LCBHZWVydCBVeXR0ZXJob2V2ZW4gd3JvdGU6DQo+ID4g
KyNkZWZpbmUgTURfQk9PVDAoeCkgKCh4ID4+IDApICYgMSkgICAgIC8qIFAwXzAgKi8NCj4gPiAr
I2RlZmluZSBNRF9CT09UMSh4KSAoKHggPj4gMSkgJiAxKSAgICAgLyogUDBfMSAqLw0KPiA+ICsj
ZGVmaW5lIE1EX0NMSyh4KSAgICgoeCA+PiAyKSAmIDEpICAgICAvKiBQMF8yICovDQo+DQo+IEdv
b2QgaWRlYSB0byB1c2UgYSBtYWNybyBmb3IgdGhhdCENCj4gSG93ZXZlciwgYWRkaW5nIG1vcmUg
KHVudXNlZCkgbWFjcm9zIG1heSB0cmlnZ2VyIG1vcmUgcXVlc3Rpb25zDQo+IGZyb20gcmV2aWV3
ZXJzLCBjZnIuIGJlbG93IDstKQ0KDQpPSywgSSBjYW4gcHVsbCB0aGUgb3RoZXIgb25lcyBvdXQu
IEknbSBPSyB3aXRoIHRoYXQuDQoNCg0KPiA+ICsjZGVmaW5lIE1EX0NMS1MoeCkgICgoeCA+PiAz
KSAmIDEpICAgICAvKiBQMF8zICovDQo+ID4gKw0KPiA+ICAvKiAtLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LQ0KPiA+ICAgKiBJbml0aWFsaXphdGlvbg0KPiA+ICAgKi8NCj4gPg0KPiA+ICt1MTYgcnpfY3Bn
X3JlYWRfbW9kZV9waW5zKHZvaWQpDQo+DQo+IHN0YXRpYyB1MTYgX19pbml0IHJ6X2NwZ19yZWFk
X21vZGVfcGlucyh2b2lkKQ0KPg0KPiBPciBkbyB5b3UgaW50ZW5kIHRvIG1ha2UgdGhpcyBhIGds
b2JhbCBmdW5jdGlvbj8NCg0KT29vcHMuIEknbGwgbWFrZSBpdCBzdGF0aWMuDQoNCg0KDQo+IFRo
ZXJlJ3MgYWxzbyBNRF9CT09UMiBhdCBQN18wLiBUbyByZWFkIHRoYXQsIHlvdSBoYXZlIHRvIG1h
cCBhbmQgcmVhZA0KPiBhIGRpZmZlcmVudCBzZXQgb2YgcmVnaXN0ZXJzLCB0aG91Z2guLi4NCg0K
R29vZCBwb2ludC4gU28uLi5nb2luZyBiYWNrIHRvIHlvdXIgZmlyc3QgY29tbWVudCwgdGhlcmUg
aXMgbm8gcmVhc29uIGZvciBtYWtpbmcgbWFjcm9zIG9yIGRpc2N1c3NpbmcgbW9kZSBwaW5zIHRo
YXQgeW91IGhhdmUgbm8gcGxhbiBvbiBkZWFsaW5nIHdpdGguDQoNCg0KPiA+ICAgICAgICAgaWYg
KHN0cmNtcChuYW1lLCAicGxsIikgPT0gMCkgew0KPiA+IC0gICAgICAgICAgICAgICAvKiBGSVhN
RTogY3BnX21vZGUgc2hvdWxkIGJlIHJlYWQgZnJvbSBHUElPLiBCdXQgbm8gR1BJTyBzdXBwb3J0
IHlldCAqLw0KPiA+IC0gICAgICAgICAgICAgICB1bnNpZ25lZCBjcGdfbW9kZSA9IDA7IC8qIGhh
cmRjb2RlZCB0byBFWFRBTCBmb3Igbm93ICovDQo+DQo+IEknZCBqdXN0IGFzc2lnbiAiTURfQ0xL
KHJ6X2NwZ19yZWFkX21vZGVfcGlucygpKSIgdG8gY3BnX21vZGUgaGVyZS4uLg0KPg0KPiA+IC0g
ICAgICAgICAgICAgICBjb25zdCBjaGFyICpwYXJlbnRfbmFtZSA9IG9mX2Nsa19nZXRfcGFyZW50
X25hbWUobnAsIGNwZ19tb2RlKTsNCj4gPiArICAgICAgICAgICAgICAgdW5zaWduZWQgaW50IGNw
Z19tb2RlOw0KPiA+ICsgICAgICAgICAgICAgICBjb25zdCBjaGFyICpwYXJlbnRfbmFtZTsNCj4g
Pg0KPiA+ICsgICAgICAgICAgICAgICBjcGdfbW9kZSA9IE1EX0NMSyhyel9jcGdfcmVhZF9tb2Rl
X3BpbnMoKSk7DQo+ID4gKyAgICAgICAgICAgICAgIHBhcmVudF9uYW1lID0gb2ZfY2xrX2dldF9w
YXJlbnRfbmFtZShucCwgY3BnX21vZGUpOw0KPg0KPi4uLiBhbmQgbm90IGJvdGhlciBzcGxpdHRp
bmcgdGhlIGxpbmVzLCBhcyB0aGV5IGRvbid0IGJyZWFrIHRoZSA4MC1jaGFycy1wZXItbGluZSBs
aW1pdC4NCg0KQnV0IEkgZ2V0IHBhaWQgcGVyIGxpbmUgb2YgY29kZSEhISAgIChqdXN0IGtpZGRp
bmcpDQpJJ2xsIG1ha2UgdGhlIGNoYW5nZXMgYW5kIHJlc2VuZC4NCg0KDQpUaGFua3MgZm9yIHlv
dXIgcmV2aWV3IQ0KDQoNCkNocmlzDQo=

[PATCH v4] clk: renesas: rz: Select EXTAL vs USB clock

[Chris Brandt]

Check the MD_CLK pin to determine the current clock mode in order to set
the pll clock parent correctly.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
V4:
* added static and __init to rz_cpg_read_mode_pins
* set cpg_mode during declaration 
v3:
* move reading GPIO port into separate function
v2:
* Switched to reading MD_CLK pin to determine mode
---
 drivers/clk/renesas/clk-rz.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/renesas/clk-rz.c b/drivers/clk/renesas/clk-rz.c
index f6312c6..6dd490c 100644
--- a/drivers/clk/renesas/clk-rz.c
+++ b/drivers/clk/renesas/clk-rz.c
@@ -25,10 +25,31 @@ struct rz_cpg {
 #define CPG_FRQCR	0x10
 #define CPG_FRQCR2	0x14
 
+#define PPR0 0xFCFE3200
+#define PIBC0 0xFCFE7000
+
+#define MD_CLK(x)   ((x >> 2) & 1)	/* P0_2 */
+
 /* -----------------------------------------------------------------------------
  * Initialization
  */
 
+static u16 __init rz_cpg_read_mode_pins(void)
+{
+	void __iomem *ppr0, *pibc0;
+	u16 modes;
+
+	ppr0 = ioremap_nocache(PPR0, 2);
+	pibc0 = ioremap_nocache(PIBC0, 2);
+	BUG_ON(!ppr0 || !pibc0);
+	iowrite16(4, pibc0);	/* enable input buffer */
+	modes = ioread16(ppr0);
+	iounmap(ppr0);
+	iounmap(pibc0);
+
+	return modes;
+}
+
 static struct clk * __init
 rz_cpg_register_clock(struct device_node *np, struct rz_cpg *cpg, const char *name)
 {
@@ -37,8 +58,7 @@ 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 */
+		unsigned int cpg_mode = MD_CLK(rz_cpg_read_mode_pins());
 		const char *parent_name = of_clk_get_parent_name(np, cpg_mode);
 
 		mult = cpg_mode ? (32 / 4) : 30;
-- 
2.9.2

Re: [PATCH v4] clk: renesas: rz: Select EXTAL vs USB clock

[Geert Uytterhoeven]

On Fri, Sep 2, 2016 at 4:32 AM, Chris Brandt <chris.brandt@renesas.com> wrote:
> Check the MD_CLK pin to determine the current clock mode in order to set
> the pll clock parent correctly.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> ---
> V4:
> * added static and __init to rz_cpg_read_mode_pins
> * set cpg_mode during declaration
> v3:
> * move reading GPIO port into separate function
> v2:
> * Switched to reading MD_CLK pin to determine mode

Thanks for the update, will queue in clk-renesas-for-v4.X...

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