Message-ID: <5b00db5b-b179-af0f-71e4-e940c6a41018@linaro.org>
Date:   Sun, 24 Apr 2022 16:30:15 +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 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.

So if it does not belong to the same address space, where is this space
defined?

Best regards,
Krzysztof

Powered by blists - more mailing lists