Re: [alsa-devel] [PATCH v3 03/15] soc: tegra: Add Tegra PMC clock registrations into PMC driver
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?
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.
- The force-enabling is applied to both OSC and EXTERN sources of
PMC_CLK_OUT_x by PMC at once.
- 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 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.
- The force-enabling is applied to both OSC and EXTERN sources of
PMC_CLK_OUT_x by PMC at once.
- 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.
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.
- The force-enabling is applied to both OSC and EXTERN sources of
PMC_CLK_OUT_x by PMC at once.
- 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.
- The force-enabling is applied to both OSC and EXTERN sources of
PMC_CLK_OUT_x by PMC at once.
- 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:
Keep the old CaR code in place.
Make CaR driver to scan whole device-tree for the legacy PMC clocks.
If legacy clock is found, then register PMC clocks from CaR.
If legacy clocks are not found, then don't register PMC clocks from
CaR.
- 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:
Keep the old CaR code in place.
Make CaR driver to scan whole device-tree for the legacy PMC clocks.
If legacy clock is found, then register PMC clocks from CaR.
If legacy clocks are not found, then don't register PMC clocks from
CaR.
- 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:
Keep the old CaR code in place.
Make CaR driver to scan whole device-tree for the legacy PMC clocks.
If legacy clock is found, then register PMC clocks from CaR.
If legacy clocks are not found, then don't register PMC clocks from
CaR.
- 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.
participants (2)
-
Dmitry Osipenko -
Peter De Schrijver