Re: [RESEND/PATCH v6 3/3] clk: qcom: Add A53 clock driver

[Georgi Djakov <georgi.djakov@linaro.org>]

On 11/03/2016 08:28 PM, Bjorn Andersson wrote:
> On Wed 02 Nov 15:55 PDT 2016, Stephen Boyd wrote:
>
>> On 11/02, Bjorn Andersson wrote:
>>> On Thu 27 Oct 18:54 PDT 2016, Stephen Boyd wrote:
>>>
>>>> On 10/19, Georgi Djakov wrote:
>>>>> Add a driver for the A53 Clock Controller. It is a hardware block that
>>>>> implements a combined mux and half integer divider functionality. It can
>>>>> choose between a fixed-rate clock or the dedicated A53 PLL. The source
>>>>> and the divider can be set both at the same time.
>>>>>
>>>>> This is required for enabling CPU frequency scaling on platforms like
>>>>> MSM8916.
>>>>>
>>>>
>>>> Please Cc DT reviewers.
>>>>
>>>>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>>>>> ---
>>>>>  .../devicetree/bindings/clock/qcom,a53cc.txt       |  22 +++
>>>>>  drivers/clk/qcom/Kconfig                           |   8 ++
>>>>>  drivers/clk/qcom/Makefile                          |   1 +
>>>>>  drivers/clk/qcom/a53cc.c                           | 155 +++++++++++++++++++++
>>>>>  4 files changed, 186 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt
>>>>>  create mode 100644 drivers/clk/qcom/a53cc.c
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
>>>>> new file mode 100644
>>>>> index 000000000000..a025f062f177
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
>>>>> @@ -0,0 +1,22 @@
>>>>> +Qualcomm A53 CPU Clock Controller Binding
>>>>> +------------------------------------------------
>>>>> +The A53 CPU Clock Controller is hardware, which provides a combined
>>>>> +mux and divider functionality for the CPU clocks. It can choose between
>>>>> +a fixed rate clock and the dedicated A53 PLL.
>>>>> +
>>>>> +Required properties :
>>>>> +- compatible : shall contain:
>>>>> +
>>>>> +			"qcom,a53cc"
>>>>> +
>>>>> +- reg : shall contain base register location and length
>>>>> +	of the APCS region
>>>>> +- #clock-cells : shall contain 1
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +	apcs: syscon@b011000 {
>>>>> +		compatible = "qcom,a53cc", "syscon";
>>>>
>>>> Why is it a syscon? Is that part used?
>>>>
>>>
>>> I use the register at offset 8 for interrupting the other subsystems, so
>>> this must be available as something I can poke.
>>>
>>> Which makes me think that this should be described as a "simple-mfd" and
>>> "syscon" with the a53cc node as a child - grabbing the regmap of the
>>> syscon parent, rather then ioremapping the same region again.
>>>
>>
>> That's sort of a question for DT reviewers. The register space
>> certainly seems like a free for all with a tilt toward power
>> management of the CPU, similar to how this was done on Krait
>> based designs.
>>
>
> Right. But this kind of mashup blocks was the reason why simple-mfd was
> put in place.
>

Ok, thanks for the comments. Then i will make it look like this:

	apcs: syscon@b011000 {
		compatible = "syscon", "simple-mfd";
		reg = <0x0b011000 0x1000>;

		a53mux: clock {
			compatible = "qcom,msm8916-a53cc";
			#clock-cells = <1>;
		};
	};

Thanks,
Georgi

>> I wonder why we didn't make up some provider/consumer binding for
>> the "kicking" feature used by SMD/RPM code. Then this could be a
>> clock provider and a "kick" provider (haha #kick-cells) and the
>> usage of syscon/regmap wouldn't be mandatory.
>>
>
> I did consider doing that, but had enough dependencies to put in place
> as it was.
>
> I'm in favour of us inventing a kicker API and it's found outside out
> use cases as well (e.g. virtio/rpmsg).
>
> Regards,
> Bjorn
>