Re: [PATCH v2 00/11] Move PMC clocks into Tegra PMC driver

[Sowjanya Komatineni]

On 11/28/19 5:06 AM, Dmitry Osipenko wrote:
> 28.11.2019 00:38, Sowjanya Komatineni ÐÐÑÐÑ:
> > On 11/27/19 9:02 AM, Sowjanya Komatineni wrote:
> > > On 11/27/19 6:31 AM, Dmitry Osipenko wrote:
> > > > 27.11.2019 07:59, Sowjanya Komatineni ÐÐÑÐÑ:
> > > > > Tegra PMC has clk_out_1, clk_out_2, clk_out_3 and blink controls which
> > > > > are currently registered by Tegra clock driver using clk_regiser_mux
> > > > > and
> > > > > clk_register_gate which performs direct Tegra PMC register access.
> > > > >
> > > > > When Tegra PMC is in secure mode, any access from non-secure world will
> > > > > not go through.
> > > > >
> > > > > This patch series adds these Tegra PMC clocks and blink controls to
> > > > > Tegra
> > > > > PMC driver with PMC as clock provider and removed them from Tegra clock
> > > > > driver. This also adds PMC specific clock id's to use in device tree
> > > > > and
> > > > > removed clock ids of PMC clock from Tegra clock driver.
> > > > >
> > > > > This series also includes patch to update clock provider from tegra_car
> > > > > to pmc in the device tree tegra210-smaug.dts that uses clk_out_2
> > > > > from PMC.
> > > > >
> > > > > [v2]:ÂÂÂ Changes between v1 and v2 are
> > > > >  ÂÂÂÂ- v2 includes patches for adding clk_out_1, clk_out_2, clk_out_3,
> > > > >  ÂÂÂÂÂ blink controls to Tegra PMC driver and removing clk-tegra-pmc.
> > > > >  ÂÂÂÂ- feedback related to pmc clocks in Tegra PMC driver from v1
> > > > >  ÂÂÂÂ- Removed patches for WB0 PLLM overrides and PLLE IDDQ PMC
> > > > > programming
> > > > >  ÂÂÂÂÂ by the clock driver using helper functions from Tegra PMC.
> > > > >
> > > > >  ÂÂÂÂÂÂÂ Note:
> > > > >  ÂÂÂÂÂ To use helper functions from PMC driver, PMC early init need to
> > > > >  ÂÂÂÂÂ happen prior to using helper functions and these helper
> > > > > functions are
> > > > >  ÂÂÂÂÂ for PLLM Override and PLLE IDDQ programming in PMC during
> > > > > PLLM/PLLE
> > > > >  ÂÂÂÂÂ clock registration which happen in clock_init prior to Tegra PMC
> > > > >  ÂÂÂÂÂ probe.
> > > > >  ÂÂÂÂÂ Moving PLLM/PLLE clocks registration to happen after Tegra PMC
> > > > >  ÂÂÂÂÂ impacts other clocks EMC, MC and corresponding tegra_emc_init and
> > > > >  ÂÂÂÂÂ tegra_mc_init.
> > > > >  ÂÂÂÂÂ This implementation of configuring PMC registers thru helper
> > > > >  ÂÂÂÂÂ functions in clock driver needs proper changes across PMC, Clock,
> > > > >  ÂÂÂÂÂ EMC and MC inits to have it work across all Tegra platforms.
> > > > >
> > > > >  ÂÂÂÂÂ Currently PLLM Override is not enabled in the bootloader so
> > > > > proper
> > > > >  ÂÂÂÂÂ patches for this fix will be taken care separately.
> > > > Hello Sowjanya,
> > > >
> > > > Could you please clarify what do you mean by "PLLM Override not enabled
> > > > in bootloader"?
> > > >
> > > > There is T124 Nyan Big Chromebook which is supported in upstream kernel,
> > > > it has PLLM Override set by bootloader. I also have T30 Nexus 7 tablet
> > > > which has the PLLM Override set by bootloader as well. It's not clear to
> > > > me whether this patch series is supposed to break these devices. If the
> > > > breakage is the case here, then I'm afraid you can't postpone supporting
> > > > the PLLM Override and a full-featured implementation is needed.
> > > Hi Dmitry,
> > >
> > > Secure boot currently is enabled only on Tegra210 and Tegra210
> > > bootloader doesn't enable PLLM override.
> > >
> > > So PLLM override/PLLE IDDQ being in clock driver currently will not
> > > break on any of existing Tegra platforms.
> > >
> > > > I briefly tried to test this series on T30 and this time it doesn't hang
> > > > on boot, but somehow WiFi MMC card detection is broken. AFAIK, the WiFi
> > > > chip uses the Blink clock source and the clock should be enabled by the
> > > > MMC core because this is how DT part looks like:
> > > >
> > > > brcm_wifi_pwrseq: wifi-pwrseq {
> > > >  ÂÂÂÂcompatible = "mmc-pwrseq-simple";
> > > >  ÂÂÂÂclocks = <&pmc TEGRA_PMC_CLK_BLINK>;
> > > >  ÂÂÂÂclock-names = "ext_clock";
> > > >  ÂÂÂÂreset-gpios =Â <&gpio TEGRA_GPIO(D, 3) GPIO_ACTIVE_LOW>;
> > > >  ÂÂÂÂpost-power-on-delay-ms = <300>;
> > > >  ÂÂÂÂpower-off-delay-us = <300>;
> > > > };
> > > >
> > > > BTW, IÂ tried this series on a T20 device which also uses the Blink
> > > > clock for WiFi card and it works. So looks like this patchset has some
> > > > problem in regards to the T30 PMC clocks implementation.
> > > >
> > > > [snip]
> > > Blink init state is set to true for both Tegra20 and Tegra30 and all
> > > go through the same blink programming sequence.
> > >
> > > Will try to add more debug messages to dump registers and will test
> > > blink through device tree on T30 and will get back...
> > define value for BLINK uses BIT macro instead of just position. Will fix
> > this in v3.
> Thanks, will try v3 once it will be ready.
>
> I took a look through the T20 board's schematics and seems it doesn't
> use the Blink clock for the WiFi, instead it uses 32k source directly
> from PMU. While T30 board schematics tells that  32k comes out from the
> Tegra chip.
>
> BTW, I'm curious what's the reason for having Blink clock always-ON on
> T20/30, any insights? Looks like it's just some relic from old clk
> driver and it should be safe to drop the always-ON.

T30 cardhu uses 32K from Tegra to WIFI but its only needed to be on 
during WIFI power up sequence and not required to be on during boot.

I had it enabled as existing clock driver enables it default and dont 
want to break things if it was left ON intentionally.

Peter/Thierry, Any reason 32K from Blink is enabled to be ON during 
clock init for T20/T30 in clock-tegra-pmc driver?

Based on the design T30 uses this for WIFI and WIFI driver should be 
handling this clock enable/disable during power up/down sequence,

so we don't have to enable it default during boot right?