* Re: [PATCH 06/12] dt-bindings: clock: qcom: Add reset for WCSSAON
From: Stephen Boyd @ 2019-07-17 20:08 UTC (permalink / raw)
To: agross, bjorn.andersson, david.brown, devicetree, gokulsri,
jassisinghbrar, linux-arm-msm, linux-clk, linux-kernel,
linux-remoteproc, mark.rutland, mturquette, ohad, robh+dt,
sricharan
In-Reply-To: <1562859668-14209-7-git-send-email-gokulsri@codeaurora.org>
Quoting Gokul Sriram Palanisamy (2019-07-11 08:41:02)
> Add binding for WCSSAON reset required for Q6v5 reset on IPQ8074 SoC.
>
> Signed-off-by: Gokul Sriram Palanisamy <gokulsri@codeaurora.org>
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> Signed-off-by: Nikhil Prakash V <nprakash@codeaurora.org>
> ---
Acked-by: Stephen Boyd <sboyd@kernel.org>
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 20:01 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <e3e9beaf-b195-305e-4010-66e824813472@gmail.com>
On 7/17/19 12:43 PM, Dmitry Osipenko wrote:
> 17.07.2019 21:54, Sowjanya Komatineni пишет:
>> On 7/17/19 11:51 AM, Sowjanya Komatineni wrote:
>>> On 7/17/19 11:32 AM, Dmitry Osipenko wrote:
>>>> 17.07.2019 20:29, Sowjanya Komatineni пишет:
>>>>> On 7/17/19 8:17 AM, Dmitry Osipenko wrote:
>>>>>> 17.07.2019 9:36, Sowjanya Komatineni пишет:
>>>>>>> On 7/16/19 11:33 PM, Dmitry Osipenko wrote:
>>>>>>>> В Tue, 16 Jul 2019 22:55:52 -0700
>>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>>>
>>>>>>>>> On 7/16/19 10:42 PM, Dmitry Osipenko wrote:
>>>>>>>>>> В Tue, 16 Jul 2019 22:25:25 -0700
>>>>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>>>>>> On 7/16/19 9:11 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>> В Tue, 16 Jul 2019 19:35:49 -0700
>>>>>>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>>>>>>>> On 7/16/19 7:18 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>> On 7/16/19 3:06 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>>> On 7/16/19 3:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>> 17.07.2019 0:35, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>> On 7/16/19 2:21 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>> 17.07.2019 0:12, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>> On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>> 16.07.2019 22:26, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Lo wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> The other thing that also need attention is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> that T124 CPUFreq
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> implicitly relies on DFLL driver to be probed
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> first, which is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> icky.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Should I add check for successful dfll clk
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> register explicitly in
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> registers?
>>>>>>>>>>>>>>>>>>>>>>>>>>> Probably you should use the "device links". See
>>>>>>>>>>>>>>>>>>>>>>>>>>> [1][2] for the
>>>>>>>>>>>>>>>>>>>>>>>>>>> example.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>>>>>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> [2]
>>>>>>>>>>>>>>>>>>>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if
>>>>>>>>>>>>>>>>>>>>>>>>>>> device_link_add() fails.
>>>>>>>>>>>>>>>>>>>>>>>>>>> And
>>>>>>>>>>>>>>>>>>>>>>>>>>> use of_find_device_by_node() to get the DFLL's
>>>>>>>>>>>>>>>>>>>>>>>>>>> device, see [3].
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> [3]
>>>>>>>>>>>>>>>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> Will go thru and add...
>>>>>>>>>>>>>>>>>>>>>>>> Looks like I initially confused this case with
>>>>>>>>>>>>>>>>>>>>>>>> getting
>>>>>>>>>>>>>>>>>>>>>>>> orphaned clock.
>>>>>>>>>>>>>>>>>>>>>>>> I'm now seeing that the DFLL driver registers the
>>>>>>>>>>>>>>>>>>>>>>>> clock and then
>>>>>>>>>>>>>>>>>>>>>>>> clk_get(dfll) should be returning EPROBE_DEFER until
>>>>>>>>>>>>>>>>>>>>>>>> DFLL driver is
>>>>>>>>>>>>>>>>>>>>>>>> probed, hence everything should be fine as-is and
>>>>>>>>>>>>>>>>>>>>>>>> there is no real
>>>>>>>>>>>>>>>>>>>>>>>> need
>>>>>>>>>>>>>>>>>>>>>>>> for the 'device link'. Sorry for the confusion!
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Sorry, I didn't follow the mail thread. Just
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> regarding the DFLL
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> part.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> As you know it, the DFLL clock is one of the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> clock sources and
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> integrated with DVFS control logic with the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> regulator. We will not
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> switch
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU to other clock sources once we switched to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL. Because the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU has
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> been regulated by the DFLL HW with the DVFS
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> table
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (CVB or OPP
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> table
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> you see
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> in the driver.). We shouldn't reparent it to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> other sources with
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> unknew
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> freq/volt pair. That's not guaranteed to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> work. We
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> allow switching to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> open-loop mode but different sources.
>>>>>>>>>>>>>>>>>>>>>>>>>>> Okay, then the CPUFreq driver will have to
>>>>>>>>>>>>>>>>>>>>>>>>>>> enforce
>>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL freq to
>>>>>>>>>>>>>>>>>>>>>>>>>>> PLLP's
>>>>>>>>>>>>>>>>>>>>>>>>>>> rate before switching to PLLP in order to have a
>>>>>>>>>>>>>>>>>>>>>>>>>>> proper CPU voltage.
>>>>>>>>>>>>>>>>>>>>>>>>>> PLLP freq is safe to work for any CPU voltage.
>>>>>>>>>>>>>>>>>>>>>>>>>> So no
>>>>>>>>>>>>>>>>>>>>>>>>>> need to enforce
>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL freq to PLLP rate before changing CCLK_G
>>>>>>>>>>>>>>>>>>>>>>>>>> source
>>>>>>>>>>>>>>>>>>>>>>>>>> to PLLP during
>>>>>>>>>>>>>>>>>>>>>>>>>> suspend
>>>>>>>>>>>>>>>>>>>>>>>>> Sorry, please ignore my above comment. During
>>>>>>>>>>>>>>>>>>>>>>>>> suspend, need to change
>>>>>>>>>>>>>>>>>>>>>>>>> CCLK_G source to PLLP when dfll is in closed loop
>>>>>>>>>>>>>>>>>>>>>>>>> mode first and
>>>>>>>>>>>>>>>>>>>>>>>>> then
>>>>>>>>>>>>>>>>>>>>>>>>> dfll need to be set to open loop.
>>>>>>>>>>>>>>>>>>>>>>>> Okay.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> And I don't exactly understand why we need to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> switch to PLLP in
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> idle
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> time.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> In SC7 entry, the dfll suspend function
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> moves it
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the open-loop
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> mode. That's
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> all. The sc7-entryfirmware will handle the rest
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> of the sequence to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> turn off
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the CPU power.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> In SC7 resume, the warmboot code will handle
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> sequence to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> turn on
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> regulator and power up the CPU cluster. And
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> leave
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it on PLL_P.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> After
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> resuming to the kernel, we re-init DFLL,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> restore
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the CPU clock
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> policy (CPU
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> runs on DFLL open-loop mode) and then moving to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> close-loop mode.
>>>>>>>>>>>>>>>>>>>>>>>>>>> The DFLL is re-inited after switching CCLK to
>>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL
>>>>>>>>>>>>>>>>>>>>>>>>>>> parent during of
>>>>>>>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>>>>>> early clocks-state restoring by CaR driver. Hence
>>>>>>>>>>>>>>>>>>>>>>>>>>> instead of having
>>>>>>>>>>>>>>>>>>>>>>>>>>> odd
>>>>>>>>>>>>>>>>>>>>>>>>>>> hacks in the CaR driver, it is much nicer to
>>>>>>>>>>>>>>>>>>>>>>>>>>> have a
>>>>>>>>>>>>>>>>>>>>>>>>>>> proper suspend-resume sequencing of the device
>>>>>>>>>>>>>>>>>>>>>>>>>>> drivers. In this case
>>>>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq
>>>>>>>>>>>>>>>>>>>>>>>>>>> driver is the driver that enables DFLL and
>>>>>>>>>>>>>>>>>>>>>>>>>>> switches
>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU to that
>>>>>>>>>>>>>>>>>>>>>>>>>>> clock
>>>>>>>>>>>>>>>>>>>>>>>>>>> source, which means that this driver is also
>>>>>>>>>>>>>>>>>>>>>>>>>>> should
>>>>>>>>>>>>>>>>>>>>>>>>>>> be responsible for
>>>>>>>>>>>>>>>>>>>>>>>>>>> management of the DFLL's state during of
>>>>>>>>>>>>>>>>>>>>>>>>>>> suspend/resume process. If
>>>>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver disables DFLL during suspend and
>>>>>>>>>>>>>>>>>>>>>>>>>>> re-enables it
>>>>>>>>>>>>>>>>>>>>>>>>>>> during
>>>>>>>>>>>>>>>>>>>>>>>>>>> resume, then looks like the CaR driver hacks
>>>>>>>>>>>>>>>>>>>>>>>>>>> around
>>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL are not
>>>>>>>>>>>>>>>>>>>>>>>>>>> needed.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> The DFLL part looks good to me. BTW, change the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> patch subject to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> "Add
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> suspend-resume support" seems more
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> appropriate to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> me.
>>>>>>>>>>>>>>>>>>>>>>>>>>>> To clarify this, the sequences for DFLL use
>>>>>>>>>>>>>>>>>>>>>>>>>>>> are as
>>>>>>>>>>>>>>>>>>>>>>>>>>>> follows (assuming
>>>>>>>>>>>>>>>>>>>>>>>>>>>> all
>>>>>>>>>>>>>>>>>>>>>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Switch to DFLL:
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 0) Save current parent and frequency
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 2) Enable DFLL
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>>>>>>>>>>>>>>>>>>>>>> For OVR regulator:
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>>>>>>>>>>>>>>>>>>>>>> For I2C regulator:
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Switch away from DFLL:
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU
>>>>>>>>>>>>>>>>>>>>>>>>>>>> frequency is ok for
>>>>>>>>>>>>>>>>>>>>>>>>>>>> any
>>>>>>>>>>>>>>>>>>>>>>>>>>>> vdd_cpu voltage
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>>>>>> I see during switch away from DFLL (suspend),
>>>>>>>>>>>>>>>>>>>>>>>>> cclk_g
>>>>>>>>>>>>>>>>>>>>>>>>> parent is not
>>>>>>>>>>>>>>>>>>>>>>>>> changed to PLLP before changing dfll to open loop
>>>>>>>>>>>>>>>>>>>>>>>>> mode.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Will add this ...
>>>>>>>>>>>>>>>>>>>>>>>> The CPUFreq driver switches parent to PLLP during
>>>>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>>> probe, similar
>>>>>>>>>>>>>>>>>>>>>>>> should be done on suspend.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> I'm also wondering if it's always safe to switch to
>>>>>>>>>>>>>>>>>>>>>>>> PLLP in the probe.
>>>>>>>>>>>>>>>>>>>>>>>> If CPU is running on a lower freq than PLLP, then
>>>>>>>>>>>>>>>>>>>>>>>> some
>>>>>>>>>>>>>>>>>>>>>>>> other more
>>>>>>>>>>>>>>>>>>>>>>>> appropriate intermediate parent should be selected.
>>>>>>>>>>>>>>>>>>>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always
>>>>>>>>>>>>>>>>>>>>>>> runs at higher
>>>>>>>>>>>>>>>>>>>>>>> rate
>>>>>>>>>>>>>>>>>>>>>>> so switching to PLL_P during CPUFreq probe prior to
>>>>>>>>>>>>>>>>>>>>>>> dfll clock enable
>>>>>>>>>>>>>>>>>>>>>>> should be safe.
>>>>>>>>>>>>>>>>>>>>>> AFAIK, PLLX could run at ~200MHz. There is also a
>>>>>>>>>>>>>>>>>>>>>> divided output of
>>>>>>>>>>>>>>>>>>>>>> PLLP
>>>>>>>>>>>>>>>>>>>>>> which CCLKG supports, the PLLP_OUT4.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Probably, realistically, CPU is always running off a
>>>>>>>>>>>>>>>>>>>>>> fast PLLX during
>>>>>>>>>>>>>>>>>>>>>> boot, but I'm wondering what may happen on KEXEC. I
>>>>>>>>>>>>>>>>>>>>>> guess ideally CPUFreq driver should also have a
>>>>>>>>>>>>>>>>>>>>>> 'shutdown' callback to teardown DFLL
>>>>>>>>>>>>>>>>>>>>>> on a reboot, but likely that there are other
>>>>>>>>>>>>>>>>>>>>>> clock-related problems as
>>>>>>>>>>>>>>>>>>>>>> well that may break KEXEC and thus it is not very
>>>>>>>>>>>>>>>>>>>>>> important at the
>>>>>>>>>>>>>>>>>>>>>> moment.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> [snip]
>>>>>>>>>>>>>>>>>>>>> During bootup CPUG sources from PLL_X. By PLL_P source
>>>>>>>>>>>>>>>>>>>>> above I meant
>>>>>>>>>>>>>>>>>>>>> PLL_P_OUT4.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> As per clock policies, PLL_X is always used for high
>>>>>>>>>>>>>>>>>>>>> freq
>>>>>>>>>>>>>>>>>>>>> like
>>>>>>>>>>>>>>>>>>>>>> 800Mhz
>>>>>>>>>>>>>>>>>>>>> and for low frequency it will be sourced from PLLP.
>>>>>>>>>>>>>>>>>>>> Alright, then please don't forget to pre-initialize
>>>>>>>>>>>>>>>>>>>> PLLP_OUT4 rate to a
>>>>>>>>>>>>>>>>>>>> reasonable value using tegra_clk_init_table or
>>>>>>>>>>>>>>>>>>>> assigned-clocks.
>>>>>>>>>>>>>>>>>>> PLLP_OUT4 rate update is not needed as it is safe to
>>>>>>>>>>>>>>>>>>> run at
>>>>>>>>>>>>>>>>>>> 408Mhz because it is below fmax @ Vmin
>>>>>>>>>>>>>>>>>> So even 204MHz CVB entries are having the same voltage as
>>>>>>>>>>>>>>>>>> 408MHz, correct? It's not instantly obvious to me from the
>>>>>>>>>>>>>>>>>> DFLL driver's code where the fmax @ Vmin is defined, I see
>>>>>>>>>>>>>>>>>> that there is the min_millivolts
>>>>>>>>>>>>>>>>>> and frequency entries starting from 204MHZ defined
>>>>>>>>>>>>>>>>>> per-table.
>>>>>>>>>>>>>>>>> Yes at Vmin CPU Fmax is ~800Mhz. So anything below that
>>>>>>>>>>>>>>>>> will
>>>>>>>>>>>>>>>>> work at Vmin voltage and PLLP max is 408Mhz.
>>>>>>>>>>>>>>>> Thank you for the clarification. It would be good to have
>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>> commented
>>>>>>>>>>>>>>>> in the code as well.
>>>>>>>>>>>>>>> OK, Will add...
>>>>>>>>>>>>>> Regarding, adding suspend/resume to CPUFreq, CPUFreq suspend
>>>>>>>>>>>>>> happens very early even before disabling non-boot CPUs and
>>>>>>>>>>>>>> also
>>>>>>>>>>>>>> need to export clock driver APIs to CPUFreq.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Was thinking of below way of implementing this...
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Clock DFLL driver Suspend:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - Save CPU clock policy registers, and Perform
>>>>>>>>>>>>>> dfll
>>>>>>>>>>>>>> suspend which sets in open loop mode
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> CPU Freq driver Suspend: does nothing
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Clock DFLL driver Resume:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - Re-init DFLL, Set in Open-Loop mode, restore
>>>>>>>>>>>>>> CPU
>>>>>>>>>>>>>> Clock policy registers which actually sets source to DFLL
>>>>>>>>>>>>>> along
>>>>>>>>>>>>>> with other CPU Policy register restore.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> CPU Freq driver Resume:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - do clk_prepare_enable which acutally sets
>>>>>>>>>>>>>> DFLL in
>>>>>>>>>>>>>> Closed loop mode
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Adding one more note: Switching CPU Clock to PLLP is not
>>>>>>>>>>>>>> needed
>>>>>>>>>>>>>> as CPU CLock can be from dfll in open-loop mode as DFLL is not
>>>>>>>>>>>>>> disabled anywhere throught the suspend/resume path and SC7
>>>>>>>>>>>>>> entry
>>>>>>>>>>>>>> FW and Warm boot code will switch CPU source to PLLP.
>>>>>>>>>>>> Since CPU resumes on PLLP, it will be cleaner to suspend it on
>>>>>>>>>>>> PLLP as well. And besides, seems that currently disabling DFLL
>>>>>>>>>>>> clock will disable DFLL completely and then you'd want to
>>>>>>>>>>>> re-init
>>>>>>>>>>>> the DFLL on resume any ways. So better to just disable DFLL
>>>>>>>>>>>> completely on suspend, which should happen on clk_disable(dfll).
>>>>>>>>>>> Will switch to PLLP during CPUFreq suspend. With decision of
>>>>>>>>>>> using
>>>>>>>>>>> clk_disable during suspend, its mandatory to switch to PLLP as
>>>>>>>>>>> DFLL
>>>>>>>>>>> is completely disabled.
>>>>>>>>>>>
>>>>>>>>>>> My earlier concern was on restoring CPU policy as we can't do
>>>>>>>>>>> that
>>>>>>>>>>> from CPUFreq driver and need export from clock driver.
>>>>>>>>>>>
>>>>>>>>>>> Clear now and will do CPU clock policy restore in after dfll
>>>>>>>>>>> re-init.
>>>>>>>>>> Why the policy can't be saved/restored by the CaR driver as a
>>>>>>>>>> context of any other clock?
>>>>>>>>> restoring cpu clock policy involves programming source and
>>>>>>>>> super_cclkg_divider.
>>>>>>>>>
>>>>>>>>> cclk_g is registered as clk_super_mux and it doesn't use
>>>>>>>>> frac_div ops
>>>>>>>>> to do save/restore its divider.
>>>>>>>> That can be changed of course and I guess it also could be as
>>>>>>>> simple as
>>>>>>>> saving and restoring of two raw u32 values of the policy/divider
>>>>>>>> registers.
>>>>>>>>
>>>>>>>>> Also, during clock context we cant restore cclk_g as cclk_g source
>>>>>>>>> will be dfll and dfll will not be resumed/re-initialized by the
>>>>>>>>> time
>>>>>>>>> clk_super_mux save/restore happens.
>>>>>>>>>
>>>>>>>>> we can't use save/restore context for dfll clk_ops because
>>>>>>>>> dfllCPU_out parent to CCLK_G is first in the clock tree and
>>>>>>>>> dfll_ref
>>>>>>>>> and dfll_soc peripheral clocks are not restored by the time dfll
>>>>>>>>> restore happens. Also dfll peripheral clock enables need to be
>>>>>>>>> restored before dfll restore happens which involves programming
>>>>>>>>> dfll
>>>>>>>>> controller for re-initialization.
>>>>>>>>>
>>>>>>>>> So dfll resume/re-init is done in clk-tegra210 at end of all clocks
>>>>>>>>> restore in V5 series but instead of in clk-tegra210 driver I moved
>>>>>>>>> now to dfll-fcpu driver pm_ops as all dfll dependencies will be
>>>>>>>>> restored thru clk_restore_context by then. This will be in V6.
>>>>>>>> Since DFLL is now guaranteed to be disabled across CaR
>>>>>>>> suspend/resume
>>>>>>>> (hence it has nothing to do in regards to CCLK) and given that PLLs
>>>>>>>> state is restored before the rest of the clocks, I don't see why
>>>>>>>> not to
>>>>>>>> implement CCLK save/restore in a generic fasion. CPU policy wull be
>>>>>>>> restored to either PLLP or PLLX (if CPUFreq driver is disabled).
>>>>>>>>
>>>>>>> CCLK_G save/restore should happen in clk_super_mux ops
>>>>>>> save/context and
>>>>>>> clk_super_mux save/restore happens very early as cclk_g is first
>>>>>>> in the
>>>>>>> clock tree and save/restore traverses through the tree top-bottom
>>>>>>> order.
>>>>>> If CCLK_G is restored before the PLLs, then just change the clocks
>>>>>> order
>>>>>> such that it won't happen.
>>>>>>
>>>>> I dont think we can change clocks order for CCLK_G.
>>>>>
>>>>> During bootup, cclk_g is registered after all pll's and peripheral
>>>>> clocks which is the way we wanted, So cclk_g will be the first one in
>>>>> the clk list as clk_register adds new clock first in the list.
>>>>>
>>>>> When clk_save_context and clk_restore_context APIs iterates over the
>>>>> list, cclk_g is the first
>>>> Looking at clk_core_restore_context(), I see that it walks up CLKs list
>>>> from parent to children, hence I don't understand how it can ever happen
>>>> that CCLK will be restored before the parent. The clocks registration
>>>> order doesn't matter at all in that case.
>>> yes from parent to children and dfllCPU_out is the top in the list and
>>> its child is cclk_g.
>>>
>>> the way clocks are registered is the order I see in the clock list and
>>> looking into clk_register API it adds new node first in the list.
>>>
>> cclkg_g & dfll register happens after all plls and peripheral clocks as
>> it need ref, soc and peripheral clocks to be enabled.
>>> So they are the last to get registered and so becomes first in the list.
>>>
>>> During save/restore context, it traverses thru this list and first in
>>> the list is dfllcpu_OUT (parent) and its child (cclk_g)
>>>
>>> saving should not be an issue at all but we cant restore cclk_g/dfll
>>> in normal way thru clk_ops restore as plls and peripherals restore
>>> doesn't happen by that time.
>>>
>> I was referring to clk_restore_context where it iterates thru root list
>> and for each core from the root list clk_core_restore does restore of
>> parent and children.
>>
>> dfllCPU_Out gets first in the list and its child is cclk_g
>>
>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/clk/clk.c#L1105
> What list you're talking about? clk_summary? It shows current *active*
> clocks configuration, if you'll try to disable CPUFreq driver then the
> parent of CCLK_G should be PLLX. Similarly when CPU is reparented to
> PLLP on driver's suspend, then PLLP is the parent.
>
>>>>>>> DFLL enable thru CPUFreq resume happens after all clk_restore_context
>>>>>>> happens. So during clk_restore_context, dfll re-init doesnt happen
>>>>>>> and
>>>>>>> doing cpu clock policy restore during super_mux clk_ops will crash as
>>>>>>> DFLL is not initialized and its clock is not enabled but CPU clock
>>>>>>> restore sets source to DFLL if we restore during super_clk_mux
>>>>>> If CPU was suspended on PLLP, then it will be restored on PLLP by
>>>>>> CaR. I
>>>>>> don't understand what DFLL has to do with the CCLK in that case during
>>>>>> the clocks restore.
>>>>> My above comment is in reference to your request of doing save/restore
>>>>> for cclk_g in normal fashion thru save/restore context. Because of the
>>>>> clk order I mentioned above, we cclk_g will be the first one to go thru
>>>>> save/context.
>>>>>
>>>>> During save_context of cclk_g, source can be from PLLX, dfll.
>>>>>
>>>>> Issue will be when we do restore during clk_restore_context of
>>>>> cclk_g as
>>>>> by that time PLLX/dfll will not be restored.
>>>>>
>>>> Seems we already agreed that DFLL will be disabled by the CPUFreq driver
>>>> on suspend. Hence CCLK can't be from DFLL if CPU is reparented to PLLP
>>>> on CPUFreq driver's suspend, otherwise CPU keeps running from a
>>>> boot-state PLLX if CPUFreq driver is disabled.
>>> Yes suspend should not be an issue but issue will be during resume
>>> where if we do cclk_g restore in normal way thru clk_restore_context,
>>> cclk_g restore happens very early as dfllCPU out is the first one that
>>> goes thru restore context and plls/peripherals are not resumed by then.
>>>
>>> CPU runs from PLLX if dfll clock enable fails during boot. So when it
>>> gets to suspend, we save CPU running clock source as either PLLX or
>>> DFLL and then we switch to PLLP.
>>>
>>>
>>> On resume, CPU runs from PLLP by warm boot code and we need to restore
>>> back its source to the one it was using from saved source context
>>> (which can be either PLLX or DFLL)
>>>
>>> So PLLs & DFLL resume need to happen before CCLKG restore/resume.
>>>
>>>
>>> With all above discussions, we do DFLL disable in CPUFreq driver on
>>> suspend and on CPUFreq resume we enable DFLL back and restore CPU
>>> clock source it was using during suspend (which will be either PLLX if
>>> dfll enable fails during probe or it will be using DFLL).
> During suspend CPU's parent shall be PLLP and not DFLL (note that it is
> disabled) after reparenting to PLLP by the CPUFreq driver.
>
CPU source context should be saved before switching to safe source of
PLLP as on resume we need to restore back to source it was using before
we switch to safe source during suspend entry.
So saved context for CPU Source will be either dfll or PLLX
>>> So i was trying to say dfll/cclk_g restore can't be done in normal way
>>> thru clk_ops save/restore context
> Let's see what happens if CPUFreq is active:
>
> 1. CPUFreq driver probe happens
> 2. CPU is reparented to PLLP
> 3. DFLL inited
> 4. CPU is reparented to DFLL
>
> 5. CPUFreq driver suspend happens
> 6. CPU is reparented to PLLP
> 7. DFLL is disabled
>
> 8. Car suspend happens
> 9. DFLL context saved
> 10. PLLP/PLLX context saved
> 11. CCLK context saved
>
> 12. Car resume happens
> 13. DFLL context restored
> 14. PLLP/PLLX context restored
> 15. CCLK context restored
>
> 16. CPUFreq driver resume happens
> 17. DFLL re-inited
> 18. CPU is reparented to DFLL
Below is the order of sequence it should be based on the order of clk
register.
My comments inline in this sequence.
1. CPUFreq driver probe happens
2. CPU is reparented to PLLP
3. DFLL inited
4. CPU is reparented to DFLL
5. CPUFreq driver suspend happens
6. Save CPU source which could be either dfll or pllx
7. CPU is reparented to safe known source PLLP
8. DFLL is disabled
8. Car suspend happens
9. DFLL context saved (With DFLL disabled in CPUFreq suspend, nothing to be saved here as last freq req will always be saved).
10. CCLK context saved (CPU clock source will be saved in CPUFreq driver suspend which could be either dfll or pllx)
11. PLLP/PLLX context saved
12. Peripheral Clock saved
12. Car resume happens
13. DFLL context restored : No DFLL context to be restored and we only need to reinitialize DFLL and re-initialize can't be done here as this is the 1st to get restored and PLL/Peripheral clocks are not restored by this time. So we can't use clk_ops restore for DFLL
14. CCLK context restored
CCLK cant be restored here as context could be either dfll or pllx which is the source orginally it was actually using before we force switch to safe PLLP for suspend entry. So we can't use clk_ops restore for DFLL
15. PLLP/PLLX context restored
16. Peripheral context restored
16. CPUFreq driver resume happens
17. DFLL re-inited (Invoking DFLL re-init in CPUFreq resume need exporting DFLL reinit from Clock driver to CPUFreq driver)
18. CPU is reparented to DFLL or PLLX based on saved context from step 9.
Note: instead of exporting, we can do DFLL re-init from clock-dfll
driver itself thru dfll-fcpu pm_ops resume. So dfll will be
re-initialized by the time CPUFreq driver resumes and switches to use
DFLL source.
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-17 19:43 UTC (permalink / raw)
To: Sowjanya Komatineni
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <351a07d4-ba90-4793-129b-b1a733f95531@nvidia.com>
17.07.2019 21:54, Sowjanya Komatineni пишет:
>
> On 7/17/19 11:51 AM, Sowjanya Komatineni wrote:
>>
>> On 7/17/19 11:32 AM, Dmitry Osipenko wrote:
>>> 17.07.2019 20:29, Sowjanya Komatineni пишет:
>>>> On 7/17/19 8:17 AM, Dmitry Osipenko wrote:
>>>>> 17.07.2019 9:36, Sowjanya Komatineni пишет:
>>>>>> On 7/16/19 11:33 PM, Dmitry Osipenko wrote:
>>>>>>> В Tue, 16 Jul 2019 22:55:52 -0700
>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>>
>>>>>>>> On 7/16/19 10:42 PM, Dmitry Osipenko wrote:
>>>>>>>>> В Tue, 16 Jul 2019 22:25:25 -0700
>>>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>>>>> On 7/16/19 9:11 PM, Dmitry Osipenko wrote:
>>>>>>>>>>> В Tue, 16 Jul 2019 19:35:49 -0700
>>>>>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>>>>>>> On 7/16/19 7:18 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>> On 7/16/19 3:06 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>> On 7/16/19 3:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>> 17.07.2019 0:35, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>> On 7/16/19 2:21 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>> 17.07.2019 0:12, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>> On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>> 16.07.2019 22:26, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>>>>>>>>>>>>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph
>>>>>>>>>>>>>>>>>>>>>>>>>>> Lo wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> The other thing that also need attention is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> that T124 CPUFreq
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> implicitly relies on DFLL driver to be probed
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> first, which is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> icky.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Should I add check for successful dfll clk
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> register explicitly in
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> registers?
>>>>>>>>>>>>>>>>>>>>>>>>>> Probably you should use the "device links". See
>>>>>>>>>>>>>>>>>>>>>>>>>> [1][2] for the
>>>>>>>>>>>>>>>>>>>>>>>>>> example.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>>>>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> [2]
>>>>>>>>>>>>>>>>>>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if
>>>>>>>>>>>>>>>>>>>>>>>>>> device_link_add() fails.
>>>>>>>>>>>>>>>>>>>>>>>>>> And
>>>>>>>>>>>>>>>>>>>>>>>>>> use of_find_device_by_node() to get the DFLL's
>>>>>>>>>>>>>>>>>>>>>>>>>> device, see [3].
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> [3]
>>>>>>>>>>>>>>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Will go thru and add...
>>>>>>>>>>>>>>>>>>>>>>> Looks like I initially confused this case with
>>>>>>>>>>>>>>>>>>>>>>> getting
>>>>>>>>>>>>>>>>>>>>>>> orphaned clock.
>>>>>>>>>>>>>>>>>>>>>>> I'm now seeing that the DFLL driver registers the
>>>>>>>>>>>>>>>>>>>>>>> clock and then
>>>>>>>>>>>>>>>>>>>>>>> clk_get(dfll) should be returning EPROBE_DEFER until
>>>>>>>>>>>>>>>>>>>>>>> DFLL driver is
>>>>>>>>>>>>>>>>>>>>>>> probed, hence everything should be fine as-is and
>>>>>>>>>>>>>>>>>>>>>>> there is no real
>>>>>>>>>>>>>>>>>>>>>>> need
>>>>>>>>>>>>>>>>>>>>>>> for the 'device link'. Sorry for the confusion!
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Sorry, I didn't follow the mail thread. Just
>>>>>>>>>>>>>>>>>>>>>>>>>>>> regarding the DFLL
>>>>>>>>>>>>>>>>>>>>>>>>>>>> part.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> As you know it, the DFLL clock is one of the
>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU
>>>>>>>>>>>>>>>>>>>>>>>>>>>> clock sources and
>>>>>>>>>>>>>>>>>>>>>>>>>>>> integrated with DVFS control logic with the
>>>>>>>>>>>>>>>>>>>>>>>>>>>> regulator. We will not
>>>>>>>>>>>>>>>>>>>>>>>>>>>> switch
>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU to other clock sources once we switched to
>>>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL. Because the
>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU has
>>>>>>>>>>>>>>>>>>>>>>>>>>>> been regulated by the DFLL HW with the DVFS
>>>>>>>>>>>>>>>>>>>>>>>>>>>> table
>>>>>>>>>>>>>>>>>>>>>>>>>>>> (CVB or OPP
>>>>>>>>>>>>>>>>>>>>>>>>>>>> table
>>>>>>>>>>>>>>>>>>>>>>>>>>>> you see
>>>>>>>>>>>>>>>>>>>>>>>>>>>> in the driver.). We shouldn't reparent it to
>>>>>>>>>>>>>>>>>>>>>>>>>>>> other sources with
>>>>>>>>>>>>>>>>>>>>>>>>>>>> unknew
>>>>>>>>>>>>>>>>>>>>>>>>>>>> freq/volt pair. That's not guaranteed to
>>>>>>>>>>>>>>>>>>>>>>>>>>>> work. We
>>>>>>>>>>>>>>>>>>>>>>>>>>>> allow switching to
>>>>>>>>>>>>>>>>>>>>>>>>>>>> open-loop mode but different sources.
>>>>>>>>>>>>>>>>>>>>>>>>>> Okay, then the CPUFreq driver will have to
>>>>>>>>>>>>>>>>>>>>>>>>>> enforce
>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL freq to
>>>>>>>>>>>>>>>>>>>>>>>>>> PLLP's
>>>>>>>>>>>>>>>>>>>>>>>>>> rate before switching to PLLP in order to have a
>>>>>>>>>>>>>>>>>>>>>>>>>> proper CPU voltage.
>>>>>>>>>>>>>>>>>>>>>>>>> PLLP freq is safe to work for any CPU voltage.
>>>>>>>>>>>>>>>>>>>>>>>>> So no
>>>>>>>>>>>>>>>>>>>>>>>>> need to enforce
>>>>>>>>>>>>>>>>>>>>>>>>> DFLL freq to PLLP rate before changing CCLK_G
>>>>>>>>>>>>>>>>>>>>>>>>> source
>>>>>>>>>>>>>>>>>>>>>>>>> to PLLP during
>>>>>>>>>>>>>>>>>>>>>>>>> suspend
>>>>>>>>>>>>>>>>>>>>>>>> Sorry, please ignore my above comment. During
>>>>>>>>>>>>>>>>>>>>>>>> suspend, need to change
>>>>>>>>>>>>>>>>>>>>>>>> CCLK_G source to PLLP when dfll is in closed loop
>>>>>>>>>>>>>>>>>>>>>>>> mode first and
>>>>>>>>>>>>>>>>>>>>>>>> then
>>>>>>>>>>>>>>>>>>>>>>>> dfll need to be set to open loop.
>>>>>>>>>>>>>>>>>>>>>>> Okay.
>>>>>>>>>>>>>>>>>>>>>>>>>>>> And I don't exactly understand why we need to
>>>>>>>>>>>>>>>>>>>>>>>>>>>> switch to PLLP in
>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU
>>>>>>>>>>>>>>>>>>>>>>>>>>>> idle
>>>>>>>>>>>>>>>>>>>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the
>>>>>>>>>>>>>>>>>>>>>>>>>>>> time.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> In SC7 entry, the dfll suspend function
>>>>>>>>>>>>>>>>>>>>>>>>>>>> moves it
>>>>>>>>>>>>>>>>>>>>>>>>>>>> the open-loop
>>>>>>>>>>>>>>>>>>>>>>>>>>>> mode. That's
>>>>>>>>>>>>>>>>>>>>>>>>>>>> all. The sc7-entryfirmware will handle the rest
>>>>>>>>>>>>>>>>>>>>>>>>>>>> of the sequence to
>>>>>>>>>>>>>>>>>>>>>>>>>>>> turn off
>>>>>>>>>>>>>>>>>>>>>>>>>>>> the CPU power.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> In SC7 resume, the warmboot code will handle
>>>>>>>>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>>>>>>> sequence to
>>>>>>>>>>>>>>>>>>>>>>>>>>>> turn on
>>>>>>>>>>>>>>>>>>>>>>>>>>>> regulator and power up the CPU cluster. And
>>>>>>>>>>>>>>>>>>>>>>>>>>>> leave
>>>>>>>>>>>>>>>>>>>>>>>>>>>> it on PLL_P.
>>>>>>>>>>>>>>>>>>>>>>>>>>>> After
>>>>>>>>>>>>>>>>>>>>>>>>>>>> resuming to the kernel, we re-init DFLL,
>>>>>>>>>>>>>>>>>>>>>>>>>>>> restore
>>>>>>>>>>>>>>>>>>>>>>>>>>>> the CPU clock
>>>>>>>>>>>>>>>>>>>>>>>>>>>> policy (CPU
>>>>>>>>>>>>>>>>>>>>>>>>>>>> runs on DFLL open-loop mode) and then moving to
>>>>>>>>>>>>>>>>>>>>>>>>>>>> close-loop mode.
>>>>>>>>>>>>>>>>>>>>>>>>>> The DFLL is re-inited after switching CCLK to
>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL
>>>>>>>>>>>>>>>>>>>>>>>>>> parent during of
>>>>>>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>>>>> early clocks-state restoring by CaR driver. Hence
>>>>>>>>>>>>>>>>>>>>>>>>>> instead of having
>>>>>>>>>>>>>>>>>>>>>>>>>> odd
>>>>>>>>>>>>>>>>>>>>>>>>>> hacks in the CaR driver, it is much nicer to
>>>>>>>>>>>>>>>>>>>>>>>>>> have a
>>>>>>>>>>>>>>>>>>>>>>>>>> proper suspend-resume sequencing of the device
>>>>>>>>>>>>>>>>>>>>>>>>>> drivers. In this case
>>>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq
>>>>>>>>>>>>>>>>>>>>>>>>>> driver is the driver that enables DFLL and
>>>>>>>>>>>>>>>>>>>>>>>>>> switches
>>>>>>>>>>>>>>>>>>>>>>>>>> CPU to that
>>>>>>>>>>>>>>>>>>>>>>>>>> clock
>>>>>>>>>>>>>>>>>>>>>>>>>> source, which means that this driver is also
>>>>>>>>>>>>>>>>>>>>>>>>>> should
>>>>>>>>>>>>>>>>>>>>>>>>>> be responsible for
>>>>>>>>>>>>>>>>>>>>>>>>>> management of the DFLL's state during of
>>>>>>>>>>>>>>>>>>>>>>>>>> suspend/resume process. If
>>>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver disables DFLL during suspend and
>>>>>>>>>>>>>>>>>>>>>>>>>> re-enables it
>>>>>>>>>>>>>>>>>>>>>>>>>> during
>>>>>>>>>>>>>>>>>>>>>>>>>> resume, then looks like the CaR driver hacks
>>>>>>>>>>>>>>>>>>>>>>>>>> around
>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL are not
>>>>>>>>>>>>>>>>>>>>>>>>>> needed.
>>>>>>>>>>>>>>>>>>>>>>>>>>>> The DFLL part looks good to me. BTW, change the
>>>>>>>>>>>>>>>>>>>>>>>>>>>> patch subject to
>>>>>>>>>>>>>>>>>>>>>>>>>>>> "Add
>>>>>>>>>>>>>>>>>>>>>>>>>>>> suspend-resume support" seems more
>>>>>>>>>>>>>>>>>>>>>>>>>>>> appropriate to
>>>>>>>>>>>>>>>>>>>>>>>>>>>> me.
>>>>>>>>>>>>>>>>>>>>>>>>>>> To clarify this, the sequences for DFLL use
>>>>>>>>>>>>>>>>>>>>>>>>>>> are as
>>>>>>>>>>>>>>>>>>>>>>>>>>> follows (assuming
>>>>>>>>>>>>>>>>>>>>>>>>>>> all
>>>>>>>>>>>>>>>>>>>>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Switch to DFLL:
>>>>>>>>>>>>>>>>>>>>>>>>>>> 0) Save current parent and frequency
>>>>>>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>>>>>>>> 2) Enable DFLL
>>>>>>>>>>>>>>>>>>>>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>>>>>>>>>>>>>>>>>>>>> For OVR regulator:
>>>>>>>>>>>>>>>>>>>>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>>>>>>>>>>>>>>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>>>>>>>>>>>>>>>>>>>>> For I2C regulator:
>>>>>>>>>>>>>>>>>>>>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>>>>>>>>>>>>>>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Switch away from DFLL:
>>>>>>>>>>>>>>>>>>>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU
>>>>>>>>>>>>>>>>>>>>>>>>>>> frequency is ok for
>>>>>>>>>>>>>>>>>>>>>>>>>>> any
>>>>>>>>>>>>>>>>>>>>>>>>>>> vdd_cpu voltage
>>>>>>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>>>>> I see during switch away from DFLL (suspend),
>>>>>>>>>>>>>>>>>>>>>>>> cclk_g
>>>>>>>>>>>>>>>>>>>>>>>> parent is not
>>>>>>>>>>>>>>>>>>>>>>>> changed to PLLP before changing dfll to open loop
>>>>>>>>>>>>>>>>>>>>>>>> mode.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Will add this ...
>>>>>>>>>>>>>>>>>>>>>>> The CPUFreq driver switches parent to PLLP during
>>>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>> probe, similar
>>>>>>>>>>>>>>>>>>>>>>> should be done on suspend.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> I'm also wondering if it's always safe to switch to
>>>>>>>>>>>>>>>>>>>>>>> PLLP in the probe.
>>>>>>>>>>>>>>>>>>>>>>> If CPU is running on a lower freq than PLLP, then
>>>>>>>>>>>>>>>>>>>>>>> some
>>>>>>>>>>>>>>>>>>>>>>> other more
>>>>>>>>>>>>>>>>>>>>>>> appropriate intermediate parent should be selected.
>>>>>>>>>>>>>>>>>>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always
>>>>>>>>>>>>>>>>>>>>>> runs at higher
>>>>>>>>>>>>>>>>>>>>>> rate
>>>>>>>>>>>>>>>>>>>>>> so switching to PLL_P during CPUFreq probe prior to
>>>>>>>>>>>>>>>>>>>>>> dfll clock enable
>>>>>>>>>>>>>>>>>>>>>> should be safe.
>>>>>>>>>>>>>>>>>>>>> AFAIK, PLLX could run at ~200MHz. There is also a
>>>>>>>>>>>>>>>>>>>>> divided output of
>>>>>>>>>>>>>>>>>>>>> PLLP
>>>>>>>>>>>>>>>>>>>>> which CCLKG supports, the PLLP_OUT4.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Probably, realistically, CPU is always running off a
>>>>>>>>>>>>>>>>>>>>> fast PLLX during
>>>>>>>>>>>>>>>>>>>>> boot, but I'm wondering what may happen on KEXEC. I
>>>>>>>>>>>>>>>>>>>>> guess ideally CPUFreq driver should also have a
>>>>>>>>>>>>>>>>>>>>> 'shutdown' callback to teardown DFLL
>>>>>>>>>>>>>>>>>>>>> on a reboot, but likely that there are other
>>>>>>>>>>>>>>>>>>>>> clock-related problems as
>>>>>>>>>>>>>>>>>>>>> well that may break KEXEC and thus it is not very
>>>>>>>>>>>>>>>>>>>>> important at the
>>>>>>>>>>>>>>>>>>>>> moment.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> [snip]
>>>>>>>>>>>>>>>>>>>> During bootup CPUG sources from PLL_X. By PLL_P source
>>>>>>>>>>>>>>>>>>>> above I meant
>>>>>>>>>>>>>>>>>>>> PLL_P_OUT4.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> As per clock policies, PLL_X is always used for high
>>>>>>>>>>>>>>>>>>>> freq
>>>>>>>>>>>>>>>>>>>> like
>>>>>>>>>>>>>>>>>>>>> 800Mhz
>>>>>>>>>>>>>>>>>>>> and for low frequency it will be sourced from PLLP.
>>>>>>>>>>>>>>>>>>> Alright, then please don't forget to pre-initialize
>>>>>>>>>>>>>>>>>>> PLLP_OUT4 rate to a
>>>>>>>>>>>>>>>>>>> reasonable value using tegra_clk_init_table or
>>>>>>>>>>>>>>>>>>> assigned-clocks.
>>>>>>>>>>>>>>>>>> PLLP_OUT4 rate update is not needed as it is safe to
>>>>>>>>>>>>>>>>>> run at
>>>>>>>>>>>>>>>>>> 408Mhz because it is below fmax @ Vmin
>>>>>>>>>>>>>>>>> So even 204MHz CVB entries are having the same voltage as
>>>>>>>>>>>>>>>>> 408MHz, correct? It's not instantly obvious to me from the
>>>>>>>>>>>>>>>>> DFLL driver's code where the fmax @ Vmin is defined, I see
>>>>>>>>>>>>>>>>> that there is the min_millivolts
>>>>>>>>>>>>>>>>> and frequency entries starting from 204MHZ defined
>>>>>>>>>>>>>>>>> per-table.
>>>>>>>>>>>>>>>> Yes at Vmin CPU Fmax is ~800Mhz. So anything below that
>>>>>>>>>>>>>>>> will
>>>>>>>>>>>>>>>> work at Vmin voltage and PLLP max is 408Mhz.
>>>>>>>>>>>>>>> Thank you for the clarification. It would be good to have
>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>> commented
>>>>>>>>>>>>>>> in the code as well.
>>>>>>>>>>>>>> OK, Will add...
>>>>>>>>>>>>> Regarding, adding suspend/resume to CPUFreq, CPUFreq suspend
>>>>>>>>>>>>> happens very early even before disabling non-boot CPUs and
>>>>>>>>>>>>> also
>>>>>>>>>>>>> need to export clock driver APIs to CPUFreq.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Was thinking of below way of implementing this...
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Clock DFLL driver Suspend:
>>>>>>>>>>>>>
>>>>>>>>>>>>> - Save CPU clock policy registers, and Perform
>>>>>>>>>>>>> dfll
>>>>>>>>>>>>> suspend which sets in open loop mode
>>>>>>>>>>>>>
>>>>>>>>>>>>> CPU Freq driver Suspend: does nothing
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Clock DFLL driver Resume:
>>>>>>>>>>>>>
>>>>>>>>>>>>> - Re-init DFLL, Set in Open-Loop mode, restore
>>>>>>>>>>>>> CPU
>>>>>>>>>>>>> Clock policy registers which actually sets source to DFLL
>>>>>>>>>>>>> along
>>>>>>>>>>>>> with other CPU Policy register restore.
>>>>>>>>>>>>>
>>>>>>>>>>>>> CPU Freq driver Resume:
>>>>>>>>>>>>>
>>>>>>>>>>>>> - do clk_prepare_enable which acutally sets
>>>>>>>>>>>>> DFLL in
>>>>>>>>>>>>> Closed loop mode
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Adding one more note: Switching CPU Clock to PLLP is not
>>>>>>>>>>>>> needed
>>>>>>>>>>>>> as CPU CLock can be from dfll in open-loop mode as DFLL is not
>>>>>>>>>>>>> disabled anywhere throught the suspend/resume path and SC7
>>>>>>>>>>>>> entry
>>>>>>>>>>>>> FW and Warm boot code will switch CPU source to PLLP.
>>>>>>>>>>> Since CPU resumes on PLLP, it will be cleaner to suspend it on
>>>>>>>>>>> PLLP as well. And besides, seems that currently disabling DFLL
>>>>>>>>>>> clock will disable DFLL completely and then you'd want to
>>>>>>>>>>> re-init
>>>>>>>>>>> the DFLL on resume any ways. So better to just disable DFLL
>>>>>>>>>>> completely on suspend, which should happen on clk_disable(dfll).
>>>>>>>>>> Will switch to PLLP during CPUFreq suspend. With decision of
>>>>>>>>>> using
>>>>>>>>>> clk_disable during suspend, its mandatory to switch to PLLP as
>>>>>>>>>> DFLL
>>>>>>>>>> is completely disabled.
>>>>>>>>>>
>>>>>>>>>> My earlier concern was on restoring CPU policy as we can't do
>>>>>>>>>> that
>>>>>>>>>> from CPUFreq driver and need export from clock driver.
>>>>>>>>>>
>>>>>>>>>> Clear now and will do CPU clock policy restore in after dfll
>>>>>>>>>> re-init.
>>>>>>>>> Why the policy can't be saved/restored by the CaR driver as a
>>>>>>>>> context of any other clock?
>>>>>>>> restoring cpu clock policy involves programming source and
>>>>>>>> super_cclkg_divider.
>>>>>>>>
>>>>>>>> cclk_g is registered as clk_super_mux and it doesn't use
>>>>>>>> frac_div ops
>>>>>>>> to do save/restore its divider.
>>>>>>> That can be changed of course and I guess it also could be as
>>>>>>> simple as
>>>>>>> saving and restoring of two raw u32 values of the policy/divider
>>>>>>> registers.
>>>>>>>
>>>>>>>> Also, during clock context we cant restore cclk_g as cclk_g source
>>>>>>>> will be dfll and dfll will not be resumed/re-initialized by the
>>>>>>>> time
>>>>>>>> clk_super_mux save/restore happens.
>>>>>>>>
>>>>>>>> we can't use save/restore context for dfll clk_ops because
>>>>>>>> dfllCPU_out parent to CCLK_G is first in the clock tree and
>>>>>>>> dfll_ref
>>>>>>>> and dfll_soc peripheral clocks are not restored by the time dfll
>>>>>>>> restore happens. Also dfll peripheral clock enables need to be
>>>>>>>> restored before dfll restore happens which involves programming
>>>>>>>> dfll
>>>>>>>> controller for re-initialization.
>>>>>>>>
>>>>>>>> So dfll resume/re-init is done in clk-tegra210 at end of all clocks
>>>>>>>> restore in V5 series but instead of in clk-tegra210 driver I moved
>>>>>>>> now to dfll-fcpu driver pm_ops as all dfll dependencies will be
>>>>>>>> restored thru clk_restore_context by then. This will be in V6.
>>>>>>> Since DFLL is now guaranteed to be disabled across CaR
>>>>>>> suspend/resume
>>>>>>> (hence it has nothing to do in regards to CCLK) and given that PLLs
>>>>>>> state is restored before the rest of the clocks, I don't see why
>>>>>>> not to
>>>>>>> implement CCLK save/restore in a generic fasion. CPU policy wull be
>>>>>>> restored to either PLLP or PLLX (if CPUFreq driver is disabled).
>>>>>>>
>>>>>> CCLK_G save/restore should happen in clk_super_mux ops
>>>>>> save/context and
>>>>>> clk_super_mux save/restore happens very early as cclk_g is first
>>>>>> in the
>>>>>> clock tree and save/restore traverses through the tree top-bottom
>>>>>> order.
>>>>> If CCLK_G is restored before the PLLs, then just change the clocks
>>>>> order
>>>>> such that it won't happen.
>>>>>
>>>> I dont think we can change clocks order for CCLK_G.
>>>>
>>>> During bootup, cclk_g is registered after all pll's and peripheral
>>>> clocks which is the way we wanted, So cclk_g will be the first one in
>>>> the clk list as clk_register adds new clock first in the list.
>>>>
>>>> When clk_save_context and clk_restore_context APIs iterates over the
>>>> list, cclk_g is the first
>>> Looking at clk_core_restore_context(), I see that it walks up CLKs list
>>> from parent to children, hence I don't understand how it can ever happen
>>> that CCLK will be restored before the parent. The clocks registration
>>> order doesn't matter at all in that case.
>>
>> yes from parent to children and dfllCPU_out is the top in the list and
>> its child is cclk_g.
>>
>> the way clocks are registered is the order I see in the clock list and
>> looking into clk_register API it adds new node first in the list.
>>
> cclkg_g & dfll register happens after all plls and peripheral clocks as
> it need ref, soc and peripheral clocks to be enabled.
>>
>> So they are the last to get registered and so becomes first in the list.
>>
>> During save/restore context, it traverses thru this list and first in
>> the list is dfllcpu_OUT (parent) and its child (cclk_g)
>>
>> saving should not be an issue at all but we cant restore cclk_g/dfll
>> in normal way thru clk_ops restore as plls and peripherals restore
>> doesn't happen by that time.
>>
>
> I was referring to clk_restore_context where it iterates thru root list
> and for each core from the root list clk_core_restore does restore of
> parent and children.
>
> dfllCPU_Out gets first in the list and its child is cclk_g
>
> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/clk/clk.c#L1105
What list you're talking about? clk_summary? It shows current *active*
clocks configuration, if you'll try to disable CPUFreq driver then the
parent of CCLK_G should be PLLX. Similarly when CPU is reparented to
PLLP on driver's suspend, then PLLP is the parent.
>>>>>> DFLL enable thru CPUFreq resume happens after all clk_restore_context
>>>>>> happens. So during clk_restore_context, dfll re-init doesnt happen
>>>>>> and
>>>>>> doing cpu clock policy restore during super_mux clk_ops will crash as
>>>>>> DFLL is not initialized and its clock is not enabled but CPU clock
>>>>>> restore sets source to DFLL if we restore during super_clk_mux
>>>>> If CPU was suspended on PLLP, then it will be restored on PLLP by
>>>>> CaR. I
>>>>> don't understand what DFLL has to do with the CCLK in that case during
>>>>> the clocks restore.
>>>> My above comment is in reference to your request of doing save/restore
>>>> for cclk_g in normal fashion thru save/restore context. Because of the
>>>> clk order I mentioned above, we cclk_g will be the first one to go thru
>>>> save/context.
>>>>
>>>> During save_context of cclk_g, source can be from PLLX, dfll.
>>>>
>>>> Issue will be when we do restore during clk_restore_context of
>>>> cclk_g as
>>>> by that time PLLX/dfll will not be restored.
>>>>
>>> Seems we already agreed that DFLL will be disabled by the CPUFreq driver
>>> on suspend. Hence CCLK can't be from DFLL if CPU is reparented to PLLP
>>> on CPUFreq driver's suspend, otherwise CPU keeps running from a
>>> boot-state PLLX if CPUFreq driver is disabled.
>>
>> Yes suspend should not be an issue but issue will be during resume
>> where if we do cclk_g restore in normal way thru clk_restore_context,
>> cclk_g restore happens very early as dfllCPU out is the first one that
>> goes thru restore context and plls/peripherals are not resumed by then.
>>
>> CPU runs from PLLX if dfll clock enable fails during boot. So when it
>> gets to suspend, we save CPU running clock source as either PLLX or
>> DFLL and then we switch to PLLP.
>>
>>
>> On resume, CPU runs from PLLP by warm boot code and we need to restore
>> back its source to the one it was using from saved source context
>> (which can be either PLLX or DFLL)
>>
>> So PLLs & DFLL resume need to happen before CCLKG restore/resume.
>>
>>
>> With all above discussions, we do DFLL disable in CPUFreq driver on
>> suspend and on CPUFreq resume we enable DFLL back and restore CPU
>> clock source it was using during suspend (which will be either PLLX if
>> dfll enable fails during probe or it will be using DFLL).
During suspend CPU's parent shall be PLLP and not DFLL (note that it is
disabled) after reparenting to PLLP by the CPUFreq driver.
>> So i was trying to say dfll/cclk_g restore can't be done in normal way
>> thru clk_ops save/restore context
Let's see what happens if CPUFreq is active:
1. CPUFreq driver probe happens
2. CPU is reparented to PLLP
3. DFLL inited
4. CPU is reparented to DFLL
5. CPUFreq driver suspend happens
6. CPU is reparented to PLLP
7. DFLL is disabled
8. Car suspend happens
9. DFLL context saved
10. PLLP/PLLX context saved
11. CCLK context saved
12. Car resume happens
13. DFLL context restored
14. PLLP/PLLX context restored
15. CCLK context restored
16. CPUFreq driver resume happens
17. DFLL re-inited
18. CPU is reparented to DFLL
^ permalink raw reply
* Re: [PATCH] riscv: dts: Add DT node for SiFive FU540 Ethernet controller driver
From: Paul Walmsley @ 2019-07-17 19:30 UTC (permalink / raw)
To: Yash Shah
Cc: robh+dt, devicetree, linux-riscv, linux-kernel, mark.rutland,
palmer, aou, sachin.ghadi
In-Reply-To: <1561097422-25130-2-git-send-email-yash.shah@sifive.com>
On Fri, 21 Jun 2019, Yash Shah wrote:
> DT node for SiFive FU540-C000 GEMGXL Ethernet controller driver added
>
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
Dropping this one until it's updated to align with whatever you pick for
the new driver DT compatible string, per
https://lore.kernel.org/linux-riscv/1558611952-13295-1-git-send-email-yash.shah@sifive.com/T/#me298e16b2feeb481116cca61b94e014433cf2977
Please send an updated DT patch at your earliest convenience so we can get
it into the v5.3-rc series.
- Paul
^ permalink raw reply
* [PATCH 4/4] mailbox: arm_mhuv2: add multi word transport protocol operations
From: Tushar Khandelwal @ 2019-07-17 19:26 UTC (permalink / raw)
To: linux-kernel
Cc: tushar.2nov, morten_bp, jassisinghbrar, nd, Morten Borup Petersen,
Tushar Khandelwal, devicetree
In-Reply-To: <20190717192616.1731-1-tushar.khandelwal@arm.com>
From: Morten Borup Petersen <morten.petersen@arm.com>
When in multi-word mode, the mailbox controller will provide a single
mailbox. It is required that the MHU device has at least 2 channel windows
available for the MHU to function in multi-word mode.
Transmitting and receiving data through the mailbox framework in
multi-word mode is done through a struct arm_mbox_msg.
Signed-off-by: Morten Borup Petersen <morten.petersen@arm.com>
Signed-off-by: Tushar Khandelwal <tushar.khandelwal@arm.com>
Cc: jassisinghbrar@gmail.com
Cc: devicetree@vger.kernel.org
---
drivers/mailbox/arm_mhu_v2.c | 225 +++++++++++++++++++++++++++++++++++
1 file changed, 225 insertions(+)
diff --git a/drivers/mailbox/arm_mhu_v2.c b/drivers/mailbox/arm_mhu_v2.c
index 0e3fa5917925..324b19bdb28a 100644
--- a/drivers/mailbox/arm_mhu_v2.c
+++ b/drivers/mailbox/arm_mhu_v2.c
@@ -430,6 +430,228 @@ static const struct mhuv2_ops mhuv2_single_word_ops = {
};
/* ========================================================================== */
+/* ================ Multi word transport protocol operations ================ */
+static inline int mhuv2_read_data_multi_word(struct arm_mhuv2 *mhuv2,
+ struct mbox_chan *chan,
+ struct arm_mbox_msg *msg)
+{
+ int ch;
+ const int channels =
+ readl_relaxed_bitfield(&mhuv2->reg.recv->MHU_CFG, NUM_CH);
+
+ msg->data = kzalloc(MHUV2_STAT_BYTES * channels, GFP_KERNEL);
+
+ for (ch = 0; ch < channels; ch++) {
+ /*
+ * Messages are expected to be received in order of most
+ * significant word to least significant word.
+ * (see mhuv2_send_data_multi_word)
+ */
+ const mhuv2_stat_reg_t word =
+ readl_relaxed(&mhuv2->reg.recv->channel[ch].STAT);
+ ((mhuv2_stat_reg_t *)msg->data)[channels - 1 - ch] = word;
+ }
+
+ msg->len = channels * MHUV2_STAT_BYTES;
+ return 0;
+}
+
+static inline int mhuv2_clear_data_multi_word(struct arm_mhuv2 *mhuv2,
+ struct mbox_chan *chan,
+ struct arm_mbox_msg *msg)
+{
+ int ch;
+ const int channels =
+ readl_relaxed_bitfield(&mhuv2->reg.recv->MHU_CFG, NUM_CH);
+
+ for (ch = 0; ch < channels; ch++) {
+ /*
+ * Last channel window must be cleared as the final operation.
+ * Upon clearing the last channel window register, which is
+ * unmasked in multi-word mode, the interrupt is deasserted.
+ */
+ writel_relaxed(
+ readl_relaxed(&mhuv2->reg.recv->channel[ch].STAT),
+ &mhuv2->reg.recv->channel[ch].STAT_CLEAR);
+ }
+ return 0;
+}
+
+static inline int __mhuv2_mw_bytes_to_send(const int bytes_in_round,
+ const int bytes_left)
+{
+ /*
+ * Bytes to send on the current channel will always be MHUV2_STAT_BYTES
+ * unless in the last round and
+ * msg->len % MHUV2_STAT_BYTES != 0
+ */
+ if (bytes_in_round % MHUV2_STAT_BYTES != 0) {
+ const int bts = bytes_left % MHUV2_STAT_BYTES;
+ return bts == 0 ? MHUV2_STAT_BYTES : bts;
+ } else {
+ return MHUV2_STAT_BYTES;
+ }
+}
+
+static inline int mhuv2_send_data_multi_word(struct arm_mhuv2 *mhuv2,
+ struct mbox_chan *chan,
+ const struct arm_mbox_msg *msg)
+{
+ /*
+ * Message will be transmitted from most significant to least
+ * significant word. This is to allow for messages shorter than
+ * $channels to still trigger the receiver interrupt which gets
+ * activated when the last STAT register is written. As an example, a
+ * 6-word message is to be written on a 4-channel MHU connection:
+ * Registers marked with '*' are masked, and will not generate an
+ * interrupt on the receiver side once written.
+ *
+ * uint32_t *data = [0x00000001],[0x00000002],[0x00000003],[0x00000004],
+ * [0x00000005], [0x00000006]
+ *
+ * ROUND 1:
+ * STAT reg To write Write sequence
+ * [ STAT 3 ] <- [0x00000001] 4 <- triggers interrupt on receiver
+ * *[ STAT 2 ] <- [0x00000002] 3
+ * *[ STAT 1 ] <- [0x00000003] 2
+ * *[ STAT 0 ] <- [0x00000004] 1
+ *
+ * data += 4 // Increment data pointer by number of STAT regs
+ *
+ * ROUND 2:
+ * STAT reg To write Write sequence
+ * [ STAT 3 ] <- [0x00000005] 2 <- triggers interrupt on receiver
+ * *[ STAT 2 ] <- [0x00000006] 1
+ * *[ STAT 1 ] <- [0x00000000]
+ * *[ STAT 0 ] <- [0x00000000]
+ */
+ int bytes_left, bytes_to_send, i, ch_idx;
+ const int ch_windows =
+ readl_relaxed_bitfield(&mhuv2->reg.recv->MHU_CFG, NUM_CH);
+ const size_t round_capacity = ch_windows * MHUV2_STAT_BYTES;
+
+ bytes_left = msg->len;
+ mhuv2_stat_reg_t *data = msg->data;
+
+ while (bytes_left > 0) {
+ /* Note: Each entry of this loop indicates a new ROUND */
+ if (*(u32 *)data == 0) {
+ dev_err(mhuv2->dev,
+ "values in *data aligned on NUM_STAT boundaries must not be zero to ensure that receiver interrupt is triggered\n",
+ ch_windows);
+ return -EINVAL;
+ }
+
+ const int bytes_in_round = bytes_left > round_capacity ?
+ round_capacity :
+ bytes_left;
+
+ for (i = (ch_windows - 1); i >= 0; i--) {
+ ch_idx = ch_windows - 1 - i;
+ /*
+ * Check whether data should be transmitted in register
+ * of index 'ch'.
+ */
+ if (bytes_in_round > (i * MHUV2_STAT_BYTES)) {
+ mhuv2_stat_reg_t word = data[i];
+
+ bytes_to_send = __mhuv2_mw_bytes_to_send(
+ bytes_in_round, bytes_left);
+
+ if (bytes_to_send != MHUV2_STAT_BYTES) {
+ word &= LSB_MASK(bytes_to_send *
+ __CHAR_BIT__);
+ }
+ while (readl_relaxed(
+ &mhuv2->reg.send->channel[ch_idx]
+ .STAT) != 0)
+ continue;
+
+ writel_relaxed(
+ word,
+ &mhuv2->reg.send->channel[ch_idx].STAT_SET);
+ bytes_left -= bytes_to_send;
+ }
+ }
+
+ data += ch_windows;
+
+ for (ch_idx = 0; ch_idx < ch_windows; ch_idx++) {
+ while (readl_relaxed(
+ &mhuv2->reg.send->channel[ch_idx].STAT) != 0)
+ continue;
+ }
+ }
+ return 0;
+}
+
+
+static inline int mhuv2_last_tx_done_multi_word(struct arm_mhuv2 *mhuv2,
+ struct mbox_chan *chan)
+{
+ int ch_idx;
+ bool tx_done = true;
+
+ for (ch_idx = 0;
+ ch_idx < readl_relaxed_bitfield(&mhuv2->reg.send->MHU_CFG, NUM_CH);
+ ch_idx++) {
+ tx_done &= readl_relaxed(
+ &mhuv2->reg.send->channel[ch_idx].STAT) == 0;
+ }
+ return tx_done;
+}
+
+static inline int mhuv2_setup_multi_word(struct arm_mhuv2 *mhuv2)
+{
+ int ret, i;
+
+ const u32 channel_windows =
+ readl_relaxed_bitfield(mhuv2->frame == RECEIVER_FRAME ?
+ &mhuv2->reg.recv->MHU_CFG :
+ &mhuv2->reg.send->MHU_CFG,
+ NUM_CH);
+ if (channel_windows < 2) {
+ dev_err(mhuv2->dev,
+ "Error: at least 2 MHU channel windows are required for using the multi-word transfer protocol");
+ return -ENODEV;
+ }
+
+ if (mhuv2->frame == RECEIVER_FRAME) {
+ /*
+ * The multi-word transport protocol mandates that all but
+ * the last status register must be masked.
+ */
+ for (i = 0; i < (channel_windows - 1); i++) {
+ writel_relaxed(-1,
+ &mhuv2->reg.recv->channel[i].MASK_SET);
+ }
+ }
+
+ mhuv2->mbox.num_chans = 1;
+ mhuv2->mbox.chans =
+ devm_kzalloc(mhuv2->dev,
+ mhuv2->mbox.num_chans * sizeof(struct mbox_chan),
+ GFP_KERNEL);
+
+ return 0;
+}
+
+static inline struct mbox_chan *
+ mhuv2_get_active_mbox_chan_multi_word(struct arm_mhuv2 *mhuv2)
+{
+ return &mhuv2->mbox.chans[0];
+}
+
+static const struct mhuv2_ops mhuv2_multi_word_ops = {
+ .read_data = mhuv2_read_data_multi_word,
+ .clear_data = mhuv2_clear_data_multi_word,
+ .send_data = mhuv2_send_data_multi_word,
+ .setup = mhuv2_setup_multi_word,
+ .last_tx_done = mhuv2_last_tx_done_multi_word,
+ .get_active_mbox_chan = mhuv2_get_active_mbox_chan_multi_word,
+};
+/* ========================================================================== */
+
/* =================== Doorbell transport protocol operations =============== */
static inline int mhuv2_read_data_doorbell(struct arm_mhuv2 *mhuv2,
@@ -740,6 +962,9 @@ static int mhuv2_probe(struct amba_device *adev, const struct amba_id *id)
/* Assign transport protocol-specific operations */
switch (mhuv2->protocol) {
+ case MULTI_WORD:
+ mhuv2->ops = &mhuv2_multi_word_ops;
+ break;
case SINGLE_WORD:
mhuv2->ops = &mhuv2_single_word_ops;
break;
--
2.17.1
^ permalink raw reply related
* [PATCH 3/4] mailbox: arm_mhuv2: add doorbell transport protocol operations
From: Tushar Khandelwal @ 2019-07-17 19:26 UTC (permalink / raw)
To: linux-kernel
Cc: tushar.2nov, morten_bp, jassisinghbrar, nd, Morten Borup Petersen,
Tushar Khandelwal, devicetree
In-Reply-To: <20190717192616.1731-1-tushar.khandelwal@arm.com>
From: Morten Borup Petersen <morten.petersen@arm.com>
In doorbell mode, the mailbox controller will provide a mailbox for each
flag bit available in the combined number of channel windows available
within the MHU device.
When in doorbell mode, the MHU should be used solely as an interrupt
generating mechanism. If data is to be transmitted, this must be done
out-band, ie. through shared memory, by a driving utilizing the mailbox
for interrupt generation.
Signed-off-by: Morten Borup Petersen <morten.petersen@arm.com>
Signed-off-by: Tushar Khandelwal <tushar.khandelwal@arm.com>
Cc:Jassi Brar <jassisinghbrar@gmail.com
Cc: devicetree@vger.kernel.org
---
drivers/mailbox/arm_mhu_v2.c | 108 +++++++++++++++++++++++++++++++++++
1 file changed, 108 insertions(+)
diff --git a/drivers/mailbox/arm_mhu_v2.c b/drivers/mailbox/arm_mhu_v2.c
index a0af683b83a2..0e3fa5917925 100644
--- a/drivers/mailbox/arm_mhu_v2.c
+++ b/drivers/mailbox/arm_mhu_v2.c
@@ -430,6 +430,111 @@ static const struct mhuv2_ops mhuv2_single_word_ops = {
};
/* ========================================================================== */
+/* =================== Doorbell transport protocol operations =============== */
+
+static inline int mhuv2_read_data_doorbell(struct arm_mhuv2 *mhuv2,
+ struct mbox_chan *chan,
+ struct arm_mbox_msg *msg)
+{
+ return 0;
+}
+
+static inline int mhuv2_clear_data_doorbell(struct arm_mhuv2 *mhuv2,
+ struct mbox_chan *chan,
+ struct arm_mbox_msg *msg)
+{
+ const u32 ch_mbox_idx = mhuv2_chan_idx(chan);
+ const u32 ch_window_idx = ch_mbox_idx / MHUV2_STAT_BITS;
+ const u32 ch_window_reg_idx = ch_mbox_idx % MHUV2_STAT_BITS;
+
+ writel_relaxed(BIT(ch_window_reg_idx),
+ &mhuv2->reg.recv->channel[ch_window_idx].STAT_CLEAR);
+ return 0;
+}
+
+static inline int mhuv2_send_data_doorbell(struct arm_mhuv2 *mhuv2,
+ struct mbox_chan *chan,
+ const struct arm_mbox_msg *msg)
+{
+ const u32 ch_mbox_idx = mhuv2_chan_idx(chan);
+ const u32 ch_window_idx = ch_mbox_idx / MHUV2_STAT_BITS;
+ const u32 ch_window_reg_idx = ch_mbox_idx % MHUV2_STAT_BITS;
+
+ writel_relaxed(
+ readl_relaxed(&mhuv2->reg.send->channel[ch_window_idx].STAT) |
+ BIT(ch_window_reg_idx),
+ &mhuv2->reg.send->channel[ch_window_idx].STAT_SET);
+ return 0;
+}
+
+static inline int mhuv2_last_tx_done_doorbell(struct arm_mhuv2 *mhuv2,
+ struct mbox_chan *chan)
+{
+ const u32 ch_mbox_idx = mhuv2_chan_idx(chan);
+ const u32 ch_window_idx = ch_mbox_idx / MHUV2_STAT_BITS;
+ const u32 ch_window_reg_idx = ch_mbox_idx % MHUV2_STAT_BITS;
+
+ return (readl_relaxed(&mhuv2->reg.send->channel[ch_window_idx].STAT) &
+ BIT(ch_window_reg_idx)) == 0;
+}
+
+static inline int mhuv2_setup_doorbell(struct arm_mhuv2 *mhuv2)
+{
+ int i;
+ const u32 channel_windows =
+ readl_relaxed_bitfield(mhuv2->frame == RECEIVER_FRAME ?
+ &mhuv2->reg.recv->MHU_CFG :
+ &mhuv2->reg.send->MHU_CFG,
+ NUM_CH);
+
+ mhuv2->mbox.num_chans = MHUV2_STAT_BITS * channel_windows;
+ mhuv2->mbox.chans =
+ devm_kzalloc(mhuv2->dev,
+ mhuv2->mbox.num_chans * sizeof(struct mbox_chan),
+ GFP_KERNEL);
+
+ for (i = 0; i < mhuv2->mbox.num_chans; i++) {
+ mhuv2->mbox.chans[i].con_priv =
+ devm_kzalloc(mhuv2->dev,
+ sizeof(struct arm_mhuv2_mbox_chan_priv),
+ GFP_KERNEL);
+ mhuv2_chan_idx(&mhuv2->mbox.chans[i]) = i;
+ }
+
+ if (mhuv2->frame == RECEIVER_FRAME) {
+ /* Ensure all status registers are unmasked */
+ for (i = 0; i < channel_windows; i++) {
+ writel_relaxed(0x0,
+ &mhuv2->reg.recv->channel[i].MASK_SET);
+ }
+ }
+
+ return 0;
+}
+
+static inline struct mbox_chan *
+ mhuv2_get_active_mbox_chan_doorbell(struct arm_mhuv2 *mhuv2)
+{
+ const u32 ch_window_idx = mhuv2_combint_idx(mhuv2);
+ const u32 ch_window_reg_idx = __find_set_bit(
+ readl_relaxed(&mhuv2->reg.recv->channel[ch_window_idx].STAT));
+ if (ch_window_reg_idx == -1)
+ return ERR_PTR(-EIO);
+
+ return &mhuv2->mbox.chans[ch_window_reg_idx +
+ ch_window_idx * MHUV2_STAT_BITS];
+}
+
+static const struct mhuv2_ops mhuv2_doorbell_ops = {
+ .read_data = mhuv2_read_data_doorbell,
+ .clear_data = mhuv2_clear_data_doorbell,
+ .send_data = mhuv2_send_data_doorbell,
+ .setup = mhuv2_setup_doorbell,
+ .last_tx_done = mhuv2_last_tx_done_doorbell,
+ .get_active_mbox_chan = mhuv2_get_active_mbox_chan_doorbell,
+};
+/* ========================================================================== */
+
/*
* MHUv2 receiver interrupt service routine
*
@@ -638,6 +743,9 @@ static int mhuv2_probe(struct amba_device *adev, const struct amba_id *id)
case SINGLE_WORD:
mhuv2->ops = &mhuv2_single_word_ops;
break;
+ case DOORBELL:
+ mhuv2->ops = &mhuv2_doorbell_ops;
+ break;
default:
return -ENODEV;
}
--
2.17.1
^ permalink raw reply related
* [PATCH 2/4] mailbox: arm_mhuv2: add arm mhuv2 driver
From: Tushar Khandelwal @ 2019-07-17 19:26 UTC (permalink / raw)
To: linux-kernel
Cc: tushar.2nov, morten_bp, jassisinghbrar, nd, Morten Borup Petersen,
Tushar Khandelwal, devicetree
In-Reply-To: <20190717192616.1731-1-tushar.khandelwal@arm.com>
From: Morten Borup Petersen <morten.petersen@arm.com>
This commit adds a driver for the ARM MHUv2 (Message Handling Unit).
The driver registers itself as a mailbox controller within the common
mailbox framework.
This commit implements the single-word transport protocol;
In single-word mode, the mailbox controller will provide a mailbox for each
channel window available in the MHU device.
Transmitting and receiving data through the mailbox framework in
single-word mode is done through a struct arm_mbox_msg.
Signed-off-by: Morten Borup Petersen <morten.petersen@arm.com>
Signed-off-by: Tushar Khandelwal <tushar.khandelwal@arm.com>
Cc: jassisinghbrar@gmail.com
Cc: devicetree@vger.kernel.org
---
drivers/mailbox/Kconfig | 7 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/arm_mhu_v2.c | 735 +++++++++++++++++++++++
include/linux/mailbox/arm-mbox-message.h | 37 ++
4 files changed, 781 insertions(+)
create mode 100644 drivers/mailbox/arm_mhu_v2.c
create mode 100644 include/linux/mailbox/arm-mbox-message.h
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index e63d29a95e76..6808bba5bf9b 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -15,6 +15,13 @@ config ARM_MHU
The controller has 3 mailbox channels, the last of which can be
used in Secure mode only.
+config ARM_MHU_V2
+ tristate "ARM MHUv2 Mailbox"
+ depends on ARM_AMBA
+ help
+ Say Y here if you want to build the ARM MHUv2 controller driver,
+ which provides unidirectional mailboxes between processing elements.
+
config PLATFORM_MHU
tristate "Platform MHU Mailbox"
depends on OF
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index f15788ccc572..6edef52d74bb 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST) += mailbox-test.o
obj-$(CONFIG_ARM_MHU) += arm_mhu.o
+obj-$(CONFIG_ARM_MHU_V2) += arm_mhu_v2.o
+
obj-$(CONFIG_PLATFORM_MHU) += platform_mhu.o
obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o
diff --git a/drivers/mailbox/arm_mhu_v2.c b/drivers/mailbox/arm_mhu_v2.c
new file mode 100644
index 000000000000..a0af683b83a2
--- /dev/null
+++ b/drivers/mailbox/arm_mhu_v2.c
@@ -0,0 +1,735 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Arm Message Handling Unit Version 2 (MHUv2) driver
+ *
+ * An MHU device may function in one of three transport protocol modes
+ * (single-word, multi-word and doorbell).
+ * This transport protocol should be specified in the device tree entry for the
+ * device. The transport protocol determines how the underlying hardware
+ * resources of the device are utilized when transmitting data.
+ *
+ * The arm MHUv2 driver registers as a mailbox controller with the common
+ * mailbox framework. Each mailbox channel represents a separate virtual
+ * communication channel through the MHU device.
+ * The number of registered mailbox channels is dependent on both the
+ * underlying hardware - mainly the number of channel windows within each MHU
+ * frame, as well as the selected transport protocol.
+ *
+ * Copyright (C) 2019 Arm Ltd.
+ */
+
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/amba/bus.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox/arm-mbox-message.h>
+#include <linux/of_address.h>
+#include <linux/interrupt.h>
+
+/* Maximum number of channel windows */
+#define MHUV2_CHANNEL_MAX 124
+/* Number of combined interrupt status registers */
+#define MHUV2_CMB_INT_ST_REG_CNT 4
+#define MHUV2_CH_UNKNOWN -1
+
+/* Channel window status register type */
+typedef u32 mhuv2_stat_reg_t;
+#define MHUV2_STAT_BYTES sizeof(mhuv2_stat_reg_t)
+#define MHUV2_STAT_BITS (MHUV2_STAT_BYTES * __CHAR_BIT__)
+
+#define LSB_MASK(n) ((1 << (n)) - 1)
+
+#ifndef PAD
+#define _PADLINE(line) pad##line
+#define _XSTR(line) _PADLINE(line)
+#define PAD _XSTR(__LINE__)
+#endif
+
+/* ====== Arm MHUv2 register defines ====== */
+
+/* Register Message Handling Unit Configuration fields */
+struct MHU_CFG_t {
+ u32 NUM_CH : 7;
+ u32 PAD : 25;
+} __packed;
+
+/* Register Implementer Identification fields */
+struct IIDR_t {
+ u32 IMPLEMENTER : 12;
+ u32 REVISION : 4;
+ u32 VARIANT : 4;
+ u32 PRODUCT_ID : 12;
+} __packed;
+
+/* Register Architecture Identification Register fields */
+struct AIDR_t {
+ u32 ARCH_MINOR_REV : 4;
+ u32 ARCH_MAJOR_REV : 4;
+ u32 PAD : 24;
+} __packed;
+
+/* register Interrupt Status fields */
+struct INT_ST_t {
+ u32 NR2R : 1;
+ u32 R2NR : 1;
+ u32 PAD : 30;
+} __packed;
+
+/* Register Interrupt Clear fields */
+struct INT_CLR_t {
+ u32 NR2R : 1;
+ u32 R2NR : 1;
+ u32 PAD : 30;
+} __packed;
+
+/* Register Interrupt Enable fields */
+struct INT_EN_t {
+ u32 R2NR : 1;
+ u32 NR2R : 1;
+ u32 CHCOMB : 1;
+ u32 PAD : 29;
+} __packed;
+
+/* Sender Channel Window fields */
+struct mhu2_send_channel_reg {
+ mhuv2_stat_reg_t STAT;
+ u8 PAD[0xC - 0x4];
+ mhuv2_stat_reg_t STAT_SET;
+ u8 PAD[0x20 - 0x10];
+} __packed;
+
+/* Sender frame register fields */
+struct mhu2_send_frame_reg {
+ struct mhu2_send_channel_reg channel[MHUV2_CHANNEL_MAX];
+ struct MHU_CFG_t MHU_CFG;
+ u32 RESP_CFG;
+ u32 ACCESS_REQUEST;
+ u32 ACCESS_READY;
+ struct INT_ST_t INT_ST;
+ struct INT_CLR_t INT_CLR;
+ struct INT_EN_t INT_EN;
+ u32 RESERVED0;
+ u32 CHCOMB_INT_ST[MHUV2_CMB_INT_ST_REG_CNT];
+ u8 PAD[0xFC8 - 0xFB0];
+ struct IIDR_t IIDR;
+ struct AIDR_t AIDR;
+} __packed;
+
+/* Receiver Channel Window fields */
+struct mhu2_recv_channel_reg {
+ mhuv2_stat_reg_t STAT;
+ mhuv2_stat_reg_t STAT_PEND;
+ mhuv2_stat_reg_t STAT_CLEAR;
+ u8 RESERVED0[0x10 - 0x0C];
+ mhuv2_stat_reg_t MASK;
+ mhuv2_stat_reg_t MASK_SET;
+ mhuv2_stat_reg_t MASK_CLEAR;
+ u8 PAD[0x20 - 0x1C];
+} __packed;
+
+/* Receiver frame register fields */
+struct mhu2_recv_frame_reg {
+ struct mhu2_recv_channel_reg channel[MHUV2_CHANNEL_MAX];
+ struct MHU_CFG_t MHU_CFG;
+ u8 RESERVED0[0xF90 - 0xF84];
+ struct INT_ST_t INT_ST;
+ struct INT_CLR_t INT_CLR;
+ struct INT_EN_t INT_EN;
+ u32 PAD;
+ mhuv2_stat_reg_t CHCOMB_INT_ST[MHUV2_CMB_INT_ST_REG_CNT];
+ u8 RESERVED2[0xFC8 - 0xFB0];
+ struct IIDR_t IIDR;
+ struct AIDR_t AIDR;
+} __packed;
+
+/* ====== Arm MHUv2 device tree property identifiers ====== */
+
+static const char *const mhuv2_protocol_dt_identifiers[] = { "single-word",
+ "multi-word",
+ "doorbell" };
+
+static const char *const mhuv2_frame_dt_identifiers[] = { "receiver",
+ "sender" };
+
+/* ====== Arm MHUv2 data structures ====== */
+
+enum mhuv2_transport_protocol { SINGLE_WORD, MULTI_WORD, DOORBELL };
+
+enum mhuv2_frame { RECEIVER_FRAME, SENDER_FRAME };
+
+/**
+ * Arm MHUv2 operations
+ *
+ * Each transport protocol must provide an implementation of the operations
+ * presented in this structure.
+ * Most operations present a struct mbox_chan* as argument. This channel will
+ * correspond to one of the virtual channels within the MHU device. What
+ * constitutes a channel within the MHU device is dependent on the transport
+ * protocol.
+ */
+struct arm_mhuv2;
+struct mhuv2_ops {
+ int (*read_data)(struct arm_mhuv2 *mhuv2, struct mbox_chan *chan,
+ struct arm_mbox_msg *msg);
+ int (*clear_data)(struct arm_mhuv2 *mhuv2, struct mbox_chan *chan,
+ struct arm_mbox_msg *msg);
+ int (*send_data)(struct arm_mhuv2 *mhuv2, struct mbox_chan *chan,
+ const struct arm_mbox_msg *msg);
+ int (*setup)(struct arm_mhuv2 *mhuv2);
+ int (*last_tx_done)(struct arm_mhuv2 *mhuv2, struct mbox_chan *chan);
+ struct mbox_chan *(*get_active_mbox_chan)(struct arm_mhuv2 *mhuv2);
+};
+
+/**
+ * Arm MHUv2 mailbox channel information
+ *
+ * A channel contains a notion of its index within the array of mailbox channels
+ * which a mailbox controller allocates.
+ */
+struct arm_mhuv2_mbox_chan_priv {
+ u32 ch_idx;
+};
+
+#define mhuv2_chan_idx(_chanptr) \
+ (((struct arm_mhuv2_mbox_chan_priv *)(_chanptr)->con_priv)->ch_idx)
+
+/**
+ * Arm MHUv2 mailbox controller data
+ *
+ * @reg: Base address of the register mapping region
+ * @protocol: Transport protocol, derived from device tree
+ * @frame: Frame type, derived from device tree
+ * @irq: Interrupt, only valid for receiver frames
+ * @mbox: Mailbox controller belonging to the MHU frame
+ * @ops: Pointer to transport-protocol specific operations
+ * @dev: Device to which it is attached
+ */
+struct arm_mhuv2 {
+ union {
+ struct mhu2_send_frame_reg __iomem *send;
+ struct mhu2_recv_frame_reg __iomem *recv;
+ } reg;
+ enum mhuv2_transport_protocol protocol;
+ enum mhuv2_frame frame;
+ unsigned int irq;
+ struct mbox_controller mbox;
+ struct mhuv2_ops *ops;
+ struct device *dev;
+};
+
+#define mhu2_from_mbox_ctrl(_mbox) container_of(_mbox, struct arm_mhuv2, mbox)
+#define mhu2_from_mbox_chan(_chan) mhu2_from_mbox_ctrl(_chan->mbox)
+
+/* Macro for reading a bitfield within a physically mapped packed struct */
+#define readl_relaxed_bitfield(_regptr, _field) \
+ ({ \
+ mhuv2_stat_reg_t _regval; \
+ BUILD_BUG_ON(sizeof(*(_regptr)) > sizeof(typeof(_regval))); \
+ _regval = readl_relaxed((_regptr)); \
+ (*(typeof((_regptr)))(&_regval))._field; \
+ })
+
+/* Macro for writing a bitfield within a physically mapped packed struct */
+#define writel_relaxed_bitfield(_value, _regptr, _field) \
+ ({ \
+ mhuv2_stat_reg_t _regval; \
+ BUILD_BUG_ON(sizeof(*_regptr) > sizeof(typeof(_regval))); \
+ _regval = readl_relaxed(_regptr); \
+ (*(typeof(_regptr))(&_regval))._field = _value; \
+ writel_relaxed(_regval, _regptr); \
+ })
+
+static inline int __find_set_bit(uint32_t val)
+{
+ const uint32_t trailing_zeros = __builtin_ctz(val);
+ return trailing_zeros == 32 ? -1 : trailing_zeros;
+}
+
+/**
+ * Get index of a set bit within the combined interrupt status registers
+ *
+ * The function will calculate the index being the offset from the LSB of the
+ * first combined interrupt status register.
+ *
+ */
+static inline int mhuv2_combint_idx(struct arm_mhuv2 *mhuv2)
+{
+ int ch_idx = 0;
+ int set_bit_index, reg_idx;
+
+ for (reg_idx = 0; reg_idx < MHUV2_CMB_INT_ST_REG_CNT; reg_idx++) {
+ mhuv2_stat_reg_t stat_reg;
+
+ stat_reg =
+ readl_relaxed(&mhuv2->reg.recv->CHCOMB_INT_ST[reg_idx]);
+ set_bit_index = __find_set_bit(stat_reg);
+ if (set_bit_index == -1) {
+ ch_idx += MHUV2_STAT_BITS;
+ } else {
+ ch_idx += set_bit_index;
+ break;
+ }
+ }
+ return (ch_idx >= (MHUV2_CMB_INT_ST_REG_CNT * MHUV2_STAT_BITS) ?
+ MHUV2_CH_UNKNOWN :
+ ch_idx);
+}
+
+/* ================ Single word transport protocol operations =============== */
+static inline int mhuv20_get_non_zero_ch_idx(struct arm_mhuv2 *mhuv2)
+{
+ /* Locate a channel window with a non-zero STAT register */
+ int ch_idx;
+ int ch = MHUV2_CH_UNKNOWN;
+
+ for (ch_idx = 0;
+ ch_idx < readl_relaxed_bitfield(&mhuv2->reg.recv->MHU_CFG, NUM_CH);
+ ch_idx++) {
+ if (readl_relaxed(&mhuv2->reg.recv->channel[ch].STAT)) {
+ ch = ch_idx;
+ break;
+ }
+ }
+ return ch;
+}
+
+static inline int mhuv2_get_non_zero_ch_idx(struct arm_mhuv2 *mhuv2)
+{
+ /* Identify index of channel window containing non-zero data */
+ switch (readl_relaxed_bitfield(&mhuv2->reg.recv->AIDR,
+ ARCH_MINOR_REV)) {
+ case 1:
+ return mhuv2_combint_idx(mhuv2);
+ default:
+ return mhuv20_get_non_zero_ch_idx(mhuv2);
+ }
+}
+
+static inline int mhuv2_read_data_single_word(struct arm_mhuv2 *mhuv2,
+ struct mbox_chan *chan,
+ struct arm_mbox_msg *msg)
+{
+ const u32 ch_idx = mhuv2_chan_idx(chan);
+
+ msg->data = kzalloc(MHUV2_STAT_BYTES, GFP_KERNEL);
+ if (!msg->data)
+ return -ENOMEM;
+
+ *(mhuv2_stat_reg_t *)msg->data =
+ readl_relaxed(&mhuv2->reg.recv->channel[ch_idx].STAT);
+ msg->len = MHUV2_STAT_BYTES;
+ return 0;
+}
+
+static inline int mhuv2_clear_data_single_word(struct arm_mhuv2 *mhuv2,
+ struct mbox_chan *chan,
+ struct arm_mbox_msg *msg)
+{
+ const u32 ch_idx = mhuv2_chan_idx(chan);
+
+ writel_relaxed(readl_relaxed(&mhuv2->reg.recv->channel[ch_idx].STAT),
+ &mhuv2->reg.recv->channel[ch_idx].STAT_CLEAR);
+ return 0;
+}
+
+static inline int mhuv2_send_data_single_word(struct arm_mhuv2 *mhuv2,
+ struct mbox_chan *chan,
+ const struct arm_mbox_msg *msg)
+{
+ const u32 ch_idx = mhuv2_chan_idx(chan);
+ int bytes_left = msg->len;
+ char *data = msg->data;
+
+ if (ch_idx >= readl_relaxed_bitfield(&mhuv2->reg.recv->MHU_CFG, NUM_CH))
+ return -ENODEV;
+
+ while (bytes_left > 0) {
+ mhuv2_stat_reg_t word = *(mhuv2_stat_reg_t *)(data);
+
+ if (bytes_left < MHUV2_STAT_BYTES)
+ word &= LSB_MASK(bytes_left * __CHAR_BIT__);
+
+ if (!word) {
+ dev_err(mhuv2->dev,
+ "Data transmitted in single-word mode must be non-zero\n");
+ return -EINVAL;
+ }
+ writel_relaxed(word,
+ &mhuv2->reg.send->channel[ch_idx].STAT_SET);
+ while (readl_relaxed(&mhuv2->reg.send->channel[ch_idx].STAT))
+ continue;
+ bytes_left -= MHUV2_STAT_BYTES;
+ data += MHUV2_STAT_BYTES;
+ }
+
+ return 0;
+}
+
+static inline int mhuv2_last_tx_done_single_word(struct arm_mhuv2 *mhuv2,
+ struct mbox_chan *chan)
+{
+ const u32 ch_idx = mhuv2_chan_idx(chan);
+
+ return readl_relaxed(&mhuv2->reg.send->channel[ch_idx].STAT) == 0;
+}
+
+static inline int mhuv2_setup_single_word(struct arm_mhuv2 *mhuv2)
+{
+ int i;
+ const u32 channel_windows =
+ readl_relaxed_bitfield(mhuv2->frame == RECEIVER_FRAME ?
+ &mhuv2->reg.recv->MHU_CFG :
+ &mhuv2->reg.send->MHU_CFG,
+ NUM_CH);
+
+ mhuv2->mbox.num_chans = channel_windows;
+ mhuv2->mbox.chans =
+ devm_kzalloc(mhuv2->dev,
+ mhuv2->mbox.num_chans * sizeof(struct mbox_chan),
+ GFP_KERNEL);
+
+ for (i = 0; i < mhuv2->mbox.num_chans; i++) {
+ mhuv2->mbox.chans[i].con_priv =
+ devm_kzalloc(mhuv2->dev,
+ sizeof(struct arm_mhuv2_mbox_chan_priv),
+ GFP_KERNEL);
+ mhuv2_chan_idx(&mhuv2->mbox.chans[i]) = i;
+ }
+
+ if (mhuv2->frame == RECEIVER_FRAME) {
+ /* Ensure all status registers are unmasked */
+ for (i = 0; i < channel_windows; i++) {
+ writel_relaxed(0x0,
+ &mhuv2->reg.recv->channel[i].MASK_SET);
+ }
+ }
+
+ return 0;
+}
+
+static inline struct mbox_chan *
+ mhuv2_get_active_mbox_chan_single_word(struct arm_mhuv2 *mhuv2)
+{
+ const u32 ch_idx = mhuv2_get_non_zero_ch_idx(mhuv2);
+
+ if (ch_idx >= mhuv2->mbox.num_chans) {
+ dev_err(mhuv2->dev,
+ "Invalid active channel in single word mode\n");
+ return ERR_PTR(-EINVAL);
+ }
+ return &mhuv2->mbox.chans[ch_idx];
+}
+
+static const struct mhuv2_ops mhuv2_single_word_ops = {
+ .read_data = mhuv2_read_data_single_word,
+ .clear_data = mhuv2_clear_data_single_word,
+ .send_data = mhuv2_send_data_single_word,
+ .setup = mhuv2_setup_single_word,
+ .last_tx_done = mhuv2_last_tx_done_single_word,
+ .get_active_mbox_chan = mhuv2_get_active_mbox_chan_single_word,
+};
+/* ========================================================================== */
+
+/*
+ * MHUv2 receiver interrupt service routine
+ *
+ * This routine will be called whenever a reception interrupt is raised on the
+ * MHU device. Given that an MHU device may manage multiple mailboxes, it is
+ * up to the protocol-specific operations to determine:
+ * - What is the active mailbox channel
+ * - Read the data within the MHU corresponding to the channel
+ * - Clear the data within the MHU corresponding to the channel
+ *
+ * These operations must also ensure to not overwrite any data which may belong
+ * to a different mailbox channel. For instance, if data is received in two
+ * channel windows in single-word mode, the ISR will read and clear the data
+ * from one of these channel windows within a pass. This will result in a status
+ * register being non-zero upon returning from this routine, which in turn
+ * will keep the interrupt asserted for a second round.
+ */
+static irqreturn_t mhuv2_rx_interrupt(int irq, void *data)
+{
+ struct arm_mbox_msg msg;
+ int status;
+ struct arm_mhuv2 *mhuv2 = data;
+ struct mbox_chan *chan = mhuv2->ops->get_active_mbox_chan(mhuv2);
+
+ msg.data = NULL;
+ msg.len = 0;
+
+ status = mhuv2->ops->read_data(mhuv2, chan, &msg);
+ if (status != 0)
+ goto rx_exit;
+
+ if (!chan->cl) {
+ dev_warn(
+ mhuv2->dev,
+ "Warning: Received data on channel not currently attached to a mailbox client\n");
+ } else {
+ mbox_chan_received_data(chan, (void *)&msg);
+ }
+
+ status = mhuv2->ops->clear_data(mhuv2, chan, &msg);
+
+rx_exit:
+ kfree(msg.data);
+
+ return status == 0 ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static bool mhuv2_last_tx_done(struct mbox_chan *chan)
+{
+ struct arm_mhuv2 *mhuv2 = mhu2_from_mbox_chan(chan);
+
+ return mhuv2->ops->last_tx_done(mhuv2, chan);
+}
+
+static int mhuv2_send_data(struct mbox_chan *chan, void *data)
+{
+ struct arm_mhuv2 *mhuv2 = mhu2_from_mbox_chan(chan);
+ struct arm_mbox_msg *msg = data;
+ int ret;
+
+ if (!mhuv2->ops->last_tx_done(mhuv2, chan))
+ return -EBUSY;
+
+ ret = mhuv2->ops->send_data(mhuv2, chan, msg);
+ return ret;
+}
+
+static int mhuv2_startup(struct mbox_chan *chan)
+{
+ struct arm_mhuv2 *mhuv2 = mhu2_from_mbox_ctrl(chan->mbox);
+
+ writel_relaxed(0x1, &mhuv2->reg.send->ACCESS_REQUEST);
+ while (!readl_relaxed(&mhuv2->reg.send->ACCESS_READY))
+ continue;
+
+ return 0;
+}
+
+static void mhuv2_shutdown(struct mbox_chan *chan)
+{
+ struct arm_mhuv2 *mhuv2 = mhu2_from_mbox_ctrl(chan->mbox);
+
+ writel_relaxed(0x0, &mhuv2->reg.send->ACCESS_REQUEST);
+}
+
+static int mhuv2_recv_startup(struct mbox_chan *chan)
+{
+ return 0;
+}
+
+static void mhuv2_recv_shutdown(struct mbox_chan *chan)
+{
+}
+
+static int mhuv2_recv_send_data(struct mbox_chan *chan, void *data)
+{
+ dev_err(chan->mbox->dev,
+ "Trying to transmit on a receiver MHU frame\n");
+ return -EIO;
+}
+
+static bool mhuv2_recv_last_tx_done(struct mbox_chan *chan)
+{
+ dev_err(chan->mbox->dev, "Trying to Tx poll on a receiver MHU frame\n");
+ return true;
+}
+
+static const struct mbox_chan_ops mhuv2_receiver_ops = {
+ .send_data = mhuv2_recv_send_data,
+ .startup = mhuv2_recv_startup,
+ .shutdown = mhuv2_recv_shutdown,
+ .last_tx_done = mhuv2_recv_last_tx_done,
+};
+
+static const struct mbox_chan_ops mhuv2_sender_ops = {
+ .send_data = mhuv2_send_data,
+ .startup = mhuv2_startup,
+ .shutdown = mhuv2_shutdown,
+ .last_tx_done = mhuv2_last_tx_done,
+};
+
+static struct mbox_chan *mhuv2_mbox_of_xlate(struct mbox_controller *ctrl,
+ const struct of_phandle_args *pa)
+{
+ struct mbox_chan *chan;
+
+ if (pa->args_count != 1)
+ return ERR_PTR(-EINVAL);
+
+ if (pa->args[0] >= ctrl->num_chans)
+ return ERR_PTR(-ENOENT);
+
+ chan = &ctrl->chans[pa->args[0]];
+ return chan;
+}
+
+static int mhuv2_probe(struct amba_device *adev, const struct amba_id *id)
+{
+ int err;
+ struct device *dev = &adev->dev;
+ const struct device_node *np = dev->of_node;
+ struct arm_mhuv2 *mhuv2;
+ const char *str;
+
+ /* Allocate memory for device */
+ mhuv2 = devm_kzalloc(dev, sizeof(*mhuv2), GFP_KERNEL);
+ if (!mhuv2)
+ return -ENOMEM;
+
+ mhuv2->dev = dev;
+
+ /* Retrieve MHU frame specifier from device tree node */
+ err = of_property_read_string(np, "mhu-frame", &str);
+ if (err) {
+ dev_err(dev, "Probe failed: MHU frame not specified.");
+ return -ENODEV;
+ } else if (strcmp(str, mhuv2_frame_dt_identifiers[SENDER_FRAME]) == 0) {
+ mhuv2->frame = SENDER_FRAME;
+ } else if (strcmp(str, mhuv2_frame_dt_identifiers[RECEIVER_FRAME]) ==
+ 0) {
+ mhuv2->frame = RECEIVER_FRAME;
+ } else {
+ dev_err(dev,
+ "Probe failed; '%s' is not a valid MHU frame specifier\n",
+ str);
+ return -ENODEV;
+ }
+
+ /* Retrieve transport protocol specifier from device tree node */
+ err = of_property_read_string(np, "mhu-protocol", &str);
+ if (err) {
+ dev_err(dev,
+ "Probe failed: no transport protocol specified\n");
+ return -ENODEV;
+ } else if (strcmp(str, mhuv2_protocol_dt_identifiers[SINGLE_WORD]) ==
+ 0) {
+ mhuv2->protocol = SINGLE_WORD;
+ } else if (strcmp(str, mhuv2_protocol_dt_identifiers[MULTI_WORD]) ==
+ 0) {
+ mhuv2->protocol = MULTI_WORD;
+ } else if (strcmp(str, mhuv2_protocol_dt_identifiers[DOORBELL]) == 0) {
+ mhuv2->protocol = DOORBELL;
+ } else {
+ dev_err(dev,
+ "Probe failed: '%s' is not a valid transport protocol specifier\n",
+ str);
+ return -ENODEV;
+ }
+
+ /* Get MHU type specific properties from device tree */
+ if (mhuv2->frame == RECEIVER_FRAME) {
+ mhuv2->reg.recv = (struct mhu2_recv_frame_reg *)of_iomap(
+ (struct device_node *)np, 0);
+ if (!mhuv2->reg.recv)
+ goto io_fail;
+ mhuv2->irq = adev->irq[0];
+ } else {
+ mhuv2->reg.send = (struct mhu2_send_frame_reg *)of_iomap(
+ (struct device_node *)np, 0);
+ if (!mhuv2->reg.send)
+ goto io_fail;
+ }
+
+ /* Assign transport protocol-specific operations */
+ switch (mhuv2->protocol) {
+ case SINGLE_WORD:
+ mhuv2->ops = &mhuv2_single_word_ops;
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ /* Mailbox controller setup */
+ mhuv2->mbox.dev = dev;
+ mhuv2->mbox.txdone_irq = false;
+ mhuv2->mbox.txdone_poll = true;
+ mhuv2->mbox.txpoll_period = 1;
+ mhuv2->mbox.of_xlate = mhuv2_mbox_of_xlate;
+ mhuv2->mbox.ops = mhuv2->frame == SENDER_FRAME ? &mhuv2_sender_ops :
+ &mhuv2_receiver_ops;
+ /*
+ * Transport protocol specific setup
+ * Setup function _must_ allocate mailbox channels according to the
+ * number of channels provided in the given transport protocol mode.
+ */
+ err = mhuv2->ops->setup(mhuv2);
+ if (err)
+ return err;
+
+ /* Request an interrupt if this is a receiver frame */
+ if (mhuv2->frame == RECEIVER_FRAME) {
+ err = request_irq(mhuv2->irq, mhuv2_rx_interrupt, IRQF_SHARED,
+ "mhuv2_link", mhuv2);
+ if (err) {
+ dev_err(dev, "unable to acquire IRQ %d\n", mhuv2->irq);
+ return err;
+ }
+ /*
+ * For minor version 1 and forward, the combined interrupt of
+ * the receiver frame must be explicitly enabled during startup.
+ */
+ if (readl_relaxed_bitfield(&mhuv2->reg.recv->AIDR,
+ ARCH_MINOR_REV) > 0) {
+ writel_relaxed_bitfield(1, &mhuv2->reg.recv->INT_EN,
+ CHCOMB);
+ }
+ }
+
+ amba_set_drvdata(adev, mhuv2);
+
+ err = mbox_controller_register(&mhuv2->mbox);
+ if (err) {
+ dev_err(dev, "failed to register ARM MHUv2 driver %d\n", err);
+ iounmap(mhuv2->frame == RECEIVER_FRAME ? mhuv2->reg.recv :
+ mhuv2->reg.send);
+ return err;
+ }
+
+ dev_info(dev, "ARM MHUv2 %s frame (%s) Mailbox driver registered\n",
+ mhuv2_frame_dt_identifiers[mhuv2->frame],
+ mhuv2_protocol_dt_identifiers[mhuv2->protocol]);
+
+ return 0;
+
+io_fail:
+ dev_err(dev, "Probe failed: failed to map '%s' frame\n",
+ mhuv2_frame_dt_identifiers[mhuv2->frame]);
+ iounmap(mhuv2->frame == RECEIVER_FRAME ? mhuv2->reg.recv :
+ mhuv2->reg.send);
+ return -ENOMEM;
+}
+
+static int mhuv2_remove(struct amba_device *adev)
+{
+ return 0;
+}
+
+static struct amba_id mhuv2_ids[] = {
+ {
+ .id = 0x4b0d1,
+ .mask = 0xfffff,
+ },
+ {
+ .id = 0xbb0d1,
+ .mask = 0xfffff,
+ },
+ { 0, 0 },
+};
+MODULE_DEVICE_TABLE(amba, mhuv2_ids);
+
+static struct amba_driver arm_mhuv2_driver = {
+ .drv = {
+ .name = "mhuv2",
+ },
+ .id_table = mhuv2_ids,
+ .probe = mhuv2_probe,
+ .remove = mhuv2_remove,
+};
+module_amba_driver(arm_mhuv2_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("ARM MHUv2 Driver");
+MODULE_AUTHOR("Morten Borup Petersen <morten.petersen@arm.com>");
diff --git a/include/linux/mailbox/arm-mbox-message.h b/include/linux/mailbox/arm-mbox-message.h
new file mode 100644
index 000000000000..112b4f927c1a
--- /dev/null
+++ b/include/linux/mailbox/arm-mbox-message.h
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Arm Mailbox Message
+ *
+ * The Arm mailbox message structure is used to pass data- and length
+ * information between a mailbox client and mailbox controller, through the
+ * provided void* of the common mailbox frameworks send- and receive APIs.
+ *
+ * This will be utilized when a mailbox controller is able to transmit
+ * more than a single word within a transmission, allowing the controller
+ * to fully utilize its available resources.
+ * No message protocol is enforced through this structure - a utilizing mailbox
+ * client driver shall implement its own message protocol, which may then be
+ * transmitted through an arm_mbox_msg.
+ *
+ * Given a message protocol of size A and an arm_mbox_msg containing data of
+ * length B, a mailbox channel may callback with B < A. In this case, the
+ * message protocol driver must implement a state machine which allows for
+ * multiple callbacks that provides parts of a full message of size A. This
+ * state machine must account for, that the length of the arm_mbox_msg received
+ * may vary between callbacks based on the underlying hardware as well as the
+ * transmitted data.
+ *
+ * Copyright (C) 2019 Arm Ltd.
+ */
+
+#ifndef _LINUX_ARM_MBOX_MESSAGE_H_
+#define _LINUX_ARM_MBOX_MESSAGE_H_
+
+#include <linux/types.h>
+
+struct arm_mbox_msg {
+ void *data;
+ size_t len;
+};
+
+#endif /* _LINUX_ARM_MBOX_MESSAGE_H_ */
--
2.17.1
^ permalink raw reply related
* [PATCH 1/4] mailbox: arm_mhuv2: add device tree binding documentation
From: Tushar Khandelwal @ 2019-07-17 19:26 UTC (permalink / raw)
To: linux-kernel
Cc: tushar.2nov, morten_bp, jassisinghbrar, nd, Morten Borup Petersen,
Tushar Khandelwal, robh+dt, mark.rutland, devicetree
In-Reply-To: <20190717192616.1731-1-tushar.khandelwal@arm.com>
From: Morten Borup Petersen <morten.petersen@arm.com>
This patch adds device tree binding for Message Handling Unit
controller version 2 from Arm.
Signed-off-by: Morten Borup Petersen <morten.petersen@arm.com>
Signed-off-by: Tushar Khandelwal <tushar.khandelwal@arm.com>
Cc: robh+dt@kernel.org
Cc: mark.rutland@arm.com
Cc: devicetree@vger.kernel.org
Cc: jassisinghbrar@gmail.com
---
.../devicetree/bindings/mailbox/arm,mhuv2.txt | 108 ++++++++++++++++++
1 file changed, 108 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
new file mode 100644
index 000000000000..3a05593414bc
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
@@ -0,0 +1,108 @@
+Arm MHUv2 Mailbox Driver
+========================
+
+The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller that has
+between 1 and 124 channel windows to provide unidirectional communication with
+remote processor(s).
+
+Given the unidirectional nature of the device, an MHUv2 mailbox may only be
+written to or read from. If a pair of MHU devices is implemented between two
+processing elements to provide bidirectional communication, these must be
+specified as two separate mailboxes.
+
+A device tree node for an Arm MHUv2 device must specify either a receiver frame
+or a sender frame, indicating which end of the unidirectional MHU device which
+the device node entry describes.
+
+An MHU device must be specified with a transport protocol. The transport
+protocol of an MHU device determines the method of data transmission as well as
+the number of provided mailboxes.
+Following are the possible transport protocol types:
+- Single-word: An MHU device implements as many mailboxes as it
+ provides channel windows. Data is transmitted through
+ the MHU registers.
+- Multi-word: An MHU device implements a single mailbox. All channel windows
+ will be used during transmission. Data is transmitted through
+ the MHU registers.
+- Doorbell: An MHU device implements as many mailboxes as there are flag
+ bits available in its channel windows. Optionally, data may
+ be transmitted through a shared memory region, wherein the MHU
+ is used strictly as an interrupt generation mechanism.
+
+Mailbox Device Node:
+====================
+
+Required properties:
+--------------------
+- compatible: Shall be "arm,mhuv2" & "arm,primecell"
+- reg: Contains the mailbox register address range (base
+ address and length)
+- #mbox-cells Shall be 1 - the index of the channel needed.
+- mhu-frame Frame type of the device.
+ Shall be either "sender" or "receiver"
+- mhu-protocol Transport protocol of the device. Shall be one of the
+ following: "single-word", "multi-word", "doorbell"
+
+Required properties (receiver frame):
+-------------------------------------
+- interrupts: Contains the interrupt information corresponding to the
+ combined interrupt of the receiver frame
+
+Example:
+--------
+
+ mbox_mw_tx: mhu@10000000 {
+ compatible = "arm,mhuv2","arm,primecell";
+ reg = <0x10000000 0x1000>;
+ clocks = <&refclk100mhz>;
+ clock-names = "apb_pclk";
+ #mbox-cells = <1>;
+ mhu-protocol = "multi-word";
+ mhu-frame = "sender";
+ };
+
+ mbox_sw_tx: mhu@10000000 {
+ compatible = "arm,mhuv2","arm,primecell";
+ reg = <0x11000000 0x1000>;
+ clocks = <&refclk100mhz>;
+ clock-names = "apb_pclk";
+ #mbox-cells = <1>;
+ mhu-protocol = "single-word";
+ mhu-frame = "sender";
+ };
+
+ mbox_db_rx: mhu@10000000 {
+ compatible = "arm,mhuv2","arm,primecell";
+ reg = <0x12000000 0x1000>;
+ clocks = <&refclk100mhz>;
+ clock-names = "apb_pclk";
+ #mbox-cells = <1>;
+ interrupts = <0 45 4>;
+ interrupt-names = "mhu_rx";
+ mhu-protocol = "doorbell";
+ mhu-frame = "receiver";
+ };
+
+ mhu_client: scb@2e000000 {
+ compatible = "fujitsu,mb86s70-scb-1.0";
+ reg = <0 0x2e000000 0x4000>;
+ mboxes =
+ // For multi-word frames, client may only instantiate a single
+ // mailbox for a mailbox controller
+ <&mbox_mw_tx 0>,
+
+ // For single-word frames, client may instantiate as many
+ // mailboxes as there are channel windows in the MHU
+ <&mbox_sw_tx 0>,
+ <&mbox_sw_tx 1>,
+ <&mbox_sw_tx 2>,
+ <&mbox_sw_tx 3>,
+
+ // For doorbell frames, client may instantiate as many mailboxes
+ // as there are bits available in the combined number of channel
+ // windows ((channel windows * 32) mailboxes)
+ <mbox_db_rx 0>,
+ <mbox_db_rx 1>,
+ ...
+ <mbox_db_rx 17>;
+ };
\ No newline at end of file
--
2.17.1
^ permalink raw reply related
* [PATCHv3] MIPS: JZ4780: DTS: Add I2C nodes
From: Alexandre GRIVEAUX @ 2019-07-17 19:19 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Alexandre GRIVEAUX
Cc: linux-mips, linux-kernel, devicetree
Add the devicetree nodes for the I2C core of the JZ4780 SoC, disabled
by default.
---
arch/mips/boot/dts/ingenic/jz4780.dtsi | 86 ++++++++++++++++++++++++++
1 file changed, 86 insertions(+)
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index b03cdec56de9..a76ecd69bfd0 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -239,6 +239,92 @@
status = "disabled";
};
+ i2c0: i2c@10050000 {
+ compatible = "ingenic,jz4780-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ reg = <0x10050000 0x1000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <60>;
+
+ clocks = <&cgu JZ4780_CLK_SMB0>;
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_i2c0_data>;
+
+ status = "disabled";
+ };
+
+ i2c1: i2c@10051000 {
+ compatible = "ingenic,jz4780-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x10051000 0x1000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <59>;
+
+ clocks = <&cgu JZ4780_CLK_SMB1>;
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_i2c1_data>;
+
+ status = "disabled";
+ };
+
+ i2c2: i2c@10052000 {
+ compatible = "ingenic,jz4780-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x10052000 0x1000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <58>;
+
+ clocks = <&cgu JZ4780_CLK_SMB2>;
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_i2c2_data>;
+
+ status = "disabled";
+ };
+
+ i2c3: i2c@10053000 {
+ compatible = "ingenic,jz4780-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x10053000 0x1000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <57>;
+
+ clocks = <&cgu JZ4780_CLK_SMB3>;
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_i2c3_data>;
+
+ status = "disabled";
+ };
+
+ i2c4: i2c@10054000 {
+ compatible = "ingenic,jz4780-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x10054000 0x1000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <56>;
+
+ clocks = <&cgu JZ4780_CLK_SMB4>;
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_i2c4_data>;
+
+ status = "disabled";
+ };
+
watchdog: watchdog@10002000 {
compatible = "ingenic,jz4780-watchdog";
reg = <0x10002000 0x10>;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 4/6] irqchip/irq-pruss-intc: Add helper functions to configure internal mapping
From: Suman Anna @ 2019-07-17 19:04 UTC (permalink / raw)
To: David Lechner, Marc Zyngier, Rob Herring, Thomas Gleixner,
Jason Cooper
Cc: Tony Lindgren, Andrew F. Davis, Roger Quadros, Lokesh Vutla,
Grygorii Strashko, Sekhar Nori, Murali Karicheri, devicetree,
linux-omap, linux-arm-kernel, linux-kernel
In-Reply-To: <22825f06-d968-03a7-585b-8cbf4123915c@lechnology.com>
On 7/17/19 12:57 PM, David Lechner wrote:
> On 7/16/19 6:29 PM, Suman Anna wrote:
>> Hi David,
>>
>> On 7/10/19 10:10 PM, David Lechner wrote:
>>> On 7/7/19 10:52 PM, Suman Anna wrote:
>>>> The PRUSS INTC receives a number of system input interrupt source
>>>> events
>>>> and supports individual control configuration and hardware
>>>> prioritization.
>>>> These input events can be mapped to some output host interrupts
>>>> through 2
>>>> levels of many-to-one mapping i.e. events to channel mapping and
>>>> channels
>>>> to host interrupts.
>>>>
>>>> This mapping information is provided through the PRU firmware that is
>>>> loaded onto a PRU core/s or through the device tree node of the PRU
>>>
>>
>> Thanks for the thorough review and alternate solutions/suggestions.
>>
>>> What will the device tree bindings for this look like?
>>
>> They would be as in the below patch you already figured.
>
> Ah, makes sense now: the mapping is defined in the remoteproc node
> rather than in the interrupt controller node.
Actually in the PRU consumer/application node, but the client driver
need not deal with invoking any special API. The functions are called
transparently by the PRU remoteproc driver when the PRU client driver
acquires a PRU. The 4th cell was used to identify the PRU from the list
of prus in the client node.
regards
Suman
>
>>
>>>
>>> Looking back at Rob's comment on the initial series [1], I still think
>>> that increasing the #interrupt-cells sounds like a reasonable solution.
>>>
>>> [1]: https://patchwork.kernel.org/patch/10697705/#22375155
>>
>> So, there are couple of reasons why I did not use an extended
>> #interrupt-cells:
>>
>> 1. There is only one irq descriptor associated with each event, and the
>> usage of events is typically per application. And the descriptor mapping
>> is done once. We can have two different applications use the same event
>> with different mappings. So we want this programming done at
>> application's usage of PRU (so done when a consumer driver acquires a
>> PRU processor(s) which are treated as an exclusive resource). All the
>> different application properties that you saw in [1] are configured at
>> the time of acquiring a PRU and reset when they release a PRU.
>>
>> 2. The configuration is performed by Linux for all host interrupts and
>> channels, and this was primarily done to save the very limited IRAM
>> space for those needed by the PRUs. From firmware's point of view, this
>> was offloaded to the ARM OS driver/infrastructure, but in general it is
>> a design by contract between a PRU client driver and its firmware. Also,
>> the DT binding semantics using interrupts property and request_irq()
>> typically limits these to interrupts only being requested by MPU, and so
>> will leave out those needed by PRUs.
>>
>
> Hmm... case 1. is a tricky one indeed. If there are going to be times where
> an event requires multiple mappings, I agree that this doesn't seem to fit
> into any existing device tree bindings.
>
>
^ permalink raw reply
* Re: [PATCH 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts
From: Suman Anna @ 2019-07-17 18:56 UTC (permalink / raw)
To: David Lechner, Marc Zyngier, Rob Herring, Thomas Gleixner,
Jason Cooper
Cc: Tony Lindgren, Andrew F. Davis, Roger Quadros, Lokesh Vutla,
Grygorii Strashko, Sekhar Nori, Murali Karicheri, devicetree,
linux-omap, linux-arm-kernel, linux-kernel
In-Reply-To: <5241ecb9-4ab2-b01c-367c-38c554c056c4@lechnology.com>
On 7/17/19 12:21 PM, David Lechner wrote:
> On 7/16/19 12:21 PM, Suman Anna wrote:
>>>> +static int pruss_intc_probe(struct platform_device *pdev)
>>>> +{
>>>> + static const char * const irq_names[] = {
>>>> + "host0", "host1", "host2", "host3",
>>>> + "host4", "host5", "host6", "host7", };
>>>> + struct device *dev = &pdev->dev;
>>>> + struct pruss_intc *intc;
>>>> + struct resource *res;
>>>> + struct irq_chip *irqchip;
>>>> + int i, irq;
>>>> +
>>>> + intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);
>>>> + if (!intc)
>>>> + return -ENOMEM;
>>>> + platform_set_drvdata(pdev, intc);
>>>> +
>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> + intc->base = devm_ioremap_resource(dev, res);
>>>> + if (IS_ERR(intc->base)) {
>>>> + dev_err(dev, "failed to parse and map intc memory
>>>> resource\n");
>>>> + return PTR_ERR(intc->base);
>>>> + }
>>>> +
>>>> + dev_dbg(dev, "intc memory: pa %pa size 0x%zx va %pK\n",
>>>> &res->start,
>>>> + (size_t)resource_size(res), intc->base);
>>>> +
>>>> + mutex_init(&intc->lock);
>>>> +
>>>> + pruss_intc_init(intc);
>>>> +
>>>> + irqchip = devm_kzalloc(dev, sizeof(*irqchip), GFP_KERNEL);
>>>> + if (!irqchip)
>>>> + return -ENOMEM;
>>>> +
>>>> + irqchip->irq_ack = pruss_intc_irq_ack;
>>>> + irqchip->irq_mask = pruss_intc_irq_mask;
>>>> + irqchip->irq_unmask = pruss_intc_irq_unmask;
>>>> + irqchip->irq_retrigger = pruss_intc_irq_retrigger;
>>>> + irqchip->irq_request_resources = pruss_intc_irq_reqres;
>>>> + irqchip->irq_release_resources = pruss_intc_irq_relres;
>>>> + irqchip->name = dev_name(dev);
>>>
>>> Should we also set `irqchip->parent_device = dev;` here?
>>>
>>> I tried it and had to add pm runtime stuff as well, otherwise
>>> requesting irqs would fail.
>>
>> I haven't seen any during my local testing. What sort of failure are you
>> seeing?
>>
>> The clocking for the overall PRUSS module will be handled in either the
>> ti-sysc driver for OMAP SoCs or in the pruss platform driver.
>>
> I was getting -EACCESS bubbling up from rpm_resume() in drivers/base/
> power/runtime.c. It was probably a mix of how I set up the device tree
> and the dummy PRUSS bus driver I made.
>
> I'm sure it will be fine with a proper PRUSS platform driver.
Yeah, ok. You just need to have the power-domains property added in the
pruss node, and the pm_runtime calls in the pruss platform driver which
are missing in Roger's series.
I have the following line on my da850 pruss node.
power-domains = <&psc0 13>;
regards
Suman
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 18:54 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <78fd19b9-b652-8ac3-1f57-3b4adadee03f@nvidia.com>
On 7/17/19 11:51 AM, Sowjanya Komatineni wrote:
>
> On 7/17/19 11:32 AM, Dmitry Osipenko wrote:
>> 17.07.2019 20:29, Sowjanya Komatineni пишет:
>>> On 7/17/19 8:17 AM, Dmitry Osipenko wrote:
>>>> 17.07.2019 9:36, Sowjanya Komatineni пишет:
>>>>> On 7/16/19 11:33 PM, Dmitry Osipenko wrote:
>>>>>> В Tue, 16 Jul 2019 22:55:52 -0700
>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>
>>>>>>> On 7/16/19 10:42 PM, Dmitry Osipenko wrote:
>>>>>>>> В Tue, 16 Jul 2019 22:25:25 -0700
>>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>>>> On 7/16/19 9:11 PM, Dmitry Osipenko wrote:
>>>>>>>>>> В Tue, 16 Jul 2019 19:35:49 -0700
>>>>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>>>>>> On 7/16/19 7:18 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>> On 7/16/19 3:06 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>> On 7/16/19 3:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>> 17.07.2019 0:35, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>> On 7/16/19 2:21 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>> 17.07.2019 0:12, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>> On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>> 16.07.2019 22:26, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>>>>>>>>>>>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph
>>>>>>>>>>>>>>>>>>>>>>>>>> Lo wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> The other thing that also need attention is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> that T124 CPUFreq
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> implicitly relies on DFLL driver to be probed
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> first, which is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> icky.
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Should I add check for successful dfll clk
>>>>>>>>>>>>>>>>>>>>>>>>>>>> register explicitly in
>>>>>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk
>>>>>>>>>>>>>>>>>>>>>>>>>>>> registers?
>>>>>>>>>>>>>>>>>>>>>>>>> Probably you should use the "device links". See
>>>>>>>>>>>>>>>>>>>>>>>>> [1][2] for the
>>>>>>>>>>>>>>>>>>>>>>>>> example.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>>>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> [2]
>>>>>>>>>>>>>>>>>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if
>>>>>>>>>>>>>>>>>>>>>>>>> device_link_add() fails.
>>>>>>>>>>>>>>>>>>>>>>>>> And
>>>>>>>>>>>>>>>>>>>>>>>>> use of_find_device_by_node() to get the DFLL's
>>>>>>>>>>>>>>>>>>>>>>>>> device, see [3].
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> [3]
>>>>>>>>>>>>>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Will go thru and add...
>>>>>>>>>>>>>>>>>>>>>> Looks like I initially confused this case with
>>>>>>>>>>>>>>>>>>>>>> getting
>>>>>>>>>>>>>>>>>>>>>> orphaned clock.
>>>>>>>>>>>>>>>>>>>>>> I'm now seeing that the DFLL driver registers the
>>>>>>>>>>>>>>>>>>>>>> clock and then
>>>>>>>>>>>>>>>>>>>>>> clk_get(dfll) should be returning EPROBE_DEFER until
>>>>>>>>>>>>>>>>>>>>>> DFLL driver is
>>>>>>>>>>>>>>>>>>>>>> probed, hence everything should be fine as-is and
>>>>>>>>>>>>>>>>>>>>>> there is no real
>>>>>>>>>>>>>>>>>>>>>> need
>>>>>>>>>>>>>>>>>>>>>> for the 'device link'. Sorry for the confusion!
>>>>>>>>>>>>>>>>>>>>>>>>>>> Sorry, I didn't follow the mail thread. Just
>>>>>>>>>>>>>>>>>>>>>>>>>>> regarding the DFLL
>>>>>>>>>>>>>>>>>>>>>>>>>>> part.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> As you know it, the DFLL clock is one of the
>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU
>>>>>>>>>>>>>>>>>>>>>>>>>>> clock sources and
>>>>>>>>>>>>>>>>>>>>>>>>>>> integrated with DVFS control logic with the
>>>>>>>>>>>>>>>>>>>>>>>>>>> regulator. We will not
>>>>>>>>>>>>>>>>>>>>>>>>>>> switch
>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU to other clock sources once we switched to
>>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL. Because the
>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU has
>>>>>>>>>>>>>>>>>>>>>>>>>>> been regulated by the DFLL HW with the DVFS
>>>>>>>>>>>>>>>>>>>>>>>>>>> table
>>>>>>>>>>>>>>>>>>>>>>>>>>> (CVB or OPP
>>>>>>>>>>>>>>>>>>>>>>>>>>> table
>>>>>>>>>>>>>>>>>>>>>>>>>>> you see
>>>>>>>>>>>>>>>>>>>>>>>>>>> in the driver.). We shouldn't reparent it to
>>>>>>>>>>>>>>>>>>>>>>>>>>> other sources with
>>>>>>>>>>>>>>>>>>>>>>>>>>> unknew
>>>>>>>>>>>>>>>>>>>>>>>>>>> freq/volt pair. That's not guaranteed to
>>>>>>>>>>>>>>>>>>>>>>>>>>> work. We
>>>>>>>>>>>>>>>>>>>>>>>>>>> allow switching to
>>>>>>>>>>>>>>>>>>>>>>>>>>> open-loop mode but different sources.
>>>>>>>>>>>>>>>>>>>>>>>>> Okay, then the CPUFreq driver will have to
>>>>>>>>>>>>>>>>>>>>>>>>> enforce
>>>>>>>>>>>>>>>>>>>>>>>>> DFLL freq to
>>>>>>>>>>>>>>>>>>>>>>>>> PLLP's
>>>>>>>>>>>>>>>>>>>>>>>>> rate before switching to PLLP in order to have a
>>>>>>>>>>>>>>>>>>>>>>>>> proper CPU voltage.
>>>>>>>>>>>>>>>>>>>>>>>> PLLP freq is safe to work for any CPU voltage.
>>>>>>>>>>>>>>>>>>>>>>>> So no
>>>>>>>>>>>>>>>>>>>>>>>> need to enforce
>>>>>>>>>>>>>>>>>>>>>>>> DFLL freq to PLLP rate before changing CCLK_G
>>>>>>>>>>>>>>>>>>>>>>>> source
>>>>>>>>>>>>>>>>>>>>>>>> to PLLP during
>>>>>>>>>>>>>>>>>>>>>>>> suspend
>>>>>>>>>>>>>>>>>>>>>>> Sorry, please ignore my above comment. During
>>>>>>>>>>>>>>>>>>>>>>> suspend, need to change
>>>>>>>>>>>>>>>>>>>>>>> CCLK_G source to PLLP when dfll is in closed loop
>>>>>>>>>>>>>>>>>>>>>>> mode first and
>>>>>>>>>>>>>>>>>>>>>>> then
>>>>>>>>>>>>>>>>>>>>>>> dfll need to be set to open loop.
>>>>>>>>>>>>>>>>>>>>>> Okay.
>>>>>>>>>>>>>>>>>>>>>>>>>>> And I don't exactly understand why we need to
>>>>>>>>>>>>>>>>>>>>>>>>>>> switch to PLLP in
>>>>>>>>>>>>>>>>>>>>>>>>>>> CPU
>>>>>>>>>>>>>>>>>>>>>>>>>>> idle
>>>>>>>>>>>>>>>>>>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the
>>>>>>>>>>>>>>>>>>>>>>>>>>> time.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> In SC7 entry, the dfll suspend function
>>>>>>>>>>>>>>>>>>>>>>>>>>> moves it
>>>>>>>>>>>>>>>>>>>>>>>>>>> the open-loop
>>>>>>>>>>>>>>>>>>>>>>>>>>> mode. That's
>>>>>>>>>>>>>>>>>>>>>>>>>>> all. The sc7-entryfirmware will handle the rest
>>>>>>>>>>>>>>>>>>>>>>>>>>> of the sequence to
>>>>>>>>>>>>>>>>>>>>>>>>>>> turn off
>>>>>>>>>>>>>>>>>>>>>>>>>>> the CPU power.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> In SC7 resume, the warmboot code will handle
>>>>>>>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>>>>>> sequence to
>>>>>>>>>>>>>>>>>>>>>>>>>>> turn on
>>>>>>>>>>>>>>>>>>>>>>>>>>> regulator and power up the CPU cluster. And
>>>>>>>>>>>>>>>>>>>>>>>>>>> leave
>>>>>>>>>>>>>>>>>>>>>>>>>>> it on PLL_P.
>>>>>>>>>>>>>>>>>>>>>>>>>>> After
>>>>>>>>>>>>>>>>>>>>>>>>>>> resuming to the kernel, we re-init DFLL,
>>>>>>>>>>>>>>>>>>>>>>>>>>> restore
>>>>>>>>>>>>>>>>>>>>>>>>>>> the CPU clock
>>>>>>>>>>>>>>>>>>>>>>>>>>> policy (CPU
>>>>>>>>>>>>>>>>>>>>>>>>>>> runs on DFLL open-loop mode) and then moving to
>>>>>>>>>>>>>>>>>>>>>>>>>>> close-loop mode.
>>>>>>>>>>>>>>>>>>>>>>>>> The DFLL is re-inited after switching CCLK to
>>>>>>>>>>>>>>>>>>>>>>>>> DFLL
>>>>>>>>>>>>>>>>>>>>>>>>> parent during of
>>>>>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>>>> early clocks-state restoring by CaR driver. Hence
>>>>>>>>>>>>>>>>>>>>>>>>> instead of having
>>>>>>>>>>>>>>>>>>>>>>>>> odd
>>>>>>>>>>>>>>>>>>>>>>>>> hacks in the CaR driver, it is much nicer to
>>>>>>>>>>>>>>>>>>>>>>>>> have a
>>>>>>>>>>>>>>>>>>>>>>>>> proper suspend-resume sequencing of the device
>>>>>>>>>>>>>>>>>>>>>>>>> drivers. In this case
>>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq
>>>>>>>>>>>>>>>>>>>>>>>>> driver is the driver that enables DFLL and
>>>>>>>>>>>>>>>>>>>>>>>>> switches
>>>>>>>>>>>>>>>>>>>>>>>>> CPU to that
>>>>>>>>>>>>>>>>>>>>>>>>> clock
>>>>>>>>>>>>>>>>>>>>>>>>> source, which means that this driver is also
>>>>>>>>>>>>>>>>>>>>>>>>> should
>>>>>>>>>>>>>>>>>>>>>>>>> be responsible for
>>>>>>>>>>>>>>>>>>>>>>>>> management of the DFLL's state during of
>>>>>>>>>>>>>>>>>>>>>>>>> suspend/resume process. If
>>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver disables DFLL during suspend and
>>>>>>>>>>>>>>>>>>>>>>>>> re-enables it
>>>>>>>>>>>>>>>>>>>>>>>>> during
>>>>>>>>>>>>>>>>>>>>>>>>> resume, then looks like the CaR driver hacks
>>>>>>>>>>>>>>>>>>>>>>>>> around
>>>>>>>>>>>>>>>>>>>>>>>>> DFLL are not
>>>>>>>>>>>>>>>>>>>>>>>>> needed.
>>>>>>>>>>>>>>>>>>>>>>>>>>> The DFLL part looks good to me. BTW, change the
>>>>>>>>>>>>>>>>>>>>>>>>>>> patch subject to
>>>>>>>>>>>>>>>>>>>>>>>>>>> "Add
>>>>>>>>>>>>>>>>>>>>>>>>>>> suspend-resume support" seems more
>>>>>>>>>>>>>>>>>>>>>>>>>>> appropriate to
>>>>>>>>>>>>>>>>>>>>>>>>>>> me.
>>>>>>>>>>>>>>>>>>>>>>>>>> To clarify this, the sequences for DFLL use
>>>>>>>>>>>>>>>>>>>>>>>>>> are as
>>>>>>>>>>>>>>>>>>>>>>>>>> follows (assuming
>>>>>>>>>>>>>>>>>>>>>>>>>> all
>>>>>>>>>>>>>>>>>>>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> Switch to DFLL:
>>>>>>>>>>>>>>>>>>>>>>>>>> 0) Save current parent and frequency
>>>>>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>>>>>>> 2) Enable DFLL
>>>>>>>>>>>>>>>>>>>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>>>>>>>>>>>>>>>>>>>> For OVR regulator:
>>>>>>>>>>>>>>>>>>>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>>>>>>>>>>>>>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>>>>>>>>>>>>>>>>>>>> For I2C regulator:
>>>>>>>>>>>>>>>>>>>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>>>>>>>>>>>>>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> Switch away from DFLL:
>>>>>>>>>>>>>>>>>>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU
>>>>>>>>>>>>>>>>>>>>>>>>>> frequency is ok for
>>>>>>>>>>>>>>>>>>>>>>>>>> any
>>>>>>>>>>>>>>>>>>>>>>>>>> vdd_cpu voltage
>>>>>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>>>> I see during switch away from DFLL (suspend),
>>>>>>>>>>>>>>>>>>>>>>> cclk_g
>>>>>>>>>>>>>>>>>>>>>>> parent is not
>>>>>>>>>>>>>>>>>>>>>>> changed to PLLP before changing dfll to open loop
>>>>>>>>>>>>>>>>>>>>>>> mode.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Will add this ...
>>>>>>>>>>>>>>>>>>>>>> The CPUFreq driver switches parent to PLLP during
>>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>> probe, similar
>>>>>>>>>>>>>>>>>>>>>> should be done on suspend.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I'm also wondering if it's always safe to switch to
>>>>>>>>>>>>>>>>>>>>>> PLLP in the probe.
>>>>>>>>>>>>>>>>>>>>>> If CPU is running on a lower freq than PLLP, then
>>>>>>>>>>>>>>>>>>>>>> some
>>>>>>>>>>>>>>>>>>>>>> other more
>>>>>>>>>>>>>>>>>>>>>> appropriate intermediate parent should be selected.
>>>>>>>>>>>>>>>>>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always
>>>>>>>>>>>>>>>>>>>>> runs at higher
>>>>>>>>>>>>>>>>>>>>> rate
>>>>>>>>>>>>>>>>>>>>> so switching to PLL_P during CPUFreq probe prior to
>>>>>>>>>>>>>>>>>>>>> dfll clock enable
>>>>>>>>>>>>>>>>>>>>> should be safe.
>>>>>>>>>>>>>>>>>>>> AFAIK, PLLX could run at ~200MHz. There is also a
>>>>>>>>>>>>>>>>>>>> divided output of
>>>>>>>>>>>>>>>>>>>> PLLP
>>>>>>>>>>>>>>>>>>>> which CCLKG supports, the PLLP_OUT4.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Probably, realistically, CPU is always running off a
>>>>>>>>>>>>>>>>>>>> fast PLLX during
>>>>>>>>>>>>>>>>>>>> boot, but I'm wondering what may happen on KEXEC. I
>>>>>>>>>>>>>>>>>>>> guess ideally CPUFreq driver should also have a
>>>>>>>>>>>>>>>>>>>> 'shutdown' callback to teardown DFLL
>>>>>>>>>>>>>>>>>>>> on a reboot, but likely that there are other
>>>>>>>>>>>>>>>>>>>> clock-related problems as
>>>>>>>>>>>>>>>>>>>> well that may break KEXEC and thus it is not very
>>>>>>>>>>>>>>>>>>>> important at the
>>>>>>>>>>>>>>>>>>>> moment.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> [snip]
>>>>>>>>>>>>>>>>>>> During bootup CPUG sources from PLL_X. By PLL_P source
>>>>>>>>>>>>>>>>>>> above I meant
>>>>>>>>>>>>>>>>>>> PLL_P_OUT4.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> As per clock policies, PLL_X is always used for high
>>>>>>>>>>>>>>>>>>> freq
>>>>>>>>>>>>>>>>>>> like
>>>>>>>>>>>>>>>>>>>> 800Mhz
>>>>>>>>>>>>>>>>>>> and for low frequency it will be sourced from PLLP.
>>>>>>>>>>>>>>>>>> Alright, then please don't forget to pre-initialize
>>>>>>>>>>>>>>>>>> PLLP_OUT4 rate to a
>>>>>>>>>>>>>>>>>> reasonable value using tegra_clk_init_table or
>>>>>>>>>>>>>>>>>> assigned-clocks.
>>>>>>>>>>>>>>>>> PLLP_OUT4 rate update is not needed as it is safe to
>>>>>>>>>>>>>>>>> run at
>>>>>>>>>>>>>>>>> 408Mhz because it is below fmax @ Vmin
>>>>>>>>>>>>>>>> So even 204MHz CVB entries are having the same voltage as
>>>>>>>>>>>>>>>> 408MHz, correct? It's not instantly obvious to me from the
>>>>>>>>>>>>>>>> DFLL driver's code where the fmax @ Vmin is defined, I see
>>>>>>>>>>>>>>>> that there is the min_millivolts
>>>>>>>>>>>>>>>> and frequency entries starting from 204MHZ defined
>>>>>>>>>>>>>>>> per-table.
>>>>>>>>>>>>>>> Yes at Vmin CPU Fmax is ~800Mhz. So anything below that
>>>>>>>>>>>>>>> will
>>>>>>>>>>>>>>> work at Vmin voltage and PLLP max is 408Mhz.
>>>>>>>>>>>>>> Thank you for the clarification. It would be good to have
>>>>>>>>>>>>>> that
>>>>>>>>>>>>>> commented
>>>>>>>>>>>>>> in the code as well.
>>>>>>>>>>>>> OK, Will add...
>>>>>>>>>>>> Regarding, adding suspend/resume to CPUFreq, CPUFreq suspend
>>>>>>>>>>>> happens very early even before disabling non-boot CPUs and
>>>>>>>>>>>> also
>>>>>>>>>>>> need to export clock driver APIs to CPUFreq.
>>>>>>>>>>>>
>>>>>>>>>>>> Was thinking of below way of implementing this...
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Clock DFLL driver Suspend:
>>>>>>>>>>>>
>>>>>>>>>>>> - Save CPU clock policy registers, and Perform
>>>>>>>>>>>> dfll
>>>>>>>>>>>> suspend which sets in open loop mode
>>>>>>>>>>>>
>>>>>>>>>>>> CPU Freq driver Suspend: does nothing
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Clock DFLL driver Resume:
>>>>>>>>>>>>
>>>>>>>>>>>> - Re-init DFLL, Set in Open-Loop mode, restore
>>>>>>>>>>>> CPU
>>>>>>>>>>>> Clock policy registers which actually sets source to DFLL
>>>>>>>>>>>> along
>>>>>>>>>>>> with other CPU Policy register restore.
>>>>>>>>>>>>
>>>>>>>>>>>> CPU Freq driver Resume:
>>>>>>>>>>>>
>>>>>>>>>>>> - do clk_prepare_enable which acutally sets
>>>>>>>>>>>> DFLL in
>>>>>>>>>>>> Closed loop mode
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Adding one more note: Switching CPU Clock to PLLP is not
>>>>>>>>>>>> needed
>>>>>>>>>>>> as CPU CLock can be from dfll in open-loop mode as DFLL is not
>>>>>>>>>>>> disabled anywhere throught the suspend/resume path and SC7
>>>>>>>>>>>> entry
>>>>>>>>>>>> FW and Warm boot code will switch CPU source to PLLP.
>>>>>>>>>> Since CPU resumes on PLLP, it will be cleaner to suspend it on
>>>>>>>>>> PLLP as well. And besides, seems that currently disabling DFLL
>>>>>>>>>> clock will disable DFLL completely and then you'd want to
>>>>>>>>>> re-init
>>>>>>>>>> the DFLL on resume any ways. So better to just disable DFLL
>>>>>>>>>> completely on suspend, which should happen on clk_disable(dfll).
>>>>>>>>> Will switch to PLLP during CPUFreq suspend. With decision of
>>>>>>>>> using
>>>>>>>>> clk_disable during suspend, its mandatory to switch to PLLP as
>>>>>>>>> DFLL
>>>>>>>>> is completely disabled.
>>>>>>>>>
>>>>>>>>> My earlier concern was on restoring CPU policy as we can't do
>>>>>>>>> that
>>>>>>>>> from CPUFreq driver and need export from clock driver.
>>>>>>>>>
>>>>>>>>> Clear now and will do CPU clock policy restore in after dfll
>>>>>>>>> re-init.
>>>>>>>> Why the policy can't be saved/restored by the CaR driver as a
>>>>>>>> context of any other clock?
>>>>>>> restoring cpu clock policy involves programming source and
>>>>>>> super_cclkg_divider.
>>>>>>>
>>>>>>> cclk_g is registered as clk_super_mux and it doesn't use
>>>>>>> frac_div ops
>>>>>>> to do save/restore its divider.
>>>>>> That can be changed of course and I guess it also could be as
>>>>>> simple as
>>>>>> saving and restoring of two raw u32 values of the policy/divider
>>>>>> registers.
>>>>>>
>>>>>>> Also, during clock context we cant restore cclk_g as cclk_g source
>>>>>>> will be dfll and dfll will not be resumed/re-initialized by the
>>>>>>> time
>>>>>>> clk_super_mux save/restore happens.
>>>>>>>
>>>>>>> we can't use save/restore context for dfll clk_ops because
>>>>>>> dfllCPU_out parent to CCLK_G is first in the clock tree and
>>>>>>> dfll_ref
>>>>>>> and dfll_soc peripheral clocks are not restored by the time dfll
>>>>>>> restore happens. Also dfll peripheral clock enables need to be
>>>>>>> restored before dfll restore happens which involves programming
>>>>>>> dfll
>>>>>>> controller for re-initialization.
>>>>>>>
>>>>>>> So dfll resume/re-init is done in clk-tegra210 at end of all clocks
>>>>>>> restore in V5 series but instead of in clk-tegra210 driver I moved
>>>>>>> now to dfll-fcpu driver pm_ops as all dfll dependencies will be
>>>>>>> restored thru clk_restore_context by then. This will be in V6.
>>>>>> Since DFLL is now guaranteed to be disabled across CaR
>>>>>> suspend/resume
>>>>>> (hence it has nothing to do in regards to CCLK) and given that PLLs
>>>>>> state is restored before the rest of the clocks, I don't see why
>>>>>> not to
>>>>>> implement CCLK save/restore in a generic fasion. CPU policy wull be
>>>>>> restored to either PLLP or PLLX (if CPUFreq driver is disabled).
>>>>>>
>>>>> CCLK_G save/restore should happen in clk_super_mux ops
>>>>> save/context and
>>>>> clk_super_mux save/restore happens very early as cclk_g is first
>>>>> in the
>>>>> clock tree and save/restore traverses through the tree top-bottom
>>>>> order.
>>>> If CCLK_G is restored before the PLLs, then just change the clocks
>>>> order
>>>> such that it won't happen.
>>>>
>>> I dont think we can change clocks order for CCLK_G.
>>>
>>> During bootup, cclk_g is registered after all pll's and peripheral
>>> clocks which is the way we wanted, So cclk_g will be the first one in
>>> the clk list as clk_register adds new clock first in the list.
>>>
>>> When clk_save_context and clk_restore_context APIs iterates over the
>>> list, cclk_g is the first
>> Looking at clk_core_restore_context(), I see that it walks up CLKs list
>> from parent to children, hence I don't understand how it can ever happen
>> that CCLK will be restored before the parent. The clocks registration
>> order doesn't matter at all in that case.
>
> yes from parent to children and dfllCPU_out is the top in the list and
> its child is cclk_g.
>
> the way clocks are registered is the order I see in the clock list and
> looking into clk_register API it adds new node first in the list.
>
cclkg_g & dfll register happens after all plls and peripheral clocks as
it need ref, soc and peripheral clocks to be enabled.
>
> So they are the last to get registered and so becomes first in the list.
>
> During save/restore context, it traverses thru this list and first in
> the list is dfllcpu_OUT (parent) and its child (cclk_g)
>
> saving should not be an issue at all but we cant restore cclk_g/dfll
> in normal way thru clk_ops restore as plls and peripherals restore
> doesn't happen by that time.
>
I was referring to clk_restore_context where it iterates thru root list
and for each core from the root list clk_core_restore does restore of
parent and children.
dfllCPU_Out gets first in the list and its child is cclk_g
https://elixir.bootlin.com/linux/v5.2.1/source/drivers/clk/clk.c#L1105
>>>>> DFLL enable thru CPUFreq resume happens after all clk_restore_context
>>>>> happens. So during clk_restore_context, dfll re-init doesnt happen
>>>>> and
>>>>> doing cpu clock policy restore during super_mux clk_ops will crash as
>>>>> DFLL is not initialized and its clock is not enabled but CPU clock
>>>>> restore sets source to DFLL if we restore during super_clk_mux
>>>> If CPU was suspended on PLLP, then it will be restored on PLLP by
>>>> CaR. I
>>>> don't understand what DFLL has to do with the CCLK in that case during
>>>> the clocks restore.
>>> My above comment is in reference to your request of doing save/restore
>>> for cclk_g in normal fashion thru save/restore context. Because of the
>>> clk order I mentioned above, we cclk_g will be the first one to go thru
>>> save/context.
>>>
>>> During save_context of cclk_g, source can be from PLLX, dfll.
>>>
>>> Issue will be when we do restore during clk_restore_context of
>>> cclk_g as
>>> by that time PLLX/dfll will not be restored.
>>>
>> Seems we already agreed that DFLL will be disabled by the CPUFreq driver
>> on suspend. Hence CCLK can't be from DFLL if CPU is reparented to PLLP
>> on CPUFreq driver's suspend, otherwise CPU keeps running from a
>> boot-state PLLX if CPUFreq driver is disabled.
>
> Yes suspend should not be an issue but issue will be during resume
> where if we do cclk_g restore in normal way thru clk_restore_context,
> cclk_g restore happens very early as dfllCPU out is the first one that
> goes thru restore context and plls/peripherals are not resumed by then.
>
> CPU runs from PLLX if dfll clock enable fails during boot. So when it
> gets to suspend, we save CPU running clock source as either PLLX or
> DFLL and then we switch to PLLP.
>
>
> On resume, CPU runs from PLLP by warm boot code and we need to restore
> back its source to the one it was using from saved source context
> (which can be either PLLX or DFLL)
>
> So PLLs & DFLL resume need to happen before CCLKG restore/resume.
>
>
> With all above discussions, we do DFLL disable in CPUFreq driver on
> suspend and on CPUFreq resume we enable DFLL back and restore CPU
> clock source it was using during suspend (which will be either PLLX if
> dfll enable fails during probe or it will be using DFLL).
>
> So i was trying to say dfll/cclk_g restore can't be done in normal way
> thru clk_ops save/restore context
>
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 18:51 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <71272e9a-0f2a-c20d-6532-7e9057ad985c@gmail.com>
On 7/17/19 11:32 AM, Dmitry Osipenko wrote:
> 17.07.2019 20:29, Sowjanya Komatineni пишет:
>> On 7/17/19 8:17 AM, Dmitry Osipenko wrote:
>>> 17.07.2019 9:36, Sowjanya Komatineni пишет:
>>>> On 7/16/19 11:33 PM, Dmitry Osipenko wrote:
>>>>> В Tue, 16 Jul 2019 22:55:52 -0700
>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>
>>>>>> On 7/16/19 10:42 PM, Dmitry Osipenko wrote:
>>>>>>> В Tue, 16 Jul 2019 22:25:25 -0700
>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>>
>>>>>>>> On 7/16/19 9:11 PM, Dmitry Osipenko wrote:
>>>>>>>>> В Tue, 16 Jul 2019 19:35:49 -0700
>>>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>>>>
>>>>>>>>>> On 7/16/19 7:18 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>> On 7/16/19 3:06 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>> On 7/16/19 3:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>> 17.07.2019 0:35, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>> On 7/16/19 2:21 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>> 17.07.2019 0:12, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>> On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>> 16.07.2019 22:26, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>>>>>>>>>>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph
>>>>>>>>>>>>>>>>>>>>>>>>> Lo wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>>>>>>>>>>>>>>>>>>> The other thing that also need attention is
>>>>>>>>>>>>>>>>>>>>>>>>>>>> that T124 CPUFreq
>>>>>>>>>>>>>>>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>>>>>>>>>>>>>>>> implicitly relies on DFLL driver to be probed
>>>>>>>>>>>>>>>>>>>>>>>>>>>> first, which is
>>>>>>>>>>>>>>>>>>>>>>>>>>>> icky.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Should I add check for successful dfll clk
>>>>>>>>>>>>>>>>>>>>>>>>>>> register explicitly in
>>>>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk
>>>>>>>>>>>>>>>>>>>>>>>>>>> registers?
>>>>>>>>>>>>>>>>>>>>>>>> Probably you should use the "device links". See
>>>>>>>>>>>>>>>>>>>>>>>> [1][2] for the
>>>>>>>>>>>>>>>>>>>>>>>> example.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> [2]
>>>>>>>>>>>>>>>>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if
>>>>>>>>>>>>>>>>>>>>>>>> device_link_add() fails.
>>>>>>>>>>>>>>>>>>>>>>>> And
>>>>>>>>>>>>>>>>>>>>>>>> use of_find_device_by_node() to get the DFLL's
>>>>>>>>>>>>>>>>>>>>>>>> device, see [3].
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> [3]
>>>>>>>>>>>>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Will go thru and add...
>>>>>>>>>>>>>>>>>>>>> Looks like I initially confused this case with getting
>>>>>>>>>>>>>>>>>>>>> orphaned clock.
>>>>>>>>>>>>>>>>>>>>> I'm now seeing that the DFLL driver registers the
>>>>>>>>>>>>>>>>>>>>> clock and then
>>>>>>>>>>>>>>>>>>>>> clk_get(dfll) should be returning EPROBE_DEFER until
>>>>>>>>>>>>>>>>>>>>> DFLL driver is
>>>>>>>>>>>>>>>>>>>>> probed, hence everything should be fine as-is and
>>>>>>>>>>>>>>>>>>>>> there is no real
>>>>>>>>>>>>>>>>>>>>> need
>>>>>>>>>>>>>>>>>>>>> for the 'device link'. Sorry for the confusion!
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> Sorry, I didn't follow the mail thread. Just
>>>>>>>>>>>>>>>>>>>>>>>>>> regarding the DFLL
>>>>>>>>>>>>>>>>>>>>>>>>>> part.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> As you know it, the DFLL clock is one of the CPU
>>>>>>>>>>>>>>>>>>>>>>>>>> clock sources and
>>>>>>>>>>>>>>>>>>>>>>>>>> integrated with DVFS control logic with the
>>>>>>>>>>>>>>>>>>>>>>>>>> regulator. We will not
>>>>>>>>>>>>>>>>>>>>>>>>>> switch
>>>>>>>>>>>>>>>>>>>>>>>>>> CPU to other clock sources once we switched to
>>>>>>>>>>>>>>>>>>>>>>>>>> DFLL. Because the
>>>>>>>>>>>>>>>>>>>>>>>>>> CPU has
>>>>>>>>>>>>>>>>>>>>>>>>>> been regulated by the DFLL HW with the DVFS table
>>>>>>>>>>>>>>>>>>>>>>>>>> (CVB or OPP
>>>>>>>>>>>>>>>>>>>>>>>>>> table
>>>>>>>>>>>>>>>>>>>>>>>>>> you see
>>>>>>>>>>>>>>>>>>>>>>>>>> in the driver.). We shouldn't reparent it to
>>>>>>>>>>>>>>>>>>>>>>>>>> other sources with
>>>>>>>>>>>>>>>>>>>>>>>>>> unknew
>>>>>>>>>>>>>>>>>>>>>>>>>> freq/volt pair. That's not guaranteed to work. We
>>>>>>>>>>>>>>>>>>>>>>>>>> allow switching to
>>>>>>>>>>>>>>>>>>>>>>>>>> open-loop mode but different sources.
>>>>>>>>>>>>>>>>>>>>>>>> Okay, then the CPUFreq driver will have to enforce
>>>>>>>>>>>>>>>>>>>>>>>> DFLL freq to
>>>>>>>>>>>>>>>>>>>>>>>> PLLP's
>>>>>>>>>>>>>>>>>>>>>>>> rate before switching to PLLP in order to have a
>>>>>>>>>>>>>>>>>>>>>>>> proper CPU voltage.
>>>>>>>>>>>>>>>>>>>>>>> PLLP freq is safe to work for any CPU voltage. So no
>>>>>>>>>>>>>>>>>>>>>>> need to enforce
>>>>>>>>>>>>>>>>>>>>>>> DFLL freq to PLLP rate before changing CCLK_G source
>>>>>>>>>>>>>>>>>>>>>>> to PLLP during
>>>>>>>>>>>>>>>>>>>>>>> suspend
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Sorry, please ignore my above comment. During
>>>>>>>>>>>>>>>>>>>>>> suspend, need to change
>>>>>>>>>>>>>>>>>>>>>> CCLK_G source to PLLP when dfll is in closed loop
>>>>>>>>>>>>>>>>>>>>>> mode first and
>>>>>>>>>>>>>>>>>>>>>> then
>>>>>>>>>>>>>>>>>>>>>> dfll need to be set to open loop.
>>>>>>>>>>>>>>>>>>>>> Okay.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> And I don't exactly understand why we need to
>>>>>>>>>>>>>>>>>>>>>>>>>> switch to PLLP in
>>>>>>>>>>>>>>>>>>>>>>>>>> CPU
>>>>>>>>>>>>>>>>>>>>>>>>>> idle
>>>>>>>>>>>>>>>>>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the
>>>>>>>>>>>>>>>>>>>>>>>>>> time.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> In SC7 entry, the dfll suspend function moves it
>>>>>>>>>>>>>>>>>>>>>>>>>> the open-loop
>>>>>>>>>>>>>>>>>>>>>>>>>> mode. That's
>>>>>>>>>>>>>>>>>>>>>>>>>> all. The sc7-entryfirmware will handle the rest
>>>>>>>>>>>>>>>>>>>>>>>>>> of the sequence to
>>>>>>>>>>>>>>>>>>>>>>>>>> turn off
>>>>>>>>>>>>>>>>>>>>>>>>>> the CPU power.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> In SC7 resume, the warmboot code will handle the
>>>>>>>>>>>>>>>>>>>>>>>>>> sequence to
>>>>>>>>>>>>>>>>>>>>>>>>>> turn on
>>>>>>>>>>>>>>>>>>>>>>>>>> regulator and power up the CPU cluster. And leave
>>>>>>>>>>>>>>>>>>>>>>>>>> it on PLL_P.
>>>>>>>>>>>>>>>>>>>>>>>>>> After
>>>>>>>>>>>>>>>>>>>>>>>>>> resuming to the kernel, we re-init DFLL, restore
>>>>>>>>>>>>>>>>>>>>>>>>>> the CPU clock
>>>>>>>>>>>>>>>>>>>>>>>>>> policy (CPU
>>>>>>>>>>>>>>>>>>>>>>>>>> runs on DFLL open-loop mode) and then moving to
>>>>>>>>>>>>>>>>>>>>>>>>>> close-loop mode.
>>>>>>>>>>>>>>>>>>>>>>>> The DFLL is re-inited after switching CCLK to DFLL
>>>>>>>>>>>>>>>>>>>>>>>> parent during of
>>>>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>>> early clocks-state restoring by CaR driver. Hence
>>>>>>>>>>>>>>>>>>>>>>>> instead of having
>>>>>>>>>>>>>>>>>>>>>>>> odd
>>>>>>>>>>>>>>>>>>>>>>>> hacks in the CaR driver, it is much nicer to have a
>>>>>>>>>>>>>>>>>>>>>>>> proper suspend-resume sequencing of the device
>>>>>>>>>>>>>>>>>>>>>>>> drivers. In this case
>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq
>>>>>>>>>>>>>>>>>>>>>>>> driver is the driver that enables DFLL and switches
>>>>>>>>>>>>>>>>>>>>>>>> CPU to that
>>>>>>>>>>>>>>>>>>>>>>>> clock
>>>>>>>>>>>>>>>>>>>>>>>> source, which means that this driver is also should
>>>>>>>>>>>>>>>>>>>>>>>> be responsible for
>>>>>>>>>>>>>>>>>>>>>>>> management of the DFLL's state during of
>>>>>>>>>>>>>>>>>>>>>>>> suspend/resume process. If
>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver disables DFLL during suspend and
>>>>>>>>>>>>>>>>>>>>>>>> re-enables it
>>>>>>>>>>>>>>>>>>>>>>>> during
>>>>>>>>>>>>>>>>>>>>>>>> resume, then looks like the CaR driver hacks around
>>>>>>>>>>>>>>>>>>>>>>>> DFLL are not
>>>>>>>>>>>>>>>>>>>>>>>> needed.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> The DFLL part looks good to me. BTW, change the
>>>>>>>>>>>>>>>>>>>>>>>>>> patch subject to
>>>>>>>>>>>>>>>>>>>>>>>>>> "Add
>>>>>>>>>>>>>>>>>>>>>>>>>> suspend-resume support" seems more appropriate to
>>>>>>>>>>>>>>>>>>>>>>>>>> me.
>>>>>>>>>>>>>>>>>>>>>>>>> To clarify this, the sequences for DFLL use are as
>>>>>>>>>>>>>>>>>>>>>>>>> follows (assuming
>>>>>>>>>>>>>>>>>>>>>>>>> all
>>>>>>>>>>>>>>>>>>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Switch to DFLL:
>>>>>>>>>>>>>>>>>>>>>>>>> 0) Save current parent and frequency
>>>>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>>>>>> 2) Enable DFLL
>>>>>>>>>>>>>>>>>>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>>>>>>>>>>>>>>>>>>> For OVR regulator:
>>>>>>>>>>>>>>>>>>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>>>>>>>>>>>>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>>>>>>>>>>>>>>>>>>> For I2C regulator:
>>>>>>>>>>>>>>>>>>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>>>>>>>>>>>>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Switch away from DFLL:
>>>>>>>>>>>>>>>>>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU
>>>>>>>>>>>>>>>>>>>>>>>>> frequency is ok for
>>>>>>>>>>>>>>>>>>>>>>>>> any
>>>>>>>>>>>>>>>>>>>>>>>>> vdd_cpu voltage
>>>>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I see during switch away from DFLL (suspend), cclk_g
>>>>>>>>>>>>>>>>>>>>>> parent is not
>>>>>>>>>>>>>>>>>>>>>> changed to PLLP before changing dfll to open loop
>>>>>>>>>>>>>>>>>>>>>> mode.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Will add this ...
>>>>>>>>>>>>>>>>>>>>> The CPUFreq driver switches parent to PLLP during the
>>>>>>>>>>>>>>>>>>>>> probe, similar
>>>>>>>>>>>>>>>>>>>>> should be done on suspend.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> I'm also wondering if it's always safe to switch to
>>>>>>>>>>>>>>>>>>>>> PLLP in the probe.
>>>>>>>>>>>>>>>>>>>>> If CPU is running on a lower freq than PLLP, then some
>>>>>>>>>>>>>>>>>>>>> other more
>>>>>>>>>>>>>>>>>>>>> appropriate intermediate parent should be selected.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always
>>>>>>>>>>>>>>>>>>>> runs at higher
>>>>>>>>>>>>>>>>>>>> rate
>>>>>>>>>>>>>>>>>>>> so switching to PLL_P during CPUFreq probe prior to
>>>>>>>>>>>>>>>>>>>> dfll clock enable
>>>>>>>>>>>>>>>>>>>> should be safe.
>>>>>>>>>>>>>>>>>>> AFAIK, PLLX could run at ~200MHz. There is also a
>>>>>>>>>>>>>>>>>>> divided output of
>>>>>>>>>>>>>>>>>>> PLLP
>>>>>>>>>>>>>>>>>>> which CCLKG supports, the PLLP_OUT4.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Probably, realistically, CPU is always running off a
>>>>>>>>>>>>>>>>>>> fast PLLX during
>>>>>>>>>>>>>>>>>>> boot, but I'm wondering what may happen on KEXEC. I
>>>>>>>>>>>>>>>>>>> guess ideally CPUFreq driver should also have a
>>>>>>>>>>>>>>>>>>> 'shutdown' callback to teardown DFLL
>>>>>>>>>>>>>>>>>>> on a reboot, but likely that there are other
>>>>>>>>>>>>>>>>>>> clock-related problems as
>>>>>>>>>>>>>>>>>>> well that may break KEXEC and thus it is not very
>>>>>>>>>>>>>>>>>>> important at the
>>>>>>>>>>>>>>>>>>> moment.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> [snip]
>>>>>>>>>>>>>>>>>> During bootup CPUG sources from PLL_X. By PLL_P source
>>>>>>>>>>>>>>>>>> above I meant
>>>>>>>>>>>>>>>>>> PLL_P_OUT4.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> As per clock policies, PLL_X is always used for high freq
>>>>>>>>>>>>>>>>>> like
>>>>>>>>>>>>>>>>>>> 800Mhz
>>>>>>>>>>>>>>>>>> and for low frequency it will be sourced from PLLP.
>>>>>>>>>>>>>>>>> Alright, then please don't forget to pre-initialize
>>>>>>>>>>>>>>>>> PLLP_OUT4 rate to a
>>>>>>>>>>>>>>>>> reasonable value using tegra_clk_init_table or
>>>>>>>>>>>>>>>>> assigned-clocks.
>>>>>>>>>>>>>>>> PLLP_OUT4 rate update is not needed as it is safe to run at
>>>>>>>>>>>>>>>> 408Mhz because it is below fmax @ Vmin
>>>>>>>>>>>>>>> So even 204MHz CVB entries are having the same voltage as
>>>>>>>>>>>>>>> 408MHz, correct? It's not instantly obvious to me from the
>>>>>>>>>>>>>>> DFLL driver's code where the fmax @ Vmin is defined, I see
>>>>>>>>>>>>>>> that there is the min_millivolts
>>>>>>>>>>>>>>> and frequency entries starting from 204MHZ defined
>>>>>>>>>>>>>>> per-table.
>>>>>>>>>>>>>> Yes at Vmin CPU Fmax is ~800Mhz. So anything below that will
>>>>>>>>>>>>>> work at Vmin voltage and PLLP max is 408Mhz.
>>>>>>>>>>>>> Thank you for the clarification. It would be good to have that
>>>>>>>>>>>>> commented
>>>>>>>>>>>>> in the code as well.
>>>>>>>>>>>> OK, Will add...
>>>>>>>>>>> Regarding, adding suspend/resume to CPUFreq, CPUFreq suspend
>>>>>>>>>>> happens very early even before disabling non-boot CPUs and also
>>>>>>>>>>> need to export clock driver APIs to CPUFreq.
>>>>>>>>>>>
>>>>>>>>>>> Was thinking of below way of implementing this...
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Clock DFLL driver Suspend:
>>>>>>>>>>>
>>>>>>>>>>> - Save CPU clock policy registers, and Perform dfll
>>>>>>>>>>> suspend which sets in open loop mode
>>>>>>>>>>>
>>>>>>>>>>> CPU Freq driver Suspend: does nothing
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Clock DFLL driver Resume:
>>>>>>>>>>>
>>>>>>>>>>> - Re-init DFLL, Set in Open-Loop mode, restore CPU
>>>>>>>>>>> Clock policy registers which actually sets source to DFLL along
>>>>>>>>>>> with other CPU Policy register restore.
>>>>>>>>>>>
>>>>>>>>>>> CPU Freq driver Resume:
>>>>>>>>>>>
>>>>>>>>>>> - do clk_prepare_enable which acutally sets DFLL in
>>>>>>>>>>> Closed loop mode
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Adding one more note: Switching CPU Clock to PLLP is not needed
>>>>>>>>>>> as CPU CLock can be from dfll in open-loop mode as DFLL is not
>>>>>>>>>>> disabled anywhere throught the suspend/resume path and SC7 entry
>>>>>>>>>>> FW and Warm boot code will switch CPU source to PLLP.
>>>>>>>>> Since CPU resumes on PLLP, it will be cleaner to suspend it on
>>>>>>>>> PLLP as well. And besides, seems that currently disabling DFLL
>>>>>>>>> clock will disable DFLL completely and then you'd want to re-init
>>>>>>>>> the DFLL on resume any ways. So better to just disable DFLL
>>>>>>>>> completely on suspend, which should happen on clk_disable(dfll).
>>>>>>>> Will switch to PLLP during CPUFreq suspend. With decision of using
>>>>>>>> clk_disable during suspend, its mandatory to switch to PLLP as DFLL
>>>>>>>> is completely disabled.
>>>>>>>>
>>>>>>>> My earlier concern was on restoring CPU policy as we can't do that
>>>>>>>> from CPUFreq driver and need export from clock driver.
>>>>>>>>
>>>>>>>> Clear now and will do CPU clock policy restore in after dfll
>>>>>>>> re-init.
>>>>>>> Why the policy can't be saved/restored by the CaR driver as a
>>>>>>> context of any other clock?
>>>>>> restoring cpu clock policy involves programming source and
>>>>>> super_cclkg_divider.
>>>>>>
>>>>>> cclk_g is registered as clk_super_mux and it doesn't use frac_div ops
>>>>>> to do save/restore its divider.
>>>>> That can be changed of course and I guess it also could be as simple as
>>>>> saving and restoring of two raw u32 values of the policy/divider
>>>>> registers.
>>>>>
>>>>>> Also, during clock context we cant restore cclk_g as cclk_g source
>>>>>> will be dfll and dfll will not be resumed/re-initialized by the time
>>>>>> clk_super_mux save/restore happens.
>>>>>>
>>>>>> we can't use save/restore context for dfll clk_ops because
>>>>>> dfllCPU_out parent to CCLK_G is first in the clock tree and dfll_ref
>>>>>> and dfll_soc peripheral clocks are not restored by the time dfll
>>>>>> restore happens. Also dfll peripheral clock enables need to be
>>>>>> restored before dfll restore happens which involves programming dfll
>>>>>> controller for re-initialization.
>>>>>>
>>>>>> So dfll resume/re-init is done in clk-tegra210 at end of all clocks
>>>>>> restore in V5 series but instead of in clk-tegra210 driver I moved
>>>>>> now to dfll-fcpu driver pm_ops as all dfll dependencies will be
>>>>>> restored thru clk_restore_context by then. This will be in V6.
>>>>> Since DFLL is now guaranteed to be disabled across CaR suspend/resume
>>>>> (hence it has nothing to do in regards to CCLK) and given that PLLs
>>>>> state is restored before the rest of the clocks, I don't see why not to
>>>>> implement CCLK save/restore in a generic fasion. CPU policy wull be
>>>>> restored to either PLLP or PLLX (if CPUFreq driver is disabled).
>>>>>
>>>> CCLK_G save/restore should happen in clk_super_mux ops save/context and
>>>> clk_super_mux save/restore happens very early as cclk_g is first in the
>>>> clock tree and save/restore traverses through the tree top-bottom order.
>>> If CCLK_G is restored before the PLLs, then just change the clocks order
>>> such that it won't happen.
>>>
>> I dont think we can change clocks order for CCLK_G.
>>
>> During bootup, cclk_g is registered after all pll's and peripheral
>> clocks which is the way we wanted, So cclk_g will be the first one in
>> the clk list as clk_register adds new clock first in the list.
>>
>> When clk_save_context and clk_restore_context APIs iterates over the
>> list, cclk_g is the first
> Looking at clk_core_restore_context(), I see that it walks up CLKs list
> from parent to children, hence I don't understand how it can ever happen
> that CCLK will be restored before the parent. The clocks registration
> order doesn't matter at all in that case.
yes from parent to children and dfllCPU_out is the top in the list and
its child is cclk_g.
the way clocks are registered is the order I see in the clock list and
looking into clk_register API it adds new node first in the list.
cclkg_g & dfll register happens after all plls and peripheral clocks as
it need ref, soc and peripheral clocks to be enabled.
So they are the last to get registered and so becomes first in the list.
During save/restore context, it traverses thru this list and first in
the list is dfllcpu_OUT (parent) and its child (cclk_g)
saving should not be an issue at all but we cant restore cclk_g/dfll in
normal way thru clk_ops restore as plls and peripherals restore doesn't
happen by that time.
>>>> DFLL enable thru CPUFreq resume happens after all clk_restore_context
>>>> happens. So during clk_restore_context, dfll re-init doesnt happen and
>>>> doing cpu clock policy restore during super_mux clk_ops will crash as
>>>> DFLL is not initialized and its clock is not enabled but CPU clock
>>>> restore sets source to DFLL if we restore during super_clk_mux
>>> If CPU was suspended on PLLP, then it will be restored on PLLP by CaR. I
>>> don't understand what DFLL has to do with the CCLK in that case during
>>> the clocks restore.
>> My above comment is in reference to your request of doing save/restore
>> for cclk_g in normal fashion thru save/restore context. Because of the
>> clk order I mentioned above, we cclk_g will be the first one to go thru
>> save/context.
>>
>> During save_context of cclk_g, source can be from PLLX, dfll.
>>
>> Issue will be when we do restore during clk_restore_context of cclk_g as
>> by that time PLLX/dfll will not be restored.
>>
> Seems we already agreed that DFLL will be disabled by the CPUFreq driver
> on suspend. Hence CCLK can't be from DFLL if CPU is reparented to PLLP
> on CPUFreq driver's suspend, otherwise CPU keeps running from a
> boot-state PLLX if CPUFreq driver is disabled.
Yes suspend should not be an issue but issue will be during resume where
if we do cclk_g restore in normal way thru clk_restore_context, cclk_g
restore happens very early as dfllCPU out is the first one that goes
thru restore context and plls/peripherals are not resumed by then.
CPU runs from PLLX if dfll clock enable fails during boot. So when it
gets to suspend, we save CPU running clock source as either PLLX or DFLL
and then we switch to PLLP.
On resume, CPU runs from PLLP by warm boot code and we need to restore
back its source to the one it was using from saved source context (which
can be either PLLX or DFLL)
So PLLs & DFLL resume need to happen before CCLKG restore/resume.
With all above discussions, we do DFLL disable in CPUFreq driver on
suspend and on CPUFreq resume we enable DFLL back and restore CPU clock
source it was using during suspend (which will be either PLLX if dfll
enable fails during probe or it will be using DFLL).
So i was trying to say dfll/cclk_g restore can't be done in normal way
thru clk_ops save/restore context
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-17 18:32 UTC (permalink / raw)
To: Sowjanya Komatineni
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <d7892bfc-2cbf-27af-518d-dc7e243815b8@nvidia.com>
17.07.2019 20:29, Sowjanya Komatineni пишет:
>
> On 7/17/19 8:17 AM, Dmitry Osipenko wrote:
>> 17.07.2019 9:36, Sowjanya Komatineni пишет:
>>> On 7/16/19 11:33 PM, Dmitry Osipenko wrote:
>>>> В Tue, 16 Jul 2019 22:55:52 -0700
>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>
>>>>> On 7/16/19 10:42 PM, Dmitry Osipenko wrote:
>>>>>> В Tue, 16 Jul 2019 22:25:25 -0700
>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>
>>>>>>> On 7/16/19 9:11 PM, Dmitry Osipenko wrote:
>>>>>>>> В Tue, 16 Jul 2019 19:35:49 -0700
>>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>>>
>>>>>>>>> On 7/16/19 7:18 PM, Sowjanya Komatineni wrote:
>>>>>>>>>> On 7/16/19 3:06 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>> On 7/16/19 3:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>> 17.07.2019 0:35, Sowjanya Komatineni пишет:
>>>>>>>>>>>>> On 7/16/19 2:21 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>> 17.07.2019 0:12, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>> On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>> 16.07.2019 22:26, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>>>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>>>>>>>>>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph
>>>>>>>>>>>>>>>>>>>>>>>> Lo wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>>>>>>>>>>>>>>>>>> The other thing that also need attention is
>>>>>>>>>>>>>>>>>>>>>>>>>>> that T124 CPUFreq
>>>>>>>>>>>>>>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>>>>>>>>>>>>>>> implicitly relies on DFLL driver to be probed
>>>>>>>>>>>>>>>>>>>>>>>>>>> first, which is
>>>>>>>>>>>>>>>>>>>>>>>>>>> icky.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> Should I add check for successful dfll clk
>>>>>>>>>>>>>>>>>>>>>>>>>> register explicitly in
>>>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk
>>>>>>>>>>>>>>>>>>>>>>>>>> registers?
>>>>>>>>>>>>>>>>>>>>>>> Probably you should use the "device links". See
>>>>>>>>>>>>>>>>>>>>>>> [1][2] for the
>>>>>>>>>>>>>>>>>>>>>>> example.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> [2]
>>>>>>>>>>>>>>>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if
>>>>>>>>>>>>>>>>>>>>>>> device_link_add() fails.
>>>>>>>>>>>>>>>>>>>>>>> And
>>>>>>>>>>>>>>>>>>>>>>> use of_find_device_by_node() to get the DFLL's
>>>>>>>>>>>>>>>>>>>>>>> device, see [3].
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> [3]
>>>>>>>>>>>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Will go thru and add...
>>>>>>>>>>>>>>>>>>>> Looks like I initially confused this case with getting
>>>>>>>>>>>>>>>>>>>> orphaned clock.
>>>>>>>>>>>>>>>>>>>> I'm now seeing that the DFLL driver registers the
>>>>>>>>>>>>>>>>>>>> clock and then
>>>>>>>>>>>>>>>>>>>> clk_get(dfll) should be returning EPROBE_DEFER until
>>>>>>>>>>>>>>>>>>>> DFLL driver is
>>>>>>>>>>>>>>>>>>>> probed, hence everything should be fine as-is and
>>>>>>>>>>>>>>>>>>>> there is no real
>>>>>>>>>>>>>>>>>>>> need
>>>>>>>>>>>>>>>>>>>> for the 'device link'. Sorry for the confusion!
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Sorry, I didn't follow the mail thread. Just
>>>>>>>>>>>>>>>>>>>>>>>>> regarding the DFLL
>>>>>>>>>>>>>>>>>>>>>>>>> part.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> As you know it, the DFLL clock is one of the CPU
>>>>>>>>>>>>>>>>>>>>>>>>> clock sources and
>>>>>>>>>>>>>>>>>>>>>>>>> integrated with DVFS control logic with the
>>>>>>>>>>>>>>>>>>>>>>>>> regulator. We will not
>>>>>>>>>>>>>>>>>>>>>>>>> switch
>>>>>>>>>>>>>>>>>>>>>>>>> CPU to other clock sources once we switched to
>>>>>>>>>>>>>>>>>>>>>>>>> DFLL. Because the
>>>>>>>>>>>>>>>>>>>>>>>>> CPU has
>>>>>>>>>>>>>>>>>>>>>>>>> been regulated by the DFLL HW with the DVFS table
>>>>>>>>>>>>>>>>>>>>>>>>> (CVB or OPP
>>>>>>>>>>>>>>>>>>>>>>>>> table
>>>>>>>>>>>>>>>>>>>>>>>>> you see
>>>>>>>>>>>>>>>>>>>>>>>>> in the driver.). We shouldn't reparent it to
>>>>>>>>>>>>>>>>>>>>>>>>> other sources with
>>>>>>>>>>>>>>>>>>>>>>>>> unknew
>>>>>>>>>>>>>>>>>>>>>>>>> freq/volt pair. That's not guaranteed to work. We
>>>>>>>>>>>>>>>>>>>>>>>>> allow switching to
>>>>>>>>>>>>>>>>>>>>>>>>> open-loop mode but different sources.
>>>>>>>>>>>>>>>>>>>>>>> Okay, then the CPUFreq driver will have to enforce
>>>>>>>>>>>>>>>>>>>>>>> DFLL freq to
>>>>>>>>>>>>>>>>>>>>>>> PLLP's
>>>>>>>>>>>>>>>>>>>>>>> rate before switching to PLLP in order to have a
>>>>>>>>>>>>>>>>>>>>>>> proper CPU voltage.
>>>>>>>>>>>>>>>>>>>>>> PLLP freq is safe to work for any CPU voltage. So no
>>>>>>>>>>>>>>>>>>>>>> need to enforce
>>>>>>>>>>>>>>>>>>>>>> DFLL freq to PLLP rate before changing CCLK_G source
>>>>>>>>>>>>>>>>>>>>>> to PLLP during
>>>>>>>>>>>>>>>>>>>>>> suspend
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Sorry, please ignore my above comment. During
>>>>>>>>>>>>>>>>>>>>> suspend, need to change
>>>>>>>>>>>>>>>>>>>>> CCLK_G source to PLLP when dfll is in closed loop
>>>>>>>>>>>>>>>>>>>>> mode first and
>>>>>>>>>>>>>>>>>>>>> then
>>>>>>>>>>>>>>>>>>>>> dfll need to be set to open loop.
>>>>>>>>>>>>>>>>>>>> Okay.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> And I don't exactly understand why we need to
>>>>>>>>>>>>>>>>>>>>>>>>> switch to PLLP in
>>>>>>>>>>>>>>>>>>>>>>>>> CPU
>>>>>>>>>>>>>>>>>>>>>>>>> idle
>>>>>>>>>>>>>>>>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the
>>>>>>>>>>>>>>>>>>>>>>>>> time.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> In SC7 entry, the dfll suspend function moves it
>>>>>>>>>>>>>>>>>>>>>>>>> the open-loop
>>>>>>>>>>>>>>>>>>>>>>>>> mode. That's
>>>>>>>>>>>>>>>>>>>>>>>>> all. The sc7-entryfirmware will handle the rest
>>>>>>>>>>>>>>>>>>>>>>>>> of the sequence to
>>>>>>>>>>>>>>>>>>>>>>>>> turn off
>>>>>>>>>>>>>>>>>>>>>>>>> the CPU power.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> In SC7 resume, the warmboot code will handle the
>>>>>>>>>>>>>>>>>>>>>>>>> sequence to
>>>>>>>>>>>>>>>>>>>>>>>>> turn on
>>>>>>>>>>>>>>>>>>>>>>>>> regulator and power up the CPU cluster. And leave
>>>>>>>>>>>>>>>>>>>>>>>>> it on PLL_P.
>>>>>>>>>>>>>>>>>>>>>>>>> After
>>>>>>>>>>>>>>>>>>>>>>>>> resuming to the kernel, we re-init DFLL, restore
>>>>>>>>>>>>>>>>>>>>>>>>> the CPU clock
>>>>>>>>>>>>>>>>>>>>>>>>> policy (CPU
>>>>>>>>>>>>>>>>>>>>>>>>> runs on DFLL open-loop mode) and then moving to
>>>>>>>>>>>>>>>>>>>>>>>>> close-loop mode.
>>>>>>>>>>>>>>>>>>>>>>> The DFLL is re-inited after switching CCLK to DFLL
>>>>>>>>>>>>>>>>>>>>>>> parent during of
>>>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>> early clocks-state restoring by CaR driver. Hence
>>>>>>>>>>>>>>>>>>>>>>> instead of having
>>>>>>>>>>>>>>>>>>>>>>> odd
>>>>>>>>>>>>>>>>>>>>>>> hacks in the CaR driver, it is much nicer to have a
>>>>>>>>>>>>>>>>>>>>>>> proper suspend-resume sequencing of the device
>>>>>>>>>>>>>>>>>>>>>>> drivers. In this case
>>>>>>>>>>>>>>>>>>>>>>> CPUFreq
>>>>>>>>>>>>>>>>>>>>>>> driver is the driver that enables DFLL and switches
>>>>>>>>>>>>>>>>>>>>>>> CPU to that
>>>>>>>>>>>>>>>>>>>>>>> clock
>>>>>>>>>>>>>>>>>>>>>>> source, which means that this driver is also should
>>>>>>>>>>>>>>>>>>>>>>> be responsible for
>>>>>>>>>>>>>>>>>>>>>>> management of the DFLL's state during of
>>>>>>>>>>>>>>>>>>>>>>> suspend/resume process. If
>>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver disables DFLL during suspend and
>>>>>>>>>>>>>>>>>>>>>>> re-enables it
>>>>>>>>>>>>>>>>>>>>>>> during
>>>>>>>>>>>>>>>>>>>>>>> resume, then looks like the CaR driver hacks around
>>>>>>>>>>>>>>>>>>>>>>> DFLL are not
>>>>>>>>>>>>>>>>>>>>>>> needed.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> The DFLL part looks good to me. BTW, change the
>>>>>>>>>>>>>>>>>>>>>>>>> patch subject to
>>>>>>>>>>>>>>>>>>>>>>>>> "Add
>>>>>>>>>>>>>>>>>>>>>>>>> suspend-resume support" seems more appropriate to
>>>>>>>>>>>>>>>>>>>>>>>>> me.
>>>>>>>>>>>>>>>>>>>>>>>> To clarify this, the sequences for DFLL use are as
>>>>>>>>>>>>>>>>>>>>>>>> follows (assuming
>>>>>>>>>>>>>>>>>>>>>>>> all
>>>>>>>>>>>>>>>>>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Switch to DFLL:
>>>>>>>>>>>>>>>>>>>>>>>> 0) Save current parent and frequency
>>>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>>>>> 2) Enable DFLL
>>>>>>>>>>>>>>>>>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>>>>>>>>>>>>>>>>>> For OVR regulator:
>>>>>>>>>>>>>>>>>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>>>>>>>>>>>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>>>>>>>>>>>>>>>>>> For I2C regulator:
>>>>>>>>>>>>>>>>>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>>>>>>>>>>>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Switch away from DFLL:
>>>>>>>>>>>>>>>>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU
>>>>>>>>>>>>>>>>>>>>>>>> frequency is ok for
>>>>>>>>>>>>>>>>>>>>>>>> any
>>>>>>>>>>>>>>>>>>>>>>>> vdd_cpu voltage
>>>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> I see during switch away from DFLL (suspend), cclk_g
>>>>>>>>>>>>>>>>>>>>> parent is not
>>>>>>>>>>>>>>>>>>>>> changed to PLLP before changing dfll to open loop
>>>>>>>>>>>>>>>>>>>>> mode.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Will add this ...
>>>>>>>>>>>>>>>>>>>> The CPUFreq driver switches parent to PLLP during the
>>>>>>>>>>>>>>>>>>>> probe, similar
>>>>>>>>>>>>>>>>>>>> should be done on suspend.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> I'm also wondering if it's always safe to switch to
>>>>>>>>>>>>>>>>>>>> PLLP in the probe.
>>>>>>>>>>>>>>>>>>>> If CPU is running on a lower freq than PLLP, then some
>>>>>>>>>>>>>>>>>>>> other more
>>>>>>>>>>>>>>>>>>>> appropriate intermediate parent should be selected.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always
>>>>>>>>>>>>>>>>>>> runs at higher
>>>>>>>>>>>>>>>>>>> rate
>>>>>>>>>>>>>>>>>>> so switching to PLL_P during CPUFreq probe prior to
>>>>>>>>>>>>>>>>>>> dfll clock enable
>>>>>>>>>>>>>>>>>>> should be safe.
>>>>>>>>>>>>>>>>>> AFAIK, PLLX could run at ~200MHz. There is also a
>>>>>>>>>>>>>>>>>> divided output of
>>>>>>>>>>>>>>>>>> PLLP
>>>>>>>>>>>>>>>>>> which CCLKG supports, the PLLP_OUT4.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Probably, realistically, CPU is always running off a
>>>>>>>>>>>>>>>>>> fast PLLX during
>>>>>>>>>>>>>>>>>> boot, but I'm wondering what may happen on KEXEC. I
>>>>>>>>>>>>>>>>>> guess ideally CPUFreq driver should also have a
>>>>>>>>>>>>>>>>>> 'shutdown' callback to teardown DFLL
>>>>>>>>>>>>>>>>>> on a reboot, but likely that there are other
>>>>>>>>>>>>>>>>>> clock-related problems as
>>>>>>>>>>>>>>>>>> well that may break KEXEC and thus it is not very
>>>>>>>>>>>>>>>>>> important at the
>>>>>>>>>>>>>>>>>> moment.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> [snip]
>>>>>>>>>>>>>>>>> During bootup CPUG sources from PLL_X. By PLL_P source
>>>>>>>>>>>>>>>>> above I meant
>>>>>>>>>>>>>>>>> PLL_P_OUT4.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> As per clock policies, PLL_X is always used for high freq
>>>>>>>>>>>>>>>>> like
>>>>>>>>>>>>>>>>>> 800Mhz
>>>>>>>>>>>>>>>>> and for low frequency it will be sourced from PLLP.
>>>>>>>>>>>>>>>> Alright, then please don't forget to pre-initialize
>>>>>>>>>>>>>>>> PLLP_OUT4 rate to a
>>>>>>>>>>>>>>>> reasonable value using tegra_clk_init_table or
>>>>>>>>>>>>>>>> assigned-clocks.
>>>>>>>>>>>>>>> PLLP_OUT4 rate update is not needed as it is safe to run at
>>>>>>>>>>>>>>> 408Mhz because it is below fmax @ Vmin
>>>>>>>>>>>>>> So even 204MHz CVB entries are having the same voltage as
>>>>>>>>>>>>>> 408MHz, correct? It's not instantly obvious to me from the
>>>>>>>>>>>>>> DFLL driver's code where the fmax @ Vmin is defined, I see
>>>>>>>>>>>>>> that there is the min_millivolts
>>>>>>>>>>>>>> and frequency entries starting from 204MHZ defined
>>>>>>>>>>>>>> per-table.
>>>>>>>>>>>>> Yes at Vmin CPU Fmax is ~800Mhz. So anything below that will
>>>>>>>>>>>>> work at Vmin voltage and PLLP max is 408Mhz.
>>>>>>>>>>>> Thank you for the clarification. It would be good to have that
>>>>>>>>>>>> commented
>>>>>>>>>>>> in the code as well.
>>>>>>>>>>> OK, Will add...
>>>>>>>>>> Regarding, adding suspend/resume to CPUFreq, CPUFreq suspend
>>>>>>>>>> happens very early even before disabling non-boot CPUs and also
>>>>>>>>>> need to export clock driver APIs to CPUFreq.
>>>>>>>>>>
>>>>>>>>>> Was thinking of below way of implementing this...
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Clock DFLL driver Suspend:
>>>>>>>>>>
>>>>>>>>>> - Save CPU clock policy registers, and Perform dfll
>>>>>>>>>> suspend which sets in open loop mode
>>>>>>>>>>
>>>>>>>>>> CPU Freq driver Suspend: does nothing
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Clock DFLL driver Resume:
>>>>>>>>>>
>>>>>>>>>> - Re-init DFLL, Set in Open-Loop mode, restore CPU
>>>>>>>>>> Clock policy registers which actually sets source to DFLL along
>>>>>>>>>> with other CPU Policy register restore.
>>>>>>>>>>
>>>>>>>>>> CPU Freq driver Resume:
>>>>>>>>>>
>>>>>>>>>> - do clk_prepare_enable which acutally sets DFLL in
>>>>>>>>>> Closed loop mode
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Adding one more note: Switching CPU Clock to PLLP is not needed
>>>>>>>>>> as CPU CLock can be from dfll in open-loop mode as DFLL is not
>>>>>>>>>> disabled anywhere throught the suspend/resume path and SC7 entry
>>>>>>>>>> FW and Warm boot code will switch CPU source to PLLP.
>>>>>>>> Since CPU resumes on PLLP, it will be cleaner to suspend it on
>>>>>>>> PLLP as well. And besides, seems that currently disabling DFLL
>>>>>>>> clock will disable DFLL completely and then you'd want to re-init
>>>>>>>> the DFLL on resume any ways. So better to just disable DFLL
>>>>>>>> completely on suspend, which should happen on clk_disable(dfll).
>>>>>>> Will switch to PLLP during CPUFreq suspend. With decision of using
>>>>>>> clk_disable during suspend, its mandatory to switch to PLLP as DFLL
>>>>>>> is completely disabled.
>>>>>>>
>>>>>>> My earlier concern was on restoring CPU policy as we can't do that
>>>>>>> from CPUFreq driver and need export from clock driver.
>>>>>>>
>>>>>>> Clear now and will do CPU clock policy restore in after dfll
>>>>>>> re-init.
>>>>>> Why the policy can't be saved/restored by the CaR driver as a
>>>>>> context of any other clock?
>>>>> restoring cpu clock policy involves programming source and
>>>>> super_cclkg_divider.
>>>>>
>>>>> cclk_g is registered as clk_super_mux and it doesn't use frac_div ops
>>>>> to do save/restore its divider.
>>>> That can be changed of course and I guess it also could be as simple as
>>>> saving and restoring of two raw u32 values of the policy/divider
>>>> registers.
>>>>
>>>>> Also, during clock context we cant restore cclk_g as cclk_g source
>>>>> will be dfll and dfll will not be resumed/re-initialized by the time
>>>>> clk_super_mux save/restore happens.
>>>>>
>>>>> we can't use save/restore context for dfll clk_ops because
>>>>> dfllCPU_out parent to CCLK_G is first in the clock tree and dfll_ref
>>>>> and dfll_soc peripheral clocks are not restored by the time dfll
>>>>> restore happens. Also dfll peripheral clock enables need to be
>>>>> restored before dfll restore happens which involves programming dfll
>>>>> controller for re-initialization.
>>>>>
>>>>> So dfll resume/re-init is done in clk-tegra210 at end of all clocks
>>>>> restore in V5 series but instead of in clk-tegra210 driver I moved
>>>>> now to dfll-fcpu driver pm_ops as all dfll dependencies will be
>>>>> restored thru clk_restore_context by then. This will be in V6.
>>>> Since DFLL is now guaranteed to be disabled across CaR suspend/resume
>>>> (hence it has nothing to do in regards to CCLK) and given that PLLs
>>>> state is restored before the rest of the clocks, I don't see why not to
>>>> implement CCLK save/restore in a generic fasion. CPU policy wull be
>>>> restored to either PLLP or PLLX (if CPUFreq driver is disabled).
>>>>
>>> CCLK_G save/restore should happen in clk_super_mux ops save/context and
>>> clk_super_mux save/restore happens very early as cclk_g is first in the
>>> clock tree and save/restore traverses through the tree top-bottom order.
>> If CCLK_G is restored before the PLLs, then just change the clocks order
>> such that it won't happen.
>>
> I dont think we can change clocks order for CCLK_G.
>
> During bootup, cclk_g is registered after all pll's and peripheral
> clocks which is the way we wanted, So cclk_g will be the first one in
> the clk list as clk_register adds new clock first in the list.
>
> When clk_save_context and clk_restore_context APIs iterates over the
> list, cclk_g is the first
Looking at clk_core_restore_context(), I see that it walks up CLKs list
from parent to children, hence I don't understand how it can ever happen
that CCLK will be restored before the parent. The clocks registration
order doesn't matter at all in that case.
>>> DFLL enable thru CPUFreq resume happens after all clk_restore_context
>>> happens. So during clk_restore_context, dfll re-init doesnt happen and
>>> doing cpu clock policy restore during super_mux clk_ops will crash as
>>> DFLL is not initialized and its clock is not enabled but CPU clock
>>> restore sets source to DFLL if we restore during super_clk_mux
>> If CPU was suspended on PLLP, then it will be restored on PLLP by CaR. I
>> don't understand what DFLL has to do with the CCLK in that case during
>> the clocks restore.
>
> My above comment is in reference to your request of doing save/restore
> for cclk_g in normal fashion thru save/restore context. Because of the
> clk order I mentioned above, we cclk_g will be the first one to go thru
> save/context.
>
> During save_context of cclk_g, source can be from PLLX, dfll.
>
> Issue will be when we do restore during clk_restore_context of cclk_g as
> by that time PLLX/dfll will not be restored.
>
Seems we already agreed that DFLL will be disabled by the CPUFreq driver
on suspend. Hence CCLK can't be from DFLL if CPU is reparented to PLLP
on CPUFreq driver's suspend, otherwise CPU keeps running from a
boot-state PLLX if CPUFreq driver is disabled.
^ permalink raw reply
* Re: [Letux-kernel] [PATCH v3 0/5] drm/panel-simple: Add panel parameters for ortustech-com37h3m05dtc/99dtc and sharp-lq070y3dg3b
From: Sam Ravnborg @ 2019-07-17 18:14 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Mark Rutland, devicetree, David Airlie, Belisko Marek,
Tomi Valkeinen, Linux Kernel Mailing List, dri-devel, Rob Herring,
Thierry Reding, Linux-OMAP, Discussions about the Letux Kernel
In-Reply-To: <33D92C0E-A430-4C34-A698-646F2592093D@goldelico.com>
Hi Nikolaus.
> >>>> BTW: should also be applied to 5.2
> >>> The drm bits are reviewed. The DT bits needs OK from DT people.
> >>> When we have OK from DT people we can apply them all to drm-misc-next.
> >>
> >> I got OK on irc from Rob to process these.
> >> All patches are now applied to drm-misc-next.
> >
> > Thanks for taking care of this!
>
> I have checked but it seems they are still not merged into linux-next.
They will appear in next merge window. They were to late to hit current
merge window, as the cut-of time is around .rc5 in the drm subsystem.
And this is not really a fix so not stable material.
Sam
^ permalink raw reply
* Re: [PATCH 4/6] irqchip/irq-pruss-intc: Add helper functions to configure internal mapping
From: David Lechner @ 2019-07-17 17:57 UTC (permalink / raw)
To: Suman Anna, Marc Zyngier, Rob Herring, Thomas Gleixner,
Jason Cooper
Cc: Tony Lindgren, Andrew F. Davis, Roger Quadros, Lokesh Vutla,
Grygorii Strashko, Sekhar Nori, Murali Karicheri, devicetree,
linux-omap, linux-arm-kernel, linux-kernel
In-Reply-To: <23ae1767-3531-ea57-2c82-f2657baa123f@ti.com>
On 7/16/19 6:29 PM, Suman Anna wrote:
> Hi David,
>
> On 7/10/19 10:10 PM, David Lechner wrote:
>> On 7/7/19 10:52 PM, Suman Anna wrote:
>>> The PRUSS INTC receives a number of system input interrupt source events
>>> and supports individual control configuration and hardware
>>> prioritization.
>>> These input events can be mapped to some output host interrupts through 2
>>> levels of many-to-one mapping i.e. events to channel mapping and channels
>>> to host interrupts.
>>>
>>> This mapping information is provided through the PRU firmware that is
>>> loaded onto a PRU core/s or through the device tree node of the PRU
>>
>
> Thanks for the thorough review and alternate solutions/suggestions.
>
>> What will the device tree bindings for this look like?
>
> They would be as in the below patch you already figured.
Ah, makes sense now: the mapping is defined in the remoteproc node
rather than in the interrupt controller node.
>
>>
>> Looking back at Rob's comment on the initial series [1], I still think
>> that increasing the #interrupt-cells sounds like a reasonable solution.
>>
>> [1]: https://patchwork.kernel.org/patch/10697705/#22375155
>
> So, there are couple of reasons why I did not use an extended
> #interrupt-cells:
>
> 1. There is only one irq descriptor associated with each event, and the
> usage of events is typically per application. And the descriptor mapping
> is done once. We can have two different applications use the same event
> with different mappings. So we want this programming done at
> application's usage of PRU (so done when a consumer driver acquires a
> PRU processor(s) which are treated as an exclusive resource). All the
> different application properties that you saw in [1] are configured at
> the time of acquiring a PRU and reset when they release a PRU.
>
> 2. The configuration is performed by Linux for all host interrupts and
> channels, and this was primarily done to save the very limited IRAM
> space for those needed by the PRUs. From firmware's point of view, this
> was offloaded to the ARM OS driver/infrastructure, but in general it is
> a design by contract between a PRU client driver and its firmware. Also,
> the DT binding semantics using interrupts property and request_irq()
> typically limits these to interrupts only being requested by MPU, and so
> will leave out those needed by PRUs.
>
Hmm... case 1. is a tricky one indeed. If there are going to be times where
an event requires multiple mappings, I agree that this doesn't seem to fit
into any existing device tree bindings.
^ permalink raw reply
* Re: [PATCH v1 1/1] arm64: dts: rockchip: Add support for TB-96AI board
From: Heiko Stübner @ 2019-07-17 17:50 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: Elon Zhang, mark.rutland, robh+dt, linux-rockchip,
linux-arm-kernel, linux-kernel, devicetree
In-Reply-To: <20190717154752.GA13269@Mani-XPS-13-9360>
Hi Elon,
Am Mittwoch, 17. Juli 2019, 17:47:52 CEST schrieb Manivannan Sadhasivam:
> Thanks for the patch. Overall, this patch needs a bit of cleanup. There
> are many nodes added which are not available in mainline.
>
> Please see comments inline.
What Manivannan said, plus some additional things inline.
> On Thu, Jul 11, 2019 at 10:12:09AM +0800, Elon Zhang wrote:
> > Add devicetree support for RK3399Pro TB-96AI board, one of
> > the 96Boards family.
> >
> > The TB-96AI board is a 96Boards Compute SOM design, launched
> > by Linaro, Rockchip and Beiqicloud.
> >
> > More information can be obtained from the following websites:
> > 1.https://www.96boards.org/product/tb-96ai/
> > 2.http://t.rock-chips.com/
> > 3.http://www.beiqicloud.com/
> >
> > This patch add basic node for the board and support booting up
> > to Fedora.
> >
> > Signed-off-by: Elon Zhang <zhangzj@rock-chips.com>
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> > index 5f2687acbf94..3d6c8d4363b5 100644
> > --- a/arch/arm64/boot/dts/rockchip/Makefile
> > +++ b/arch/arm64/boot/dts/rockchip/Makefile
> > @@ -27,3 +27,4 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rock960.dtb
> > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rockpro64.dtb
> > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire.dtb
> > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire-excavator.dtb
> > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399pro-tb-96ai.dtb
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3399pro-tb-96ai.dts b/arch/arm64/boot/dts/rockchip/rk3399pro-tb-96ai.dts
> > new file mode 100644
> > index 000000000000..1935df99065d
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/rockchip/rk3399pro-tb-96ai.dts
> > @@ -0,0 +1,629 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Copyright (c) 2019 Fuzhou Rockchip Electronics Co., Ltd.
> > + */
> > +
> > +/dts-v1/;
> > +#include "rk3399pro.dtsi"
> > +#include "rk3399-opp.dtsi"
> > +
> > +/ {
> > + compatible = "rockchip,rk3399pro-tb-96ai", "rockchip,rk3399pro";
> > +
>
> I think the manufacturer of this board is, Xiamen Beiqi Technology Co. Ltd.
> So, the compatible should be:
>
> compatible = "beiqui,rk3399pro-tb-96ai", "rockchip,rk3399pro";
>
> And there should a separate patch to add the vendor prefix. You can refer
> below patch:
>
> https://lkml.org/lkml/2019/7/17/39
>
> > + chosen {
> > + stdout-path = "serial2:1500000n8";
> > + };
> > +
> > + xin32k: xin32k {
> > + compatible = "fixed-clock";
> > + clock-frequency = <32768>;
> > + clock-output-names = "xin32k";
> > + #clock-cells = <0>;
> > + };
> > +
> > + vcc_phy: vcc-phy-regulator {
> > + compatible = "regulator-fixed";
> > + regulator-name = "vcc_phy";
> > + regulator-always-on;
> > + regulator-boot-on;
> > + };
please model a real power-tree following the board-schematics.
This dangling unconnected vcc_phy regulator is a good
indicator of things just being copied from the vendor bsp devicetree.
So from a power-input down through the different converters
that generate the subvoltages. And please also use the real
voltage-rail-names used in the schematics.
For reference just look at most rockchip arm64 boards
(rk3399-rock960, rk3399-puma and a lot more)
And if you look at regulator/regulator_summary in debugfs,
it should create an actual tree structure ;-) .
> > + vcc5v0_sys: vccsys {
> > + compatible = "regulator-fixed";
> > + regulator-name = "vcc5v0_sys";
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <5000000>;
> > + regulator-max-microvolt = <5000000>;
> > + };
> > +
> > + vdd_log: vdd_log {
> > + compatible = "regulator-fixed";
> > + regulator-name = "vdd_log";
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <900000>;
> > + regulator-max-microvolt = <900000>;
> > + };
> > +
> > + leds: gpio-leds {
> > + compatible = "gpio-leds";
> > + pinctrl-names = "default";
> > + pinctrl-0 =<&leds_gpio>;
>
> Leave a space after =
>
> > +
> > + led@1 {
> > + gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>;
> > + label = "system_work_led1";
> > + retain-state-suspended;
> > + };
> > +
>
> We are using a standard LED formats for all 96Boards. Please see,
> rk3399-rock960.dts for reference. Since there is only user leds
> (apart from power leds), just define those as per the format.
>
> > + led@2 {
> > + gpios = <&gpio2 4 GPIO_ACTIVE_HIGH>;
> > + label = "system_work_led2";
> > + retain-state-suspended;
> > + };
> > +
> > + led@3 {
> > + gpios = <&gpio2 3 GPIO_ACTIVE_HIGH>;
> > + label = "system_work_led3";
> > + retain-state-suspended;
> > + };
> > + };
> > +};
> > +
> > +&cpu_l0 {
> > + cpu-supply = <&vdd_cpu_l>;
> > +};
> > +
> > +&cpu_l1 {
> > + cpu-supply = <&vdd_cpu_l>;
> > +};
> > +
> > +&cpu_l2 {
> > + cpu-supply = <&vdd_cpu_l>;
> > +};
> > +
> > +&cpu_l3 {
> > + cpu-supply = <&vdd_cpu_l>;
> > +};
> > +
> > +&cpu_b0 {
> > + cpu-supply = <&vdd_cpu_b>;
> > +};
> > +
> > +&cpu_b1 {
> > + cpu-supply = <&vdd_cpu_b>;
> > +};
> > +
> > +&emmc_phy {
> > + status = "okay";
> > +};
> > +
> > +&i2c0 {
> > + status = "okay";
> > + i2c-scl-rising-time-ns = <180>;
> > + i2c-scl-falling-time-ns = <30>;
> > + clock-frequency = <400000>;
> > +
> > + rk809: pmic@20 {
> > + compatible = "rockchip,rk809";
>
> It looks like this driver is not present in mainline yet. I can see
> some old patches in lkml archive but not sure about the status of those.
> So, please remove this node and use dummy regulators where applicable.
Actually the rk809 support went in just this merge-window some days ago.
So we're good here.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e444f6d68c07bc01a3a3d5905409dbe1ca391d26
> > + reg = <0x20>;
> > + interrupt-parent = <&gpio1>;
> > + interrupts = <RK_PC2 IRQ_TYPE_LEVEL_LOW>;
> > + pinctrl-names = "default", "pmic-sleep",
> > + "pmic-power-off", "pmic-reset";
> > + pinctrl-0 = <&pmic_int_l>;
> > + pinctrl-1 = <&soc_slppin_slp>, <&rk809_slppin_slp>;
> > + pinctrl-2 = <&soc_slppin_gpio>, <&rk809_slppin_pwrdn>;
> > + pinctrl-3 = <&soc_slppin_gpio>,<&rk809_slppin_null>;
> > + rockchip,system-power-controller;
> > + pmic-reset-func = <1>;
> > + wakeup-source;
> > + #clock-cells = <1>;
> > + clock-output-names = "rk808-clkout1", "rk808-clkout2";
> > +
> > + vcc1-supply = <&vcc5v0_sys>;
> > + vcc2-supply = <&vcc5v0_sys>;
> > + vcc3-supply = <&vcc5v0_sys>;
> > + vcc4-supply = <&vcc5v0_sys>;
> > + vcc5-supply = <&vcc_buck5>;
> > + vcc6-supply = <&vcc_buck5>;
> > + vcc7-supply = <&vcc3v3_sys>;
> > + vcc8-supply = <&vcc3v3_sys>;
> > + vcc9-supply = <&vcc5v0_sys>;
> > +
> > + pwrkey {
> > + status = "okay";
> > + };
> > +
> > + rtc {
> > + status = "okay";
> > + };
> > +
> > + pinctrl_rk8xx: pinctrl_rk8xx {
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > +
> > + rk809_slppin_null: rk809_slppin_null {
> > + pins = "gpio_slp";
> > + function = "pin_fun0";
> > + };
> > +
> > + rk809_slppin_slp: rk809_slppin_slp {
> > + pins = "gpio_slp";
> > + function = "pin_fun1";
> > + };
> > +
> > + rk809_slppin_pwrdn: rk809_slppin_pwrdn {
> > + pins = "gpio_slp";
> > + function = "pin_fun2";
> > + };
> > +
> > + rk809_slppin_rst: rk809_slppin_rst {
> > + pins = "gpio_slp";
> > + function = "pin_fun3";
> > + };
> > + };
> > +
> > + regulators {
> > + vdd_center: DCDC_REG1 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <750000>;
> > + regulator-max-microvolt = <1350000>;
> > + regulator-initial-mode = <0x2>;
> > + regulator-name = "vdd_center";
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + regulator-suspend-microvolt = <900000>;
> > + };
> > + };
> > +
> > + vdd_cpu_l: DCDC_REG2 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <750000>;
> > + regulator-max-microvolt = <1350000>;
> > + regulator-ramp-delay = <6001>;
> > + regulator-initial-mode = <0x2>;
> > + regulator-name = "vdd_cpu_l";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vcc_ddr: DCDC_REG3 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-name = "vcc_ddr";
> > + regulator-initial-mode = <0x2>;
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + };
> > + };
> > +
> > + vcc3v3_sys: DCDC_REG4 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > + regulator-initial-mode = <0x2>;
> > + regulator-name = "vcc3v3_sys";
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + regulator-suspend-microvolt = <3300000>;
> > + };
> > + };
> > +
> > + vcc_buck5: DCDC_REG5 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <2200000>;
> > + regulator-max-microvolt = <2200000>;
> > + regulator-name = "vcc_buck5";
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + regulator-suspend-microvolt = <2200000>;
> > + };
> > + };
> > +
> > + vcca_0v9: LDO_REG1 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <900000>;
> > + regulator-max-microvolt = <900000>;
> > + regulator-name = "vcca_0v9";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vcc_1v8: LDO_REG2 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <1800000>;
> > +
> > + regulator-name = "vcc_1v8";
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + regulator-suspend-microvolt = <1800000>;
> > + };
> > + };
> > +
> > + vcc0v9_soc: LDO_REG3 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <900000>;
> > + regulator-max-microvolt = <900000>;
> > +
> > + regulator-name = "vcc0v9_soc";
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + regulator-suspend-microvolt = <900000>;
> > + };
> > + };
> > +
> > + vcca_1v8: LDO_REG4 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <1800000>;
> > +
> > + regulator-name = "vcca_1v8";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vdd1v5_dvp: LDO_REG5 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <1500000>;
> > + regulator-max-microvolt = <1500000>;
> > +
> > + regulator-name = "vdd1v5_dvp";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vcc_1v5: LDO_REG6 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <1500000>;
> > + regulator-max-microvolt = <1500000>;
> > +
> > + regulator-name = "vcc_1v5";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vcc_3v0: LDO_REG7 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <3000000>;
> > + regulator-max-microvolt = <3000000>;
> > +
> > + regulator-name = "vcc_3v0";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vccio_sd: LDO_REG8 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <3300000>;
> > +
> > + regulator-name = "vccio_sd";
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + regulator-suspend-microvolt = <3300000>;
> > + };
> > + };
> > +
> > + vcc_sd: LDO_REG9 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > +
> > + regulator-name = "vcc_sd";
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + regulator-suspend-microvolt = <3300000>;
> > + };
> > + };
> > +
> > + vcc5v0_usb: SWITCH_REG1 {
> > + regulator-min-microvolt = <5000000>;
> > + regulator-max-microvolt = <5000000>;
> > +
> > + regulator-name = "vcc5v0_usb";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vccio_3v3: SWITCH_REG2 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > +
> > + regulator-name = "vccio_3v3";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > + };
> > + };
> > +
> > + vdd_cpu_b: tcs452x@1c {
> > + compatible = "tcs,tcs452x";
>
> Again, there is no driver for this.
>
> > + reg = <0x1c>;
> > + vin-supply = <&vcc5v0_sys>;
> > + regulator-compatible = "fan53555-reg";
> > + pinctrl-0 = <&vsel1_gpio>;
> > + vsel-gpios = <&gpio1 RK_PC1 GPIO_ACTIVE_HIGH>;
> > + regulator-name = "vdd_cpu_b";
> > + regulator-min-microvolt = <712500>;
> > + regulator-max-microvolt = <1500000>;
> > + regulator-ramp-delay = <2300>;
> > + fcs,suspend-voltage-selector = <1>;
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-initial-state = <3>;
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vdd_gpu: tcs452x@10 {
> > + compatible = "tcs,tcs452x";
>
> ditto.
>
> > + status = "okay";
> > + reg = <0x10>;
> > + vin-supply = <&vcc5v0_sys>;
> > + regulator-compatible = "fan53555-reg";
> > + pinctrl-0 = <&vsel2_gpio>;
> > + vsel-gpios = <&gpio1 RK_PB6 GPIO_ACTIVE_HIGH>;
> > + regulator-name = "vdd_gpu";
> > + regulator-min-microvolt = <735000>;
> > + regulator-max-microvolt = <1400000>;
> > + regulator-ramp-delay = <2300>;
> > + fcs,suspend-voltage-selector = <1>;
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > +};
> > +
> > +&i2c8 {
> > + status = "okay";
> > + i2c-scl-rising-time-ns = <345>;
> > + i2c-scl-falling-time-ns = <11>;
> > + clock-frequency = <100000>;
> > +
> > + fusb0: fusb30x@22 {
> > + compatible = "fairchild,fusb302";
>
> ditto, please remove.
>
> > + reg = <0x22>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&fusb0_int>;
> > + int-n-gpios = <&gpio1 RK_PA2 GPIO_ACTIVE_HIGH>;
> > + vbus-5v-gpios = <&gpio0 RK_PA1 GPIO_ACTIVE_LOW>;
> > + status = "okay";
> > + };
> > +
> > +};
> > +
> > +&io_domains {
> > + status = "okay";
> > + bt656-supply = <&vcca_1v8>; /* APIO2_VDD */
> > + audio-supply = <&vcca_1v8>; /* APIO5_VDD */
> > + sdmmc-supply = <&vccio_sd>; /* SDMMC0_VDD */
> > + gpio1830-supply = <&vcc_1v8>; /* APIO4_VDD */
> > +};
> > +
> > +&pinctrl {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&npu_ref_clk>;
> > +
> > + fusb30x {
> > + fusb0_int: fusb0-int {
> > + rockchip,pins =
> > + <1 RK_PA2 0 &pcfg_pull_up>;
> > + };
> > + };
>
> not needed.
>
> > +
> > + gpio-leds {
> > + leds_gpio: leds-gpio {
> > + rockchip,pins =
> > + <2 5 RK_FUNC_GPIO &pcfg_pull_up>,
> > + <2 4 RK_FUNC_GPIO &pcfg_pull_up>,
> > + <2 3 RK_FUNC_GPIO &pcfg_pull_up>;
>
> I don't think we need pull-up here.
>
> > + };
> > + };
> > +
> > + npu_clk {
> > + npu_ref_clk: npu-ref-clk {
> > + rockchip,pins =
> > + <0 RK_PA2 1 &pcfg_pull_none>;
> > + };
> > + };
> > +
> > + pmic {
> > + pmic_int_l: pmic-int-l {
> > + rockchip,pins =
> > + <1 RK_PC2 0 &pcfg_pull_up>;
> > + };
> > +
> > + soc_slppin_gpio: soc-slppin-gpio {
> > + rockchip,pins =
> > + <1 RK_PA5 0 &pcfg_output_low>;
> > + };
> > +
> > + soc_slppin_slp: soc-slppin-slp {
> > + rockchip,pins =
> > + <1 RK_PA5 1 &pcfg_pull_down>;
> > + };
> > +
> > + vsel1_gpio: vsel1-gpio {
> > + rockchip,pins =
> > + <1 RK_PC1 0 &pcfg_pull_down>;
> > + };
> > +
> > + vsel2_gpio: vsel2-gpio {
> > + rockchip,pins =
> > + <1 RK_PB6 0 &pcfg_pull_down>;
> > + };
> > + };
>
> not needed.
>
> > +
> > + usb3 {
> > + usb3_host_en: usb3-host-en {
> > + rockchip,pins =
> > + <2 RK_PA2 RK_FUNC_GPIO &pcfg_output_high>;
> > + };
> > + };
> > +};
> > +
> > +&pmu_io_domains {
> > + status = "okay";
> > + pmu1830-supply = <&vcc_1v8>;
> > +};
> > +
> > +&pwm0 {
> > + status = "okay";
> > +};
> > +
> > +&pwm2 {
> > + status = "okay";
> > +};
> > +
> > +&saradc {
> > + status = "okay";
> > + vref-supply = <&vcc_1v8>;
> > +};
> > +
> > +&sdhci {
> > + bus-width = <8>;
> > + mmc-hs400-1_8v;
> > + supports-emmc;
>
> there is no such property, so please remove. Since this controller is
> used for emmc, you can use "no-sd" and "no-sdio" properties if needed.
>
> > + non-removable;
> > + keep-power-in-suspend;
> > + mmc-hs400-enhanced-strobe;
> > + status = "okay";
> > +};
> > +
> > +&sdmmc {
> > + clock-frequency = <150000000>;
> > + clock-freq-min-max = <400000 150000000>;
> > + supports-sd;
>
> Same as above, property not available. Use, "no-sdio" and "no-emmc" if
> needed.
>
> > + bus-width = <4>;
> > + cap-mmc-highspeed;
> > + cap-sd-highspeed;
> > + disable-wp;
> > + num-slots = <1>;
>
> not needed.
>
> > + vqmmc-supply = <&vccio_sd>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
> > + status = "okay";
> > +};
> > +
> > +&tcphy0 {
> > + extcon = <&fusb0>;
this comes from the vendor kernel, which mimiks the old chrome-os
of having a extcon and not using the newer type-c framework.
So drop this please.
Heiko
> > + status = "okay";
> > +};
> > +
> > +&tcphy1 {
> > + status = "okay";
> > +};
> > +
> > +&tsadc {
> > + rockchip,hw-tshut-mode = <1>; /* tshut mode 0:CRU 1:GPIO */
> > + rockchip,hw-tshut-polarity = <1>; /* tshut polarity 0:LOW 1:HIGH */
>
> Any clue about shutdown temperature? Not necessary now but good to have.
>
> Thanks,
> Mani
> > + status = "okay";
> > +};
> > +
> > +&u2phy0 {
> > + status = "okay";
> > + extcon = <&fusb0>;
> > +
> > + u2phy0_otg: otg-port {
> > + status = "okay";
> > + };
> > +
> > + u2phy0_host: host-port {
> > + phy-supply = <&vcc5v0_usb>;
> > + status = "okay";
> > + };
> > +};
> > +
> > +&u2phy1 {
> > + status = "okay";
> > +
> > + u2phy1_otg: otg-port {
> > + status = "okay";
> > + };
> > +
> > + u2phy1_host: host-port {
> > + phy-supply = <&vcc5v0_usb>;
> > + status = "okay";
> > + };
> > +};
> > +
> > +&uart0 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&uart0_xfer &uart0_cts>;
> > + status = "okay";
> > +};
> > +
> > +&uart2 {
> > + status = "okay";
> > +};
> > +
> > +&uart4 {
> > + status = "okay";
> > +};
> > +
> > +&usb_host0_ehci {
> > + status = "okay";
> > +};
> > +
> > +&usb_host1_ehci {
> > + status = "okay";
> > +};
> > +
> > +&usb_host0_ohci {
> > + status = "okay";
> > +};
> > +
> > +&usb_host1_ohci {
> > + status = "okay";
> > +};
> > +
> > +&usbdrd3_0 {
> > + extcon = <&fusb0>;
> > + status = "okay";
> > +};
> > +
> > +&usbdrd3_1 {
> > + status = "okay";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&usb3_host_en>;
> > +};
> > +
> > +&usbdrd_dwc3_0 {
> > + status = "okay";
> > +};
> > +
> > +&usbdrd_dwc3_1 {
> > + snps,dis-u3-autosuspend-quirk;
> > + status = "okay";
> > +};
> > +
>
^ permalink raw reply
* Re: [PATCHv8 4/5] coresight: etm4x: Add ETM PIDs for SDM845 and MSM8996
From: Mathieu Poirier @ 2019-07-17 17:40 UTC (permalink / raw)
To: Sai Prakash Ranjan
Cc: Greg Kroah-Hartman, Suzuki K Poulose, Leo Yan, Alexander Shishkin,
Mike Leach, Rob Herring, Bjorn Andersson, devicetree, David Brown,
Mark Rutland, Rajendra Nayak, Vivek Gautam, Sibi Sankar,
linux-arm-kernel, Linux Kernel Mailing List, linux-arm-msm,
Marc Gonzalez
In-Reply-To: <2694eae0731a07eeda11f666526ccff8c6b5842e.1562940244.git.saiprakash.ranjan@codeaurora.org>
On Fri, 12 Jul 2019 at 08:17, Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> Instead of overriding the peripheral id(PID) check in AMBA
> by hardcoding them in DT, add the PIDs to the ETM4x driver.
> Here we use Unique Component Identifier(UCI) for MSM8996
> since the ETM and CPU debug module shares the same PIDs.
> SDM845 does not support CPU debug module.
>
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-etm4x.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> index 7bcac8896fc1..28bcc0e58d7a 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> @@ -1192,11 +1192,15 @@ static struct amba_cs_uci_id uci_id_etm4[] = {
> };
>
> static const struct amba_id etm4_ids[] = {
> - CS_AMBA_ID(0x000bb95d), /* Cortex-A53 */
> - CS_AMBA_ID(0x000bb95e), /* Cortex-A57 */
> - CS_AMBA_ID(0x000bb95a), /* Cortex-A72 */
> - CS_AMBA_ID(0x000bb959), /* Cortex-A73 */
> - CS_AMBA_UCI_ID(0x000bb9da, uci_id_etm4), /* Cortex-A35 */
> + CS_AMBA_ID(0x000bb95d), /* Cortex-A53 */
> + CS_AMBA_ID(0x000bb95e), /* Cortex-A57 */
> + CS_AMBA_ID(0x000bb95a), /* Cortex-A72 */
> + CS_AMBA_ID(0x000bb959), /* Cortex-A73 */
> + CS_AMBA_UCI_ID(0x000bb9da, uci_id_etm4),/* Cortex-A35 */
> + CS_AMBA_UCI_ID(0x000f0205, uci_id_etm4),/* Qualcomm Kryo */
> + CS_AMBA_UCI_ID(0x000f0211, uci_id_etm4),/* Qualcomm Kryo */
> + CS_AMBA_ID(0x000bb802), /* Qualcomm Kryo 385 Cortex-A55 */
> + CS_AMBA_ID(0x000bb803), /* Qualcomm Kryo 385 Cortex-A75 */
> {},
> };
>
I have also queued this patch.
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
^ permalink raw reply
* Re: [Letux-kernel] [PATCH v3 0/5] drm/panel-simple: Add panel parameters for ortustech-com37h3m05dtc/99dtc and sharp-lq070y3dg3b
From: H. Nikolaus Schaller @ 2019-07-17 17:33 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Mark Rutland, devicetree, David Airlie, Belisko Marek,
Tomi Valkeinen, Linux Kernel Mailing List, dri-devel, Rob Herring,
Thierry Reding, Linux-OMAP, Discussions about the Letux Kernel
In-Reply-To: <CF1D9929-58D2-4E75-932A-870D11BBFBDE@goldelico.com>
Hi Sam,
> Am 26.06.2019 um 14:40 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>
> Hi Sam,
>
>> Am 26.06.2019 um 09:33 schrieb Sam Ravnborg <sam@ravnborg.org>:
>>
>> Hi Nikolaus.
>>
>>>>
>>>> any progress towards merging this somewhere? It did not yet arrive in linux-next.
>>>>
>>>> BTW: should also be applied to 5.2
>>> The drm bits are reviewed. The DT bits needs OK from DT people.
>>> When we have OK from DT people we can apply them all to drm-misc-next.
>>
>> I got OK on irc from Rob to process these.
>> All patches are now applied to drm-misc-next.
>
> Thanks for taking care of this!
I have checked but it seems they are still not merged into linux-next.
BR and thanks,
Nikolaus
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 17:29 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <16f8b146-2581-a842-4997-53ab05b62c70@gmail.com>
On 7/17/19 8:17 AM, Dmitry Osipenko wrote:
> 17.07.2019 9:36, Sowjanya Komatineni пишет:
>> On 7/16/19 11:33 PM, Dmitry Osipenko wrote:
>>> В Tue, 16 Jul 2019 22:55:52 -0700
>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>
>>>> On 7/16/19 10:42 PM, Dmitry Osipenko wrote:
>>>>> В Tue, 16 Jul 2019 22:25:25 -0700
>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>
>>>>>> On 7/16/19 9:11 PM, Dmitry Osipenko wrote:
>>>>>>> В Tue, 16 Jul 2019 19:35:49 -0700
>>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>>
>>>>>>>> On 7/16/19 7:18 PM, Sowjanya Komatineni wrote:
>>>>>>>>> On 7/16/19 3:06 PM, Sowjanya Komatineni wrote:
>>>>>>>>>> On 7/16/19 3:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>>> 17.07.2019 0:35, Sowjanya Komatineni пишет:
>>>>>>>>>>>> On 7/16/19 2:21 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>> 17.07.2019 0:12, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>> On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>> 16.07.2019 22:26, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>>>>>>>>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph
>>>>>>>>>>>>>>>>>>>>>>> Lo wrote:
>>>>>>>>>>>>>>>>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>>>>>>>>>>>>>>>>> The other thing that also need attention is
>>>>>>>>>>>>>>>>>>>>>>>>>> that T124 CPUFreq
>>>>>>>>>>>>>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>>>>>>>>>>>>>> implicitly relies on DFLL driver to be probed
>>>>>>>>>>>>>>>>>>>>>>>>>> first, which is
>>>>>>>>>>>>>>>>>>>>>>>>>> icky.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Should I add check for successful dfll clk
>>>>>>>>>>>>>>>>>>>>>>>>> register explicitly in
>>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk
>>>>>>>>>>>>>>>>>>>>>>>>> registers?
>>>>>>>>>>>>>>>>>>>>>> Probably you should use the "device links". See
>>>>>>>>>>>>>>>>>>>>>> [1][2] for the
>>>>>>>>>>>>>>>>>>>>>> example.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> [2]
>>>>>>>>>>>>>>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if
>>>>>>>>>>>>>>>>>>>>>> device_link_add() fails.
>>>>>>>>>>>>>>>>>>>>>> And
>>>>>>>>>>>>>>>>>>>>>> use of_find_device_by_node() to get the DFLL's
>>>>>>>>>>>>>>>>>>>>>> device, see [3].
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> [3]
>>>>>>>>>>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Will go thru and add...
>>>>>>>>>>>>>>>>>>> Looks like I initially confused this case with getting
>>>>>>>>>>>>>>>>>>> orphaned clock.
>>>>>>>>>>>>>>>>>>> I'm now seeing that the DFLL driver registers the
>>>>>>>>>>>>>>>>>>> clock and then
>>>>>>>>>>>>>>>>>>> clk_get(dfll) should be returning EPROBE_DEFER until
>>>>>>>>>>>>>>>>>>> DFLL driver is
>>>>>>>>>>>>>>>>>>> probed, hence everything should be fine as-is and
>>>>>>>>>>>>>>>>>>> there is no real
>>>>>>>>>>>>>>>>>>> need
>>>>>>>>>>>>>>>>>>> for the 'device link'. Sorry for the confusion!
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Sorry, I didn't follow the mail thread. Just
>>>>>>>>>>>>>>>>>>>>>>>> regarding the DFLL
>>>>>>>>>>>>>>>>>>>>>>>> part.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> As you know it, the DFLL clock is one of the CPU
>>>>>>>>>>>>>>>>>>>>>>>> clock sources and
>>>>>>>>>>>>>>>>>>>>>>>> integrated with DVFS control logic with the
>>>>>>>>>>>>>>>>>>>>>>>> regulator. We will not
>>>>>>>>>>>>>>>>>>>>>>>> switch
>>>>>>>>>>>>>>>>>>>>>>>> CPU to other clock sources once we switched to
>>>>>>>>>>>>>>>>>>>>>>>> DFLL. Because the
>>>>>>>>>>>>>>>>>>>>>>>> CPU has
>>>>>>>>>>>>>>>>>>>>>>>> been regulated by the DFLL HW with the DVFS table
>>>>>>>>>>>>>>>>>>>>>>>> (CVB or OPP
>>>>>>>>>>>>>>>>>>>>>>>> table
>>>>>>>>>>>>>>>>>>>>>>>> you see
>>>>>>>>>>>>>>>>>>>>>>>> in the driver.). We shouldn't reparent it to
>>>>>>>>>>>>>>>>>>>>>>>> other sources with
>>>>>>>>>>>>>>>>>>>>>>>> unknew
>>>>>>>>>>>>>>>>>>>>>>>> freq/volt pair. That's not guaranteed to work. We
>>>>>>>>>>>>>>>>>>>>>>>> allow switching to
>>>>>>>>>>>>>>>>>>>>>>>> open-loop mode but different sources.
>>>>>>>>>>>>>>>>>>>>>> Okay, then the CPUFreq driver will have to enforce
>>>>>>>>>>>>>>>>>>>>>> DFLL freq to
>>>>>>>>>>>>>>>>>>>>>> PLLP's
>>>>>>>>>>>>>>>>>>>>>> rate before switching to PLLP in order to have a
>>>>>>>>>>>>>>>>>>>>>> proper CPU voltage.
>>>>>>>>>>>>>>>>>>>>> PLLP freq is safe to work for any CPU voltage. So no
>>>>>>>>>>>>>>>>>>>>> need to enforce
>>>>>>>>>>>>>>>>>>>>> DFLL freq to PLLP rate before changing CCLK_G source
>>>>>>>>>>>>>>>>>>>>> to PLLP during
>>>>>>>>>>>>>>>>>>>>> suspend
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Sorry, please ignore my above comment. During
>>>>>>>>>>>>>>>>>>>> suspend, need to change
>>>>>>>>>>>>>>>>>>>> CCLK_G source to PLLP when dfll is in closed loop
>>>>>>>>>>>>>>>>>>>> mode first and
>>>>>>>>>>>>>>>>>>>> then
>>>>>>>>>>>>>>>>>>>> dfll need to be set to open loop.
>>>>>>>>>>>>>>>>>>> Okay.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> And I don't exactly understand why we need to
>>>>>>>>>>>>>>>>>>>>>>>> switch to PLLP in
>>>>>>>>>>>>>>>>>>>>>>>> CPU
>>>>>>>>>>>>>>>>>>>>>>>> idle
>>>>>>>>>>>>>>>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the
>>>>>>>>>>>>>>>>>>>>>>>> time.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> In SC7 entry, the dfll suspend function moves it
>>>>>>>>>>>>>>>>>>>>>>>> the open-loop
>>>>>>>>>>>>>>>>>>>>>>>> mode. That's
>>>>>>>>>>>>>>>>>>>>>>>> all. The sc7-entryfirmware will handle the rest
>>>>>>>>>>>>>>>>>>>>>>>> of the sequence to
>>>>>>>>>>>>>>>>>>>>>>>> turn off
>>>>>>>>>>>>>>>>>>>>>>>> the CPU power.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> In SC7 resume, the warmboot code will handle the
>>>>>>>>>>>>>>>>>>>>>>>> sequence to
>>>>>>>>>>>>>>>>>>>>>>>> turn on
>>>>>>>>>>>>>>>>>>>>>>>> regulator and power up the CPU cluster. And leave
>>>>>>>>>>>>>>>>>>>>>>>> it on PLL_P.
>>>>>>>>>>>>>>>>>>>>>>>> After
>>>>>>>>>>>>>>>>>>>>>>>> resuming to the kernel, we re-init DFLL, restore
>>>>>>>>>>>>>>>>>>>>>>>> the CPU clock
>>>>>>>>>>>>>>>>>>>>>>>> policy (CPU
>>>>>>>>>>>>>>>>>>>>>>>> runs on DFLL open-loop mode) and then moving to
>>>>>>>>>>>>>>>>>>>>>>>> close-loop mode.
>>>>>>>>>>>>>>>>>>>>>> The DFLL is re-inited after switching CCLK to DFLL
>>>>>>>>>>>>>>>>>>>>>> parent during of
>>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>> early clocks-state restoring by CaR driver. Hence
>>>>>>>>>>>>>>>>>>>>>> instead of having
>>>>>>>>>>>>>>>>>>>>>> odd
>>>>>>>>>>>>>>>>>>>>>> hacks in the CaR driver, it is much nicer to have a
>>>>>>>>>>>>>>>>>>>>>> proper suspend-resume sequencing of the device
>>>>>>>>>>>>>>>>>>>>>> drivers. In this case
>>>>>>>>>>>>>>>>>>>>>> CPUFreq
>>>>>>>>>>>>>>>>>>>>>> driver is the driver that enables DFLL and switches
>>>>>>>>>>>>>>>>>>>>>> CPU to that
>>>>>>>>>>>>>>>>>>>>>> clock
>>>>>>>>>>>>>>>>>>>>>> source, which means that this driver is also should
>>>>>>>>>>>>>>>>>>>>>> be responsible for
>>>>>>>>>>>>>>>>>>>>>> management of the DFLL's state during of
>>>>>>>>>>>>>>>>>>>>>> suspend/resume process. If
>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver disables DFLL during suspend and
>>>>>>>>>>>>>>>>>>>>>> re-enables it
>>>>>>>>>>>>>>>>>>>>>> during
>>>>>>>>>>>>>>>>>>>>>> resume, then looks like the CaR driver hacks around
>>>>>>>>>>>>>>>>>>>>>> DFLL are not
>>>>>>>>>>>>>>>>>>>>>> needed.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> The DFLL part looks good to me. BTW, change the
>>>>>>>>>>>>>>>>>>>>>>>> patch subject to
>>>>>>>>>>>>>>>>>>>>>>>> "Add
>>>>>>>>>>>>>>>>>>>>>>>> suspend-resume support" seems more appropriate to
>>>>>>>>>>>>>>>>>>>>>>>> me.
>>>>>>>>>>>>>>>>>>>>>>> To clarify this, the sequences for DFLL use are as
>>>>>>>>>>>>>>>>>>>>>>> follows (assuming
>>>>>>>>>>>>>>>>>>>>>>> all
>>>>>>>>>>>>>>>>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Switch to DFLL:
>>>>>>>>>>>>>>>>>>>>>>> 0) Save current parent and frequency
>>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>>>> 2) Enable DFLL
>>>>>>>>>>>>>>>>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>>>>>>>>>>>>>>>>> For OVR regulator:
>>>>>>>>>>>>>>>>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>>>>>>>>>>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>>>>>>>>>>>>>>>>> For I2C regulator:
>>>>>>>>>>>>>>>>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>>>>>>>>>>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Switch away from DFLL:
>>>>>>>>>>>>>>>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU
>>>>>>>>>>>>>>>>>>>>>>> frequency is ok for
>>>>>>>>>>>>>>>>>>>>>>> any
>>>>>>>>>>>>>>>>>>>>>>> vdd_cpu voltage
>>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> I see during switch away from DFLL (suspend), cclk_g
>>>>>>>>>>>>>>>>>>>> parent is not
>>>>>>>>>>>>>>>>>>>> changed to PLLP before changing dfll to open loop
>>>>>>>>>>>>>>>>>>>> mode.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Will add this ...
>>>>>>>>>>>>>>>>>>> The CPUFreq driver switches parent to PLLP during the
>>>>>>>>>>>>>>>>>>> probe, similar
>>>>>>>>>>>>>>>>>>> should be done on suspend.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I'm also wondering if it's always safe to switch to
>>>>>>>>>>>>>>>>>>> PLLP in the probe.
>>>>>>>>>>>>>>>>>>> If CPU is running on a lower freq than PLLP, then some
>>>>>>>>>>>>>>>>>>> other more
>>>>>>>>>>>>>>>>>>> appropriate intermediate parent should be selected.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always
>>>>>>>>>>>>>>>>>> runs at higher
>>>>>>>>>>>>>>>>>> rate
>>>>>>>>>>>>>>>>>> so switching to PLL_P during CPUFreq probe prior to
>>>>>>>>>>>>>>>>>> dfll clock enable
>>>>>>>>>>>>>>>>>> should be safe.
>>>>>>>>>>>>>>>>> AFAIK, PLLX could run at ~200MHz. There is also a
>>>>>>>>>>>>>>>>> divided output of
>>>>>>>>>>>>>>>>> PLLP
>>>>>>>>>>>>>>>>> which CCLKG supports, the PLLP_OUT4.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Probably, realistically, CPU is always running off a
>>>>>>>>>>>>>>>>> fast PLLX during
>>>>>>>>>>>>>>>>> boot, but I'm wondering what may happen on KEXEC. I
>>>>>>>>>>>>>>>>> guess ideally CPUFreq driver should also have a
>>>>>>>>>>>>>>>>> 'shutdown' callback to teardown DFLL
>>>>>>>>>>>>>>>>> on a reboot, but likely that there are other
>>>>>>>>>>>>>>>>> clock-related problems as
>>>>>>>>>>>>>>>>> well that may break KEXEC and thus it is not very
>>>>>>>>>>>>>>>>> important at the
>>>>>>>>>>>>>>>>> moment.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> [snip]
>>>>>>>>>>>>>>>> During bootup CPUG sources from PLL_X. By PLL_P source
>>>>>>>>>>>>>>>> above I meant
>>>>>>>>>>>>>>>> PLL_P_OUT4.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> As per clock policies, PLL_X is always used for high freq
>>>>>>>>>>>>>>>> like
>>>>>>>>>>>>>>>>> 800Mhz
>>>>>>>>>>>>>>>> and for low frequency it will be sourced from PLLP.
>>>>>>>>>>>>>>> Alright, then please don't forget to pre-initialize
>>>>>>>>>>>>>>> PLLP_OUT4 rate to a
>>>>>>>>>>>>>>> reasonable value using tegra_clk_init_table or
>>>>>>>>>>>>>>> assigned-clocks.
>>>>>>>>>>>>>> PLLP_OUT4 rate update is not needed as it is safe to run at
>>>>>>>>>>>>>> 408Mhz because it is below fmax @ Vmin
>>>>>>>>>>>>> So even 204MHz CVB entries are having the same voltage as
>>>>>>>>>>>>> 408MHz, correct? It's not instantly obvious to me from the
>>>>>>>>>>>>> DFLL driver's code where the fmax @ Vmin is defined, I see
>>>>>>>>>>>>> that there is the min_millivolts
>>>>>>>>>>>>> and frequency entries starting from 204MHZ defined
>>>>>>>>>>>>> per-table.
>>>>>>>>>>>> Yes at Vmin CPU Fmax is ~800Mhz. So anything below that will
>>>>>>>>>>>> work at Vmin voltage and PLLP max is 408Mhz.
>>>>>>>>>>> Thank you for the clarification. It would be good to have that
>>>>>>>>>>> commented
>>>>>>>>>>> in the code as well.
>>>>>>>>>> OK, Will add...
>>>>>>>>> Regarding, adding suspend/resume to CPUFreq, CPUFreq suspend
>>>>>>>>> happens very early even before disabling non-boot CPUs and also
>>>>>>>>> need to export clock driver APIs to CPUFreq.
>>>>>>>>>
>>>>>>>>> Was thinking of below way of implementing this...
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Clock DFLL driver Suspend:
>>>>>>>>>
>>>>>>>>> - Save CPU clock policy registers, and Perform dfll
>>>>>>>>> suspend which sets in open loop mode
>>>>>>>>>
>>>>>>>>> CPU Freq driver Suspend: does nothing
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Clock DFLL driver Resume:
>>>>>>>>>
>>>>>>>>> - Re-init DFLL, Set in Open-Loop mode, restore CPU
>>>>>>>>> Clock policy registers which actually sets source to DFLL along
>>>>>>>>> with other CPU Policy register restore.
>>>>>>>>>
>>>>>>>>> CPU Freq driver Resume:
>>>>>>>>>
>>>>>>>>> - do clk_prepare_enable which acutally sets DFLL in
>>>>>>>>> Closed loop mode
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Adding one more note: Switching CPU Clock to PLLP is not needed
>>>>>>>>> as CPU CLock can be from dfll in open-loop mode as DFLL is not
>>>>>>>>> disabled anywhere throught the suspend/resume path and SC7 entry
>>>>>>>>> FW and Warm boot code will switch CPU source to PLLP.
>>>>>>> Since CPU resumes on PLLP, it will be cleaner to suspend it on
>>>>>>> PLLP as well. And besides, seems that currently disabling DFLL
>>>>>>> clock will disable DFLL completely and then you'd want to re-init
>>>>>>> the DFLL on resume any ways. So better to just disable DFLL
>>>>>>> completely on suspend, which should happen on clk_disable(dfll).
>>>>>> Will switch to PLLP during CPUFreq suspend. With decision of using
>>>>>> clk_disable during suspend, its mandatory to switch to PLLP as DFLL
>>>>>> is completely disabled.
>>>>>>
>>>>>> My earlier concern was on restoring CPU policy as we can't do that
>>>>>> from CPUFreq driver and need export from clock driver.
>>>>>>
>>>>>> Clear now and will do CPU clock policy restore in after dfll
>>>>>> re-init.
>>>>> Why the policy can't be saved/restored by the CaR driver as a
>>>>> context of any other clock?
>>>> restoring cpu clock policy involves programming source and
>>>> super_cclkg_divider.
>>>>
>>>> cclk_g is registered as clk_super_mux and it doesn't use frac_div ops
>>>> to do save/restore its divider.
>>> That can be changed of course and I guess it also could be as simple as
>>> saving and restoring of two raw u32 values of the policy/divider
>>> registers.
>>>
>>>> Also, during clock context we cant restore cclk_g as cclk_g source
>>>> will be dfll and dfll will not be resumed/re-initialized by the time
>>>> clk_super_mux save/restore happens.
>>>>
>>>> we can't use save/restore context for dfll clk_ops because
>>>> dfllCPU_out parent to CCLK_G is first in the clock tree and dfll_ref
>>>> and dfll_soc peripheral clocks are not restored by the time dfll
>>>> restore happens. Also dfll peripheral clock enables need to be
>>>> restored before dfll restore happens which involves programming dfll
>>>> controller for re-initialization.
>>>>
>>>> So dfll resume/re-init is done in clk-tegra210 at end of all clocks
>>>> restore in V5 series but instead of in clk-tegra210 driver I moved
>>>> now to dfll-fcpu driver pm_ops as all dfll dependencies will be
>>>> restored thru clk_restore_context by then. This will be in V6.
>>> Since DFLL is now guaranteed to be disabled across CaR suspend/resume
>>> (hence it has nothing to do in regards to CCLK) and given that PLLs
>>> state is restored before the rest of the clocks, I don't see why not to
>>> implement CCLK save/restore in a generic fasion. CPU policy wull be
>>> restored to either PLLP or PLLX (if CPUFreq driver is disabled).
>>>
>> CCLK_G save/restore should happen in clk_super_mux ops save/context and
>> clk_super_mux save/restore happens very early as cclk_g is first in the
>> clock tree and save/restore traverses through the tree top-bottom order.
> If CCLK_G is restored before the PLLs, then just change the clocks order
> such that it won't happen.
>
I dont think we can change clocks order for CCLK_G.
During bootup, cclk_g is registered after all pll's and peripheral
clocks which is the way we wanted, So cclk_g will be the first one in
the clk list as clk_register adds new clock first in the list.
When clk_save_context and clk_restore_context APIs iterates over the
list, cclk_g is the first
>> DFLL enable thru CPUFreq resume happens after all clk_restore_context
>> happens. So during clk_restore_context, dfll re-init doesnt happen and
>> doing cpu clock policy restore during super_mux clk_ops will crash as
>> DFLL is not initialized and its clock is not enabled but CPU clock
>> restore sets source to DFLL if we restore during super_clk_mux
> If CPU was suspended on PLLP, then it will be restored on PLLP by CaR. I
> don't understand what DFLL has to do with the CCLK in that case during
> the clocks restore.
My above comment is in reference to your request of doing save/restore
for cclk_g in normal fashion thru save/restore context. Because of the
clk order I mentioned above, we cclk_g will be the first one to go thru
save/context.
During save_context of cclk_g, source can be from PLLX, dfll.
Issue will be when we do restore during clk_restore_context of cclk_g as
by that time PLLX/dfll will not be restored.
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
From: Sudeep Holla @ 2019-07-17 17:28 UTC (permalink / raw)
To: Peng Fan
Cc: robh+dt@kernel.org, mark.rutland@arm.com,
jassisinghbrar@gmail.com, andre.przywara@arm.com,
f.fainelli@gmail.com, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, dl-linux-imx, Sudeep Holla
In-Reply-To: <1563184103-8493-2-git-send-email-peng.fan@nxp.com>
This looks much better now.
On Mon, Jul 15, 2019 at 10:10:10AM +0000, Peng Fan wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> The ARM SMC/HVC mailbox binding describes a firmware interface to trigger
> actions in software layers running in the EL2 or EL3 exception levels.
> The term "ARM" here relates to the SMC instruction as part of the ARM
> instruction set, not as a standard endorsed by ARM Ltd.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>
> V3:
> Convert to yaml
> Drop interrupt
> Introudce transports to indicate mem/reg
> The func id is still kept as optional, because like SCMI it only
> cares about message.
>
> V2:
> Introduce interrupts as a property.
>
> .../devicetree/bindings/mailbox/arm-smc.yaml | 124 +++++++++++++++++++++
> 1 file changed, 124 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
>
> diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> new file mode 100644
> index 000000000000..da9b1a03bc4e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> @@ -0,0 +1,124 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ARM SMC Mailbox Interface
> +
> +maintainers:
> + - Peng Fan <peng.fan@nxp.com>
> +
> +description: |
> + This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor
> + call) instruction to trigger a mailbox-connected activity in firmware,
> + executing on the very same core as the caller. By nature this operation
> + is synchronous and this mailbox provides no way for asynchronous messages
> + to be delivered the other way round, from firmware to the OS, but
> + asynchronous notification could also be supported. However the value of
> + r0/w0/x0 the firmware returns after the smc call is delivered as a received
> + message to the mailbox framework, so a synchronous communication can be
> + established, for a asynchronous notification, no value will be returned.
> + The exact meaning of both the action the mailbox triggers as well as the
> + return value is defined by their users and is not subject to this binding.
> +
> + One use case of this mailbox is the SCMI interface, which uses shared memory
> + to transfer commands and parameters, and a mailbox to trigger a function
> + call. This allows SoCs without a separate management processor (or when
> + such a processor is not available or used) to use this standardized
> + interface anyway.
> +
> + This binding describes no hardware, but establishes a firmware interface.
> + Upon receiving an SMC using one of the described SMC function identifiers,
> + the firmware is expected to trigger some mailbox connected functionality.
> + The communication follows the ARM SMC calling convention.
> + Firmware expects an SMC function identifier in r0 or w0. The supported
> + identifiers are passed from consumers, or listed in the the arm,func-ids
> + properties as described below. The firmware can return one value in
> + the first SMC result register, it is expected to be an error value,
> + which shall be propagated to the mailbox client.
> +
> + Any core which supports the SMC or HVC instruction can be used, as long as
> + a firmware component running in EL3 or EL2 is handling these calls.
> +
> +properties:
> + compatible:
> + const: arm,smc-mbox
> +
> + "#mbox-cells":
> + const: 1
> +
> + arm,num-chans:
> + description: The number of channels supported.
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + method:
> + items:
> + - enum:
> + - smc
> + - hvc
> +
> + transports:
> + items:
> + - enum:
> + - mem
> + - reg
> +
> + arm,func-ids:
> + description: |
> + An array of 32-bit values specifying the function IDs used by each
> + mailbox channel. Those function IDs follow the ARM SMC calling
> + convention standard [1].
> +
> + There is one identifier per channel and the number of supported
> + channels is determined by the length of this array.
> + minItems: 0
> + maxItems: 4096 # Should be enough?
I am new to yaml, is there a way to say the number of entries here must
match arm,num-chans ? And not sure if min/maxItems matter then ?
> +
> +required:
> + - compatible
> + - "#mbox-cells"
> + - arm,num-chans
> + - transports
> + - method
> +
Why is arm,func-ids optional ? Is there any standard arm,func-ids we can
resort to. Sorry I know you expect ARM Ltd to answer that, but I just want
to raise the point that we don't have one today and hence it can't be
optional. Or I am missing something ?
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH 2/6] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts
From: David Lechner @ 2019-07-17 17:21 UTC (permalink / raw)
To: Suman Anna, Marc Zyngier, Rob Herring, Thomas Gleixner,
Jason Cooper
Cc: Tony Lindgren, Andrew F. Davis, Roger Quadros, Lokesh Vutla,
Grygorii Strashko, Sekhar Nori, Murali Karicheri, devicetree,
linux-omap, linux-arm-kernel, linux-kernel
In-Reply-To: <54e871a8-dfa4-f65b-5869-d1661144cbf0@ti.com>
On 7/16/19 12:21 PM, Suman Anna wrote:
>>> +static int pruss_intc_probe(struct platform_device *pdev)
>>> +{
>>> + static const char * const irq_names[] = {
>>> + "host0", "host1", "host2", "host3",
>>> + "host4", "host5", "host6", "host7", };
>>> + struct device *dev = &pdev->dev;
>>> + struct pruss_intc *intc;
>>> + struct resource *res;
>>> + struct irq_chip *irqchip;
>>> + int i, irq;
>>> +
>>> + intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);
>>> + if (!intc)
>>> + return -ENOMEM;
>>> + platform_set_drvdata(pdev, intc);
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + intc->base = devm_ioremap_resource(dev, res);
>>> + if (IS_ERR(intc->base)) {
>>> + dev_err(dev, "failed to parse and map intc memory resource\n");
>>> + return PTR_ERR(intc->base);
>>> + }
>>> +
>>> + dev_dbg(dev, "intc memory: pa %pa size 0x%zx va %pK\n", &res->start,
>>> + (size_t)resource_size(res), intc->base);
>>> +
>>> + mutex_init(&intc->lock);
>>> +
>>> + pruss_intc_init(intc);
>>> +
>>> + irqchip = devm_kzalloc(dev, sizeof(*irqchip), GFP_KERNEL);
>>> + if (!irqchip)
>>> + return -ENOMEM;
>>> +
>>> + irqchip->irq_ack = pruss_intc_irq_ack;
>>> + irqchip->irq_mask = pruss_intc_irq_mask;
>>> + irqchip->irq_unmask = pruss_intc_irq_unmask;
>>> + irqchip->irq_retrigger = pruss_intc_irq_retrigger;
>>> + irqchip->irq_request_resources = pruss_intc_irq_reqres;
>>> + irqchip->irq_release_resources = pruss_intc_irq_relres;
>>> + irqchip->name = dev_name(dev);
>>
>> Should we also set `irqchip->parent_device = dev;` here?
>>
>> I tried it and had to add pm runtime stuff as well, otherwise
>> requesting irqs would fail.
>
> I haven't seen any during my local testing. What sort of failure are you
> seeing?
>
> The clocking for the overall PRUSS module will be handled in either the
> ti-sysc driver for OMAP SoCs or in the pruss platform driver.
>
I was getting -EACCESS bubbling up from rpm_resume() in drivers/base/
power/runtime.c. It was probably a mix of how I set up the device tree
and the dummy PRUSS bus driver I made.
I'm sure it will be fine with a proper PRUSS platform driver.
^ permalink raw reply
* Re: [PATCHv8 3/5] arm64: dts: qcom: msm8996: Add Coresight support
From: Mathieu Poirier @ 2019-07-17 17:00 UTC (permalink / raw)
To: Sai Prakash Ranjan
Cc: Greg Kroah-Hartman, Suzuki K Poulose, Leo Yan, Alexander Shishkin,
Mike Leach, Rob Herring, Bjorn Andersson, devicetree, David Brown,
Mark Rutland, Rajendra Nayak, Vivek Gautam, Sibi Sankar,
linux-arm-kernel, linux-kernel, linux-arm-msm, Marc Gonzalez
In-Reply-To: <2fa725fbc09306f1a95befc62715a708b4c0fad0.1562940244.git.saiprakash.ranjan@codeaurora.org>
On Fri, Jul 12, 2019 at 07:46:25PM +0530, Sai Prakash Ranjan wrote:
> From: Vivek Gautam <vivek.gautam@codeaurora.org>
>
> Enable coresight support by adding device nodes for the
> available source, sinks and channel blocks on msm8996.
>
> This also adds coresight cpu debug nodes.
>
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Acked-By: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> arch/arm64/boot/dts/qcom/msm8996.dtsi | 434 ++++++++++++++++++++++++++
> 1 file changed, 434 insertions(+)
>
We've gone trhough 8 iteration of this set and I'm still finding checkpatch
problems, and I'm not referring to lines over 80 characters.
> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> index 96c0a481f454..8968431e772c 100644
> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> @@ -633,6 +633,440 @@
> reg = <0x300000 0x90000>;
> };
>
> + stm@3002000 {
> + compatible = "arm,coresight-stm", "arm,primecell";
> + reg = <0x3002000 0x1000>,
> + <0x8280000 0x180000>;
> + reg-names = "stm-base", "stm-stimulus-base";
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> + clock-names = "apb_pclk", "atclk";
> +
> + out-ports {
> + port {
> + stm_out: endpoint {
> + remote-endpoint =
> + <&funnel0_in>;
> + };
> + };
> + };
> + };
> +
> + tpiu@3020000 {
> + compatible = "arm,coresight-tpiu", "arm,primecell";
> + reg = <0x3020000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> + clock-names = "apb_pclk", "atclk";
> +
> + in-ports {
> + port {
> + tpiu_in: endpoint {
> + remote-endpoint =
> + <&replicator_out1>;
> + };
> + };
> + };
> + };
> +
> + funnel@3021000 {
> + compatible = "arm,coresight-funnel", "arm,primecell";
> + reg = <0x3021000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> + clock-names = "apb_pclk", "atclk";
> +
> + in-ports {
> + port {
> + funnel0_in: endpoint {
> + remote-endpoint =
> + <&stm_out>;
> + };
> + };
> + };
> +
> + out-ports {
> + port {
> + funnel0_out: endpoint {
> + remote-endpoint =
> + <&merge_funnel_in0>;
> + };
> + };
> + };
> + };
> +
> + funnel@3022000 {
> + compatible = "arm,coresight-funnel", "arm,primecell";
> + reg = <0x3022000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> + clock-names = "apb_pclk", "atclk";
> +
> + in-ports {
> + port {
> + funnel1_in: endpoint {
> + remote-endpoint =
> + <&apss_merge_funnel_out>;
> + };
> + };
> + };
> +
> + out-ports {
> + port {
> + funnel1_out: endpoint {
> + remote-endpoint =
> + <&merge_funnel_in1>;
> + };
> + };
> + };
> + };
> +
> + funnel@3025000 {
> + compatible = "arm,coresight-funnel", "arm,primecell";
> + reg = <0x3025000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> + clock-names = "apb_pclk", "atclk";
> +
> + in-ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + merge_funnel_in0: endpoint {
> + remote-endpoint =
> + <&funnel0_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + merge_funnel_in1: endpoint {
> + remote-endpoint =
> + <&funnel1_out>;
> + };
> + };
> + };
> +
> + out-ports {
> + port {
> + merge_funnel_out: endpoint {
> + remote-endpoint =
> + <&etf_in>;
> + };
> + };
> + };
> + };
> +
> + replicator@3026000 {
> + compatible = "arm,coresight-dynamic-replicator", "arm,primecell";
> + reg = <0x3026000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> + clock-names = "apb_pclk", "atclk";
> +
> + in-ports {
> + port {
> + replicator_in: endpoint {
> + remote-endpoint =
> + <&etf_out>;
> + };
> + };
> + };
> +
> + out-ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + replicator_out0: endpoint {
> + remote-endpoint =
> + <&etr_in>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + replicator_out1: endpoint {
> + remote-endpoint =
> + <&tpiu_in>;
> + };
> + };
> + };
> + };
> +
> + etf@3027000 {
> + compatible = "arm,coresight-tmc", "arm,primecell";
> + reg = <0x3027000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> + clock-names = "apb_pclk", "atclk";
> +
> + in-ports {
> + port {
> + etf_in: endpoint {
> + remote-endpoint =
> + <&merge_funnel_out>;
> + };
> + };
> + };
> +
> + out-ports {
> + port {
> + etf_out: endpoint {
> + remote-endpoint =
> + <&replicator_in>;
> + };
> + };
> + };
> + };
> +
> + etr@3028000 {
> + compatible = "arm,coresight-tmc", "arm,primecell";
> + reg = <0x3028000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> + clock-names = "apb_pclk", "atclk";
> + arm,scatter-gather;
> +
> + in-ports {
> + port {
> + etr_in: endpoint {
> + remote-endpoint =
> + <&replicator_out0>;
> + };
> + };
> + };
> + };
> +
> + debug@3810000 {
> + compatible = "arm,coresight-cpu-debug", "arm,primecell";
> + reg = <0x3810000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>;
> + clock-names = "apb_pclk";
> +
> + cpu = <&CPU0>;
> + };
> +
> + etm@3840000 {
> + compatible = "arm,coresight-etm4x", "arm,primecell";
> + reg = <0x3840000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> + clock-names = "apb_pclk", "atclk";
> +
> + cpu = <&CPU0>;
> +
> + out-ports {
> + port {
> + etm0_out: endpoint {
> + remote-endpoint =
> + <&apss_funnel0_in0>;
> + };
> + };
> + };
> + };
> +
> + debug@3910000 {
> + compatible = "arm,coresight-cpu-debug", "arm,primecell";
> + reg = <0x3910000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>;
> + clock-names = "apb_pclk";
> +
> + cpu = <&CPU1>;
> + };
> +
> + etm@3940000 {
> + compatible = "arm,coresight-etm4x", "arm,primecell";
> + reg = <0x3940000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> + clock-names = "apb_pclk", "atclk";
> +
> + cpu = <&CPU1>;
> +
> + out-ports {
> + port {
> + etm1_out: endpoint {
> + remote-endpoint =
> + <&apss_funnel0_in1>;
> + };
> + };
> + };
> + };
> +
> + funnel@39b0000 { /* APSS Funnel 0 */
> + compatible = "arm,coresight-funnel", "arm,primecell";
> + reg = <0x39b0000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> + clock-names = "apb_pclk", "atclk";
> +
> + in-ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + apss_funnel0_in0: endpoint {
> + remote-endpoint = <&etm0_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + apss_funnel0_in1: endpoint {
> + remote-endpoint = <&etm1_out>;
> + };
> + };
> + };
> +
> + out-ports {
> + port {
> + apss_funnel0_out: endpoint {
> + remote-endpoint =
> + <&apss_merge_funnel_in0>;
> + };
> + };
> + };
> + };
> +
> + debug@3a10000 {
> + compatible = "arm,coresight-cpu-debug", "arm,primecell";
> + reg = <0x3a10000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>;
> + clock-names = "apb_pclk";
> +
> + cpu = <&CPU2>;
> + };
> +
> + etm@3a40000 {
> + compatible = "arm,coresight-etm4x", "arm,primecell";
> + reg = <0x3a40000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> + clock-names = "apb_pclk", "atclk";
> +
> + cpu = <&CPU2>;
> +
> + out-ports {
> + port {
> + etm2_out: endpoint {
> + remote-endpoint =
> + <&apss_funnel1_in0>;
> + };
> + };
> + };
> + };
> +
> + debug@3b10000 {
> + compatible = "arm,coresight-cpu-debug", "arm,primecell";
> + reg = <0x3b10000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>;
> + clock-names = "apb_pclk";
> +
> + cpu = <&CPU3>;
> + };
> +
> + etm@3b40000 {
> + compatible = "arm,coresight-etm4x", "arm,primecell";
> + reg = <0x3b40000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> + clock-names = "apb_pclk", "atclk";
> +
> + cpu = <&CPU3>;
> +
> + out-ports {
> + port {
> + etm3_out: endpoint {
> + remote-endpoint =
> + <&apss_funnel1_in1>;
> + };
> + };
> + };
> + };
> +
> + funnel@3bb0000 { /* APSS Funnel 1 */
> + compatible = "arm,coresight-funnel", "arm,primecell";
> + reg = <0x3bb0000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> + clock-names = "apb_pclk", "atclk";
> +
> + in-ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + apss_funnel1_in0: endpoint {
> + remote-endpoint = <&etm2_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + apss_funnel1_in1: endpoint {
> + remote-endpoint = <&etm3_out>;
> + };
> + };
> + };
> +
> + out-ports {
> + port {
> + apss_funnel1_out: endpoint {
> + remote-endpoint =
> + <&apss_merge_funnel_in1>;
> + };
> + };
> + };
> + };
> +
> + funnel@3bc0000 {
> + compatible = "arm,coresight-funnel", "arm,primecell";
> + reg = <0x3bc0000 0x1000>;
> +
> + clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> + clock-names = "apb_pclk", "atclk";
> +
> + in-ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + apss_merge_funnel_in0: endpoint {
> + remote-endpoint =
> + <&apss_funnel0_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + apss_merge_funnel_in1: endpoint {
> + remote-endpoint =
> + <&apss_funnel1_out>;
> + };
> + };
> + };
> +
> + out-ports {
> + port {
> + apss_merge_funnel_out: endpoint {
> + remote-endpoint =
> + <&funnel1_in>;
> + };
> + };
> + };
> + };
> +
> kryocc: clock-controller@6400000 {
> compatible = "qcom,apcc-msm8996";
> reg = <0x6400000 0x90000>;
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
^ permalink raw reply
* [PATCHv2] MIPS: JZ4780: DTS: Add I2C nodes Ralf Baechle <ralf@linux-mips.org>,
From: Alexandre GRIVEAUX @ 2019-07-17 17:00 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Alexandre GRIVEAUX
Cc: linux-mips, linux-kernel, devicetree
Add the devicetree nodes for the I2C core of the JZ4780 SoC, disabled
by default.
---
arch/mips/boot/dts/ingenic/jz4780.dtsi | 86 ++++++++++++++++++++++++++
1 file changed, 86 insertions(+)
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index b03cdec56de9..a76ecd69bfd0 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -239,6 +239,92 @@
status = "disabled";
};
+ i2c0: i2c@10050000 {
+ compatible = "ingenic,jz4780-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ reg = <0x10050000 0x1000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <60>;
+
+ clocks = <&cgu JZ4780_CLK_SMB0>;
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_i2c0_data>;
+
+ status = "disabled";
+ };
+
+ i2c1: i2c@10051000 {
+ compatible = "ingenic,jz4780-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x10051000 0x1000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <59>;
+
+ clocks = <&cgu JZ4780_CLK_SMB1>;
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_i2c1_data>;
+
+ status = "disabled";
+ };
+
+ i2c2: i2c@10052000 {
+ compatible = "ingenic,jz4780-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x10052000 0x1000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <58>;
+
+ clocks = <&cgu JZ4780_CLK_SMB2>;
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_i2c2_data>;
+
+ status = "disabled";
+ };
+
+ i2c3: i2c@10053000 {
+ compatible = "ingenic,jz4780-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x10053000 0x1000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <57>;
+
+ clocks = <&cgu JZ4780_CLK_SMB3>;
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_i2c3_data>;
+
+ status = "disabled";
+ };
+
+ i2c4: i2c@10054000 {
+ compatible = "ingenic,jz4780-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x10054000 0x1000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <56>;
+
+ clocks = <&cgu JZ4780_CLK_SMB4>;
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_i2c4_data>;
+
+ status = "disabled";
+ };
+
watchdog: watchdog@10002000 {
compatible = "ingenic,jz4780-watchdog";
reg = <0x10002000 0x10>;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v1 27/50] ARM: dts: exynos: align bus_wcore OPPs in Exynos5420
From: Lukasz Luba @ 2019-07-17 16:58 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc@vger.kernel.org, linux-clk, mturquette, sboyd,
Bartłomiej Żołnierkiewicz, kgene, mark.rutland,
robh+dt, Chanwoo Choi, kyungmin.park, Andrzej Hajda,
Marek Szyprowski, s.nawrocki, myungjoo.ham
In-Reply-To: <CAJKOXPfFZL8q9hM1vPsLq+Qxe-gMz4c8j0jgFKfdf5qs68MTmA@mail.gmail.com>
Hi Krzysztof,
On 7/17/19 12:15 PM, Krzysztof Kozlowski wrote:
> On Mon, 15 Jul 2019 at 14:44, Lukasz Luba <l.luba@partner.samsung.com> wrote:
>>
>> This is the most important bus in the Exynos5x SoC. The whole communication
>> inside SoC does through that bus (apart from direct requests from CCI to
>> DRAM controller). It is also modeled as a master bus in devfreq framework.
>> It is also the only one OPP table throughout other buses which has voltage
>> values. The devfreq software controls the speed of that bus and other
>> buses. The other buses follows the rate of the master. There is only one
>> regulator. The old lowest OPP had pair 925mV, 84MHz which is enough for
>
> s/lowest/slowest/
please see below
>
>> this frequency. However, due to the fact that the other buses follows the
>> WCORE bus by taking the OPP from their table with the same id, e.g. opp02,
>> the children frequency should be stable with the set voltage.
>> It could cause random faults very hard to debug.
>> Thus, the patch removes the lowest OPP to make other buses' lowest OPPs
>
> s/lowest/slowest/
Actually, I have double checked that, because we always used this
terminology: low OPP, high OPP, lower OPPs, higher OPPs. I can change
it here for you, but I think this is not something that people are used
to. Please check EAS pdf documentation or this file:
https://www.kernel.org/doc/Documentation/scheduler/sched-energy.txt
i.e. "running at a lower OPP" or "high OPPs", "lowest OPPs".
Regards,
Lukasz
>
>> working. The new lowest OPP has voltage high enough for buses working up
>> to 333MHz. It also changes the frequencies of the OPPs to align them to
>> PLL value such that it is possible to set them using only a divider without
>> reprogramming OPP.
>
> Reprogramming OPP? What is it?
>
>> Reprogramming the PLL was not set, so the real frequency
>
> I understood from the previous that reprogramming the OPP (PLL?) was
> happening... Please rephrase entire sentence.
>
> BR,
> Krzysztof
>
>> values were not the one from the OPP table, which could confuse the
>> governor algorithms which relay on OPP speed values making the system to
>> behave weird.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>> arch/arm/boot/dts/exynos5420.dtsi | 12 ++++--------
>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
>> index f8c36ff0d4c3..a355c76af5a5 100644
>> --- a/arch/arm/boot/dts/exynos5420.dtsi
>> +++ b/arch/arm/boot/dts/exynos5420.dtsi
>> @@ -1107,22 +1107,18 @@
>> compatible = "operating-points-v2";
>>
>> opp00 {
>> - opp-hz = /bits/ 64 <84000000>;
>> - opp-microvolt = <925000>;
>> + opp-hz = /bits/ 64 <150000000>;
>> + opp-microvolt = <950000>;
>> };
>> opp01 {
>> - opp-hz = /bits/ 64 <111000000>;
>> + opp-hz = /bits/ 64 <200000000>;
>> opp-microvolt = <950000>;
>> };
>> opp02 {
>> - opp-hz = /bits/ 64 <222000000>;
>> + opp-hz = /bits/ 64 <300000000>;
>> opp-microvolt = <950000>;
>> };
>> opp03 {
>> - opp-hz = /bits/ 64 <333000000>;
>> - opp-microvolt = <950000>;
>> - };
>> - opp04 {
>> opp-hz = /bits/ 64 <400000000>;
>> opp-microvolt = <987500>;
>> };
>> --
>> 2.17.1
>>
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox