| Message ID | 1405091990-28567-9-git-send-email-j.anaszewski@samsung.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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