Re: [PATCH v3 03/15] soc: tegra: Add Tegra PMC clock registrations into PMC driver

[Sowjanya Komatineni <skomatineni@nvidia.com>]

On 12/11/19 7:45 PM, Sowjanya Komatineni wrote:
>
> On 12/11/19 5:39 PM, Dmitry Osipenko wrote:
>> 11.12.2019 21:50, Sowjanya Komatineni пишет:
>>> On 12/10/19 5:06 PM, Sowjanya Komatineni wrote:
>>>> On 12/10/19 9:41 AM, Dmitry Osipenko wrote:
>>>>> 10.12.2019 19:53, Sowjanya Komatineni пишет:
>>>>>> On 12/9/19 3:03 PM, Sowjanya Komatineni wrote:
>>>>>>> On 12/9/19 12:46 PM, Sowjanya Komatineni wrote:
>>>>>>>> On 12/9/19 12:12 PM, Dmitry Osipenko wrote:
>>>>>>>>> 08.12.2019 00:36, Sowjanya Komatineni пишет:
>>>>>>>>>> On 12/7/19 11:59 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>> On 12/7/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>> 07.12.2019 18:53, Dmitry Osipenko пишет:
>>>>>>>>>>>>> 07.12.2019 18:47, Dmitry Osipenko пишет:
>>>>>>>>>>>>>> 07.12.2019 17:28, Dmitry Osipenko пишет:
>>>>>>>>>>>>>>> 06.12.2019 05:48, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>> Tegra210 and prior Tegra PMC has clk_out_1, clk_out_2,
>>>>>>>>>>>>>>>> clk_out_3
>>>>>>>>>>>>>>>> with
>>>>>>>>>>>>>>>> mux and gate for each of these clocks.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Currently these PMC clocks are registered by Tegra clock
>>>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>>>> using
>>>>>>>>>>>>>>>> clk_register_mux and clk_register_gate by passing PMC base
>>>>>>>>>>>>>>>> address
>>>>>>>>>>>>>>>> and register offsets and PMC programming for these clocks
>>>>>>>>>>>>>>>> happens
>>>>>>>>>>>>>>>> through direct PMC access by the clock driver.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> With this, when PMC is in secure mode any direct PMC 
>>>>>>>>>>>>>>>> access
>>>>>>>>>>>>>>>> from the
>>>>>>>>>>>>>>>> non-secure world does not go through and these clocks will
>>>>>>>>>>>>>>>> not be
>>>>>>>>>>>>>>>> functional.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This patch adds these clocks registration with PMC as a 
>>>>>>>>>>>>>>>> clock
>>>>>>>>>>>>>>>> provider
>>>>>>>>>>>>>>>> for these clocks. clk_ops callback implementations for 
>>>>>>>>>>>>>>>> these
>>>>>>>>>>>>>>>> clocks
>>>>>>>>>>>>>>>> uses tegra_pmc_readl and tegra_pmc_writel which 
>>>>>>>>>>>>>>>> supports PMC
>>>>>>>>>>>>>>>> programming
>>>>>>>>>>>>>>>> in secure mode and non-secure mode.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni 
>>>>>>>>>>>>>>>> <skomatineni@nvidia.com>
>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>> [snip]
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> +static const struct clk_ops pmc_clk_gate_ops = {
>>>>>>>>>>>>>>>> +    .is_enabled = pmc_clk_is_enabled,
>>>>>>>>>>>>>>>> +    .enable = pmc_clk_enable,
>>>>>>>>>>>>>>>> +    .disable = pmc_clk_disable,
>>>>>>>>>>>>>>>> +};
>>>>>>>>>>>>>>> What's the benefit of separating GATE from the MUX?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I think it could be a single clock.
>>>>>>>>>>>>>> According to TRM:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 1. GATE and MUX are separate entities.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 2. GATE is the parent of MUX (see PMC's CLK_OUT paths 
>>>>>>>>>>>>>> diagram
>>>>>>>>>>>>>> in TRM).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 3. PMC doesn't gate EXTPERIPH clock but could "force-enable"
>>>>>>>>>>>>>> it,
>>>>>>>>>>>>>> correct?
>>>>>>>>>> Was following existing clk-tegra-pmc as I am not sure of 
>>>>>>>>>> reason for
>>>>>>>>>> having these clocks registered as separate mux and gate clocks.
>>>>>>>>>>
>>>>>>>>>> Yes, PMC clocks can be registered as single clock and can use
>>>>>>>>>> clk_ops
>>>>>>>>>> for set/get parent and enable/disable.
>>>>>>>>>>
>>>>>>>>>> enable/disable of PMC clocks is for force-enable to force the
>>>>>>>>>> clock to
>>>>>>>>>> run regardless of ACCEPT_REQ or INVERT_REQ.
>>>>>>>>>>
>>>>>>>>>>>>> 4. clk_m_div2/4 are internal PMC OSC dividers and thus these
>>>>>>>>>>>>> clocks
>>>>>>>>>>>>> should belong to PMC.
>>>>>>>>>>>> Also, it should be "osc" and not "clk_m".
>>>>>>>>>>> I followed the same parents as it were in existing 
>>>>>>>>>>> clk-tegra-pmc
>>>>>>>>>>> driver.
>>>>>>>>>>>
>>>>>>>>>>> Yeah they are wrong and they should be from osc and not clk_m.
>>>>>>>>>>>
>>>>>>>>>>> Will fix in next version.
>>>>>>>>>>>
>>>>>>> Reg clk_m_div2/3, they are dividers at OSC pad and not really 
>>>>>>> internal
>>>>>>> to PMC block.
>>>>>>>
>>>>>>> current clock driver creates clk_m_div clocks which should 
>>>>>>> actually be
>>>>>>> osc_div2/osc_div4 clocks with osc as parent.
>>>>>>>
>>>>>>> There are no clk_m_div2 and clk_m_div4 from clk_m
>>>>>>>
>>>>>>> Will fix this in next version.
>>>>>>>
>>>>>>>>> Could you please describe the full EXTPERIPH clock topology and
>>>>>>>>> how the
>>>>>>>>> pinmux configuration is related to it all?
>>>>>>>>>
>>>>>>>>> What is internal to the Tegra chip and what are the external
>>>>>>>>> outputs?
>>>>>>>>>
>>>>>>>>> Is it possible to bypass PMC on T30+ for the EXTPERIPH clocks?
>>>>>>>> PMC CLK1/2/3 possible sources are OSC_DIV1, OSC_DIV2, OSC_DIV4,
>>>>>>>> EXTPERIPH from CAR.
>>>>>>>>
>>>>>>>> OSC_DIV1/2/4 are with internal dividers at the OSC Pads
>>>>>>>>
>>>>>>>> EXTPERIPH is from CAR and it has reset and enable controls 
>>>>>>>> along with
>>>>>>>> clock source selections to choose one of the PLLA_OUT0, CLK_S,
>>>>>>>> PLLP_OUT0, CLK_M, PLLE_OUT0
>>>>>>>>
>>>>>>>> So, PMC CLK1/2/4 possible parents are OSC_DIV1, OSC_DIV2, 
>>>>>>>> OSC_DIV4,
>>>>>>>> EXTERN.
>>>>>>>>
>>>>>>>>
>>>>>>>> CLK1/2/3 also has Pinmux to route EXTPERIPH output on to these 
>>>>>>>> pins.
>>>>>>>>
>>>>>>>>
>>>>>>>> When EXTERN output clock is selected for these PMC clocks thru
>>>>>>>> CLKx_SRC_SEL, output clock is from driver by EXTPERIPH from CAR 
>>>>>>>> via
>>>>>>>> Pinmux logic or driven as per CLKx_SRC_SEL bypassing pinmux 
>>>>>>>> based on
>>>>>>>> CLKx_ACCEPT_REQ bit.
>>>>>>>>
>>>>>>>>
>>>>>>>> PMC Clock control register has bit CLKx_ACCEPT_REQ
>>>>>>>> When CLKx_ACCEPT_REQ = 0, output clock driver is from by EXTPERIPH
>>>>>>>> through the pinmux
>>>>>>>> When CLKx_ACCEPT_REQ = 1, output clock is based on CLKx_SRC_SEL 
>>>>>>>> bits
>>>>>>>> (OSC_DIV1/2/4 and EXTPERIPH clock bypassing the pinmux)
>>>>>>>>
>>>>>>>> FORCE_EN bit in PMC CLock control register forces the clock to run
>>>>>>>> regardless of this.
>>>>>> PMC clock gate is based on the state of CLKx_ACCEPT_REQ and FORCE_EN
>>>>>> like explained above.
>>>>>>
>>>>>> CLKx_ACCEPT_REQ is 0 default and FORCE_EN acts as gate to
>>>>>> enable/disable
>>>>>> EXTPERIPH clock output to PMC CLK_OUT_1/2/3.
>>>>> [and to enable OSC as well]
>>>>>
>>>>>> So I believe we need to register as MUX and Gate rather than as a
>>>>>> single
>>>>>> clock. Please confirm.
>>>>> 1. The force-enabling is applied to both OSC and EXTERN sources of
>>>>> PMC_CLK_OUT_x by PMC at once.
>>>>>
>>>>> 2. Both of PMC's force-enabling and OSC/EXTERN selection is internal
>>>>> to PMC.
>>>>>
>>>>> Should be better to define it as a single "pmc_clk_out_x". I don't 
>>>>> see
>>>>> any good reasons for differentiating PMC's Gate from the MUX, it's a
>>>>> single hardware unit from a point of view of the rest of the system.
>>>>>
>>>>> Peter, do you have any objections?
>>>> We added fallback option for audio mclk and also added check for
>>>> assigned-clock-parents dt property in audio driver and if its not then
>>>> we do parent init configuration in audio driver.
>>>>
>>>> Current clock driver creates 2 separate clocks clk_out_1_mux and
>>>> clk_out_1 for each pmc clock in clock driver and uses extern1 as
>>>> parent to clk_out_1_mux and clk_out_1_mux is parent to clk_out_1.
>>>>
>>>> With change of registering each pmc clock as a single clock, when we
>>>> do parent init assignment in audio driver when
>>>> assigned-clock-properties are not used in DT (as we removed parent
>>>> inits for extern and clk_outs from clock driver), we should still try
>>>> to get clock based on clk_out_1_mux as parent assignment of extern1 is
>>>> for clk_out_1_mux as per existing clock tree.
>>>>
>>>> clk_out_1_mux clock retrieve will fail with this change of single
>>>> clock when any new platform device tree doesn't specify
>>>> assigned-clock-parents properties and tegra_asoc_utils_init fails.
>> You made the PMC/CaR changes before the audio changes, the clk_out_1_mux
>> won't exist for the audio driver patches.
>>
>> If you care about bisect-ability of the patches, then the clock and
>> audio changes need to be done in a single patch. But I don't think that
>> it's worthwhile.
>>
>>>> With single clock, extern1 is the parent for clk_out_1 and with
>>>> separate clocks for mux and gate, extern1 is the parent for
>>>> clk_out_1_mux.
>>> If we move to single clock now, it need one more additional fallback
>>> implementation in audio driver during parent configuration as
>>> clk_out_1_mux will not be there with single clock change and 
>>> old/current
>>> kernel has it as it uses separate clocks for pmc mux and gate.
>> Why additional fallback? Additional to what?
>>
>>> Also, with single clock for both PMC mux and gate now, new DT should 
>>> use
>>> extern1 as parent to CLK_OUT_1 as CLK_OUT_1_MUX will not be there old
>>> PMC dt-bindings has separate clocks for MUX (CLK_OUT_1_MUX) and gate
>>> (CLK_OUT_1)
>>>
>>> DT bindings will not be compatible b/w old and new changes if we 
>>> move to
>>> Single PMC clock now.
>> Sorry, I don't understand what you're meaning by the "new changes".
>>
>>> Should we go with same separate clocks to have it compatible to avoid
>>> all this?
>>>
> The reason we added mclk fallback and also for doing parent 
> configuration based on presence of assigned-clock-parents property is 
> to have old dt compatible with new kernel and also to have new dt 
> compatible with old kernel.
>
> So the point I was mentioning is to have new DT to work with old 
> kernel, setting extern1 as parent to clk_out_1 (with single pmc clock) 
> through assigned-clock-parents in DT will fail as old kernel has mux 
> and gate as separate clocks and parent configuration is for mux clock 
> (clk_out_1_mux)
>
Sorry never mind, with old kernel clock driver does all parent 
configuration so should be ok. So no additional fallbacks are needed 
except to the one we already added.

OK, So its just that changes are slightly more to switch to single clock 
compared to using separate clocks as gate clk_ops (which are needed 
anyway for blink control) of clock enable and disable can't be used for 
clk_out_1 enable/disable and need additional clk_enable and disable 
callbacks.

Will make changes to use single clock..