Re: [PATCH v2 4/5] ARM: dts: omap5: describe control for ckobuffer

From: Peter Ujfalusi
Date: Mon May 09 2016 - 08:11:08 EST

On 05/09/16 14:52, Tero Kristo wrote:
> On 09/05/16 14:18, H. Nikolaus Schaller wrote:
>> Hi,
>>
>>> Am 28.04.2016 um 15:23 schrieb Tero Kristo <t-kristo@xxxxxx>:
>>>
>>> On 28/04/16 12:12, H. Nikolaus Schaller wrote:
>>>> Hi Tero,
>>>>
>>>>> Am 28.04.2016 um 10:03 schrieb Tero Kristo <t-kristo@xxxxxx>:
>>>>>
>>>>> On 27/04/16 17:35, H. Nikolaus Schaller wrote:
>>>>>> HI,
>>>>>>
>>>>>>> Am 27.04.2016 um 16:23 schrieb Peter Ujfalusi <peter.ujfalusi@xxxxxx>:
>>>>>>>
>>>>>>> On 04/27/2016 05:10 PM, Tero Kristo wrote:
>>>>>>>> On 27/04/16 16:10, H. Nikolaus Schaller wrote:
>>>>>>>>>
>>>>>>>>>> Am 27.04.2016 um 14:31 schrieb Tero Kristo <t-kristo@xxxxxx>:
>>>>>>>>>>
>>>>>>>>>> On 27/04/16 09:04, H. Nikolaus Schaller wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Am 26.04.2016 um 19:27 schrieb Tony Lindgren <tony@xxxxxxxxxxx>:
>>>>>>>>>>>>
>>>>>>>>>>>> Tero,
>>>>>>>>>>>>
>>>>>>>>>>>> * H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> [160418 11:23]:
>>>>>>>>>>>>> OMAP5 has a register to control if the ckobuffer is enabled
>>>>>>>>>>>>> and defines the polarity. ckobuffer is required to drive a twl6040
>>>>>>>>>>>>> with the system clock. Hence, add the pinctrl,single to the
>>>>>>>>>>>>> OMAP5 SoC description so that omap5-board-common can
>>>>>>>>>>>>> set up the ckobuffer as required.
>>>>>>>>>>>>
>>>>>>>>>>>> Is this really a mux or should it be a mux clock?
>>>>>>>>>>>
>>>>>>>>>>> It is a pinmux setting for the clock out buffer to choose what signal
>>>>>>>>>>> (and polarity) is presented on the fref_xtal_clk pad.
>>>>>>>>>>>
>>>>>>>>>>> The register is part of the CTRL_MODULE_WKUP.
>>>>>>>>>>> The clock signal is the xtal master clock of the whole SoC.
>>>>>>>>>>>
>>>>>>>>>>> Although there is a bit to choose an alternate clock, there is no
>>>>>>>>>>> alternate in the OMAP5 silicon.
>>>>>>>>>>>
>>>>>>>>>>> Therefore I would say it is about padconf and not clock or clock mux
>>>>>>>>>>> related.
>>>>>>>>>>>
>>>>>>>>>>> It just happens to be a clock signal which can be routed to this
>>>>>>>>>>> pad.
>>>>>>>>>>
>>>>>>>>>> The two could very well be implemented as clock nodes, a mux and a
>>>>>>>>>> gate.
>>>>>>>>>> This would describe the hardware functionality better imo, if the
>>>>>>>>>> assumptions made here are correct. Implementing the control as pinctrl
>>>>>>>>>> hacks looks rather weird to me.
>>>>>>>>>
>>>>>>>>> Why do you consider it a "pinctrl hack"? IMHO it is not a hack, but 100%
>>>>>>>>> proper use of pinctrl.
>>>>>>>>
>>>>>>>> It is just the level of abstraction we are talking about here. If it is a
>>>>>>>> clock we are controlling, we should rather control it as a clock
>>>>>>>> (higher level
>>>>>>>> abstraction), not a pin.
>>>>>>>
>>>>>>> I second this. I think it is better to have a simple gate clock and handle
>>>>>>> only CONTROL_CKOBUFFER:CKOBUFFER_CLK_EN (bit 28) only as the other bits
>>>>>>> does
>>>>>>> not have real use.
>>>>>>>
>>>>>>> Then we can add clk API support for this. On most OMAP4 devices the
>>>>>>> clock is
>>>>>>> always on,
>>>>>>
>>>>>> this is why I am raising the question if we really want to control it on
>>>>>> the omap5 or just
>>>>>> turn it on for all omap5 boards like the omap4 appears to do... I.e. if
>>>>>> turning the pin on
>>>>>> as a pinctrl is IMHO sufficient for all practical purposes.
>>>>>>
>>>>>>> so the board DTS file need to provide a dummy clock, or we can make
>>>>>>> the high precision clock also as optional (on panda both OMAP4 and twl6040
>>>>>>> uses the same reference clock).
>>>>>>
>>>>>> Hm. It looks as if implementing this (and clock gating) is beyond my
>>>>>> experiences.
>>>>>> But I am happy to test a proposal on our omap5 board.
>>>>>>
>>>>>> BR and thanks,
>>>>>> Nikolaus
>>>>>
>>>>> See the inline patch, this implements the fref_xtal_ck. I had to add some
>>>>> kernel code also to cope with the new SCM area, but the same area can now
>>>>> be accessed via syscon also if needed.
>>>>
>>>> Looks interesting, although quite complex to enable a single SoC pad at
>>>> boot time...
>>>
>>> Yea it gives plenty of other things for you also. syscon, integration with
>>> clock framework, etc.
>>>
>>>>
>>>> Will asap study how it works and test. And of course report results.
>>>
>>> Thanks, Tero.
>>>
>>>>
>>>> Thanks and BR,
>>>> Nikolaus
>>>>
>>>>>
>>>>> From: Tero Kristo <t-kristo@xxxxxx>
>>>>> Date: Thu, 28 Apr 2016 11:00:57 +0300
>>>>> Subject: [PATCH] ARM: omap5: add support for fref_xtal_ck
>>>>>
>>>>> Signed-off-by: Tero Kristo <t-kristo@xxxxxx>
>>>>> ---
>>>>> arch/arm/boot/dts/omap5.dtsi | 22 ++++++++++++++++++++++
>>>>> arch/arm/boot/dts/omap54xx-clocks.dtsi | 10 ++++++++++
>>>>> arch/arm/mach-omap2/control.c | 20 ++++++++++++++++----
>>>>> include/linux/clk/ti.h | 1 +
>>>>> 4 files changed, 49 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/omap5.dtsi b/arch/arm/boot/dts/omap5.dtsi
>>>>> index 38805eb..bdc6528 100644
>>>>> --- a/arch/arm/boot/dts/omap5.dtsi
>>>>> +++ b/arch/arm/boot/dts/omap5.dtsi
>>>>> @@ -277,6 +277,28 @@
>>>>> pinctrl-single,register-width = <16>;
>>>>> pinctrl-single,function-mask = <0x7fff>;
>>>>> };
>>>>> +
>>>>> + omap5_scm_wkup_pad_conf: omap5_scm_wkup_pad_conf@cda0 {
>>>>> + compatible = "ti,omap5-scm-wkup-pad-conf",
>>>>> + "simple-bus";
>>>>> + reg = <0xcda0 0x60>;
>>>>> + #address-cells = <1>;
>>>>> + #size-cells = <1>;
>>>>> + ranges = <0 0xcda0 0x60>;
>>>>> +
>>>>> + scm_wkup_pad_conf: scm_conf@0 {
>>>>> + compatible = "syscon", "simple-bus";
>>>>> + reg = <0x0 0x60>;
>>>>> + #address-cells = <1>;
>>>>> + #size-cells = <1>;
>>>>> + ranges = <0 0x0 0x60>;
>>>>> +
>>>>> + scm_wkup_pad_conf_clocks: clocks@0 {
>>>>> + #address-cells = <1>;
>>>>> + #size-cells = <0>;
>>>>> + };
>>>>> + };
>>>>> + };
>>>>> };
>>>>>
>>>>> ocmcram: ocmcram@40300000 {
>>>>> diff --git a/arch/arm/boot/dts/omap54xx-clocks.dtsi
>>>>> b/arch/arm/boot/dts/omap54xx-clocks.dtsi
>>>>> index 83b425f..f970dac 100644
>>>>> --- a/arch/arm/boot/dts/omap54xx-clocks.dtsi
>>>>> +++ b/arch/arm/boot/dts/omap54xx-clocks.dtsi
>>>>> @@ -1388,3 +1388,13 @@
>>>>> reg = <0x021c>;
>>>>> };
>>>>> };
>>>>> +
>>>>> +&scm_wkup_pad_conf_clocks {
>>>>> + fref_xtal_ck: fref_xtal_ck {
>>>>> + #clocks-cells = <0>;
>>>>> + compatible = "ti,gate-clock";
>>>>> + clocks = <&sys_clkin>;
>>>>> + ti,bit-shift = <28>;
>>>>> + reg = <0x14>;
>>>>> + };
>>>>> +};
>>>>> diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
>>>>> index 1662071..5956641 100644
>>>>> --- a/arch/arm/mach-omap2/control.c
>>>>> +++ b/arch/arm/mach-omap2/control.c
>>>>> @@ -623,6 +623,7 @@ void __init omap3_ctrl_init(void)
>>>>>
>>>>> struct control_init_data {
>>>>> int index;
>>>>> + void __iomem *mem;
>>>>> s16 offset;
>>>>> };
>>>>>
>>>>> @@ -635,6 +636,10 @@ static const struct control_init_data
>>>>> omap2_ctrl_data = {
>>>>> .offset = -OMAP2_CONTROL_GENERAL,
>>>>> };
>>>>>
>>>>> +static const struct control_init_data ctrl_aux_data = {
>>>>> + .index = TI_CLKM_CTRL_AUX,
>>>>> +};
>>>>> +
>>>>> static const struct of_device_id omap_scrm_dt_match_table[] = {
>>>>> { .compatible = "ti,am3-scm", .data = &ctrl_data },
>>>>> { .compatible = "ti,am4-scm", .data = &ctrl_data },
>>>>> @@ -644,6 +649,7 @@ static const struct of_device_id
>>>>> omap_scrm_dt_match_table[] = {
>>>>> { .compatible = "ti,dm816-scrm", .data = &ctrl_data },
>>>>> { .compatible = "ti,omap4-scm-core", .data = &ctrl_data },
>>>>> { .compatible = "ti,omap5-scm-core", .data = &ctrl_data },
>>>>> + { .compatible = "ti,omap5-scm-wkup-pad-conf", .data = &ctrl_aux_data },
>>>>> { .compatible = "ti,dra7-scm-core", .data = &ctrl_data },
>>>>> { }
>>>>> };
>>>>> @@ -660,15 +666,21 @@ int __init omap2_control_base_init(void)
>>>>> struct device_node *np;
>>>>> const struct of_device_id *match;
>>>>> struct control_init_data *data;
>>>>> + void __iomem *mem;
>>>>>
>>>>> for_each_matching_node_and_match(np, omap_scrm_dt_match_table, &match) {
>>>>> data = (struct control_init_data *)match->data;
>>>>>
>>>>> - omap2_ctrl_base = of_iomap(np, 0);
>>>>> - if (!omap2_ctrl_base)
>>>>> + mem = of_iomap(np, 0);
>>>>> + if (!mem)
>>>>> return -ENOMEM;
>>>>>
>>>>> - omap2_ctrl_offset = data->offset;
>>>>> + if (data->index == TI_CLKM_CTRL) {
>>>>> + omap2_ctrl_base = mem;
>>>>> + omap2_ctrl_offset = data->offset;
>>>>> + }
>>>>> +
>>>>> + data->mem = mem;
>>>>> }
>>>>>
>>>>> return 0;
>>>>> @@ -713,7 +725,7 @@ int __init omap_control_init(void)
>>>>> } else {
>>>>> /* No scm_conf found, direct access */
>>>>> ret = omap2_clk_provider_init(np, data->index, NULL,
>>>>> - omap2_ctrl_base);
>>>>> + data->mem);
>>>>> if (ret)
>>>>> return ret;
>>>>> }
>>>>> diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
>>>>> index dc5164a..be25aa8 100644
>>>>> --- a/include/linux/clk/ti.h
>>>>> +++ b/include/linux/clk/ti.h
>>>>> @@ -195,6 +195,7 @@ enum {
>>>>> TI_CLKM_PRM,
>>>>> TI_CLKM_SCRM,
>>>>> TI_CLKM_CTRL,
>>>>> + TI_CLKM_CTRL_AUX,
>>>>> TI_CLKM_PLLSS,
>>>>> CLK_MAX_MEMMAPS
>>>>> };
>>>>
>>>
>>
>> finally I found some time to apply your patches. Sorry for the long time.
>>
>> Unfortunately, it does not work. Neither on omap5evm nor on our omap5 hardware.
>> I get no sound on the twl6040 - just white noise (which can be controlled in
>> level through
>> amixer so it is created on the digital input side of the twl6040).
>>
>> So I think your patch is missing a detail compared to my simple solution.
>
> Did you implement anything on the audio driver side? The audio driver must
> enable the clock implemented by this patch.

I did, can test it in 10 minutes or so...

>
> -Tero
>
>>
>> BR,
>> Nikolaus
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>


--
Péter