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

[Sowjanya Komatineni <skomatineni@nvidia.com>]

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)