[PATCH v2 8/9] Documentation: devicetree: Document sclk-jpeg clock for exynos3250 SoC

[PATCH v2 8/9] Documentation: devicetree: Document sclk-jpeg clock for exynos3250 SoC

[Jacek Anaszewski]

JPEG IP on Exynos3250 SoC requires enabling two clock
gates for its operation. This patch documents this
requirement.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: devicetree@vger.kernel.org
---
 .../bindings/media/exynos-jpeg-codec.txt           |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
index 937b755..3142745 100644
--- a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
+++ b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
@@ -3,9 +3,12 @@ Samsung S5P/EXYNOS SoC series JPEG codec
 Required properties:
 
 - compatible	: should be one of:
-		  "samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg";
+		  "samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg",
+		  "samsung,exynos3250-jpeg";
 - reg		: address and length of the JPEG codec IP register set;
 - interrupts	: specifies the JPEG codec IP interrupt;
-- clocks	: should contain the JPEG codec IP gate clock specifier, from the
+- clocks	: should contain the JPEG codec IP gate clock specifier and
+		  for the Exynos3250 SoC additionally the SCLK_JPEG entry; from the
 		  common clock bindings;
-- clock-names	: should contain "jpeg" entry.
+- clock-names	: should contain "jpeg" entry and additionally "sclk-jpeg" entry
+		  for Exynos3250 SoC
-- 
1.7.9.5

Re: [PATCH v2 8/9] Documentation: devicetree: Document sclk-jpeg clock for exynos3250 SoC

[Mark Rutland]

On Fri, Jul 11, 2014 at 04:19:49PM +0100, Jacek Anaszewski wrote:
> JPEG IP on Exynos3250 SoC requires enabling two clock
> gates for its operation. This patch documents this
> requirement.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: devicetree@vger.kernel.org
> ---
>  .../bindings/media/exynos-jpeg-codec.txt           |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
> index 937b755..3142745 100644
> --- a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
> +++ b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
> @@ -3,9 +3,12 @@ Samsung S5P/EXYNOS SoC series JPEG codec
>  Required properties:
>  
>  - compatible	: should be one of:
> -		  "samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg";
> +		  "samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg",
> +		  "samsung,exynos3250-jpeg";
>  - reg		: address and length of the JPEG codec IP register set;
>  - interrupts	: specifies the JPEG codec IP interrupt;
> -- clocks	: should contain the JPEG codec IP gate clock specifier, from the
> +- clocks	: should contain the JPEG codec IP gate clock specifier and
> +		  for the Exynos3250 SoC additionally the SCLK_JPEG entry; from the
>  		  common clock bindings;
> -- clock-names	: should contain "jpeg" entry.
> +- clock-names	: should contain "jpeg" entry and additionally "sclk-jpeg" entry
> +		  for Exynos3250 SoC

Please turn this into a list for easier reading, e.g.

- clock-names: should contain:
  * "jpeg" for the gate clock.
  * "sclk-jpeg" for the SCLK_JPEG clock (only for Exynos3250).

You could also define clocks in terms of clock-names to avoid
redundancy.

The SCLK_JPEG name sounds like a global name for the clock. Is there a
name for the input line on the JPEG block this is plugged into?

Thanks,
Mark.

Re: [PATCH v2 8/9] Documentation: devicetree: Document sclk-jpeg clock for exynos3250 SoC

[Sylwester Nawrocki]

On 14/07/14 11:56, Mark Rutland wrote:
>> diff --git a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
>> > index 937b755..3142745 100644
>> > --- a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
>> > +++ b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
>> > @@ -3,9 +3,12 @@ Samsung S5P/EXYNOS SoC series JPEG codec
>> >  Required properties:
>> >  
>> >  - compatible	: should be one of:
>> > -		  "samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg";
>> > +		  "samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg",
>> > +		  "samsung,exynos3250-jpeg";
>> >  - reg		: address and length of the JPEG codec IP register set;
>> >  - interrupts	: specifies the JPEG codec IP interrupt;
>> > -- clocks	: should contain the JPEG codec IP gate clock specifier, from the
>> > +- clocks	: should contain the JPEG codec IP gate clock specifier and
>> > +		  for the Exynos3250 SoC additionally the SCLK_JPEG entry; from the
>> >  		  common clock bindings;
>> > -- clock-names	: should contain "jpeg" entry.
>> > +- clock-names	: should contain "jpeg" entry and additionally "sclk-jpeg" entry
>> > +		  for Exynos3250 SoC
>
> Please turn this into a list for easier reading, e.g.
> 
> - clock-names: should contain:
>   * "jpeg" for the gate clock.
>   * "sclk-jpeg" for the SCLK_JPEG clock (only for Exynos3250).
> 
> You could also define clocks in terms of clock-names to avoid
> redundancy.
> 
> The SCLK_JPEG name sounds like a global name for the clock. Is there a
> name for the input line on the JPEG block this is plugged into?

There is unfortunately no such name for SCLK_JPEG clock in the IP's block
documentation. For most of the multimedia IPs clocks are documented
only in the clock controller chapter, hence the names may appear global.
Probably "gate", "sclk" would be good names, rather than "<IP_NAME>",
"<IP_NAME>-sclk". But people kept using the latter convention and now
it's spread all over and it's hard to change it.
Since now we can't rename "jpeg" and other IPs I'd assume it's best
to stay with "jpeg", "sclk-jpeg".

--
Regards,
Sylwester

Re: [PATCH v2 8/9] Documentation: devicetree: Document sclk-jpeg clock for exynos3250 SoC

[Arnd Bergmann]

On Tuesday 22 July 2014 13:42:00 Sylwester Nawrocki wrote:
> On 14/07/14 11:56, Mark Rutland wrote:
> >> diff --git a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
> >> > index 937b755..3142745 100644
> >> > --- a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
> >> > +++ b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
> >> > @@ -3,9 +3,12 @@ Samsung S5P/EXYNOS SoC series JPEG codec
> >> >  Required properties:
> >> >  
> >> >  - compatible      : should be one of:
> >> > -            "samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg";
> >> > +            "samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg",
> >> > +            "samsung,exynos3250-jpeg";
> >> >  - reg             : address and length of the JPEG codec IP register set;
> >> >  - interrupts      : specifies the JPEG codec IP interrupt;
> >> > -- clocks  : should contain the JPEG codec IP gate clock specifier, from the
> >> > +- clocks  : should contain the JPEG codec IP gate clock specifier and
> >> > +            for the Exynos3250 SoC additionally the SCLK_JPEG entry; from the
> >> >              common clock bindings;
> >> > -- clock-names     : should contain "jpeg" entry.
> >> > +- clock-names     : should contain "jpeg" entry and additionally "sclk-jpeg" entry
> >> > +            for Exynos3250 SoC
> >
> > Please turn this into a list for easier reading, e.g.
> > 
> > - clock-names: should contain:
> >   * "jpeg" for the gate clock.
> >   * "sclk-jpeg" for the SCLK_JPEG clock (only for Exynos3250).
> > 
> > You could also define clocks in terms of clock-names to avoid
> > redundancy.
> > 
> > The SCLK_JPEG name sounds like a global name for the clock. Is there a
> > name for the input line on the JPEG block this is plugged into?
> 
> There is unfortunately no such name for SCLK_JPEG clock in the IP's block
> documentation. For most of the multimedia IPs clocks are documented
> only in the clock controller chapter, hence the names may appear global.
> Probably "gate", "sclk" would be good names, rather than "<IP_NAME>",
> "<IP_NAME>-sclk". But people kept using the latter convention and now
> it's spread all over and it's hard to change it.
> Since now we can't rename "jpeg" and other IPs I'd assume it's best
> to stay with "jpeg", "sclk-jpeg".

We just had the exact same discussion about the addition of the sclk for
the adc in exynos3250 and ended up calling it just "sclk" instead of "sclk-adc"
there. I think it would be best to do the same here and use "sclk" instead
of "sclk-jpeg".

	Arnd

Re: [PATCH v2 8/9] Documentation: devicetree: Document sclk-jpeg clock for exynos3250 SoC

[Sylwester Nawrocki]

On 22/07/14 13:48, Arnd Bergmann wrote:
>>>>> -- clocks  : should contain the JPEG codec IP gate clock specifier, from the
>>>>> > >> > +- clocks  : should contain the JPEG codec IP gate clock specifier and
>>>>> > >> > +            for the Exynos3250 SoC additionally the SCLK_JPEG entry; from the
>>>>> > >> >              common clock bindings;
>>>>> > >> > -- clock-names     : should contain "jpeg" entry.
>>>>> > >> > +- clock-names     : should contain "jpeg" entry and additionally "sclk-jpeg" entry
>>>>> > >> > +            for Exynos3250 SoC
>>> > >
>>> > > Please turn this into a list for easier reading, e.g.
>>> > > 
>>> > > - clock-names: should contain:
>>> > >   * "jpeg" for the gate clock.
>>> > >   * "sclk-jpeg" for the SCLK_JPEG clock (only for Exynos3250).
>>> > > 
>>> > > You could also define clocks in terms of clock-names to avoid
>>> > > redundancy.
>>> > > 
>>> > > The SCLK_JPEG name sounds like a global name for the clock. Is there a
>>> > > name for the input line on the JPEG block this is plugged into?
>> > 
>> > There is unfortunately no such name for SCLK_JPEG clock in the IP's block
>> > documentation. For most of the multimedia IPs clocks are documented
>> > only in the clock controller chapter, hence the names may appear global.
>> > Probably "gate", "sclk" would be good names, rather than "<IP_NAME>",
>> > "<IP_NAME>-sclk". But people kept using the latter convention and now
>> > it's spread all over and it's hard to change it.
>> > Since now we can't rename "jpeg" and other IPs I'd assume it's best
>> > to stay with "jpeg", "sclk-jpeg".
>
> We just had the exact same discussion about the addition of the sclk for
> the adc in exynos3250 and ended up calling it just "sclk" instead of "sclk-adc"
> there. I think it would be best to do the same here and use "sclk" instead
> of "sclk-jpeg".

All right, then I would rephrase it to:

- clock-names   : should contain:
  		   - "jpeg" for the common gate clock,
		   - "sclk" for the special clock (only for Exynos3250).
- clocks	: should contain the clock specifier and clock ID list
  		  matching entries in the clock-names property, according
		  to the common clock bindings.

I went through documentation of these clocks in various SoCs' datasheets:
exynos4210, exynos4212/4412, exynos3250, exynos5250 and I think for all
SoCs the "jpeg" clock can be referred as "gating all clocks for the IP".
That means there is a single bit in a CMU register masking all the clocks
for the IP, I suppose this includes the control bus (APB) clock and the
IP functional ("special") clock.

It looks like e.g. exynos4412 also has the SCLK clock, after muxes and
a divider, so rate can be configured for this clock.  However there is
no separate gate for SCLK as in case of exynos3250. Thus there is no
need to to enable/disable the second clock on anything except exynos3250
currently.

I think ideally sclk should also be defined for SoCs like exynos4x12,
exynos5250, even if now drivers are not touching sclk. All in all the
IP functional clock frequency should be normally set to some known value,
now we rely on the default divider value which results in divider
ratio = 1.
It would break backward compatibility though if we now made sclk
mandatory. I'm inclined to also specify sclk for exynos4x12, just
not sure if it should be optional or mandatory.

-- 
Regards,
Sylwester

Re: [PATCH v2 8/9] Documentation: devicetree: Document sclk-jpeg clock for exynos3250 SoC

[Arnd Bergmann]

On Tuesday 22 July 2014 16:18:25 Sylwester Nawrocki wrote:
> 
> All right, then I would rephrase it to:
> 
> - clock-names   : should contain:
>                    - "jpeg" for the common gate clock,
>                    - "sclk" for the special clock (only for Exynos3250).
> - clocks        : should contain the clock specifier and clock ID list
>                   matching entries in the clock-names property, according
>                   to the common clock bindings.
> 
> I went through documentation of these clocks in various SoCs' datasheets:
> exynos4210, exynos4212/4412, exynos3250, exynos5250 and I think for all
> SoCs the "jpeg" clock can be referred as "gating all clocks for the IP".
> That means there is a single bit in a CMU register masking all the clocks
> for the IP, I suppose this includes the control bus (APB) clock and the
> IP functional ("special") clock.
> 
> It looks like e.g. exynos4412 also has the SCLK clock, after muxes and
> a divider, so rate can be configured for this clock.  However there is
> no separate gate for SCLK as in case of exynos3250. Thus there is no
> need to to enable/disable the second clock on anything except exynos3250
> currently.
> 
> I think ideally sclk should also be defined for SoCs like exynos4x12,
> exynos5250, even if now drivers are not touching sclk. All in all the
> IP functional clock frequency should be normally set to some known value,
> now we rely on the default divider value which results in divider
> ratio = 1.
> It would break backward compatibility though if we now made sclk
> mandatory. I'm inclined to also specify sclk for exynos4x12, just
> not sure if it should be optional or mandatory.

I'd vote for listing it as an optional clock independent of the compatible
string and changing the driver to just use it when it's provided.

	Arnd

Re: [PATCH v2 8/9] Documentation: devicetree: Document sclk-jpeg clock for exynos3250 SoC

[Sylwester Nawrocki]

On 22/07/14 16:44, Arnd Bergmann wrote:
> I'd vote for listing it as an optional clock independent of the compatible
> string and changing the driver to just use it when it's provided.

That sounds good to me, thanks for the suggestion.

-- 
Regards,
Sylwester