Messages in this thread | | | Subject | Re: [PATCH v3 03/15] soc: tegra: Add Tegra PMC clock registrations into PMC driver | From | Sowjanya Komatineni <> | Date | Wed, 11 Dec 2019 19:45:31 -0800 |
| |
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)
| |