On 7/4/19 8:22 PM, Jon Hunter wrote: > > On 14/06/2019 08:46, JC Kuo wrote: >> PLLE hardware power sequencer references PEX/SATA UPHY PLL hardware >> power sequencers' output to enable/disable PLLE. PLLE hardware power >> sequencer has to be enabled only after PEX/SATA UPHY PLL's sequencers >> are enabled. >> >> Signed-off-by: JC Kuo <jckuo@xxxxxxxxxx> >> --- >> drivers/clk/tegra/clk-pll.c | 12 ------------ >> 1 file changed, 12 deletions(-) >> >> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c >> index 1583f5fc992f..e6de65987fd2 100644 >> --- a/drivers/clk/tegra/clk-pll.c >> +++ b/drivers/clk/tegra/clk-pll.c >> @@ -2469,18 +2469,6 @@ static int clk_plle_tegra210_enable(struct clk_hw *hw) >> pll_writel(val, PLLE_SS_CTRL, pll); >> udelay(1); >> >> - val = pll_readl_misc(pll); >> - val &= ~PLLE_MISC_IDDQ_SW_CTRL; >> - pll_writel_misc(val, pll); >> - >> - val = pll_readl(pll->params->aux_reg, pll); >> - val |= (PLLE_AUX_USE_LOCKDET | PLLE_AUX_SS_SEQ_INCLUDE); >> - val &= ~(PLLE_AUX_ENABLE_SWCTL | PLLE_AUX_SS_SWCTL); >> - pll_writel(val, pll->params->aux_reg, pll); >> - udelay(1); >> - val |= PLLE_AUX_SEQ_ENABLE; >> - pll_writel(val, pll->params->aux_reg, pll); >> - >> out: >> if (pll->lock) >> spin_unlock_irqrestore(pll->lock, flags); >> > > So this function is called clk_plle_tegra210_enable() and is called by > the CCF enable callback. However, after the above change, does this mean > that this no longer enables the PLL? I understand that that is what you > want, but from an architecture perspective, it seems incorrect to have > an enable function that when called does not enable the PLL as expected. > > I don't fully understand why we need to add the new helpers from the > previous patch and we cannot use the CCF APIs directly? > > If you really need to split the existing enable function, then the CCF > does have prepare and enable callbacks that can be used. > > Cheers > Jon > Hi Jon, Thanks for review. With this change, clk_plle_tegra210_enable() still enables PLLE (by setting PLLE_BASE_ENABLE bit). It just skips the procedure to enable the hardware sequencer (controlled by PLLE_AUX_SEQ_ENABLE bit). PLLE has a hardware power sequencer logic which is a state machine that can power on/off PLLE without any software intervention. The sequencer has two inputs, one from XUSB UPHY PLL and the other from SATA UPHY PLL. PLLE provides reference clock to XUSB and SATA UPHY PLLs. When both of the downstream PLLs are powered-off, PLLE hardware power sequencer will automatically power off PLLE for power saving. XUSB and SATA UPHY PLLs also have their own hardware power sequencer logic. XUSB UPHY PLL is shared between XUSB SuperSpeed ports and PCIE controllers. The XUSB UPHY PLL hardware power sequencer has inputs from XUSB and PCIE. When all of the XUSB SuperSpeed ports and PCIE controllers are in low power state, XUSB UPHY PLL hardware power sequencer automatically power off PLL and signals idle to PLLE hardware power sequencer. Similar applies to SATA UPHY PLL. Therefore, before enabling PLLE hardware power sequencer, software must ensure that both XUSB and SATA UPHY PLLs hardware power sequencers are enabled properly. Tegra210 XUSB PADCTL driver (drivers/phy/tegra/xusb-tegra210.c) is in charge of enabling XUSB and SATA UPHY PLLs in software controlled power state. Sequencers are managed by CAR registers, so xusb-tegra210.c invokes the following APIs to get hardware power sequencers enabled. tegra210_xusb_pll_hw_control_enable() tegra210_xusb_pll_hw_sequence_start() tegra210_sata_pll_hw_control_enable() tegra210_sata_pll_hw_sequence_start() Thanks, JC
