* Re: [PATCH v4, 22/33] drm/mediatek: add background color input select function for ovl/ovl_2l
From: CK Hu @ 2019-07-17 5:58 UTC (permalink / raw)
To: yongqiang.niu
Cc: Philipp Zabel, Rob Herring, Matthias Brugger, David Airlie,
Daniel Vetter, Mark Rutland, dri-devel, devicetree, linux-kernel,
linux-arm-kernel, linux-mediatek
In-Reply-To: <1562625253-29254-23-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Tue, 2019-07-09 at 06:34 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> This patch add background color input select function for ovl/ovl_2l
>
> ovl include 4 DRAM layer and 1 background color layer
> ovl_2l include 4 DRAM layer and 1 background color layer
> DRAM layer frame buffer data from render hardware, GPU for example.
> backgournd color layer is embed in ovl/ovl_2l, we can only set
> it color, but not support DRAM frame buffer.
>
> for ovl0->ovl0_2l direct link usecase,
> we need set ovl0_2l background color intput select from ovl0
> if render send DRAM buffer layer number <=4, all these layer read
> by ovl.
> layer0 is at the bottom of all layers.
> layer3 is at the top of all layers.
> if render send DRAM buffer layer numbfer >=4 && <=6
> ovl0 read layer0~3
> ovl0_2l read layer4~5
> layer5 is at the top ot all these layers.
>
> the decision of how to setting ovl0/ovl0_2l read these layer data
> is controlled in mtk crtc, which will be another patch
>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index a0ab760..8ca4965 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -27,6 +27,8 @@
> #define DISP_REG_OVL_EN 0x000c
> #define DISP_REG_OVL_RST 0x0014
> #define DISP_REG_OVL_ROI_SIZE 0x0020
> +#define DISP_REG_OVL_DATAPATH_CON 0x0024
> +#define OVL_BGCLR_SEL_IN BIT(2)
> #define DISP_REG_OVL_ROI_BGCLR 0x0028
> #define DISP_REG_OVL_SRC_CON 0x002c
> #define DISP_REG_OVL_CON(n) (0x0030 + 0x20 * (n))
> @@ -245,6 +247,24 @@ static void mtk_ovl_layer_config(struct mtk_ddp_comp *comp, unsigned int idx,
> mtk_ovl_layer_on(comp, idx);
> }
>
> +static void mtk_ovl_bgclr_in_on(struct mtk_ddp_comp *comp)
> +{
> + unsigned int reg;
> +
> + reg = readl(comp->regs + DISP_REG_OVL_DATAPATH_CON);
> + reg = reg | OVL_BGCLR_SEL_IN;
> + writel(reg, comp->regs + DISP_REG_OVL_DATAPATH_CON);
> +}
> +
> +static void mtk_ovl_bgclr_in_off(struct mtk_ddp_comp *comp)
> +{
> + unsigned int reg;
> +
> + reg = readl(comp->regs + DISP_REG_OVL_DATAPATH_CON);
> + reg = reg & ~OVL_BGCLR_SEL_IN;
> + writel(reg, comp->regs + DISP_REG_OVL_DATAPATH_CON);
> +}
> +
> static const struct mtk_ddp_comp_funcs mtk_disp_ovl_funcs = {
> .config = mtk_ovl_config,
> .start = mtk_ovl_start,
> @@ -255,6 +275,8 @@ static void mtk_ovl_layer_config(struct mtk_ddp_comp *comp, unsigned int idx,
> .layer_on = mtk_ovl_layer_on,
> .layer_off = mtk_ovl_layer_off,
> .layer_config = mtk_ovl_layer_config,
> + .bgclr_in_on = mtk_ovl_bgclr_in_on,
> + .bgclr_in_off = mtk_ovl_bgclr_in_off,
> };
>
> static int mtk_disp_ovl_bind(struct device *dev, struct device *master,
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 5:55 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: <20190717084221.2e9af56c@dimatab>
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.
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.
>> Also I don't see Tegra124 CPU Freq driver using flag
>> CPUFREQ_NEED_INITIAL_FREQ_CHECK.
>>
>> Tegra124 CPUFreq driver is not using cpufreq_driver
>>
>>
>> <https://elixir.bootlin.com/linux/v5.2.1/ident/cpufreq_driver>
>>
> T124 driver is a wrapper around cpufreq-dt driver.
^ permalink raw reply
* Re: [PATCH v4, 21/33] drm/mediatek: add function to background color input select for ovl/ovl_2l direct link
From: CK Hu @ 2019-07-17 5:53 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, David Airlie, linux-kernel, dri-devel,
Rob Herring, linux-mediatek, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1562625253-29254-22-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Tue, 2019-07-09 at 06:34 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> This patch add function to background color input select for ovl/ovl_2l direct link
> for ovl/ovl_2l direct link usecase, we need set background color
> input select for these hardware.
> this is preparation patch for ovl/ovl_2l usecase
>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> index a0ea8c9..ec6f329a 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> @@ -92,6 +92,8 @@ struct mtk_ddp_comp_funcs {
> struct mtk_plane_state *state);
> void (*gamma_set)(struct mtk_ddp_comp *comp,
> struct drm_crtc_state *state);
> + void (*bgclr_in_on)(struct mtk_ddp_comp *comp);
> + void (*bgclr_in_off)(struct mtk_ddp_comp *comp);
> };
>
> struct mtk_ddp_comp {
> @@ -172,6 +174,18 @@ static inline void mtk_ddp_gamma_set(struct mtk_ddp_comp *comp,
> comp->funcs->gamma_set(comp, state);
> }
>
> +static inline void mtk_ddp_comp_bgclr_in_on(struct mtk_ddp_comp *comp)
> +{
> + if (comp->funcs && comp->funcs->bgclr_in_on)
> + comp->funcs->bgclr_in_on(comp);
> +}
> +
> +static inline void mtk_ddp_comp_bgclr_in_off(struct mtk_ddp_comp *comp)
> +{
> + if (comp->funcs && comp->funcs->bgclr_in_off)
> + comp->funcs->bgclr_in_off(comp);
> +}
> +
> int mtk_ddp_comp_get_id(struct device_node *node,
> enum mtk_ddp_comp_type comp_type);
> int mtk_ddp_comp_init(struct device *dev, struct device_node *comp_node,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v4, 13/33] drm/mediatek: add mmsys private data for ddp path config
From: CK Hu @ 2019-07-17 5:46 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, Philipp Zabel, David Airlie,
linux-kernel, dri-devel, Rob Herring, linux-mediatek,
Daniel Vetter, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1562625253-29254-14-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Tue, 2019-07-09 at 06:33 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> This patch add mmsys private data for ddp path config
> all these register offset and value will be different in future SOC
> add these define into mmsys private data
> u32 ovl0_mout_en;
> u32 rdma0_sout_sel_in;
> u32 rdma0_sout_color0;
> u32 rdma1_sout_sel_in;
> u32 rdma1_sout_dpi0;
> u32 rdma1_sout_dsi0;
> u32 dpi0_sel_in;
> u32 dpi0_sel_in_rdma1;
> u32 dsi0_sel_in;
> u32 dsi0_sel_in_rdma1;
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 4 ++
> drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 89 ++++++++++++++++++++++++---------
> drivers/gpu/drm/mediatek/mtk_drm_ddp.h | 5 ++
> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 3 ++
> drivers/gpu/drm/mediatek/mtk_drm_drv.h | 3 ++
> 5 files changed, 79 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index e520b56..5eac376 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -49,6 +49,7 @@ struct mtk_drm_crtc {
> bool pending_planes;
>
> void __iomem *config_regs;
> + const struct mtk_mmsys_reg_data *mmsys_reg_data;
> struct mtk_disp_mutex *mutex;
> unsigned int ddp_comp_nr;
> struct mtk_ddp_comp **ddp_comp;
> @@ -270,6 +271,7 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> DRM_DEBUG_DRIVER("mediatek_ddp_ddp_path_setup\n");
> for (i = 0; i < mtk_crtc->ddp_comp_nr - 1; i++) {
> mtk_ddp_add_comp_to_path(mtk_crtc->config_regs,
> + mtk_crtc->mmsys_reg_data,
> mtk_crtc->ddp_comp[i]->id,
> mtk_crtc->ddp_comp[i + 1]->id);
> mtk_disp_mutex_add_comp(mtk_crtc->mutex,
> @@ -318,6 +320,7 @@ static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *mtk_crtc)
> mtk_disp_mutex_disable(mtk_crtc->mutex);
> for (i = 0; i < mtk_crtc->ddp_comp_nr - 1; i++) {
> mtk_ddp_remove_comp_from_path(mtk_crtc->config_regs,
> + mtk_crtc->mmsys_reg_data,
> mtk_crtc->ddp_comp[i]->id,
> mtk_crtc->ddp_comp[i + 1]->id);
> mtk_disp_mutex_remove_comp(mtk_crtc->mutex,
> @@ -549,6 +552,7 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
> return -ENOMEM;
>
> mtk_crtc->config_regs = priv->config_regs;
> + mtk_crtc->mmsys_reg_data = priv->data->reg_data;
> mtk_crtc->ddp_comp_nr = path_len;
> mtk_crtc->ddp_comp = devm_kmalloc_array(dev, mtk_crtc->ddp_comp_nr,
> sizeof(*mtk_crtc->ddp_comp),
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> index 47b3e35..7819fd31 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> @@ -175,6 +175,19 @@ struct mtk_ddp {
> const struct mtk_ddp_data *data;
> };
>
> +struct mtk_mmsys_reg_data {
> + u32 ovl0_mout_en;
> + u32 rdma0_sout_sel_in;
> + u32 rdma0_sout_color0;
> + u32 rdma1_sout_sel_in;
> + u32 rdma1_sout_dpi0;
> + u32 rdma1_sout_dsi0;
> + u32 dpi0_sel_in;
> + u32 dpi0_sel_in_rdma1;
> + u32 dsi0_sel_in;
> + u32 dsi0_sel_in_rdma1;
> +};
rdma0_sout_sel_in, rdma0_sout_color0, and rdma1_sout_dsi0 are not used
in this patch, so remove from this patch.
> +
> static const unsigned int mt2701_mutex_mod[DDP_COMPONENT_ID_MAX] = {
> [DDP_COMPONENT_BLS] = MT2701_MUTEX_MOD_DISP_BLS,
> [DDP_COMPONENT_COLOR0] = MT2701_MUTEX_MOD_DISP_COLOR,
> @@ -253,17 +266,34 @@ struct mtk_ddp {
> .mutex_sof_reg = MT2701_DISP_MUTEX0_SOF0,
> };
>
> -static unsigned int mtk_ddp_mout_en(enum mtk_ddp_comp_id cur,
> +const struct mtk_mmsys_reg_data mt2701_mmsys_reg_data = {
> + .ovl0_mout_en = DISP_REG_CONFIG_DISP_OVL_MOUT_EN,
> + .dsi0_sel_in = DISP_REG_CONFIG_DSI_SEL,
> + .dsi0_sel_in_rdma1 = DSI_SEL_IN_RDMA,
> +};
> +
> +const struct mtk_mmsys_reg_data mt8173_mmsys_reg_data = {
> + .ovl0_mout_en = DISP_REG_CONFIG_DISP_OVL0_MOUT_EN,
> + .rdma1_sout_sel_in = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN,
> + .rdma1_sout_dpi0 = RDMA1_SOUT_DPI0,
> + .dpi0_sel_in = DISP_REG_CONFIG_DPI_SEL_IN,
> + .dpi0_sel_in_rdma1 = DPI0_SEL_IN_RDMA1,
> + .dsi0_sel_in = DISP_REG_CONFIG_DSIE_SEL_IN,
> + .dsi0_sel_in_rdma1 = DSI0_SEL_IN_RDMA1,
> +};
> +
> +static unsigned int mtk_ddp_mout_en(const struct mtk_mmsys_reg_data *data,
> + enum mtk_ddp_comp_id cur,
> enum mtk_ddp_comp_id next,
> unsigned int *addr)
> {
> unsigned int value;
>
> if (cur == DDP_COMPONENT_OVL0 && next == DDP_COMPONENT_COLOR0) {
> - *addr = DISP_REG_CONFIG_DISP_OVL0_MOUT_EN;
> + *addr = data->ovl0_mout_en;
> value = OVL0_MOUT_EN_COLOR0;
> } else if (cur == DDP_COMPONENT_OVL0 && next == DDP_COMPONENT_RDMA0) {
> - *addr = DISP_REG_CONFIG_DISP_OVL_MOUT_EN;
> + *addr = data->ovl0_mout_en;
> value = OVL_MOUT_EN_RDMA;
> } else if (cur == DDP_COMPONENT_OD0 && next == DDP_COMPONENT_RDMA0) {
> *addr = DISP_REG_CONFIG_DISP_OD_MOUT_EN;
> @@ -305,8 +335,8 @@ static unsigned int mtk_ddp_mout_en(enum mtk_ddp_comp_id cur,
> *addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
> value = RDMA1_SOUT_DSI3;
> } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI0) {
> - *addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
> - value = RDMA1_SOUT_DPI0;
> + *addr = data->rdma1_sout_sel_in;
> + value = data->rdma1_sout_dpi0;
> } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI1) {
> *addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
> value = RDMA1_SOUT_DPI1;
> @@ -332,7 +362,8 @@ static unsigned int mtk_ddp_mout_en(enum mtk_ddp_comp_id cur,
> return value;
> }
>
> -static unsigned int mtk_ddp_sel_in(enum mtk_ddp_comp_id cur,
> +static unsigned int mtk_ddp_sel_in(const struct mtk_mmsys_reg_data *data,
> + enum mtk_ddp_comp_id cur,
> enum mtk_ddp_comp_id next,
> unsigned int *addr)
> {
> @@ -342,14 +373,14 @@ static unsigned int mtk_ddp_sel_in(enum mtk_ddp_comp_id cur,
> *addr = DISP_REG_CONFIG_DISP_COLOR0_SEL_IN;
> value = COLOR0_SEL_IN_OVL0;
> } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI0) {
> - *addr = DISP_REG_CONFIG_DPI_SEL_IN;
> - value = DPI0_SEL_IN_RDMA1;
> + *addr = data->dpi0_sel_in;
> + value = data->dpi0_sel_in_rdma1;
> } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI1) {
> *addr = DISP_REG_CONFIG_DPI_SEL_IN;
> value = DPI1_SEL_IN_RDMA1;
> } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI0) {
> - *addr = DISP_REG_CONFIG_DSIE_SEL_IN;
> - value = DSI0_SEL_IN_RDMA1;
> + *addr = data->dsi0_sel_in;
> + value = data->dsi0_sel_in_rdma1;
> } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI1) {
> *addr = DISP_REG_CONFIG_DSIO_SEL_IN;
> value = DSI1_SEL_IN_RDMA1;
> @@ -390,37 +421,44 @@ static unsigned int mtk_ddp_sel_in(enum mtk_ddp_comp_id cur,
> return value;
> }
>
> -static void mtk_ddp_sout_sel(void __iomem *config_regs,
> - enum mtk_ddp_comp_id cur,
> - enum mtk_ddp_comp_id next)
> +static unsigned int mtk_ddp_sout_sel(const struct mtk_mmsys_reg_data *data,
> + enum mtk_ddp_comp_id cur,
> + enum mtk_ddp_comp_id next,
> + unsigned int *addr)
> {
> + unsigned int value;
> +
> if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DSI0) {
> - writel_relaxed(BLS_TO_DSI_RDMA1_TO_DPI1,
> - config_regs + DISP_REG_CONFIG_OUT_SEL);
> + *addr = DISP_REG_CONFIG_OUT_SEL;
> + value = BLS_TO_DSI_RDMA1_TO_DPI1;
> } else if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DPI0) {
> - writel_relaxed(BLS_TO_DPI_RDMA1_TO_DSI,
> - config_regs + DISP_REG_CONFIG_OUT_SEL);
> - } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI0) {
> - writel_relaxed(DSI_SEL_IN_RDMA,
> - config_regs + DISP_REG_CONFIG_DSI_SEL);
> + *addr = DISP_REG_CONFIG_OUT_SEL;
> + value = BLS_TO_DPI_RDMA1_TO_DSI;
> + } else {
> + value = 0;
> }
> +
> + return value;
> }
>
> void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
> + const struct mtk_mmsys_reg_data *reg_data,
> enum mtk_ddp_comp_id cur,
> enum mtk_ddp_comp_id next)
> {
> unsigned int addr, value, reg;
>
> - value = mtk_ddp_mout_en(cur, next, &addr);
> + value = mtk_ddp_mout_en(reg_data, cur, next, &addr);
> if (value) {
> reg = readl_relaxed(config_regs + addr) | value;
> writel_relaxed(reg, config_regs + addr);
> }
>
> - mtk_ddp_sout_sel(config_regs, cur, next);
> + value = mtk_ddp_sout_sel(reg_data, cur, next, &addr);
> + if (value)
> + writel_relaxed(value, config_regs + addr);
I think the register could be written inside mtk_ddp_sout_sel(), why do
you move out of that function?
Regards,
CK
>
> - value = mtk_ddp_sel_in(cur, next, &addr);
> + value = mtk_ddp_sel_in(reg_data, cur, next, &addr);
> if (value) {
> reg = readl_relaxed(config_regs + addr) | value;
> writel_relaxed(reg, config_regs + addr);
> @@ -428,18 +466,19 @@ void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
> }
>
> void mtk_ddp_remove_comp_from_path(void __iomem *config_regs,
> + const struct mtk_mmsys_reg_data *reg_data,
> enum mtk_ddp_comp_id cur,
> enum mtk_ddp_comp_id next)
> {
> unsigned int addr, value, reg;
>
> - value = mtk_ddp_mout_en(cur, next, &addr);
> + value = mtk_ddp_mout_en(reg_data, cur, next, &addr);
> if (value) {
> reg = readl_relaxed(config_regs + addr) & ~value;
> writel_relaxed(reg, config_regs + addr);
> }
>
> - value = mtk_ddp_sel_in(cur, next, &addr);
> + value = mtk_ddp_sel_in(reg_data, cur, next, &addr);
> if (value) {
> reg = readl_relaxed(config_regs + addr) & ~value;
> writel_relaxed(reg, config_regs + addr);
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp.h
> index f9a7991..43dabb6 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.h
> @@ -19,11 +19,16 @@
> struct regmap;
> struct device;
> struct mtk_disp_mutex;
> +struct mtk_mmsys_reg_data;
>
> +extern const struct mtk_mmsys_reg_data mt2701_mmsys_reg_data;
> +extern const struct mtk_mmsys_reg_data mt8173_mmsys_reg_data;
> void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
> + const struct mtk_mmsys_reg_data *reg_data,
> enum mtk_ddp_comp_id cur,
> enum mtk_ddp_comp_id next);
> void mtk_ddp_remove_comp_from_path(void __iomem *config_regs,
> + const struct mtk_mmsys_reg_data *reg_data,
> enum mtk_ddp_comp_id cur,
> enum mtk_ddp_comp_id next);
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 57ce470..5f94259 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -197,6 +197,7 @@ static int mtk_atomic_commit(struct drm_device *drm,
> .main_len = ARRAY_SIZE(mt2701_mtk_ddp_main),
> .ext_path = mt2701_mtk_ddp_ext,
> .ext_len = ARRAY_SIZE(mt2701_mtk_ddp_ext),
> + .reg_data = &mt2701_mmsys_reg_data,
> .shadow_register = true,
> };
>
> @@ -207,6 +208,7 @@ static int mtk_atomic_commit(struct drm_device *drm,
> .ext_len = ARRAY_SIZE(mt2712_mtk_ddp_ext),
> .third_path = mt2712_mtk_ddp_third,
> .third_len = ARRAY_SIZE(mt2712_mtk_ddp_third),
> + .reg_data = &mt8173_mmsys_reg_data,
> };
>
> static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
> @@ -214,6 +216,7 @@ static int mtk_atomic_commit(struct drm_device *drm,
> .main_len = ARRAY_SIZE(mt8173_mtk_ddp_main),
> .ext_path = mt8173_mtk_ddp_ext,
> .ext_len = ARRAY_SIZE(mt8173_mtk_ddp_ext),
> + .reg_data = &mt8173_mmsys_reg_data,
> };
>
> static int mtk_drm_kms_init(struct drm_device *drm)
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> index ecc00ca..1e6d74b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> @@ -15,6 +15,7 @@
> #define MTK_DRM_DRV_H
>
> #include <linux/io.h>
> +#include "mtk_drm_ddp.h"
> #include "mtk_drm_ddp_comp.h"
>
> #define MAX_CRTC 3
> @@ -36,6 +37,8 @@ struct mtk_mmsys_driver_data {
> const enum mtk_ddp_comp_id *third_path;
> unsigned int third_len;
>
> + const struct mtk_mmsys_reg_data *reg_data;
> +
> bool shadow_register;
> };
>
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-17 5:42 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: <77df234f-aa40-0319-a593-f1f19f0f1c2a@nvidia.com>
В 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?
> Also I don't see Tegra124 CPU Freq driver using flag
> CPUFREQ_NEED_INITIAL_FREQ_CHECK.
>
> Tegra124 CPUFreq driver is not using cpufreq_driver
>
>
> <https://elixir.bootlin.com/linux/v5.2.1/ident/cpufreq_driver>
>
T124 driver is a wrapper around cpufreq-dt driver.
^ permalink raw reply
* Re: [PATCH v4, 12/33] drm/mediatek: split DISP_REG_CONFIG_DSI_SEL setting into another use case
From: CK Hu @ 2019-07-17 5:35 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, Philipp Zabel, David Airlie,
linux-kernel, dri-devel, Rob Herring, linux-mediatek,
Daniel Vetter, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1562625253-29254-13-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Tue, 2019-07-09 at 06:33 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> Here is two modifition in this patch:
> 1.bls->dpi0 and rdma1->dsi are differen usecase,
> Split DISP_REG_CONFIG_DSI_SEL setting into anther usecase
> 2.remove DISP_REG_CONFIG_DPI_SEL setting, DPI_SEL_IN_BLS is 0 and
> this is same with hardware defautl setting,
>
You move 2 register setting out of the path from BLS to DPI0, does this
path still work? Please make sure that all modification could work on
all supported SoC.
Regards,
CK
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> index d015c1a..47b3e35 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> @@ -400,10 +400,9 @@ static void mtk_ddp_sout_sel(void __iomem *config_regs,
> } else if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DPI0) {
> writel_relaxed(BLS_TO_DPI_RDMA1_TO_DSI,
> config_regs + DISP_REG_CONFIG_OUT_SEL);
> + } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI0) {
> writel_relaxed(DSI_SEL_IN_RDMA,
> config_regs + DISP_REG_CONFIG_DSI_SEL);
> - writel_relaxed(DPI_SEL_IN_BLS,
> - config_regs + DISP_REG_CONFIG_DPI_SEL);
> }
> }
>
^ permalink raw reply
* Re: [PATCH v4, 11/33] drm/mediatek: add mutex sof register offset into ddp private data
From: CK Hu @ 2019-07-17 5:31 UTC (permalink / raw)
To: yongqiang.niu
Cc: Philipp Zabel, Rob Herring, Matthias Brugger, David Airlie,
Daniel Vetter, Mark Rutland, dri-devel, devicetree, linux-kernel,
linux-arm-kernel, linux-mediatek
In-Reply-To: <1562625253-29254-12-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Tue, 2019-07-09 at 06:33 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> mutex sof register offset will be private data of ddp
>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> index ab396ee..d015c1a 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> @@ -42,12 +42,13 @@
> #define DISP_REG_CONFIG_DPI_SEL 0x064
>
> #define MT2701_DISP_MUTEX0_MOD0 0x2c
> +#define MT2701_DISP_MUTEX0_SOF0 0x30
>
> #define DISP_REG_MUTEX_EN(n) (0x20 + 0x20 * (n))
> #define DISP_REG_MUTEX(n) (0x24 + 0x20 * (n))
> #define DISP_REG_MUTEX_RST(n) (0x28 + 0x20 * (n))
> #define DISP_REG_MUTEX_MOD(mutex_mod_reg, n) (mutex_mod_reg + 0x20 * (n))
> -#define DISP_REG_MUTEX_SOF(n) (0x30 + 0x20 * (n))
> +#define DISP_REG_MUTEX_SOF(mutex_sof_reg, n) (mutex_sof_reg + 0x20 * (n))
> #define DISP_REG_MUTEX_MOD2(n) (0x34 + 0x20 * (n))
>
> #define INT_MUTEX BIT(1)
> @@ -163,6 +164,7 @@ struct mtk_ddp_data {
> const unsigned int *mutex_mod;
> const unsigned int *mutex_sof;
> const unsigned int mutex_mod_reg;
> + const unsigned int mutex_sof_reg;
> };
>
> struct mtk_ddp {
> @@ -234,18 +236,21 @@ struct mtk_ddp {
> .mutex_mod = mt2701_mutex_mod,
> .mutex_sof = mt2712_mutex_sof,
> .mutex_mod_reg = MT2701_DISP_MUTEX0_MOD0,
> + .mutex_sof_reg = MT2701_DISP_MUTEX0_SOF0,
> };
>
> static const struct mtk_ddp_data mt2712_ddp_driver_data = {
> .mutex_mod = mt2712_mutex_mod,
> .mutex_sof = mt2712_mutex_sof,
> .mutex_mod_reg = MT2701_DISP_MUTEX0_MOD0,
> + .mutex_sof_reg = MT2701_DISP_MUTEX0_SOF0,
> };
>
> static const struct mtk_ddp_data mt8173_ddp_driver_data = {
> .mutex_mod = mt8173_mutex_mod,
> .mutex_sof = mt2712_mutex_sof,
> .mutex_mod_reg = MT2701_DISP_MUTEX0_MOD0,
> + .mutex_sof_reg = MT2701_DISP_MUTEX0_SOF0,
> };
>
> static unsigned int mtk_ddp_mout_en(enum mtk_ddp_comp_id cur,
> @@ -527,7 +532,8 @@ void mtk_disp_mutex_add_comp(struct mtk_disp_mutex *mutex,
> }
>
> writel_relaxed(ddp->data->mutex_sof[sof_id],
> - ddp->regs + DISP_REG_MUTEX_SOF(mutex->id));
> + ddp->regs +
> + DISP_REG_MUTEX_SOF(ddp->data->mutex_sof_reg, mutex->id));
> }
>
> void mtk_disp_mutex_remove_comp(struct mtk_disp_mutex *mutex,
> @@ -549,7 +555,8 @@ void mtk_disp_mutex_remove_comp(struct mtk_disp_mutex *mutex,
> case DDP_COMPONENT_DPI1:
> writel_relaxed(MUTEX_SOF_SINGLE_MODE,
> ddp->regs +
> - DISP_REG_MUTEX_SOF(mutex->id));
> + DISP_REG_MUTEX_SOF(ddp->data->mutex_sof_reg,
> + mutex->id));
> break;
> default:
> if (ddp->data->mutex_mod[id] < 32) {
^ permalink raw reply
* Re: [PATCH v4, 10/33] drm/mediatek: add mutex sof into ddp private data
From: CK Hu @ 2019-07-17 5:28 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, David Airlie, linux-kernel, dri-devel,
Rob Herring, linux-mediatek, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1562625253-29254-11-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Tue, 2019-07-09 at 06:33 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> mutex sof will be ddp private data
>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 43 +++++++++++++++++++++++++++-------
> 1 file changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> index 8bde2cf..ab396ee 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> @@ -149,8 +149,19 @@ struct mtk_disp_mutex {
> bool claimed;
> };
>
> +enum mtk_ddp_mutex_sof_id {
> + DDP_MUTEX_SOF_SINGLE_MODE,
> + DDP_MUTEX_SOF_DSI0,
> + DDP_MUTEX_SOF_DSI1,
> + DDP_MUTEX_SOF_DPI0,
> + DDP_MUTEX_SOF_DPI1,
> + DDP_MUTEX_SOF_DSI2,
> + DDP_MUTEX_SOF_DSI3,
> +};
> +
> struct mtk_ddp_data {
> const unsigned int *mutex_mod;
> + const unsigned int *mutex_sof;
> const unsigned int mutex_mod_reg;
> };
>
> @@ -209,18 +220,31 @@ struct mtk_ddp {
> [DDP_COMPONENT_WDMA1] = MT8173_MUTEX_MOD_DISP_WDMA1,
> };
>
> +static const unsigned int mt2712_mutex_sof[DDP_MUTEX_SOF_DSI3 + 1] = {
> + [DDP_MUTEX_SOF_SINGLE_MODE] = MUTEX_SOF_SINGLE_MODE,
> + [DDP_MUTEX_SOF_DSI0] = MUTEX_SOF_DSI0,
> + [DDP_MUTEX_SOF_DSI1] = MUTEX_SOF_DSI1,
> + [DDP_MUTEX_SOF_DPI0] = MUTEX_SOF_DPI0,
> + [DDP_MUTEX_SOF_DPI1] = MUTEX_SOF_DPI1,
> + [DDP_MUTEX_SOF_DSI2] = MUTEX_SOF_DSI2,
> + [DDP_MUTEX_SOF_DSI3] = MUTEX_SOF_DSI3,
> +};
> +
> static const struct mtk_ddp_data mt2701_ddp_driver_data = {
> .mutex_mod = mt2701_mutex_mod,
> + .mutex_sof = mt2712_mutex_sof,
> .mutex_mod_reg = MT2701_DISP_MUTEX0_MOD0,
> };
>
> static const struct mtk_ddp_data mt2712_ddp_driver_data = {
> .mutex_mod = mt2712_mutex_mod,
> + .mutex_sof = mt2712_mutex_sof,
> .mutex_mod_reg = MT2701_DISP_MUTEX0_MOD0,
> };
>
> static const struct mtk_ddp_data mt8173_ddp_driver_data = {
> .mutex_mod = mt8173_mutex_mod,
> + .mutex_sof = mt2712_mutex_sof,
> .mutex_mod_reg = MT2701_DISP_MUTEX0_MOD0,
> };
>
> @@ -462,28 +486,29 @@ void mtk_disp_mutex_add_comp(struct mtk_disp_mutex *mutex,
> struct mtk_ddp *ddp = container_of(mutex, struct mtk_ddp,
> mutex[mutex->id]);
> unsigned int reg;
> + unsigned int sof_id;
> unsigned int offset;
>
> WARN_ON(&ddp->mutex[mutex->id] != mutex);
>
> switch (id) {
> case DDP_COMPONENT_DSI0:
> - reg = MUTEX_SOF_DSI0;
> + sof_id = DDP_MUTEX_SOF_DSI0;
> break;
> case DDP_COMPONENT_DSI1:
> - reg = MUTEX_SOF_DSI0;
> + sof_id = DDP_MUTEX_SOF_DSI0;
> break;
> case DDP_COMPONENT_DSI2:
> - reg = MUTEX_SOF_DSI2;
> + sof_id = DDP_MUTEX_SOF_DSI2;
> break;
> case DDP_COMPONENT_DSI3:
> - reg = MUTEX_SOF_DSI3;
> + sof_id = DDP_MUTEX_SOF_DSI3;
> break;
> case DDP_COMPONENT_DPI0:
> - reg = MUTEX_SOF_DPI0;
> + sof_id = DDP_MUTEX_SOF_DPI0;
> break;
> case DDP_COMPONENT_DPI1:
> - reg = MUTEX_SOF_DPI1;
> + sof_id = DDP_MUTEX_SOF_DPI1;
> break;
> default:
> if (ddp->data->mutex_mod[id] < 32) {
> @@ -501,7 +526,8 @@ void mtk_disp_mutex_add_comp(struct mtk_disp_mutex *mutex,
> return;
> }
>
> - writel_relaxed(reg, ddp->regs + DISP_REG_MUTEX_SOF(mutex->id));
> + writel_relaxed(ddp->data->mutex_sof[sof_id],
> + ddp->regs + DISP_REG_MUTEX_SOF(mutex->id));
> }
>
> void mtk_disp_mutex_remove_comp(struct mtk_disp_mutex *mutex,
> @@ -522,7 +548,8 @@ void mtk_disp_mutex_remove_comp(struct mtk_disp_mutex *mutex,
> case DDP_COMPONENT_DPI0:
> case DDP_COMPONENT_DPI1:
> writel_relaxed(MUTEX_SOF_SINGLE_MODE,
> - ddp->regs + DISP_REG_MUTEX_SOF(mutex->id));
> + ddp->regs +
> + DISP_REG_MUTEX_SOF(mutex->id));
> break;
> default:
> if (ddp->data->mutex_mod[id] < 32) {
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 5:25 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: <20190717071105.3750a021@dimatab>
[-- Attachment #1: Type: text/plain, Size: 12863 bytes --]
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.
Also I don't see Tegra124 CPU Freq driver using flag
CPUFREQ_NEED_INITIAL_FREQ_CHECK.
Tegra124 CPUFreq driver is not using cpufreq_driver
<https://elixir.bootlin.com/linux/v5.2.1/ident/cpufreq_driver>
[-- Attachment #2: Type: text/html, Size: 16128 bytes --]
^ permalink raw reply
* Re: [PATCH v4, 09/33] drm/mediatek: add mutex mod register offset into ddp private data
From: CK Hu @ 2019-07-17 5:23 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, Philipp Zabel, David Airlie,
linux-kernel, dri-devel, Rob Herring, linux-mediatek,
Daniel Vetter, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1562625253-29254-10-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Tue, 2019-07-09 at 06:33 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> mutex mod register offset will be private data of ddp.
>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> index 412b82f..8bde2cf 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> @@ -41,12 +41,14 @@
> #define DISP_REG_CONFIG_DSI_SEL 0x050
> #define DISP_REG_CONFIG_DPI_SEL 0x064
>
> -#define DISP_REG_MUTEX_EN(n) (0x20 + 0x20 * (n))
> -#define DISP_REG_MUTEX(n) (0x24 + 0x20 * (n))
> -#define DISP_REG_MUTEX_RST(n) (0x28 + 0x20 * (n))
> -#define DISP_REG_MUTEX_MOD(n) (0x2c + 0x20 * (n))
> -#define DISP_REG_MUTEX_SOF(n) (0x30 + 0x20 * (n))
> -#define DISP_REG_MUTEX_MOD2(n) (0x34 + 0x20 * (n))
> +#define MT2701_DISP_MUTEX0_MOD0 0x2c
> +
> +#define DISP_REG_MUTEX_EN(n) (0x20 + 0x20 * (n))
> +#define DISP_REG_MUTEX(n) (0x24 + 0x20 * (n))
> +#define DISP_REG_MUTEX_RST(n) (0x28 + 0x20 * (n))
> +#define DISP_REG_MUTEX_MOD(mutex_mod_reg, n) (mutex_mod_reg + 0x20 * (n))
> +#define DISP_REG_MUTEX_SOF(n) (0x30 + 0x20 * (n))
> +#define DISP_REG_MUTEX_MOD2(n) (0x34 + 0x20 * (n))
>
> #define INT_MUTEX BIT(1)
>
> @@ -149,6 +151,7 @@ struct mtk_disp_mutex {
>
> struct mtk_ddp_data {
> const unsigned int *mutex_mod;
> + const unsigned int mutex_mod_reg;
> };
>
> struct mtk_ddp {
> @@ -208,14 +211,17 @@ struct mtk_ddp {
>
> static const struct mtk_ddp_data mt2701_ddp_driver_data = {
> .mutex_mod = mt2701_mutex_mod,
> + .mutex_mod_reg = MT2701_DISP_MUTEX0_MOD0,
> };
>
> static const struct mtk_ddp_data mt2712_ddp_driver_data = {
> .mutex_mod = mt2712_mutex_mod,
> + .mutex_mod_reg = MT2701_DISP_MUTEX0_MOD0,
> };
>
> static const struct mtk_ddp_data mt8173_ddp_driver_data = {
> .mutex_mod = mt8173_mutex_mod,
> + .mutex_mod_reg = MT2701_DISP_MUTEX0_MOD0,
> };
>
> static unsigned int mtk_ddp_mout_en(enum mtk_ddp_comp_id cur,
> @@ -481,7 +487,8 @@ void mtk_disp_mutex_add_comp(struct mtk_disp_mutex *mutex,
> break;
> default:
> if (ddp->data->mutex_mod[id] < 32) {
> - offset = DISP_REG_MUTEX_MOD(mutex->id);
> + offset = DISP_REG_MUTEX_MOD(ddp->data->mutex_mod_reg,
> + mutex->id);
> reg = readl_relaxed(ddp->regs + offset);
> reg |= 1 << ddp->data->mutex_mod[id];
> writel_relaxed(reg, ddp->regs + offset);
> @@ -519,7 +526,8 @@ void mtk_disp_mutex_remove_comp(struct mtk_disp_mutex *mutex,
> break;
> default:
> if (ddp->data->mutex_mod[id] < 32) {
> - offset = DISP_REG_MUTEX_MOD(mutex->id);
> + offset = DISP_REG_MUTEX_MOD(ddp->data->mutex_mod_reg,
> + mutex->id);
> reg = readl_relaxed(ddp->regs + offset);
> reg &= ~(1 << ddp->data->mutex_mod[id]);
> writel_relaxed(reg, ddp->regs + offset);
^ permalink raw reply
* Re: [PATCH v4, 08/33] drm/mediatek: add mutex mod into ddp private data
From: CK Hu @ 2019-07-17 5:22 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, David Airlie, linux-kernel, dri-devel,
Rob Herring, linux-mediatek, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1562625253-29254-9-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Tue, 2019-07-09 at 06:33 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> except mutex mod, mutex mod reg,mutex sof reg,
> and mutex sof id will be ddp private data
Reviewed-by: CK Hu <ck.hu@mediatek.com>
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 41 +++++++++++++++++++++++++---------
> 1 file changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> index 579ce28..412b82f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> @@ -147,12 +147,16 @@ struct mtk_disp_mutex {
> bool claimed;
> };
>
> +struct mtk_ddp_data {
> + const unsigned int *mutex_mod;
> +};
> +
> struct mtk_ddp {
> struct device *dev;
> struct clk *clk;
> void __iomem *regs;
> struct mtk_disp_mutex mutex[10];
> - const unsigned int *mutex_mod;
> + const struct mtk_ddp_data *data;
> };
>
> static const unsigned int mt2701_mutex_mod[DDP_COMPONENT_ID_MAX] = {
> @@ -202,6 +206,18 @@ struct mtk_ddp {
> [DDP_COMPONENT_WDMA1] = MT8173_MUTEX_MOD_DISP_WDMA1,
> };
>
> +static const struct mtk_ddp_data mt2701_ddp_driver_data = {
> + .mutex_mod = mt2701_mutex_mod,
> +};
> +
> +static const struct mtk_ddp_data mt2712_ddp_driver_data = {
> + .mutex_mod = mt2712_mutex_mod,
> +};
> +
> +static const struct mtk_ddp_data mt8173_ddp_driver_data = {
> + .mutex_mod = mt8173_mutex_mod,
> +};
> +
> static unsigned int mtk_ddp_mout_en(enum mtk_ddp_comp_id cur,
> enum mtk_ddp_comp_id next,
> unsigned int *addr)
> @@ -464,15 +480,15 @@ void mtk_disp_mutex_add_comp(struct mtk_disp_mutex *mutex,
> reg = MUTEX_SOF_DPI1;
> break;
> default:
> - if (ddp->mutex_mod[id] < 32) {
> + if (ddp->data->mutex_mod[id] < 32) {
> offset = DISP_REG_MUTEX_MOD(mutex->id);
> reg = readl_relaxed(ddp->regs + offset);
> - reg |= 1 << ddp->mutex_mod[id];
> + reg |= 1 << ddp->data->mutex_mod[id];
> writel_relaxed(reg, ddp->regs + offset);
> } else {
> offset = DISP_REG_MUTEX_MOD2(mutex->id);
> reg = readl_relaxed(ddp->regs + offset);
> - reg |= 1 << (ddp->mutex_mod[id] - 32);
> + reg |= 1 << (ddp->data->mutex_mod[id] - 32);
> writel_relaxed(reg, ddp->regs + offset);
> }
> return;
> @@ -502,15 +518,15 @@ void mtk_disp_mutex_remove_comp(struct mtk_disp_mutex *mutex,
> ddp->regs + DISP_REG_MUTEX_SOF(mutex->id));
> break;
> default:
> - if (ddp->mutex_mod[id] < 32) {
> + if (ddp->data->mutex_mod[id] < 32) {
> offset = DISP_REG_MUTEX_MOD(mutex->id);
> reg = readl_relaxed(ddp->regs + offset);
> - reg &= ~(1 << ddp->mutex_mod[id]);
> + reg &= ~(1 << ddp->data->mutex_mod[id]);
> writel_relaxed(reg, ddp->regs + offset);
> } else {
> offset = DISP_REG_MUTEX_MOD2(mutex->id);
> reg = readl_relaxed(ddp->regs + offset);
> - reg &= ~(1 << (ddp->mutex_mod[id] - 32));
> + reg &= ~(1 << (ddp->data->mutex_mod[id] - 32));
> writel_relaxed(reg, ddp->regs + offset);
> }
> break;
> @@ -585,7 +601,7 @@ static int mtk_ddp_probe(struct platform_device *pdev)
> return PTR_ERR(ddp->regs);
> }
>
> - ddp->mutex_mod = of_device_get_match_data(dev);
> + ddp->data = of_device_get_match_data(dev);
>
> platform_set_drvdata(pdev, ddp);
>
> @@ -598,9 +614,12 @@ static int mtk_ddp_remove(struct platform_device *pdev)
> }
>
> static const struct of_device_id ddp_driver_dt_match[] = {
> - { .compatible = "mediatek,mt2701-disp-mutex", .data = mt2701_mutex_mod},
> - { .compatible = "mediatek,mt2712-disp-mutex", .data = mt2712_mutex_mod},
> - { .compatible = "mediatek,mt8173-disp-mutex", .data = mt8173_mutex_mod},
> + { .compatible = "mediatek,mt2701-disp-mutex",
> + .data = &mt2701_ddp_driver_data},
> + { .compatible = "mediatek,mt2712-disp-mutex",
> + .data = &mt2712_ddp_driver_data},
> + { .compatible = "mediatek,mt8173-disp-mutex",
> + .data = &mt8173_ddp_driver_data},
> {},
> };
> MODULE_DEVICE_TABLE(of, ddp_driver_dt_match);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH 11/13] arm64: dts: qcom: qcs404: Add CPR and populate OPP table
From: Viresh Kumar @ 2019-07-17 4:49 UTC (permalink / raw)
To: Niklas Cassel
Cc: Andy Gross, linux-arm-msm, jorge.ramirez-ortiz, sboyd, vireshk,
bjorn.andersson, ulf.hansson, Rob Herring, Mark Rutland,
devicetree, linux-kernel
In-Reply-To: <20190716105318.GA26592@centauri>
On 16-07-19, 12:53, Niklas Cassel wrote:
> Here I cheated and simply used get_cpu_device(0).
>
> Since I cheated, I used get_cpu_device(0) always,
> so even when CPU1,CPU2,CPU3 is attached, dev_pm_opp_get_opp_count(cpu0) is
> still 0.
>
> I added a print in
> [ 3.836533] cpr_set_performance: number of OPPs for dev: cpu0: 3
>
> And there I can see that OPP count is 3, so it appears that with the
> current code, we need to wait until cpufreq-dt.c:cpufreq_init()
> has been called, maybe dev_pm_opp_of_cpumask_add_table() needs
> to be called before dev_pm_opp_get_opp_count(cpu0) actually returns 3.
>
> cpufreq_init() is called by platform_device_register_simple("cpufreq-dt", -1,
> NULL, 0);
> which is called after dev_pm_opp_attach_genpd().
>
> What I don't understand is that dev_pm_opp_attach_genpd() actually returns
> a OPP table. So why do we need to wait for dev_pm_opp_of_cpumask_add_table(),
> before either dev_pm_opp_get_opp_count(cpu0) or
> dev_pm_opp_get_opp_count(genpd_virtdev_for_cpu0) returns 3?
Ah, I see the problems now. No, cpufreq table can't be available at
this point of time and we aren't going to change that. It is the right
thing to do.
Now, even if the kernel isn't written in a way which works for you, it
isn't right to put more things in DT than required. DT is (should be)
very much independent of the Linux kernel.
So we have to parse DT to find highest frequency for each
required-opp. Best is to put that code in the OPP core and use it from
your driver.
--
viresh
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-17 4:11 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: <055457fd-621b-6c93-b671-d5e5380698c6@nvidia.com>
В 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).
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 4: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: <20190717065431.27f27a07@dimatab>
On 7/16/19 8:54 PM, Dmitry Osipenko wrote:
> В Tue, 16 Jul 2019 19:18:19 -0700
> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>
>> 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
>
> It doesn't matter much when CPUFreq driver suspends, it's only
> important that it suspends before CaR.
>
> I'm not sure why do you need anything else from DFLL driver other than
> what is already exposed via generic CCF API. It looks to me
> that switching CPU's parent clock away from DFLL and then disabling
> DFLL's clock is enough for suspend, accordingly to what Peter wrote. And
> resuming is the same as what's done on CPUFreq's driver probe. The CCLK
> policy should be saved and restored by the CaR driver, you don't need to
> care about it. The cpufreq-dt driver sets
> CPUFREQ_NEED_INITIAL_FREQ_CHECK, hence you don't need to care about
> restoring the original CPU freq on resume, IIUC.
>
>
CPU policy save/restore are part of this patch and it wasn't there
earlier and when I moved suspend/resume to clock-dfll I moved cpu
restore also to clock-dfll driver.
Thought to restore cpu source to dfll in CPUFreq driver which requires
exporting API from clock driver. But leaving CPU restore in Tegra210
clock driver, we can do dfll suspend/resume thru CPUFreq driver pm_ops
without exporting clock APIs
Will update ...
> With
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller
From: Andrew Jeffery @ 2019-07-17 3:57 UTC (permalink / raw)
To: Rob Herring
Cc: linux-mmc, Ulf Hansson, Mark Rutland, Joel Stanley, Adrian Hunter,
devicetree,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-aspeed, linux-kernel@vger.kernel.org, Ryan Chen
In-Reply-To: <CAL_JsqKMo_uv4Ur-D4NaUXk94hGJeRt5fg+0998dDjJCTgumGg@mail.gmail.com>
On Wed, 17 Jul 2019, at 00:27, Rob Herring wrote:
> On Mon, Jul 15, 2019 at 6:36 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> >
> >
> > On Tue, 16 Jul 2019, at 07:47, Rob Herring wrote:
> > > On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > >
> > > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the
> > > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit
> > > > data bus if only a single slot is enabled.
> > > >
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > > ---
> > > > In v2:
> > > >
> > > > * Rename to aspeed,sdhci.yaml
> > > > * Rename sd-controller compatible
> > > > * Add `maxItems: 1` for reg properties
> > > > * Move sdhci subnode description to patternProperties
> > > > * Drop sdhci compatible requirement
> > > > * #address-cells and #size-cells are required
> > > > * Prevent additional properties
> > > > * Implement explicit ranges in example
> > > > * Remove slot property
> > > >
> > > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++
> > > > 1 file changed, 90 insertions(+)
> > > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > new file mode 100644
> > > > index 000000000000..67a691c3348c
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > @@ -0,0 +1,90 @@
> > > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: ASPEED SD/SDIO/eMMC Controller
> > > > +
> > > > +maintainers:
> > > > + - Andrew Jeffery <andrew@aj.id.au>
> > > > + - Ryan Chen <ryanchen.aspeed@gmail.com>
> > > > +
> > > > +description: |+
> > > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO
> > > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if
> > > > + only a single slot is enabled.
> > > > +
> > > > + The two slots are supported by a common configuration area. As the SDHCIs for
> > > > + the slots are dependent on the common configuration area, they are described
> > > > + as child nodes.
> > > > +
> > > > +properties:
> > > > + compatible:
> > > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ]
> > >
> > > This is actually a list of 4 strings. Please reformat to 1 per line.
> >
> > On reflection that's obvious, but also a somewhat subtle interaction with the
> > preference for no quotes (the obvious caveat being "except where required").
>
> It wasn't something I'd run into before. I'm working on a check, but
> unfortunately we can only check for quotes not needed and can't check
> for missing quotes.
>
> > Thanks for pointing it out.
> >
> > I have been running `make dt_binding_check` and `make dtbs_check` over
> > these, looks like I need to up my game a bit though. Do you do additional things
> > in your workflow?
>
> That should have thrown the warnings. If you aren't seeing those, do
> you have dtschema package installed (see
> Documentation/devicetree/writing-schema.md)?
I do have it installed, but as mentioned previously there's a fair few
warnings emitted currently by the Aspeed devicetrees, so it might have
got lost in the noise. I've started to clean that up, though probably need
some direction there too.
Separately I'm currently trying to track down an issue where I get errors
on the Aspeed dts cpu nodes about failing to match the riscv CPU
compatibles, it seems dt-validate isn't finding the ARM CPU compatible
strings. It feels more annoying to track down that I'd like.
> Or it could be erroring
> out on something else first. There's a few breakages that I'm trying
> to fix.
Okay. I'll keep an eye on the dt-schema repo.
Cheers,
Andrew
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-17 3:54 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: <c6c0a205-c083-fd46-361c-175bd8840c6e@nvidia.com>
В Tue, 16 Jul 2019 19:18:19 -0700
Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
> 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
It doesn't matter much when CPUFreq driver suspends, it's only
important that it suspends before CaR.
I'm not sure why do you need anything else from DFLL driver other than
what is already exposed via generic CCF API. It looks to me
that switching CPU's parent clock away from DFLL and then disabling
DFLL's clock is enough for suspend, accordingly to what Peter wrote. And
resuming is the same as what's done on CPUFreq's driver probe. The CCLK
policy should be saved and restored by the CaR driver, you don't need to
care about it. The cpufreq-dt driver sets
CPUFREQ_NEED_INITIAL_FREQ_CHECK, hence you don't need to care about
restoring the original CPU freq on resume, IIUC.
^ permalink raw reply
* Re: [PATCH] dt-bindings: pinctrl: aspeed: Fix 'compatible' schema errors
From: Andrew Jeffery @ 2019-07-17 3:51 UTC (permalink / raw)
To: Rob Herring, Joel Stanley
Cc: devicetree, Linus Walleij, Linux ARM, linux-aspeed,
open list:GPIO SUBSYSTEM
In-Reply-To: <CAL_Jsq+AJDNZ-676iP=vv6G-pjWqBJyZ3bJ26o7i=c=KWbozSw@mail.gmail.com>
On Wed, 17 Jul 2019, at 00:35, Rob Herring wrote:
> On Mon, Jul 15, 2019 at 5:17 PM Joel Stanley <joel@jms.id.au> wrote:
> >
> > On Mon, 15 Jul 2019 at 22:37, Rob Herring <robh@kernel.org> wrote:
> > >
> > > The Aspeed pinctl schema have errors in the 'compatible' schema:
> > >
> > > Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml: \
> > > properties:compatible:enum: ['aspeed', 'ast2400-pinctrl', 'aspeed', 'g4-pinctrl'] has non-unique elements
> > > Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml: \
> > > properties:compatible:enum: ['aspeed', 'ast2500-pinctrl', 'aspeed', 'g5-pinctrl'] has non-unique elements
> > >
> > > Flow style sequences have to be quoted if the vales contain ','. Fix
> > > this by using the more common one line per entry formatting.
> >
> > >
> > > properties:
> > > compatible:
> > > - enum: [ aspeed,ast2400-pinctrl, aspeed,g4-pinctrl ]
> > > + enum:
> > > + - aspeed,ast2400-pinctrl
> > > + - aspeed,g4-pinctrl
> >
> > Thanks for the fix. However, we've standardised on the first form for
> > all of our device trees, so we can drop the second compatible string
> > from the bindings.
>
> Doing that would introduce validation warnings until the dts file is
> updated. So we still need this change until that happens.
My series takes care of that.
Andrew
^ permalink raw reply
* Re: [PATCH 2/3 v3] dt-bindings: gpio: aspeed: Add SGPIO support
From: Andrew Jeffery @ 2019-07-17 3:49 UTC (permalink / raw)
To: Hongwei Zhang, Joel Stanley, Linus Walleij, devicetree
Cc: Mark Rutland, linux-aspeed, linux-gpio, linux-kernel,
Bartosz Golaszewski, Rob Herring, linux-arm-kernel
In-Reply-To: <1563313711-17961-1-git-send-email-hongweiz@ami.com>
Hello Hongwei,
On Wed, 17 Jul 2019, at 07:18, Hongwei Zhang wrote:
> Add bindings to support SGPIO on AST2400 or AST2500.
>
> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> ---
> .../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 ++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> new file mode 100644
> index 0000000..8c3a747
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> @@ -0,0 +1,55 @@
> +Aspeed SGPIO controller Device Tree Bindings
> +-------------------------------------------
> +
> +This SGPIO controller is for ASPEED AST2500 SoC, it supports up to 80
> full
> +featured Serial GPIOs. Each of the Serial GPIO pins can be programmed
> to
> +support the following options:
> +- Support interrupt option for each input port and various interrupt
> + sensitivity option (level-high, level-low, edge-high, edge-low)
> +- Support reset tolerance option for each output port
> +- Directly connected to APB bus and its shift clock is from APB bus
> clock
> + divided by a programmable value.
> +- Co-work with external signal-chained TTL components (74LV165/74LV595)
Nice description.
> +
> +
> +Required properties:
> +
> +- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
> +
> +- #gpio-cells : Should be two
> + - First cell is the GPIO line number
> + - Second cell is used to specify optional
> + parameters (unused)
> +
> +- reg : Address and length of the register set for the device
> +- gpio-controller : Marks the device node as a GPIO controller.
> +- interrupts : Interrupt specifier (see interrupt bindings for
> + details)
> +
> +- interrupt-controller : Mark the GPIO controller as an
> interrupt-controller
> +
> +- nr-gpios : number of GPIO pins to serialise.
> + (should be multiple of 8, up to 80 pins; 0 if not used)
It's unclear to me what you mean by "0 if not used" here. The property is
required, so its description in a devicetree should always have a non-zero
value of `status = "okay";`, as 0 is an invalid value according to the
datasheet (sensibly so). If `status = "disabled";` then it doesn't really
matter, which makes the comment not terribly useful.
> +
> +- clocks : A phandle to the APB clock for SGPM clock
> division
> +
> +- bus-frequency : SGPM CLK frequency, derived from APB bus clock by a
> programmable devisor
I'd leave off the parent clock information. Practically speaking it's probably
always going to be the APB clock, but who knows. From a devicetree writer's
perspective they just want to say "make it 7MHz" or whatever speed they,
and it shouldn't matter too much how we get there.
Finally, as mentioned on the driver patch, please send v4 without the history
at the bottom.
Cheers,
Andrew
> +
> +
> +The sgpio and interrupt properties are further described in their
> respective bindings documentation:
> +
> +- Documentation/devicetree/bindings/sgpio/gpio.txt
> +- Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +
> + Example:
> + sgpio@1e780200 {
> + #gpio-cells = <2>;
> + compatible = "aspeed,ast2500-sgpio";
> + gpio-controller;
> + interrupts = <40>;
> + reg = <0x1e780200 0x0100>;
> + clocks = <&syscon ASPEED_CLK_APB>;
> + interrupt-controller;
> + nr-gpios = <8>;
> + bus-frequency = <12000000>;
> + };
> --
> 2.7.4
>
>
> Thanks Andrew, please see above v3 and inline comments at below.
> --Hongwei
>
> > From: Andrew Jeffery <andrew@aj.id.au>
> > Sent: Sunday, July 14, 2019 10:25 PM
> > To: Hongwei Zhang; Joel Stanley; Linus Walleij; devicetree@vger.kernel.org
> > Cc: Rob Herring; Mark Rutland; Bartosz Golaszewski; linux-aspeed@lists.ozlabs.org; linux-
> > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-gpio@vger.kernel.org
> > Subject: Re: [PATCH 2/3 v2] dt-bindings: gpio: aspeed: Add SGPIO support
> >
> > Hello Hongwei,
> >
> > On Sat, 13 Jul 2019, at 05:44, Hongwei Zhang wrote:
> > > Add bindings to support SGPIO on AST2400 or AST2500.
> > >
> > > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> > > ---
> > > .../devicetree/bindings/gpio/sgpio-aspeed.txt | 43 ++++++++++++++++++++++
> > > 1 file changed, 43 insertions(+)
> > > create mode 100755
> > > Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > > b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > > new file mode 100755
> > > index 0000000..3ae2b79
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > > @@ -0,0 +1,43 @@
> > > +Aspeed SGPIO controller Device Tree Bindings
> > > +-------------------------------------------
> > > +
> > > +Required properties:
> > > +- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
> > > +
> > > +- #gpio-cells : Should be two
> > > + - First cell is the GPIO line number
> > > + - Second cell is used to specify optional
> > > + parameters (unused)
> > > +
> > > +- reg : Address and length of the register set for the device
> > > +- gpio-controller : Marks the device node as a GPIO controller.
> > > +- interrupts : Interrupt specifier (see interrupt bindings for
> > > + details)
> > > +
> > > +- interrupt-controller : Mark the GPIO controller as an
> > > interrupt-controller
> > > +
> > > +- nr-gpios : number of GPIO pins to serialise. (should be multiple of
> > > 8, up to 80 pins)
> > > + if not specified, defaults to 80.
> >
> > This appears to be a statement about the driver implementation, but bindings documents are about
> > describing hardware. Reading the datasheet it actually appears the ASPEED SGPIO hardware comes up
> > in what is "technically" a forbidden state (equivalent to `nr-gpios = <0>;`), though the device is also
> > disabled at this point, so it's probably moot. The point is the true default value from a hardware
> > perspective is 0, not 80, so if we're going to talk about default values, 0 would be more appropriate.
> > However:
> >
> > You've also listed nr-gpios under the "Required properties" header, but the description suggests it's
> > optional. It's either one or the other, please lets be clear about it. On that front, lets make it nr-gpios
> > *not* optional (i.e. make it
> > required) thus force the specification of how many SGPIOs we want to emit on the bus. This value is
> > coupled to the platform design, so I don't think there's ever a scenario where we want nr-gpios to take a
> > default value.
> >
>
> Added some descriptions and updated nr-gpios, please see v3.
>
> > > +
> > > +- clocks : A phandle to the APB clock for SGPM clock
> > > division
> > > +
> > > +- bus-frequency : SGPM CLK frequency, if not specified defaults to 1
> > > MHz
> >
> > Again here with the default value - SGPM CLK period is derived from PCLK by the expression `period =
> > PCLK * 2 *(GPIO254[31:16] + 1)`, where GPIO254's initialisation state is `GPIO254[31:16] = 0`, which
> > gives a default SGPM bus frequency of PCLK / 2. This is likely not going to be 1MHz (more like ~12MHz).
> >
> > Lets just make the property required. That way we avoid any ambiguity about the bus frequency and
> > thus don't need words about defaults that turn out to be about the driver, not about the hardware.
> >
>
> updated, please see v3.
>
> > Finally, when updating patches in response to feedback, please send the full series again, and bump the
> > series version number. That way people can review a coherent set of patches and not have to hunt
> > around and (fail to) collate the correct combination. It makes it easier to say "Reviewed-by:" on your
> > patches :)
> >
> > Cheers,
> >
> > Andrew
>
^ permalink raw reply
* Re: [PATCH v4, 04/33] dt-bindings: mediatek: add dither description for mt8183 display
From: CK Hu @ 2019-07-17 3:40 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, David Airlie, linux-kernel, dri-devel,
Rob Herring, linux-mediatek, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1562625253-29254-5-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
This version is identical to previous version, and Rob has gave a
'Reviwed-by' tag on previous version, so you should keep that tag on
this version, so we don't need to review this patch again.
Regards,
CK
On Tue, 2019-07-09 at 06:33 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> Update device tree binding documention for the display subsystem for
> Mediatek MT8183 SOCs
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> index cf5fb08..afd3c90 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> @@ -33,6 +33,7 @@ Required properties (all function blocks):
> "mediatek,<chip>-disp-wdma" - write DMA
> "mediatek,<chip>-disp-ccorr" - color correction
> "mediatek,<chip>-disp-color" - color processor
> + "mediatek,<chip>-disp-dither" - dither
> "mediatek,<chip>-disp-aal" - adaptive ambient light controller
> "mediatek,<chip>-disp-gamma" - gamma correction
> "mediatek,<chip>-disp-merge" - merge streams from two RDMA sources
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v4, 03/33] dt-bindings: mediatek: add ccorr description for mt8183 display
From: CK Hu @ 2019-07-17 3:37 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, David Airlie, linux-kernel, dri-devel,
Rob Herring, linux-mediatek, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1562625253-29254-4-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
This version is identical to previous version, and Rob has gave a
'Reviwed-by' tag on previous version, so you should keep that tag on
this version, so we don't need to review this patch again.
Regards,
CK
On Tue, 2019-07-09 at 06:33 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> Update device tree binding documention for the display subsystem for
> Mediatek MT8183 SOCs
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> index 8c4700f..cf5fb08 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> @@ -31,6 +31,7 @@ Required properties (all function blocks):
> "mediatek,<chip>-disp-ovl-2l" - overlay (2 layers, blending, csc)
> "mediatek,<chip>-disp-rdma" - read DMA / line buffer
> "mediatek,<chip>-disp-wdma" - write DMA
> + "mediatek,<chip>-disp-ccorr" - color correction
> "mediatek,<chip>-disp-color" - color processor
> "mediatek,<chip>-disp-aal" - adaptive ambient light controller
> "mediatek,<chip>-disp-gamma" - gamma correction
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH] Input: atmel_mxt_ts - allow specification of config name
From: Dmitry Torokhov @ 2019-07-17 3:33 UTC (permalink / raw)
To: Lucas Stach
Cc: Nick Dyer, Rob Herring, linux-input, devicetree, kernel,
patchwork-lst, Chris Healy
In-Reply-To: <20190712135338.6211-1-l.stach@pengutronix.de>
Hi Lucas,
On Fri, Jul 12, 2019 at 03:53:38PM +0200, Lucas Stach wrote:
> From: Nick Dyer <nick@shmanahar.org>
>
> Some systems require different touchscreen configurations depending on the
> populated touchscreen. To allow different configs to co-exist in the file
> system we need a way to tell the driver, which config should be loaded.
>
> Signed-off-by: Nick Dyer <nick@shmanahar.org>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> lst:
> - switch to property name to not use underscore
> - Add more elaborate commit message
I believe we have already discussed this patch and back then I said that
the "atmel" prefix was not appropriate, as it describes not hardware
specifics but rather Linux kernel behavior and therefore we should be
using "linux,cfg-name" as the property. Unfortunately that version of
the patch got lost in my mailbox.
I will fish it out and apply.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v4, 01/33] dt-bindings: mediatek: add binding for mt8183 display
From: CK Hu @ 2019-07-17 3:22 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, David Airlie, linux-kernel, dri-devel,
Rob Herring, linux-mediatek, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1562625253-29254-2-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Tue, 2019-07-09 at 06:33 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> Update device tree binding documention for the display subsystem for
> Mediatek MT8183 SOCs
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> .../bindings/display/mediatek/mediatek,display.txt | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,display.txt
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,display.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,display.txt
> new file mode 100644
> index 0000000..951d2a8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,display.txt
> @@ -0,0 +1,21 @@
> +Mediatek Display Device
> +============================
> +
> +The Mediatek Display Device provides power control to the system.
> +
> +Required Properties:
> +
> +- compatible: Should be one of:
> + - "mediatek,mt8183-display"
I think this is "mediatek,mt8183-mmsys". In [1], Matthias has agreed
that we could keep work on his patch, so you should apply his patch and
remove this patch. In [2], I've found that MT8183 scpsys has some
problem with Matthias' patch, so please also fix this problem.
[1] https://patchwork.kernel.org/patch/10686327/
[2] https://patchwork.kernel.org/patch/11005731/
Regards,
CK
> +
> +The Display Device power name are defined in
> +include\dt-bindings\power\mt*-power.h
> +
> +
> +Example:
> +
> +display_components: dispsys@14000000 {
> + compatible = "mediatek,mt8183-display";
> + reg = <0 0x14000000 0 0x1000>;
> + power-domains = <&scpsys MT8183_POWER_DOMAIN_DISP>;
> +};
> \ No newline at end of file
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 2:35 UTC (permalink / raw)
To: Dmitry Osipenko, Peter De Schrijver, Joseph Lo
Cc: 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: <c6c0a205-c083-fd46-361c-175bd8840c6e@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.
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 2:18 UTC (permalink / raw)
To: Dmitry Osipenko, Peter De Schrijver, Joseph Lo
Cc: 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: <707c4679-fde6-1714-ced0-dcf7ca8380a9@nvidia.com>
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
^ permalink raw reply
* [PATCH v11 18/18] MAINTAINERS: add proc sysctl KUnit test to PROC SYSCTL section
From: Brendan Higgins @ 2019-07-17 1:55 UTC (permalink / raw)
To: frowand.list-Re5JQEeQqe8AvxtiuMwx3w,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
jpoimboe-H+wXaHxf7aLQT0dZR+AlfA, keescook-hpIqsD4AKlfQT0dZR+AlfA,
kieran.bingham-ryLnwIuWjnjg/C1BVhZhaw,
mcgrof-DgEjT+Ai2ygdnm+yROfE0A, peterz-wEGCiKHe2LqWVfeAwA7xHQ,
robh-DgEjT+Ai2ygdnm+yROfE0A, sboyd-DgEjT+Ai2ygdnm+yROfE0A,
shuah-DgEjT+Ai2ygdnm+yROfE0A, tytso-3s7WtUTddSA,
yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A
Cc: pmladek-IBi9RG/b67k, linux-doc-u79uwXL29TY76Z2rM5mHXA,
amir73il-Re5JQEeQqe8AvxtiuMwx3w, Brendan Higgins,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
Alexander.Levin-0li6OtcxBFHby3iVrkZq2A,
linux-kselftest-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
khilman-rdvid1DuHRBWk0Htik3J/w, knut.omang-QHcLZuEGTsvQT0dZR+AlfA,
wfg-VuQAYsv1563Yd54FQh9/CA, joel-U3u1mxZcP9KHXe+LvDLADg,
rientjes-hpIqsD4AKlfQT0dZR+AlfA, Iurii Zaikin,
jdike-OPE4K8JWMJJBDgjK7y7TUQ,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kbuild-u79uwXL29TY76Z2rM5mHXA, Tim.Bird-7U/KSKJipcs,
linux-um-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
rostedt-nx8X9YLhiw1AfugRpC6u6w, julia.lawall-L2FTfq7BK8M,
kunit-dev-/JYPxA39Uh5TLH3MbocFFw, richard-/L3Ra7n9ekc,
rdunlap-wEGCiKHe2LqWVfeAwA7xHQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, daniel-/w4YWyX8dFk,
mpe-Gsx/Oe8HsFggBc27wqDAHg, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20190717015543.152251-1-brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Add entry for the new proc sysctl KUnit test to the PROC SYSCTL section,
and add Iurii as a maintainer.
Signed-off-by: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Iurii Zaikin <yzaikin-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
Reviewed-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
Acked-by: Luis Chamberlain <mcgrof-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
MAINTAINERS | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 48d04d180a988..f8204c75114da 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12721,12 +12721,14 @@ F: Documentation/filesystems/proc.txt
PROC SYSCTL
M: Luis Chamberlain <mcgrof-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
M: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
+M: Iurii Zaikin <yzaikin-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
L: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
L: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
S: Maintained
F: fs/proc/proc_sysctl.c
F: include/linux/sysctl.h
F: kernel/sysctl.c
+F: kernel/sysctl-test.c
F: tools/testing/selftests/sysctl/
PS3 NETWORK SUPPORT
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply related
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