[v5,0/7] Couple improvements for Tegra clk driver

This series fixes couple minor standalone problems of the Tegra clk
driver.

Changelog:

v5: - Corrected example in the schema binding to silence dt_binding_check
      warning.

    - The Tegra124 binding is factored out into standalone binding since
      Tegra124 has properties that aren't used by other SoCs and I couldn't
      figure out how to make them conditional in schema.

v4: - Added new patch that converts DT bindings to schema.

v3: - Added acks from Thierry Reding that he gave to v2.

    - Added new patch "clk: tegra: Don't allow zero clock rate for PLLs".

v2: - Added these new patches:

      clk: tegra: Halve SCLK rate on Tegra20
      MAINTAINERS: Hand Tegra clk driver to Jon and Thierry

v1: - Collected clk patches into a single series.

Dmitry Osipenko (7):
  clk: tegra30: Use 300MHz for video decoder by default
  clk: tegra: Fix refcounting of gate clocks
  clk: tegra: Ensure that PLLU configuration is applied properly
  clk: tegra: Halve SCLK rate on Tegra20
  MAINTAINERS: Hand Tegra clk driver to Jon and Thierry
  clk: tegra: Don't allow zero clock rate for PLLs
  dt-bindings: clock: tegra: Convert to schema

 CREDITS                                       |   6 +
 .../bindings/clock/nvidia,tegra114-car.txt    |  63 ----------
 .../bindings/clock/nvidia,tegra124-car.txt    | 107 ----------------
 .../bindings/clock/nvidia,tegra124-car.yaml   | 115 ++++++++++++++++++
 .../bindings/clock/nvidia,tegra20-car.txt     |  63 ----------
 .../bindings/clock/nvidia,tegra20-car.yaml    |  69 +++++++++++
 .../bindings/clock/nvidia,tegra210-car.txt    |  56 ---------
 .../bindings/clock/nvidia,tegra30-car.txt     |  63 ----------
 MAINTAINERS                                   |   4 +-
 drivers/clk/tegra/clk-periph-gate.c           |  72 +++++++----
 drivers/clk/tegra/clk-periph.c                |  11 ++
 drivers/clk/tegra/clk-pll.c                   |  12 +-
 drivers/clk/tegra/clk-tegra20.c               |   6 +-
 drivers/clk/tegra/clk-tegra30.c               |   2 +-
 14 files changed, 261 insertions(+), 388 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra114-car.txt
 delete mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
 create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra124-car.yaml
 delete mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
 create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
 delete mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra210-car.txt
 delete mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra30-car.txt

On Wed, Mar 17, 2021 at 10:30:01PM +0300, Dmitry Osipenko wrote:
> The refcounting of the gate clocks has a bug causing the enable_refcnt

> to underflow when unused clocks are disabled. This happens because clk

> provider erroneously bumps the refcount if clock is enabled at a boot

> time, which it shouldn't be doing, and it does this only for the gate

> clocks, while peripheral clocks are using the same gate ops and the

> peripheral clocks are missing the initial bump. Hence the refcount of

> the peripheral clocks is 0 when unused clocks are disabled and then the

> counter is decremented further by the gate ops, causing the integer

> underflow.

[...]
> diff --git a/drivers/clk/tegra/clk-periph-gate.c b/drivers/clk/tegra/clk-periph-gate.c

> index 4b31beefc9fc..3c4259fec82e 100644

> --- a/drivers/clk/tegra/clk-periph-gate.c

> +++ b/drivers/clk/tegra/clk-periph-gate.c

[...]
> @@ -91,21 +108,28 @@ static void clk_periph_disable(struct clk_hw *hw)

>  

>  	spin_lock_irqsave(&periph_ref_lock, flags);

>  

> -	gate->enable_refcnt[gate->clk_num]--;

> -	if (gate->enable_refcnt[gate->clk_num] > 0) {

> -		spin_unlock_irqrestore(&periph_ref_lock, flags);

> -		return;

> -	}

> +	WARN_ON(!gate->enable_refcnt[gate->clk_num]);

> +

> +	if (gate->enable_refcnt[gate->clk_num]-- == 1)

> +		clk_periph_disable_locked(hw);


Nit: "if (--n == 0)" seems more natural, as you want to call
clk_periph_disable_locked() when the refcount goes down to 0.

[...]
>  	/*

> -	 * If peripheral is in the APB bus then read the APB bus to

> -	 * flush the write operation in apb bus. This will avoid the

> -	 * peripheral access after disabling clock

> +	 * Some clocks are duplicated and some of them are marked as critical,

> +	 * like fuse and fuse_burn for example, thus the enable_refcnt will

> +	 * be non-zero here id the "unused" duplicate is disabled by CCF.


s/id/if/ ?

Best Regards
Michał Mirosław

18.03.2021 12:12, Michał Mirosław пишет:
> On Wed, Mar 17, 2021 at 10:30:01PM +0300, Dmitry Osipenko wrote:

>> The refcounting of the gate clocks has a bug causing the enable_refcnt

>> to underflow when unused clocks are disabled. This happens because clk

>> provider erroneously bumps the refcount if clock is enabled at a boot

>> time, which it shouldn't be doing, and it does this only for the gate

>> clocks, while peripheral clocks are using the same gate ops and the

>> peripheral clocks are missing the initial bump. Hence the refcount of

>> the peripheral clocks is 0 when unused clocks are disabled and then the

>> counter is decremented further by the gate ops, causing the integer

>> underflow.

> [...]

>> diff --git a/drivers/clk/tegra/clk-periph-gate.c b/drivers/clk/tegra/clk-periph-gate.c

>> index 4b31beefc9fc..3c4259fec82e 100644

>> --- a/drivers/clk/tegra/clk-periph-gate.c

>> +++ b/drivers/clk/tegra/clk-periph-gate.c

> [...]

>> @@ -91,21 +108,28 @@ static void clk_periph_disable(struct clk_hw *hw)

>>  

>>  	spin_lock_irqsave(&periph_ref_lock, flags);

>>  

>> -	gate->enable_refcnt[gate->clk_num]--;

>> -	if (gate->enable_refcnt[gate->clk_num] > 0) {

>> -		spin_unlock_irqrestore(&periph_ref_lock, flags);

>> -		return;

>> -	}

>> +	WARN_ON(!gate->enable_refcnt[gate->clk_num]);

>> +

>> +	if (gate->enable_refcnt[gate->clk_num]-- == 1)

>> +		clk_periph_disable_locked(hw);

> 

> Nit: "if (--n == 0)" seems more natural, as you want to call

> clk_periph_disable_locked() when the refcount goes down to 0.

> 

> [...]

>>  	/*

>> -	 * If peripheral is in the APB bus then read the APB bus to

>> -	 * flush the write operation in apb bus. This will avoid the

>> -	 * peripheral access after disabling clock

>> +	 * Some clocks are duplicated and some of them are marked as critical,

>> +	 * like fuse and fuse_burn for example, thus the enable_refcnt will

>> +	 * be non-zero here id the "unused" duplicate is disabled by CCF.

> 

> s/id/if/ ?


I'll update this patch over the weekend, thanks!