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

[Tero Kristo <t-kristo@ti.com>]

On 09/05/16 14:18, H. Nikolaus Schaller wrote:
> Hi,
>
>> Am 28.04.2016 um 15:23 schrieb Tero Kristo <t-kristo@ti.com>:
>>
>> On 28/04/16 12:12, H. Nikolaus Schaller wrote:
>>> Hi Tero,
>>>
>>>> Am 28.04.2016 um 10:03 schrieb Tero Kristo <t-kristo@ti.com>:
>>>>
>>>> On 27/04/16 17:35, H. Nikolaus Schaller wrote:
>>>>> HI,
>>>>>
>>>>>> Am 27.04.2016 um 16:23 schrieb Peter Ujfalusi <peter.ujfalusi@ti.com>:
>>>>>>
>>>>>> 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@ti.com>:
>>>>>>>>>
>>>>>>>>> On 27/04/16 09:04, H. Nikolaus Schaller wrote:
>>>>>>>>>>
>>>>>>>>>>> Am 26.04.2016 um 19:27 schrieb Tony Lindgren <tony@atomide.com>:
>>>>>>>>>>>
>>>>>>>>>>> Tero,
>>>>>>>>>>>
>>>>>>>>>>> * H. Nikolaus Schaller <hns@goldelico.com> [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@ti.com>
>>>> 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@ti.com>
>>>> ---
>>>> 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.

-Tero

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