| Message ID | 1575600535-26877-4-git-send-email-skomatineni@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Move PMC clocks into Tegra PMC driver | expand |
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> > --- > drivers/soc/tegra/pmc.c | 305 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 305 insertions(+) > > diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c > index ea0e11a09c12..b8f6eb0ed8aa 100644 > --- a/drivers/soc/tegra/pmc.c > +++ b/drivers/soc/tegra/pmc.c > @@ -13,6 +13,9 @@ > > #include <linux/arm-smccc.h> > #include <linux/clk.h> > +#include <linux/clk-provider.h> > +#include <linux/clkdev.h> > +#include <linux/clk/clk-conf.h> > #include <linux/clk/tegra.h> > #include <linux/debugfs.h> > #include <linux/delay.h> > @@ -48,6 +51,7 @@ > #include <dt-bindings/pinctrl/pinctrl-tegra-io-pad.h> > #include <dt-bindings/gpio/tegra186-gpio.h> > #include <dt-bindings/gpio/tegra194-gpio.h> > +#include <dt-bindings/soc/tegra-pmc.h> > > #define PMC_CNTRL 0x0 > #define PMC_CNTRL_INTR_POLARITY BIT(17) /* inverts INTR polarity */ > @@ -100,6 +104,7 @@ > #define PMC_WAKE2_STATUS 0x168 > #define PMC_SW_WAKE2_STATUS 0x16c > > +#define PMC_CLK_OUT_CNTRL 0x1a8 > #define PMC_SENSOR_CTRL 0x1b0 > #define PMC_SENSOR_CTRL_SCRATCH_WRITE BIT(2) > #define PMC_SENSOR_CTRL_ENABLE_RST BIT(1) > @@ -155,6 +160,83 @@ > #define TEGRA_SMC_PMC_READ 0xaa > #define TEGRA_SMC_PMC_WRITE 0xbb > > +struct pmc_clk_mux { > + struct clk_hw hw; > + unsigned long offs; > + u32 mask; > + u32 shift; > +}; > + > +#define to_pmc_clk_mux(_hw) container_of(_hw, struct pmc_clk_mux, hw) > + > +struct pmc_clk_gate { > + struct clk_hw hw; > + unsigned long offs; > + u32 shift; > +}; > + > +#define to_pmc_clk_gate(_hw) container_of(_hw, struct pmc_clk_gate, hw) > + > +struct pmc_clk_init_data { > + char *mux_name; > + char *gate_name; > + const char **parents; > + int num_parents; > + int mux_id; > + int gate_id; > + char *dev_name; > + u8 mux_shift; > + u8 gate_shift; > +}; > + > +static const char *clk_out1_parents[] = { "clk_m", "clk_m_div2", > + "clk_m_div4", "extern1", > +}; > + > +static const char *clk_out2_parents[] = { "clk_m", "clk_m_div2", > + "clk_m_div4", "extern2", > +}; > + > +static const char *clk_out3_parents[] = { "clk_m", "clk_m_div2", > + "clk_m_div4", "extern3", > +}; > + > +static const struct pmc_clk_init_data tegra_pmc_clks_data[] = { > + { > + .mux_name = "clk_out_1_mux", > + .gate_name = "clk_out_1", > + .parents = clk_out1_parents, > + .num_parents = ARRAY_SIZE(clk_out1_parents), > + .mux_id = TEGRA_PMC_CLK_OUT_1_MUX, > + .gate_id = TEGRA_PMC_CLK_OUT_1, > + .dev_name = "extern1", > + .mux_shift = 6, > + .gate_shift = 2, > + }, > + { > + .mux_name = "clk_out_2_mux", > + .gate_name = "clk_out_2", > + .parents = clk_out2_parents, > + .num_parents = ARRAY_SIZE(clk_out2_parents), > + .mux_id = TEGRA_PMC_CLK_OUT_2_MUX, > + .gate_id = TEGRA_PMC_CLK_OUT_2, > + .dev_name = "extern2", > + .mux_shift = 14, > + .gate_shift = 10, > + }, > + { > + .mux_name = "clk_out_3_mux", > + .gate_name = "clk_out_3", > + .parents = clk_out3_parents, > + .num_parents = ARRAY_SIZE(clk_out3_parents), > + .mux_id = TEGRA_PMC_CLK_OUT_3_MUX, > + .gate_id = TEGRA_PMC_CLK_OUT_3, > + .dev_name = "extern3", > + .mux_shift = 22, > + .gate_shift = 18, > + }, > +}; > + > struct tegra_powergate { > struct generic_pm_domain genpd; > struct tegra_pmc *pmc; > @@ -254,6 +336,9 @@ struct tegra_pmc_soc { > */ > const struct tegra_wake_event *wake_events; > unsigned int num_wake_events; > + > + const struct pmc_clk_init_data *pmc_clks_data; > + unsigned int num_pmc_clks; > }; > > static const char * const tegra186_reset_sources[] = { > @@ -2163,6 +2248,211 @@ static int tegra_pmc_clk_notify_cb(struct notifier_block *nb, > return NOTIFY_OK; > } > > +static void pmc_clk_fence_udelay(u32 offset) > +{ > + tegra_pmc_readl(pmc, offset); > + /* pmc clk propagation delay 2 us */ > + udelay(2); > +} > + > +static u8 pmc_clk_mux_get_parent(struct clk_hw *hw) > +{ > + struct pmc_clk_mux *mux = to_pmc_clk_mux(hw); > + int num_parents = clk_hw_get_num_parents(hw); > + u32 val; > + > + val = tegra_pmc_readl(pmc, mux->offs) >> mux->shift; > + val &= mux->mask; > + > + if (val >= num_parents) > + return -EINVAL; How this could ever happen? Why are you returning negative value for u8? It doesn't different from returning val >= num_parents. > + return val; > +} > + > +static int pmc_clk_mux_set_parent(struct clk_hw *hw, u8 index) > +{ > + struct pmc_clk_mux *mux = to_pmc_clk_mux(hw); > + u32 val; > + > + val = tegra_pmc_readl(pmc, mux->offs); > + val &= ~(mux->mask << mux->shift); > + val |= index << mux->shift; > + tegra_pmc_writel(pmc, val, mux->offs); > + pmc_clk_fence_udelay(mux->offs); > + > + return 0; > +} > + > +static const struct clk_ops pmc_clk_mux_ops = { > + .get_parent = pmc_clk_mux_get_parent, > + .set_parent = pmc_clk_mux_set_parent, > + .determine_rate = __clk_mux_determine_rate, > +}; > + > +static struct clk * > +tegra_pmc_clk_mux_register(const char *name, const char * const *parent_names, > + int num_parents, unsigned long flags, > + unsigned long offset, u32 shift, u32 mask) > +{ > + struct clk_init_data init; > + struct pmc_clk_mux *mux; > + > + mux = kzalloc(sizeof(*mux), GFP_KERNEL); > + if (!mux) > + return ERR_PTR(-ENOMEM); > + > + init.name = name; > + init.ops = &pmc_clk_mux_ops; > + init.parent_names = parent_names; > + init.num_parents = num_parents; > + init.flags = flags; > + > + mux->hw.init = &init; > + mux->offs = offset; > + mux->mask = mask; > + mux->shift = shift; > + > + return clk_register(NULL, &mux->hw); > +} > + > +static int pmc_clk_is_enabled(struct clk_hw *hw) > +{ > + struct pmc_clk_gate *gate = to_pmc_clk_gate(hw); > + > + return tegra_pmc_readl(pmc, gate->offs) & BIT(gate->shift) ? 1 : 0; > +} > + > +static void pmc_clk_set_state(struct clk_hw *hw, int state) > +{ > + struct pmc_clk_gate *gate = to_pmc_clk_gate(hw); > + u32 val; > + > + val = tegra_pmc_readl(pmc, gate->offs); > + val = state ? (val | BIT(gate->shift)) : (val & ~BIT(gate->shift)); > + tegra_pmc_writel(pmc, val, gate->offs); > + pmc_clk_fence_udelay(gate->offs); > +} > + > +static int pmc_clk_enable(struct clk_hw *hw) > +{ > + pmc_clk_set_state(hw, 1); > + > + return 0; > +} > + > +static void pmc_clk_disable(struct clk_hw *hw) > +{ > + pmc_clk_set_state(hw, 0); > +} > + > +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. > +static struct clk * > +tegra_pmc_clk_gate_register(const char *name, const char *parent_name, > + unsigned long flags, unsigned long offset, > + u32 shift) > +{ > + struct clk_init_data init; > + struct pmc_clk_gate *gate; > + > + gate = kzalloc(sizeof(*gate), GFP_KERNEL); > + if (!gate) > + return ERR_PTR(-ENOMEM); > + > + init.name = name; > + init.ops = &pmc_clk_gate_ops; > + init.parent_names = &parent_name; > + init.num_parents = 1; > + init.flags = flags; > + > + gate->hw.init = &init; > + gate->offs = offset; > + gate->shift = shift; > + > + return clk_register(NULL, &gate->hw); > +} > + > +static void tegra_pmc_clock_register(struct tegra_pmc *pmc, > + struct device_node *np) > +{ > + struct clk *clkmux, *clk; > + struct clk_onecell_data *clk_data; > + unsigned int num_clks; > + int i, ret; > + > + /* each pmc clock output has a mux and a gate */ > + num_clks = pmc->soc->num_pmc_clks * 2; > + > + if (!num_clks) > + return; > + > + clk_data = kmalloc(sizeof(*clk_data), GFP_KERNEL); > + if (!clk_data) > + return; > + > + clk_data->clks = kcalloc(TEGRA_PMC_CLK_MAX, sizeof(*clk_data->clks), > + GFP_KERNEL); > + if (!clk_data->clks) > + goto free_clkdata; > + > + clk_data->clk_num = TEGRA_PMC_CLK_MAX; > + > + for (i = 0; i < TEGRA_PMC_CLK_MAX; i++) > + clk_data->clks[i] = ERR_PTR(-ENOENT); > + > + for (i = 0; i < pmc->soc->num_pmc_clks; i++) { > + const struct pmc_clk_init_data *data; > + > + data = pmc->soc->pmc_clks_data + i; > + > + clkmux = tegra_pmc_clk_mux_register(data->mux_name, > + data->parents, > + data->num_parents, > + CLK_SET_RATE_NO_REPARENT | > + CLK_SET_RATE_PARENT, > + PMC_CLK_OUT_CNTRL, > + data->mux_shift, 3); > + if (IS_ERR(clkmux)) > + goto free_clks; > + > + clk_data->clks[data->mux_id] = clkmux; > + > + clk = tegra_pmc_clk_gate_register(data->gate_name, > + data->mux_name, > + CLK_SET_RATE_PARENT, > + PMC_CLK_OUT_CNTRL, > + data->gate_shift); > + if (IS_ERR(clk)) > + goto free_clks; > + > + clk_data->clks[data->gate_id] = clk; > + > + ret = clk_set_parent(clk, clkmux); > + if (ret < 0) { > + pr_err("failed to set parent of %s to %s: %d\n", > + __clk_get_name(clk), > + __clk_get_name(clkmux), ret); > + } is this really needed? GATE clock has a single parent, the MUX.
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? [snip]
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? 4. clk_m_div2/4 are internal PMC OSC dividers and thus these clocks should belong to PMC.
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? > > 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".
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. >> 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?
09.12.2019 23:46, Sowjanya Komatineni пишет: > > 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. >>>> >> 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 Are you sure that EXTPERIPH has a reset? What will it reset? Why it's not documented in TRM? > 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. Could you please clarify what are "these" pins? Perhaps you meant the EXTERN pin of PMC? > 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. Okay.
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?
10.12.2019 20:48, Sowjanya Komatineni пишет: > > On 12/10/19 9:41 AM, Dmitry Osipenko wrote: >> 09.12.2019 23:46, Sowjanya Komatineni пишет: >>> 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. >>>>>> >>>> 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 >> Are you sure that EXTPERIPH has a reset? What will it reset? Why it's >> not documented in TRM? > Yes, Extperiph1/2/3 has RST part of CAR RST_DEVICES_V bits 24/25/26 Are these bits not documented in a public TRMs? I checked T30/114/124/210 TRMs and CLK_RST_CONTROLLER_RST_DEVICES_V_0 doesn't have those bits in the docs. >>> 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. >> Could you please clarify what are "these" pins? Perhaps you meant the >> EXTERN pin of PMC? > By CLK1/2/3 pins, I am referring to CLK_OUT_1/2/3 pins from Tegra I see now what you meant, thanks. [snip}
10.12.2019 22:18, Sowjanya Komatineni пишет: > > On 12/10/19 10:30 AM, Dmitry Osipenko wrote: >> 10.12.2019 20:48, Sowjanya Komatineni пишет: >>> On 12/10/19 9:41 AM, Dmitry Osipenko wrote: >>>> 09.12.2019 23:46, Sowjanya Komatineni пишет: >>>>> 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. >>>>>>>> >>>>>> 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 >>>> Are you sure that EXTPERIPH has a reset? What will it reset? Why it's >>>> not documented in TRM? >>> Yes, Extperiph1/2/3 has RST part of CAR RST_DEVICES_V bits 24/25/26 >> Are these bits not documented in a public TRMs? I checked >> T30/114/124/210 TRMs and CLK_RST_CONTROLLER_RST_DEVICES_V_0 doesn't have >> those bits in the docs. >> > Yeah these bits are missing in all Tegra TRM docs. Will request for > having EXTPERIPH reset bits to be updated in TRM... Thanks
On Tue, Dec 10, 2019 at 08:41:56PM +0300, Dmitry Osipenko wrote: .. > > > > 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? The reason to have separate gate and mux clocks, is to preserve compatibility with existing users. Otherwise the current users would need to figure out if there's a single clock or 2 clocks to configure. I don't think adding that code in each user is worth it only to have a sligthly nicer modelling of the hardware. Cheers, Peter.
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? >
11.12.2019 18:10, Peter De Schrijver пишет: > On Tue, Dec 10, 2019 at 08:41:56PM +0300, Dmitry Osipenko wrote: > > .. > >>> >>> 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? > > The reason to have separate gate and mux clocks, is to preserve compatibility > with existing users. > Otherwise the current users would need to figure out if there's a > single clock or 2 clocks to configure. I don't think adding that code in > each user is worth it only to have a sligthly nicer modelling of the > hardware. Could you please clarify what do you mean by the "existing users"? AFAIK, nothing in kernel uses mux clocks.
12.12.2019 06:54, Sowjanya Komatineni пишет: > > 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.. Please wait for the Peter's reply.
On Thu, Dec 12, 2019 at 04:43:53AM +0300, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 11.12.2019 18:10, Peter De Schrijver пишет: > > On Tue, Dec 10, 2019 at 08:41:56PM +0300, Dmitry Osipenko wrote: > > > > .. > > > >>> > >>> 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? > > > > The reason to have separate gate and mux clocks, is to preserve compatibility > > with existing users. > > Otherwise the current users would need to figure out if there's a > > single clock or 2 clocks to configure. I don't think adding that code in > > each user is worth it only to have a sligthly nicer modelling of the > > hardware. > > Could you please clarify what do you mean by the "existing users"? > AFAIK, nothing in kernel uses mux clocks. The DT clk bindings allow for parent initialization, so it's certainly possible there are some DTs which rely on this. We promised to never break the bindings, which changing to 1 clock would do. Peter.
16.12.2019 15:20, Peter De Schrijver пишет: > On Thu, Dec 12, 2019 at 04:43:53AM +0300, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 11.12.2019 18:10, Peter De Schrijver пишет: >>> On Tue, Dec 10, 2019 at 08:41:56PM +0300, Dmitry Osipenko wrote: >>> >>> .. >>> >>>>> >>>>> 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? >>> >>> The reason to have separate gate and mux clocks, is to preserve compatibility >>> with existing users. >>> Otherwise the current users would need to figure out if there's a >>> single clock or 2 clocks to configure. I don't think adding that code in >>> each user is worth it only to have a sligthly nicer modelling of the >>> hardware. >> >> Could you please clarify what do you mean by the "existing users"? >> AFAIK, nothing in kernel uses mux clocks. > > The DT clk bindings allow for parent initialization, so it's certainly > possible there are some DTs which rely on this. We promised to never > break the bindings, which changing to 1 clock would do. What about this variant: 1. Keep the old CaR code in place. 2. Make CaR driver to scan whole device-tree for the legacy PMC clocks. 3. If legacy clock is found, then register PMC clocks from CaR. 4. If legacy clocks are not found, then don't register PMC clocks from CaR. 5. Add clocks support to the PMC driver and only register them if legacy clocks are not registered by CaR. Now both old and new DTBs can co-exist and work, everyone happy.
On Mon, Dec 16, 2019 at 05:23:23PM +0300, Dmitry Osipenko wrote: > >> Could you please clarify what do you mean by the "existing users"? > >> AFAIK, nothing in kernel uses mux clocks. > > > > The DT clk bindings allow for parent initialization, so it's certainly > > possible there are some DTs which rely on this. We promised to never > > break the bindings, which changing to 1 clock would do. > > What about this variant: > > 1. Keep the old CaR code in place. > > 2. Make CaR driver to scan whole device-tree for the legacy PMC clocks. > > 3. If legacy clock is found, then register PMC clocks from CaR. > > 4. If legacy clocks are not found, then don't register PMC clocks from > CaR. > > 5. Add clocks support to the PMC driver and only register them if > legacy clocks are not registered by CaR. > > Now both old and new DTBs can co-exist and work, everyone happy. That seems even more work.. Today the only upstream user is audio. Currently these clocks are setup by the CAR clock driver. However as they will move to the PMC driver, this mechanism cannot be used. Hence the plan is to modify the audio driver to check for the PMC clocks in DT and if they are not available use the CAR clocks as fallback. The whole reason the clocks move to the PMC driver, is that when PMC becomes secure, all accesses have to go via an SMC. Given that we don't want SMCs all over the Tegra drivers, it's a good opportunity to move the PMC clock handling into the PMC driver. PMC can be secure with both 'new' and old DTs, so just registering the PMC clocks in the CAR driver if legacy clocks are found in the DT, won't always work. Peter.
On Mon, Dec 16, 2019 at 05:11:32PM +0200, Peter De Schrijver wrote: > External email: Use caution opening links or attachments > > > On Mon, Dec 16, 2019 at 05:23:23PM +0300, Dmitry Osipenko wrote: > > >> Could you please clarify what do you mean by the "existing users"? > > >> AFAIK, nothing in kernel uses mux clocks. > > > > > > The DT clk bindings allow for parent initialization, so it's certainly > > > possible there are some DTs which rely on this. We promised to never > > > break the bindings, which changing to 1 clock would do. > > > > What about this variant: > > > > 1. Keep the old CaR code in place. > > > > 2. Make CaR driver to scan whole device-tree for the legacy PMC clocks. > > > > 3. If legacy clock is found, then register PMC clocks from CaR. > > > > 4. If legacy clocks are not found, then don't register PMC clocks from > > CaR. > > > > 5. Add clocks support to the PMC driver and only register them if > > legacy clocks are not registered by CaR. > > > > Now both old and new DTBs can co-exist and work, everyone happy. > > That seems even more work.. Today the only upstream user is audio. > Currently these clocks are setup by the CAR clock driver. However > as they will move to the PMC driver, this mechanism cannot be used. > Hence the plan is to modify the audio driver to check for the PMC clocks > in DT and if they are not available use the CAR clocks as fallback. > The whole reason the clocks move to the PMC driver, is that when PMC > becomes secure, all accesses have to go via an SMC. Given that we don't > want SMCs all over the Tegra drivers, it's a good opportunity to move > the PMC clock handling into the PMC driver. PMC can be secure with both > 'new' and old DTs, so just registering the PMC clocks in the CAR driver > if legacy clocks are found in the DT, won't always work. > Given the audio driver needs to change anyway, we can indeed use 1 clock per PMC clk_out_ rather than 2 as we have today. As this models the hw slightly better, I think we should do that as you suggested. Peter.
16.12.2019 18:24, Peter De Schrijver пишет: > On Mon, Dec 16, 2019 at 05:11:32PM +0200, Peter De Schrijver wrote: >> External email: Use caution opening links or attachments >> >> >> On Mon, Dec 16, 2019 at 05:23:23PM +0300, Dmitry Osipenko wrote: >>>>> Could you please clarify what do you mean by the "existing users"? >>>>> AFAIK, nothing in kernel uses mux clocks. >>>> >>>> The DT clk bindings allow for parent initialization, so it's certainly >>>> possible there are some DTs which rely on this. We promised to never >>>> break the bindings, which changing to 1 clock would do. >>> >>> What about this variant: >>> >>> 1. Keep the old CaR code in place. >>> >>> 2. Make CaR driver to scan whole device-tree for the legacy PMC clocks. >>> >>> 3. If legacy clock is found, then register PMC clocks from CaR. >>> >>> 4. If legacy clocks are not found, then don't register PMC clocks from >>> CaR. >>> >>> 5. Add clocks support to the PMC driver and only register them if >>> legacy clocks are not registered by CaR. >>> >>> Now both old and new DTBs can co-exist and work, everyone happy. >> >> That seems even more work.. Today the only upstream user is audio. >> Currently these clocks are setup by the CAR clock driver. However >> as they will move to the PMC driver, this mechanism cannot be used. >> Hence the plan is to modify the audio driver to check for the PMC clocks >> in DT and if they are not available use the CAR clocks as fallback. >> The whole reason the clocks move to the PMC driver, is that when PMC >> becomes secure, all accesses have to go via an SMC. Given that we don't >> want SMCs all over the Tegra drivers, it's a good opportunity to move >> the PMC clock handling into the PMC driver. PMC can be secure with both >> 'new' and old DTs, so just registering the PMC clocks in the CAR driver >> if legacy clocks are found in the DT, won't always work. >> > > Given the audio driver needs to change anyway, we can indeed use 1 clock > per PMC clk_out_ rather than 2 as we have today. As this models the hw > slightly better, I think we should do that as you suggested. > > Peter. > Okay, let's try and see how it will go.
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c index ea0e11a09c12..b8f6eb0ed8aa 100644 --- a/drivers/soc/tegra/pmc.c +++ b/drivers/soc/tegra/pmc.c @@ -13,6 +13,9 @@ #include <linux/arm-smccc.h> #include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/clkdev.h> +#include <linux/clk/clk-conf.h> #include <linux/clk/tegra.h> #include <linux/debugfs.h> #include <linux/delay.h> @@ -48,6 +51,7 @@ #include <dt-bindings/pinctrl/pinctrl-tegra-io-pad.h> #include <dt-bindings/gpio/tegra186-gpio.h> #include <dt-bindings/gpio/tegra194-gpio.h> +#include <dt-bindings/soc/tegra-pmc.h> #define PMC_CNTRL 0x0 #define PMC_CNTRL_INTR_POLARITY BIT(17) /* inverts INTR polarity */ @@ -100,6 +104,7 @@ #define PMC_WAKE2_STATUS 0x168 #define PMC_SW_WAKE2_STATUS 0x16c +#define PMC_CLK_OUT_CNTRL 0x1a8 #define PMC_SENSOR_CTRL 0x1b0 #define PMC_SENSOR_CTRL_SCRATCH_WRITE BIT(2) #define PMC_SENSOR_CTRL_ENABLE_RST BIT(1) @@ -155,6 +160,83 @@ #define TEGRA_SMC_PMC_READ 0xaa #define TEGRA_SMC_PMC_WRITE 0xbb +struct pmc_clk_mux { + struct clk_hw hw; + unsigned long offs; + u32 mask; + u32 shift; +}; + +#define to_pmc_clk_mux(_hw) container_of(_hw, struct pmc_clk_mux, hw) + +struct pmc_clk_gate { + struct clk_hw hw; + unsigned long offs; + u32 shift; +}; + +#define to_pmc_clk_gate(_hw) container_of(_hw, struct pmc_clk_gate, hw) + +struct pmc_clk_init_data { + char *mux_name; + char *gate_name; + const char **parents; + int num_parents; + int mux_id; + int gate_id; + char *dev_name; + u8 mux_shift; + u8 gate_shift; +}; + +static const char *clk_out1_parents[] = { "clk_m", "clk_m_div2", + "clk_m_div4", "extern1", +}; + +static const char *clk_out2_parents[] = { "clk_m", "clk_m_div2", + "clk_m_div4", "extern2", +}; + +static const char *clk_out3_parents[] = { "clk_m", "clk_m_div2", + "clk_m_div4", "extern3", +}; + +static const struct pmc_clk_init_data tegra_pmc_clks_data[] = { + { + .mux_name = "clk_out_1_mux", + .gate_name = "clk_out_1", + .parents = clk_out1_parents, + .num_parents = ARRAY_SIZE(clk_out1_parents), + .mux_id = TEGRA_PMC_CLK_OUT_1_MUX, + .gate_id = TEGRA_PMC_CLK_OUT_1, + .dev_name = "extern1", + .mux_shift = 6, + .gate_shift = 2, + }, + { + .mux_name = "clk_out_2_mux", + .gate_name = "clk_out_2", + .parents = clk_out2_parents, + .num_parents = ARRAY_SIZE(clk_out2_parents), + .mux_id = TEGRA_PMC_CLK_OUT_2_MUX, + .gate_id = TEGRA_PMC_CLK_OUT_2, + .dev_name = "extern2", + .mux_shift = 14, + .gate_shift = 10, + }, + { + .mux_name = "clk_out_3_mux", + .gate_name = "clk_out_3", + .parents = clk_out3_parents, + .num_parents = ARRAY_SIZE(clk_out3_parents), + .mux_id = TEGRA_PMC_CLK_OUT_3_MUX, + .gate_id = TEGRA_PMC_CLK_OUT_3, + .dev_name = "extern3", + .mux_shift = 22, + .gate_shift = 18, + }, +}; + struct tegra_powergate { struct generic_pm_domain genpd; struct tegra_pmc *pmc; @@ -254,6 +336,9 @@ struct tegra_pmc_soc { */ const struct tegra_wake_event *wake_events; unsigned int num_wake_events; + + const struct pmc_clk_init_data *pmc_clks_data; + unsigned int num_pmc_clks; }; static const char * const tegra186_reset_sources[] = { @@ -2163,6 +2248,211 @@ static int tegra_pmc_clk_notify_cb(struct notifier_block *nb, return NOTIFY_OK; } +static void pmc_clk_fence_udelay(u32 offset) +{ + tegra_pmc_readl(pmc, offset); + /* pmc clk propagation delay 2 us */ + udelay(2); +} + +static u8 pmc_clk_mux_get_parent(struct clk_hw *hw) +{ + struct pmc_clk_mux *mux = to_pmc_clk_mux(hw); + int num_parents = clk_hw_get_num_parents(hw); + u32 val; + + val = tegra_pmc_readl(pmc, mux->offs) >> mux->shift; + val &= mux->mask; + + if (val >= num_parents) + return -EINVAL; + + return val; +} + +static int pmc_clk_mux_set_parent(struct clk_hw *hw, u8 index) +{ + struct pmc_clk_mux *mux = to_pmc_clk_mux(hw); + u32 val; + + val = tegra_pmc_readl(pmc, mux->offs); + val &= ~(mux->mask << mux->shift); + val |= index << mux->shift; + tegra_pmc_writel(pmc, val, mux->offs); + pmc_clk_fence_udelay(mux->offs); + + return 0; +} + +static const struct clk_ops pmc_clk_mux_ops = { + .get_parent = pmc_clk_mux_get_parent, + .set_parent = pmc_clk_mux_set_parent, + .determine_rate = __clk_mux_determine_rate, +}; + +static struct clk * +tegra_pmc_clk_mux_register(const char *name, const char * const *parent_names, + int num_parents, unsigned long flags, + unsigned long offset, u32 shift, u32 mask) +{ + struct clk_init_data init; + struct pmc_clk_mux *mux; + + mux = kzalloc(sizeof(*mux), GFP_KERNEL); + if (!mux) + return ERR_PTR(-ENOMEM); + + init.name = name; + init.ops = &pmc_clk_mux_ops; + init.parent_names = parent_names; + init.num_parents = num_parents; + init.flags = flags; + + mux->hw.init = &init; + mux->offs = offset; + mux->mask = mask; + mux->shift = shift; + + return clk_register(NULL, &mux->hw); +} + +static int pmc_clk_is_enabled(struct clk_hw *hw) +{ + struct pmc_clk_gate *gate = to_pmc_clk_gate(hw); + + return tegra_pmc_readl(pmc, gate->offs) & BIT(gate->shift) ? 1 : 0; +} + +static void pmc_clk_set_state(struct clk_hw *hw, int state) +{ + struct pmc_clk_gate *gate = to_pmc_clk_gate(hw); + u32 val; + + val = tegra_pmc_readl(pmc, gate->offs); + val = state ? (val | BIT(gate->shift)) : (val & ~BIT(gate->shift)); + tegra_pmc_writel(pmc, val, gate->offs); + pmc_clk_fence_udelay(gate->offs); +} + +static int pmc_clk_enable(struct clk_hw *hw) +{ + pmc_clk_set_state(hw, 1); + + return 0; +} + +static void pmc_clk_disable(struct clk_hw *hw) +{ + pmc_clk_set_state(hw, 0); +} + +static const struct clk_ops pmc_clk_gate_ops = { + .is_enabled = pmc_clk_is_enabled, + .enable = pmc_clk_enable, + .disable = pmc_clk_disable, +}; + +static struct clk * +tegra_pmc_clk_gate_register(const char *name, const char *parent_name, + unsigned long flags, unsigned long offset, + u32 shift) +{ + struct clk_init_data init; + struct pmc_clk_gate *gate; + + gate = kzalloc(sizeof(*gate), GFP_KERNEL); + if (!gate) + return ERR_PTR(-ENOMEM); + + init.name = name; + init.ops = &pmc_clk_gate_ops; + init.parent_names = &parent_name; + init.num_parents = 1; + init.flags = flags; + + gate->hw.init = &init; + gate->offs = offset; + gate->shift = shift; + + return clk_register(NULL, &gate->hw); +} + +static void tegra_pmc_clock_register(struct tegra_pmc *pmc, + struct device_node *np) +{ + struct clk *clkmux, *clk; + struct clk_onecell_data *clk_data; + unsigned int num_clks; + int i, ret; + + /* each pmc clock output has a mux and a gate */ + num_clks = pmc->soc->num_pmc_clks * 2; + + if (!num_clks) + return; + + clk_data = kmalloc(sizeof(*clk_data), GFP_KERNEL); + if (!clk_data) + return; + + clk_data->clks = kcalloc(TEGRA_PMC_CLK_MAX, sizeof(*clk_data->clks), + GFP_KERNEL); + if (!clk_data->clks) + goto free_clkdata; + + clk_data->clk_num = TEGRA_PMC_CLK_MAX; + + for (i = 0; i < TEGRA_PMC_CLK_MAX; i++) + clk_data->clks[i] = ERR_PTR(-ENOENT); + + for (i = 0; i < pmc->soc->num_pmc_clks; i++) { + const struct pmc_clk_init_data *data; + + data = pmc->soc->pmc_clks_data + i; + + clkmux = tegra_pmc_clk_mux_register(data->mux_name, + data->parents, + data->num_parents, + CLK_SET_RATE_NO_REPARENT | + CLK_SET_RATE_PARENT, + PMC_CLK_OUT_CNTRL, + data->mux_shift, 3); + if (IS_ERR(clkmux)) + goto free_clks; + + clk_data->clks[data->mux_id] = clkmux; + + clk = tegra_pmc_clk_gate_register(data->gate_name, + data->mux_name, + CLK_SET_RATE_PARENT, + PMC_CLK_OUT_CNTRL, + data->gate_shift); + if (IS_ERR(clk)) + goto free_clks; + + clk_data->clks[data->gate_id] = clk; + + ret = clk_set_parent(clk, clkmux); + if (ret < 0) { + pr_err("failed to set parent of %s to %s: %d\n", + __clk_get_name(clk), + __clk_get_name(clkmux), ret); + } + + clk_register_clkdev(clk, data->dev_name, data->gate_name); + } + + of_clk_add_provider(np, of_clk_src_onecell_get, clk_data); + + return; + +free_clks: + kfree(clk_data->clks); +free_clkdata: + kfree(clk_data); + WARN(1, "failed to register Tegra PMC clocks\n"); +} + static int tegra_pmc_probe(struct platform_device *pdev) { void __iomem *base; @@ -2281,6 +2571,7 @@ static int tegra_pmc_probe(struct platform_device *pdev) pmc->base = base; mutex_unlock(&pmc->powergates_lock); + tegra_pmc_clock_register(pmc, pdev->dev.of_node); platform_set_drvdata(pdev, pmc); return 0; @@ -2422,6 +2713,8 @@ static const struct tegra_pmc_soc tegra20_pmc_soc = { .num_reset_sources = 0, .reset_levels = NULL, .num_reset_levels = 0, + .pmc_clks_data = NULL, + .num_pmc_clks = 0, }; static const char * const tegra30_powergates[] = { @@ -2469,6 +2762,8 @@ static const struct tegra_pmc_soc tegra30_pmc_soc = { .num_reset_sources = ARRAY_SIZE(tegra30_reset_sources), .reset_levels = NULL, .num_reset_levels = 0, + .pmc_clks_data = tegra_pmc_clks_data, + .num_pmc_clks = ARRAY_SIZE(tegra_pmc_clks_data), }; static const char * const tegra114_powergates[] = { @@ -2520,6 +2815,8 @@ static const struct tegra_pmc_soc tegra114_pmc_soc = { .num_reset_sources = ARRAY_SIZE(tegra30_reset_sources), .reset_levels = NULL, .num_reset_levels = 0, + .pmc_clks_data = tegra_pmc_clks_data, + .num_pmc_clks = ARRAY_SIZE(tegra_pmc_clks_data), }; static const char * const tegra124_powergates[] = { @@ -2631,6 +2928,8 @@ static const struct tegra_pmc_soc tegra124_pmc_soc = { .num_reset_sources = ARRAY_SIZE(tegra30_reset_sources), .reset_levels = NULL, .num_reset_levels = 0, + .pmc_clks_data = tegra_pmc_clks_data, + .num_pmc_clks = ARRAY_SIZE(tegra_pmc_clks_data), }; static const char * const tegra210_powergates[] = { @@ -2745,6 +3044,8 @@ static const struct tegra_pmc_soc tegra210_pmc_soc = { .num_reset_levels = 0, .num_wake_events = ARRAY_SIZE(tegra210_wake_events), .wake_events = tegra210_wake_events, + .pmc_clks_data = tegra_pmc_clks_data, + .num_pmc_clks = ARRAY_SIZE(tegra_pmc_clks_data), }; #define TEGRA186_IO_PAD_TABLE(_pad) \ @@ -2874,6 +3175,8 @@ static const struct tegra_pmc_soc tegra186_pmc_soc = { .num_reset_levels = ARRAY_SIZE(tegra186_reset_levels), .num_wake_events = ARRAY_SIZE(tegra186_wake_events), .wake_events = tegra186_wake_events, + .pmc_clks_data = NULL, + .num_pmc_clks = 0, }; static const struct tegra_io_pad_soc tegra194_io_pads[] = { @@ -2991,6 +3294,8 @@ static const struct tegra_pmc_soc tegra194_pmc_soc = { .num_reset_levels = ARRAY_SIZE(tegra186_reset_levels), .num_wake_events = ARRAY_SIZE(tegra194_wake_events), .wake_events = tegra194_wake_events, + .pmc_clks_data = NULL, + .num_pmc_clks = 0, }; static const struct of_device_id tegra_pmc_match[] = {
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> --- drivers/soc/tegra/pmc.c | 305 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 305 insertions(+)