Date:   Tue, 26 Apr 2022 13:40:27 +0800
From:   Cixi Geng <gengcixi@...il.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
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

Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org> 于2022年4月25日周一 17:00写道：
>
> 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?

the anlg_phy_g5_regs is not a clock controller.
In fact, this is just to provide an address for other modules to call regmap.
not provide a clk interface or device.
The clk configuration of rpll is based on the anlg_phy_g5_regs register.
The analog_g5 asic document is not only used to configure rpll, but also other
functions can be configured, but currently our driver is only used to provide
configuration rpll, so the range of the device node of rpll can be less than or
equal to the range of anlg_phy_g5_regs.
Hope this could explains your question

Best regards,
Cixi
>
> 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