Message-ID: <0423e827-9592-ce6f-74ca-111a099a263f@linaro.org>
Date:   Mon, 25 Apr 2022 11:00:11 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Cixi Geng <gengcixi@...il.com>
Cc:     Michael Turquette <mturquette@...libre.com>, sboyd@...nel.org,
        Rob Herring <robh+dt@...nel.org>,
        krzysztof.kozlowski+dt@...aro.org,
        Orson Zhai <orsonzhai@...il.com>,
        "baolin.wang7@...il.com" <baolin.wang7@...il.com>,
        Chunyan Zhang <zhang.lyra@...il.com>,
        linux-clk@...r.kernel.org,
        Devicetree List <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 1/3] dt-bindings: clk: sprd: Add bindings for ums512
 clock controller

On 24/04/2022 17:12, Cixi Geng wrote:
> Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org> 于2022年4月24日周日 22:30写道：
>>
>> On 24/04/2022 16:22, Cixi Geng wrote:
>>>>>>
>>>>>> Neither here nor later you did not answer the question - why do you need
>>>>>> such complex construction, instead of adding syscon to the clock controller?
>>>>>>
>>>>>> Let me paste again my concerns:
>>>>>>
>>>>>>   You have nodes with reg but without unit address ("rpll"). These nodes
>>>>>>   are modeled as children but they are not children - it's a workaround
>>>>>>   for exposing syscon, isn't it? The sc9863a looks like broken design,
>>>>>>   so please do not duplicate it here.
>>>>>>
>>>>>> IOW, sc9863a uses similar pattern as here and the DTS is made wrong.
>>>>>> Because of this you need to create complex ways to get the regmap for
>>>>>> the clock controller... Why not making it simple? Clock controller with
>>>>>> syscon?
>>>>>
>>>>> I find the history discuss about the sp9863 clock[1] and last
>>>>> ums512-clk dt-bindings patch[2] which from chunyan.
>>>>> please refer to the reasons below.
>>>>>
>>>>> These clocks are at the same register range with global registers.
>>>>> the registers shared with more than one devices  which  basically
>>>>> are multimedia devices. You may noticed that these are all gate
>>>>> clocks which are in the global registers  ranges and are used to
>>>>> controll the enable status of some devices or some part of devices.
>>>>>
>>>>> [1] https://lore.kernel.org/all/CAAfSe-s0gcehu0ZDj=FTe5S7CzAHC5mahXBH2fJm7mXS7Xys1Q@mail.gmail.com/#r
>>>>> [2] https://lore.kernel.org/all/163425295208.1688384.11023187625793114662@swboyd.mtv.corp.google.com/#r
>>>>
>>>> Which looks like discussion about different bindings. You had there a
>>>> clock controller and additional clock device using "sprd,syscon". Why
>>>> the rpll is a subdevice and not a part of clock controller. The same as
>>>> all other clocks coming from that clock-controller, right? What is so
>>>> special about rpll that is is a separate device, not part of the clock
>>>> controller? It's the same address space, isn't it?
>>> The hardware spec design these clocks are not belonged to the syscon,
>>> the phandle is only used to get virtual  map address for clocks which
>>> have the same phsical address base with one syscon.(I don't know the
>>> historical reason for this design) It also can wroten a clock sperated from
>>> syscon by add the reg which same as syscon. but will lead to a duplicate
>>> mapping--one is from the clock,and one is from syscon. which make difficulty
>>>  in analyzing some panic problems.
>>
>> I don't understand still. You said that they do not belong to same
>> address space, right? But the sprd,ums512-apahb-gate in this patch or
>> mentioned rpll
>> (https://elixir.bootlin.com/linux/v5.18-rc3/source/arch/arm64/boot/dts/sprd/sharkl3.dtsi#L106)
>> does not reference any other address space. It's entire address space is
>> the same as address space of glbregs.
> Maybe I didn't describe clearly, what I said is these clocks isn't the
> syscom sub-clock.
> from chunyan's explain:
>  they  are at the same register range with global registers. in
> originally we put them
> directly onto the bus indeed when submitting the patches for SC9863A
> clocks last year,
> and it had a private property named 'sprd,syscon' which could provide
> regmap for these clocks.
> after follow Rob's suggetion we make them a child of the syscon. these
> are all gate clocks which
> are in the global registers ranges and are used to controll the enable
> status of some devices
> or some part of devices.

You need to help me here with the naming. What is "global registers"
range? Let's focus on sharkl3.dtsi and syscon@...5c000 with "rpll".

You have a clock controller @4035c000, which provides several clocks,
right? Then you have a rpll also @4035c000, so the register range is the
same. The register range is the same, isn't it?

I still don't see the answer to my question - why do you need a child
device for one clock if this looks like part of the clock-controller?

Best regards,
Krzysztof

Powered by blists - more mailing lists