[v2 0/4] ARM: tegra: convert device tree files to use CLK defines

[v2 0/4] ARM: tegra: convert device tree files to use CLK defines

[Hiroshi Doyu]

Hi,

With new dtc+cpp feature, we can get rid of magic numbers in dts*
files. To get rid of Tegra clock magic number, the following patches
were created with a script run as below. This can be applied for
Tegra114 clock as well.

for x in 20 30; do
    cat drivers/clk/tegra/clk-tegra$x.c | enum2define.py \
    	> arch/arm/boot/dts/tegra$x-car.h 2>/tmp/tegra$x-car.sed
    sed -i -f /tmp/tegra$x-car.sed \
    	Documentation/devicetree/bindings/clock/nvidia\,tegra$x-car.txt \
	arch/arm/boot/dts/*
done

We plan to share those DT header files with kernel source later[1].

This series depends on:

  [PATCH 0/9] ARM: tegra: use new dtc+cpp feature
  http://lists.infradead.org/pipermail/linux-arm-kernel/2013-February/149613.html

v1:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-February/149672.html

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-February/149804.html

Hiroshi Doyu (4):
  ARM: tegra20: create a DT header defining CLK IDs
  ARM: tegra20: convert device tree files to use CLK defines
  ARM: tegra30: create a DT header defining CLK IDs
  ARM: tegra30: convert device tree files to use CLK defines

 .../bindings/clock/nvidia,tegra20-car.txt          |    2 +-
 .../bindings/clock/nvidia,tegra30-car.txt          |    2 +-
 arch/arm/boot/dts/tegra20-car.h                    |  114 +++++++++++++
 arch/arm/boot/dts/tegra20-paz00.dtsp               |    2 +-
 arch/arm/boot/dts/tegra20.dtsip                    |   85 +++++-----
 arch/arm/boot/dts/tegra30-car.h                    |  171 ++++++++++++++++++++
 arch/arm/boot/dts/tegra30.dtsip                    |   87 +++++-----
 7 files changed, 375 insertions(+), 88 deletions(-)
 create mode 100644 arch/arm/boot/dts/tegra20-car.h
 create mode 100644 arch/arm/boot/dts/tegra30-car.h

--
1.7.9.5

[v2 1/4] ARM: tegra20: create a DT header defining CLK IDs

[Stephen Warren]

On 02/14/2013 11:59 AM, Hiroshi Doyu wrote:
> To replace magic number in tegra_car:
> 
> -               clocks = <&tegra_car 28>;
> +               clocks = <&tegra_car CLK_HOST1X>;

> diff --git a/arch/arm/boot/dts/tegra20-car.h b/arch/arm/boot/dts/tegra20-car.h

Sorry, forgot a couple small comments the last time around.

This file should probably have some header indicating which binding it
describes, rather like the GPIO header in my patch series.

> +#define CLK_CPU 0

I'd suggest naming that TEGRA20_CLK_CPU, so that the various different
clock headers don't conflict. It's not too likely that more than one of
the /Tegra/ clock headers will be included at once, but it doesn't seem
that unlikely that a board file could end up having a Tegra clock header
included plus various other clock headers for some other chip that has
some clock outputs.

Oh, and I don't think you updated e.g. nvidia,tegra20-car.txt to remove
the list of clocks.

BTW, I assume this patch includes the changes from my recent "clk:
tegra: fix driver to match DT binding" and anything else similar that's
in the most recent Tegra for-next?

[v2 1/4] ARM: tegra20: create a DT header defining CLK IDs

[Hiroshi Doyu]

Stephen Warren <swarren@wwwdotorg.org> wrote @ Thu, 14 Feb 2013 21:15:28 +0100:

> Oh, and I don't think you updated e.g. nvidia,tegra20-car.txt to remove
> the list of clocks.

Not yet removed. I think that this could be done with the patch which
allows kernel source to include DT header files.

> BTW, I assume this patch includes the changes from my recent "clk:
> tegra: fix driver to match DT binding" and anything else similar that's
> in the most recent Tegra for-next?

Of course not, that's a little bit too recent.

I'll rerun my script against the laste Tegra for-next and post them as
v3(inc. Tegra114)

[v2 1/4] ARM: tegra20: create a DT header defining CLK IDs

[Stephen Warren]

On 02/14/2013 01:34 PM, Hiroshi Doyu wrote:
> Stephen Warren <swarren@wwwdotorg.org> wrote @ Thu, 14 Feb 2013 21:15:28 +0100:
> 
>> Oh, and I don't think you updated e.g. nvidia,tegra20-car.txt to remove
>> the list of clocks.
> 
> Not yet removed. I think that this could be done with the patch which
> allows kernel source to include DT header files.

By defining the header file, you're really causing that header to be
part of the binding definition. I think we should move the code from the
binding .txt file to the header, rather than duplicating it and then
removing it. The kernel doesn't include the binding .txt file right now,
so I don't see any correlation with making it include the header vs.
when we remove the list from the binding document.

[v2 1/4] ARM: tegra20: create a DT header defining CLK IDs

[Peter De Schrijver]

On Thu, Feb 14, 2013 at 09:15:28PM +0100, Stephen Warren wrote:
> On 02/14/2013 11:59 AM, Hiroshi Doyu wrote:
> > To replace magic number in tegra_car:
> > 
> > -               clocks = <&tegra_car 28>;
> > +               clocks = <&tegra_car CLK_HOST1X>;
> 
> > diff --git a/arch/arm/boot/dts/tegra20-car.h b/arch/arm/boot/dts/tegra20-car.h
> 
> Sorry, forgot a couple small comments the last time around.
> 
> This file should probably have some header indicating which binding it
> describes, rather like the GPIO header in my patch series.
> 
> > +#define CLK_CPU 0
> 
> I'd suggest naming that TEGRA20_CLK_CPU, so that the various different
> clock headers don't conflict. It's not too likely that more than one of
> the /Tegra/ clock headers will be included at once, but it doesn't seem
> that unlikely that a board file could end up having a Tegra clock header
> included plus various other clock headers for some other chip that has
> some clock outputs.
> 

I would suggest removing this clock. It's not actually implemented in the CCF
and rather useless. If you would gate the CPU clock from the CPU by writing to
this register, how would you ungate it? :) Note that this would gate the clock
to all CPUs.

Cheers,

Peter.

[v2 1/4] ARM: tegra20: create a DT header defining CLK IDs

[Stephen Warren]

On 02/15/2013 02:24 AM, Peter De Schrijver wrote:
> On Thu, Feb 14, 2013 at 09:15:28PM +0100, Stephen Warren wrote:
>> On 02/14/2013 11:59 AM, Hiroshi Doyu wrote:
>>> To replace magic number in tegra_car:
>>>
>>> -               clocks = <&tegra_car 28>;
>>> +               clocks = <&tegra_car CLK_HOST1X>;
>>
>>> diff --git a/arch/arm/boot/dts/tegra20-car.h b/arch/arm/boot/dts/tegra20-car.h
>>
>> Sorry, forgot a couple small comments the last time around.
>>
>> This file should probably have some header indicating which binding it
>> describes, rather like the GPIO header in my patch series.
>>
>>> +#define CLK_CPU 0
>>
>> I'd suggest naming that TEGRA20_CLK_CPU, so that the various different
>> clock headers don't conflict. It's not too likely that more than one of
>> the /Tegra/ clock headers will be included at once, but it doesn't seem
>> that unlikely that a board file could end up having a Tegra clock header
>> included plus various other clock headers for some other chip that has
>> some clock outputs.
>>
> 
> I would suggest removing this clock. It's not actually implemented in the CCF
> and rather useless. If you would gate the CPU clock from the CPU by writing to
> this register, how would you ungate it? :) Note that this would gate the clock
> to all CPUs.

(Note that my comment was re: all clocks, not just that one clock)

Can't the PMC or flow-controller ungate the clock based on some event?
Either way, that clock definition exists in HW, right? So I don't think
there's actually any harm in including the definition in the binding
even if we never implement/use it.

[v2 1/4] ARM: tegra20: create a DT header defining CLK IDs

[Peter De Schrijver]

On Fri, Feb 15, 2013 at 05:45:45PM +0100, Stephen Warren wrote:

...

> > I would suggest removing this clock. It's not actually implemented in the CCF
> > and rather useless. If you would gate the CPU clock from the CPU by writing to
> > this register, how would you ungate it? :) Note that this would gate the clock
> > to all CPUs.
> 
> (Note that my comment was re: all clocks, not just that one clock)
> 
> Can't the PMC or flow-controller ungate the clock based on some event?

I don't think the flow-controller controls this gate. The usual way of
clockgating a core is to execute a WFI instruction. That will trigger
clockgating the core, unless the flow-controller has been programmed to do
something else. The flow-controller will ungate the clock when there is an
interrupt.

> Either way, that clock definition exists in HW, right? So I don't think
> there's actually any harm in including the definition in the binding
> even if we never implement/use it.

The clock definition seems to exist in HW yes, the corresponding resetbit
however is marked as 'reserved' in the Tegra114 documentation.

Cheers,

Peter.

[v2 1/4] ARM: tegra20: create a DT header defining CLK IDs

[Peter De Schrijver]

On Thu, Feb 21, 2013 at 01:25:36PM +0100, Peter De Schrijver wrote:
> On Fri, Feb 15, 2013 at 05:45:45PM +0100, Stephen Warren wrote:
> 
> ...
> 
> > > I would suggest removing this clock. It's not actually implemented in the CCF
> > > and rather useless. If you would gate the CPU clock from the CPU by writing to
> > > this register, how would you ungate it? :) Note that this would gate the clock
> > > to all CPUs.
> > 
> > (Note that my comment was re: all clocks, not just that one clock)
> > 
> > Can't the PMC or flow-controller ungate the clock based on some event?
> 
> I don't think the flow-controller controls this gate. The usual way of
> clockgating a core is to execute a WFI instruction. That will trigger
> clockgating the core, unless the flow-controller has been programmed to do
> something else. The flow-controller will ungate the clock when there is an
> interrupt.
> 
> > Either way, that clock definition exists in HW, right? So I don't think
> > there's actually any harm in including the definition in the binding
> > even if we never implement/use it.
> 
> The clock definition seems to exist in HW yes, the corresponding resetbit
> however is marked as 'reserved' in the Tegra114 documentation.

Apparently from Tegra114 onwards, this bit no longer exists. In previous
chips, it is just turned by the bootloader running on the AVP and stays on
from there on. So at least for Tegra114 I think we remove this clock from
the DT binding.

Cheers,

Peter.