Re: [PATCH v2] clk: shmobile: r8a7795: Add SDHI clocks

Re: [PATCH v2] clk: shmobile: r8a7795: Add SDHI clocks

[Dirk Behme]

On 14.01.2016 19:24, Geert Uytterhoeven wrote:
> Hi Dirk,
>
> On Sat, Jan 9, 2016 at 7:42 AM, Dirk Behme <dirk.behme@gmail.com> wrote:
>> On 24.12.2015 11:09, Dirk Behme wrote:
>>> Add R8A7795 SDHI clocks.
>
> Thanks for your patch!
>
>>> Signed-off-by: Dirk Behme <dirk.behme@gmail.com>
>>> ---
>>> Changes in v2: Add the missing *H clocks and correct the dividers.
>>>
>>> This replaces v1
>>>
>>> http://www.spinics.net/lists/linux-sh/msg47464.html
>>>
>>>    drivers/clk/shmobile/r8a7795-cpg-mssr.c | 13 ++++++++++++-
>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/shmobile/r8a7795-cpg-mssr.c
>>> b/drivers/clk/shmobile/r8a7795-cpg-mssr.c
>>> index 05479e6..f30ed32 100644
>>> --- a/drivers/clk/shmobile/r8a7795-cpg-mssr.c
>>> +++ b/drivers/clk/shmobile/r8a7795-cpg-mssr.c
>>> @@ -100,8 +100,15 @@ static const struct cpg_core_clk r8a7795_core_clks[]
>>> __initconst = {
>>>          DEF_FIXED("s3d2",       R8A7795_CLK_S3D2,  CLK_S3,         2, 1),
>>>          DEF_FIXED("s3d4",       R8A7795_CLK_S3D4,  CLK_S3,         4, 1),
>>>          DEF_FIXED("cl",         R8A7795_CLK_CL,    CLK_PLL1_DIV2, 48, 1),
>>> +       DEF_FIXED("sd0h",       R8A7795_CLK_SD0H,  CLK_PLL1_DIV2,  2, 1),
>>> +       DEF_FIXED("sd0",        R8A7795_CLK_SD0,   CLK_PLL1_DIV2,  8, 1),
>>> +       DEF_FIXED("sd1h",       R8A7795_CLK_SD1H,  CLK_PLL1_DIV2,  2, 1),
>>> +       DEF_FIXED("sd1",        R8A7795_CLK_SD1,   CLK_PLL1_DIV2,  8, 1),
>>> +       DEF_FIXED("sd2h",       R8A7795_CLK_SD2H,  CLK_PLL1_DIV2,  2, 1),
>>> +       DEF_FIXED("sd2",        R8A7795_CLK_SD2,   CLK_PLL1_DIV2,  8, 1),
>>> +       DEF_FIXED("sd3h",       R8A7795_CLK_SD3H,  CLK_PLL1_DIV2,  2, 1),
>>> +       DEF_FIXED("sd3",        R8A7795_CLK_SD3,   CLK_PLL1_DIV2,  8, 1),
>
> The dividers for these clocks are not fixed, they are controlled by the
> SDnCKCR registers.
>
> Unfortunately the register layout is more complicated than on R-Car Gen2, so
> you can no longer use clk_register_divider_table(), but have to write a custom
> clock driver.
>
> For an initial version, a simple "read-only" version that just calls
> clk_register_fixed_factor() with divider values read from the hardware
> registers may be good enough. But for full support, you need a driver that
> can program the registers, too.


Anything like

https://git.kernel.org/cgit/linux/kernel/git/horms/renesas-bsp.git/commit/drivers/clk/shmobile/clk-rcar-gen3.c?h=v4.2/rcar-3.0.x&id=cd10385afc15cef6bfbaea4aa5da41193b24fe82

?

Best regards

Dirk

Re: [PATCH v2] clk: shmobile: r8a7795: Add SDHI clocks

[Geert Uytterhoeven]

Hi Dirk,

On Fri, Jan 15, 2016 at 11:11 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> On 14.01.2016 19:24, Geert Uytterhoeven wrote:
>> On Sat, Jan 9, 2016 at 7:42 AM, Dirk Behme <dirk.behme@gmail.com> wrote:
>>> On 24.12.2015 11:09, Dirk Behme wrote:
>>>> Add R8A7795 SDHI clocks.
>>>> Signed-off-by: Dirk Behme <dirk.behme@gmail.com>
>>>> ---
>>>> Changes in v2: Add the missing *H clocks and correct the dividers.
>>>>
>>>> This replaces v1
>>>>
>>>> http://www.spinics.net/lists/linux-sh/msg47464.html
>>>>
>>>>    drivers/clk/shmobile/r8a7795-cpg-mssr.c | 13 ++++++++++++-
>>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clk/shmobile/r8a7795-cpg-mssr.c
>>>> b/drivers/clk/shmobile/r8a7795-cpg-mssr.c
>>>> index 05479e6..f30ed32 100644
>>>> --- a/drivers/clk/shmobile/r8a7795-cpg-mssr.c
>>>> +++ b/drivers/clk/shmobile/r8a7795-cpg-mssr.c
>>>> @@ -100,8 +100,15 @@ static const struct cpg_core_clk
>>>> r8a7795_core_clks[]
>>>> __initconst = {
>>>>          DEF_FIXED("s3d2",       R8A7795_CLK_S3D2,  CLK_S3,         2,
>>>> 1),
>>>>          DEF_FIXED("s3d4",       R8A7795_CLK_S3D4,  CLK_S3,         4,
>>>> 1),
>>>>          DEF_FIXED("cl",         R8A7795_CLK_CL,    CLK_PLL1_DIV2, 48,
>>>> 1),
>>>> +       DEF_FIXED("sd0h",       R8A7795_CLK_SD0H,  CLK_PLL1_DIV2,  2,
>>>> 1),
>>>> +       DEF_FIXED("sd0",        R8A7795_CLK_SD0,   CLK_PLL1_DIV2,  8,
>>>> 1),
>>>> +       DEF_FIXED("sd1h",       R8A7795_CLK_SD1H,  CLK_PLL1_DIV2,  2,
>>>> 1),
>>>> +       DEF_FIXED("sd1",        R8A7795_CLK_SD1,   CLK_PLL1_DIV2,  8,
>>>> 1),
>>>> +       DEF_FIXED("sd2h",       R8A7795_CLK_SD2H,  CLK_PLL1_DIV2,  2,
>>>> 1),
>>>> +       DEF_FIXED("sd2",        R8A7795_CLK_SD2,   CLK_PLL1_DIV2,  8,
>>>> 1),
>>>> +       DEF_FIXED("sd3h",       R8A7795_CLK_SD3H,  CLK_PLL1_DIV2,  2,
>>>> 1),
>>>> +       DEF_FIXED("sd3",        R8A7795_CLK_SD3,   CLK_PLL1_DIV2,  8,
>>>> 1),
>>
>>
>> The dividers for these clocks are not fixed, they are controlled by the
>> SDnCKCR registers.
>>
>> Unfortunately the register layout is more complicated than on R-Car Gen2,
>> so
>> you can no longer use clk_register_divider_table(), but have to write a
>> custom
>> clock driver.
>>
>> For an initial version, a simple "read-only" version that just calls
>> clk_register_fixed_factor() with divider values read from the hardware
>> registers may be good enough. But for full support, you need a driver that
>> can program the registers, too.
>
>
>
> Anything like
>
> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas-bsp.git/commit/drivers/clk/shmobile/clk-rcar-gen3.c?h=v4.2/rcar-3.0.x&id=cd10385afc15cef6bfbaea4aa5da41193b24fe82
>
> ?

Yes.

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: shmobile: r8a7795: Add SDHI clocks

[Dirk Behme]

Hi Geert,

On 15.01.2016 11:20, Geert Uytterhoeven wrote:
> Hi Dirk,
>
> On Fri, Jan 15, 2016 at 11:11 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> On 14.01.2016 19:24, Geert Uytterhoeven wrote:
>>> On Sat, Jan 9, 2016 at 7:42 AM, Dirk Behme <dirk.behme@gmail.com> wrote:
>>>> On 24.12.2015 11:09, Dirk Behme wrote:
>>>>> Add R8A7795 SDHI clocks.
>>>>> Signed-off-by: Dirk Behme <dirk.behme@gmail.com>
>>>>> ---
>>>>> Changes in v2: Add the missing *H clocks and correct the dividers.
>>>>>
>>>>> This replaces v1
>>>>>
>>>>> http://www.spinics.net/lists/linux-sh/msg47464.html
>>>>>
>>>>>     drivers/clk/shmobile/r8a7795-cpg-mssr.c | 13 ++++++++++++-
>>>>>     1 file changed, 12 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/clk/shmobile/r8a7795-cpg-mssr.c
>>>>> b/drivers/clk/shmobile/r8a7795-cpg-mssr.c
>>>>> index 05479e6..f30ed32 100644
>>>>> --- a/drivers/clk/shmobile/r8a7795-cpg-mssr.c
>>>>> +++ b/drivers/clk/shmobile/r8a7795-cpg-mssr.c
>>>>> @@ -100,8 +100,15 @@ static const struct cpg_core_clk
>>>>> r8a7795_core_clks[]
>>>>> __initconst = {
>>>>>           DEF_FIXED("s3d2",       R8A7795_CLK_S3D2,  CLK_S3,         2,
>>>>> 1),
>>>>>           DEF_FIXED("s3d4",       R8A7795_CLK_S3D4,  CLK_S3,         4,
>>>>> 1),
>>>>>           DEF_FIXED("cl",         R8A7795_CLK_CL,    CLK_PLL1_DIV2, 48,
>>>>> 1),
>>>>> +       DEF_FIXED("sd0h",       R8A7795_CLK_SD0H,  CLK_PLL1_DIV2,  2,
>>>>> 1),
>>>>> +       DEF_FIXED("sd0",        R8A7795_CLK_SD0,   CLK_PLL1_DIV2,  8,
>>>>> 1),
>>>>> +       DEF_FIXED("sd1h",       R8A7795_CLK_SD1H,  CLK_PLL1_DIV2,  2,
>>>>> 1),
>>>>> +       DEF_FIXED("sd1",        R8A7795_CLK_SD1,   CLK_PLL1_DIV2,  8,
>>>>> 1),
>>>>> +       DEF_FIXED("sd2h",       R8A7795_CLK_SD2H,  CLK_PLL1_DIV2,  2,
>>>>> 1),
>>>>> +       DEF_FIXED("sd2",        R8A7795_CLK_SD2,   CLK_PLL1_DIV2,  8,
>>>>> 1),
>>>>> +       DEF_FIXED("sd3h",       R8A7795_CLK_SD3H,  CLK_PLL1_DIV2,  2,
>>>>> 1),
>>>>> +       DEF_FIXED("sd3",        R8A7795_CLK_SD3,   CLK_PLL1_DIV2,  8,
>>>>> 1),
>>>
>>>
>>> The dividers for these clocks are not fixed, they are controlled by the
>>> SDnCKCR registers.
>>>
>>> Unfortunately the register layout is more complicated than on R-Car Gen2,
>>> so
>>> you can no longer use clk_register_divider_table(), but have to write a
>>> custom
>>> clock driver.
>>>
>>> For an initial version, a simple "read-only" version that just calls
>>> clk_register_fixed_factor() with divider values read from the hardware
>>> registers may be good enough. But for full support, you need a driver that
>>> can program the registers, too.
>>
>>
>>
>> Anything like
>>
>> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas-bsp.git/commit/drivers/clk/shmobile/clk-rcar-gen3.c?h=v4.2/rcar-3.0.x&id=cd10385afc15cef6bfbaea4aa5da41193b24fe82


I've had a look to that.

I think we can pick all the functions

+static const struct clk_ops cpg_sd_clock_ops = {
+	.enable = cpg_sd_clock_enable,
+	.disable = cpg_sd_clock_disable,
+	.is_enabled = cpg_sd_clock_is_enabled,
+	.recalc_rate = cpg_sd_clock_recalc_rate,
+	.round_rate = cpg_sd_clock_round_rate,
+	.set_rate = cpg_sd_clock_set_rate,
+};

unchanged from that patch.

However, I'm not sure how to interface this to cpg_mssr_probe()? It's 
similar to cpg_mssr_register_mod_clk(), but not identical so that this 
could be reused.

We could extend r8a7795_cpg_mssr_info by anything like an additional

/* Dynamic clocks */
.dyn_clks = /* Add an array allowing to pass various clk_ops structs */
...

?

Or we could hard code it like

https://git.kernel.org/cgit/linux/kernel/git/horms/renesas-bsp.git/commit/drivers/clk/shmobile/clk-rcar-gen3.c?h=v4.2/rcar-3.0.x&id=cd10385afc15cef6bfbaea4aa5da41193b24fe82

is doing it with

+	} else if (!strcmp(name, "sd0")) {
+		return cpg_sd_clk_register(name, cpg->reg + CPG_SD0CKCR, np);
+	} else if (!strcmp(name, "sd1")) {
+		return cpg_sd_clk_register(name, cpg->reg + CPG_SD1CKCR, np);
+	} else if (!strcmp(name, "sd2")) {
+		return cpg_sd_clk_register(name, cpg->reg + CPG_SD2CKCR, np);
+	} else if (!strcmp(name, "sd3")) {
+		return cpg_sd_clk_register(name, cpg->reg + CPG_SD3CKCR, np);

and add this to e.g. r8a7795_cpg_clk_register(). But, hmm.


What do you think? Opinions? Examples?


Best regards

Dirk

Re: [PATCH v2] clk: shmobile: r8a7795: Add SDHI clocks

[Geert Uytterhoeven]

Hi Dirk,

On Sun, Jan 17, 2016 at 10:47 AM, Dirk Behme <dirk.behme@gmail.com> wrote:
> On 15.01.2016 11:20, Geert Uytterhoeven wrote:
>> On Fri, Jan 15, 2016 at 11:11 AM, Dirk Behme <dirk.behme@de.bosch.com>
>> wrote:
>>> On 14.01.2016 19:24, Geert Uytterhoeven wrote:
>>>> On Sat, Jan 9, 2016 at 7:42 AM, Dirk Behme <dirk.behme@gmail.com> wrote:
>>>>> On 24.12.2015 11:09, Dirk Behme wrote:
>>>>>> Add R8A7795 SDHI clocks.
>>>>>> Signed-off-by: Dirk Behme <dirk.behme@gmail.com>
>>>>>> ---
>>>>>> Changes in v2: Add the missing *H clocks and correct the dividers.

>>>> The dividers for these clocks are not fixed, they are controlled by the
>>>> SDnCKCR registers.
>>>>
>>>> Unfortunately the register layout is more complicated than on R-Car
>>>> Gen2,
>>>> so
>>>> you can no longer use clk_register_divider_table(), but have to write a
>>>> custom
>>>> clock driver.
>>>>
>>>> For an initial version, a simple "read-only" version that just calls
>>>> clk_register_fixed_factor() with divider values read from the hardware
>>>> registers may be good enough. But for full support, you need a driver
>>>> that
>>>> can program the registers, too.
>>>
>>> Anything like
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas-bsp.git/commit/drivers/clk/shmobile/clk-rcar-gen3.c?h=v4.2/rcar-3.0.x&id=cd10385afc15cef6bfbaea4aa5da41193b24fe82
>
> I've had a look to that.
>
> I think we can pick all the functions
>
> +static const struct clk_ops cpg_sd_clock_ops = {
> +       .enable = cpg_sd_clock_enable,
> +       .disable = cpg_sd_clock_disable,
> +       .is_enabled = cpg_sd_clock_is_enabled,
> +       .recalc_rate = cpg_sd_clock_recalc_rate,
> +       .round_rate = cpg_sd_clock_round_rate,
> +       .set_rate = cpg_sd_clock_set_rate,
> +};
>
> unchanged from that patch.
>
> However, I'm not sure how to interface this to cpg_mssr_probe()? It's
> similar to cpg_mssr_register_mod_clk(), but not identical so that this could
> be reused.
>
> We could extend r8a7795_cpg_mssr_info by anything like an additional
>
> /* Dynamic clocks */
> .dyn_clks = /* Add an array allowing to pass various clk_ops structs */
> ...
>
> ?
>
> Or we could hard code it like
>
> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas-bsp.git/commit/drivers/clk/shmobile/clk-rcar-gen3.c?h=v4.2/rcar-3.0.x&id=cd10385afc15cef6bfbaea4aa5da41193b24fe82
>
> is doing it with
>
> +       } else if (!strcmp(name, "sd0")) {
> +               return cpg_sd_clk_register(name, cpg->reg + CPG_SD0CKCR,
> np);
> +       } else if (!strcmp(name, "sd1")) {
> +               return cpg_sd_clk_register(name, cpg->reg + CPG_SD1CKCR,
> np);
> +       } else if (!strcmp(name, "sd2")) {
> +               return cpg_sd_clk_register(name, cpg->reg + CPG_SD2CKCR,
> np);
> +       } else if (!strcmp(name, "sd3")) {
> +               return cpg_sd_clk_register(name, cpg->reg + CPG_SD3CKCR,
> np);
>
> and add this to e.g. r8a7795_cpg_clk_register(). But, hmm.
>
>
> What do you think? Opinions? Examples?

Please extend enum r8a7795_clk_types, and handle them in
r8a7795_cpg_clk_register() based on the enum values.

You can find an (old) example in the prototype I did for r8a7791:
https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/tree/drivers/clk/shmobile/clk-r8a7791-cpg-mssr.c?h=topic/cpg-mssr-v4

Thanks!

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: shmobile: r8a7795: Add SDHI clocks

[Dirk Behme]

On 20.01.2016 09:16, Geert Uytterhoeven wrote:
> Hi Dirk,
>
> On Sun, Jan 17, 2016 at 10:47 AM, Dirk Behme <dirk.behme@gmail.com> wrote:
>> On 15.01.2016 11:20, Geert Uytterhoeven wrote:
>>> On Fri, Jan 15, 2016 at 11:11 AM, Dirk Behme <dirk.behme@de.bosch.com>
>>> wrote:
>>>> On 14.01.2016 19:24, Geert Uytterhoeven wrote:
>>>>> On Sat, Jan 9, 2016 at 7:42 AM, Dirk Behme <dirk.behme@gmail.com> wrote:
>>>>>> On 24.12.2015 11:09, Dirk Behme wrote:
>>>>>>> Add R8A7795 SDHI clocks.
>>>>>>> Signed-off-by: Dirk Behme <dirk.behme@gmail.com>
>>>>>>> ---
>>>>>>> Changes in v2: Add the missing *H clocks and correct the dividers.
>
>>>>> The dividers for these clocks are not fixed, they are controlled by the
>>>>> SDnCKCR registers.
>>>>>
>>>>> Unfortunately the register layout is more complicated than on R-Car
>>>>> Gen2,
>>>>> so
>>>>> you can no longer use clk_register_divider_table(), but have to write a
>>>>> custom
>>>>> clock driver.
>>>>>
>>>>> For an initial version, a simple "read-only" version that just calls
>>>>> clk_register_fixed_factor() with divider values read from the hardware
>>>>> registers may be good enough. But for full support, you need a driver
>>>>> that
>>>>> can program the registers, too.
>>>>
>>>> Anything like
>>>>
>>>> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas-bsp.git/commit/drivers/clk/shmobile/clk-rcar-gen3.c?h=v4.2/rcar-3.0.x&id=cd10385afc15cef6bfbaea4aa5da41193b24fe82
>>
>> I've had a look to that.
>>
>> I think we can pick all the functions
>>
>> +static const struct clk_ops cpg_sd_clock_ops = {
>> +       .enable = cpg_sd_clock_enable,
>> +       .disable = cpg_sd_clock_disable,
>> +       .is_enabled = cpg_sd_clock_is_enabled,
>> +       .recalc_rate = cpg_sd_clock_recalc_rate,
>> +       .round_rate = cpg_sd_clock_round_rate,
>> +       .set_rate = cpg_sd_clock_set_rate,
>> +};
>>
>> unchanged from that patch.
>>
>> However, I'm not sure how to interface this to cpg_mssr_probe()? It's
>> similar to cpg_mssr_register_mod_clk(), but not identical so that this could
>> be reused.
>>
>> We could extend r8a7795_cpg_mssr_info by anything like an additional
>>
>> /* Dynamic clocks */
>> .dyn_clks = /* Add an array allowing to pass various clk_ops structs */
>> ...
>>
>> ?
>>
>> Or we could hard code it like
>>
>> https://git.kernel.org/cgit/linux/kernel/git/horms/renesas-bsp.git/commit/drivers/clk/shmobile/clk-rcar-gen3.c?h=v4.2/rcar-3.0.x&id=cd10385afc15cef6bfbaea4aa5da41193b24fe82
>>
>> is doing it with
>>
>> +       } else if (!strcmp(name, "sd0")) {
>> +               return cpg_sd_clk_register(name, cpg->reg + CPG_SD0CKCR,
>> np);
>> +       } else if (!strcmp(name, "sd1")) {
>> +               return cpg_sd_clk_register(name, cpg->reg + CPG_SD1CKCR,
>> np);
>> +       } else if (!strcmp(name, "sd2")) {
>> +               return cpg_sd_clk_register(name, cpg->reg + CPG_SD2CKCR,
>> np);
>> +       } else if (!strcmp(name, "sd3")) {
>> +               return cpg_sd_clk_register(name, cpg->reg + CPG_SD3CKCR,
>> np);
>>
>> and add this to e.g. r8a7795_cpg_clk_register(). But, hmm.
>>
>>
>> What do you think? Opinions? Examples?
>
> Please extend enum r8a7795_clk_types, and handle them in
> r8a7795_cpg_clk_register() based on the enum values.
>
> You can find an (old) example in the prototype I did for r8a7791:
> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/tree/drivers/clk/shmobile/clk-r8a7791-cpg-mssr.c?h=topic/cpg-mssr-v4


Whats about anything like

http://marc.info/?l=linux-sh&m=145346559429931

?

Best regards

Dirk