| Message ID | 1463560892-7209-3-git-send-email-gabriel.fernandez@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 18, 2016 at 10:41:23AM +0200, Gabriel Fernandez wrote: > This patch allows fine tuning of the quads FS for audio clocks > accuracy. > > Signed-off-by: Olivier Bideau <olivier.bideau@st.com> > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org> > --- > .../devicetree/bindings/clock/st/st,flexgen.txt | 1 + > drivers/clk/st/clk-flexgen.c | 24 ++++++++++++++++++++++ > 2 files changed, 25 insertions(+) > > diff --git a/Documentation/devicetree/bindings/clock/st/st,flexgen.txt b/Documentation/devicetree/bindings/clock/st/st,flexgen.txt > index b7ee5c7..15b33c7 100644 > --- a/Documentation/devicetree/bindings/clock/st/st,flexgen.txt > +++ b/Documentation/devicetree/bindings/clock/st/st,flexgen.txt > @@ -60,6 +60,7 @@ This binding uses the common clock binding[2]. > Required properties: > - compatible : shall be: > "st,flexgen" > + "st,stih407-clkgend0", "st,flexgen" (enable clock propagation on parent) What do "d0" and "d2" refer to? This seems to indicate you have too much clock detail in the DT (with individual clocks described) or not enough with genericish compatible strings. What happens for the mext clock you need to adjust the flags on? You should be able to make these adjustments without DT updates. Perhaps you need a wider fixing of clock compatible strings. Rob
On 25 May 2016 at 19:24, Rob Herring <robh@kernel.org> wrote: > > On Wed, May 18, 2016 at 10:41:23AM +0200, Gabriel Fernandez wrote: > > This patch allows fine tuning of the quads FS for audio clocks > > accuracy. > > > > Signed-off-by: Olivier Bideau <olivier.bideau@st.com> > > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org> > > --- > > .../devicetree/bindings/clock/st/st,flexgen.txt | 1 + > > drivers/clk/st/clk-flexgen.c | 24 ++++++++++++++++++++++ > > 2 files changed, 25 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/clock/st/st,flexgen.txt b/Documentation/devicetree/bindings/clock/st/st,flexgen.txt > > index b7ee5c7..15b33c7 100644 > > --- a/Documentation/devicetree/bindings/clock/st/st,flexgen.txt > > +++ b/Documentation/devicetree/bindings/clock/st/st,flexgen.txt > > @@ -60,6 +60,7 @@ This binding uses the common clock binding[2]. > > Required properties: > > - compatible : shall be: > > "st,flexgen" > > + "st,stih407-clkgend0", "st,flexgen" (enable clock propagation on parent) > > What do "d0" and "d2" refer to? > > This seems to indicate you have too much clock detail in the DT (with > individual clocks described) or not enough with genericish compatible > strings. What happens for the mext clock you need to adjust the flags > on? You should be able to make these adjustments without DT updates. > Perhaps you need a wider fixing of clock compatible strings. > > Rob Sorry i sent my response in html... Hi Rob, Thanks for reviewing. Can i remove " st,stih407-clkgend0" & " st,stih407-clkgend2" compatible strings and add proprieties instead ? I only need to activate 2 features and then we can keep generic compatible strings. e.g. clk_s_d2_flexgen: clk-s-d2-flexgen { #clock-cells = <1>; compatible = "st,flexgen"; ... st-sync-mode; st-update -parent-rate; }; for d0_flexgen i will enable only st-sync-mode; Best Regards
On Thu, May 26, 2016 at 4:49 AM, Gabriel Fernandez <gabriel.fernandez@linaro.org> wrote: > On 25 May 2016 at 19:24, Rob Herring <robh@kernel.org> wrote: >> >> On Wed, May 18, 2016 at 10:41:23AM +0200, Gabriel Fernandez wrote: >> > This patch allows fine tuning of the quads FS for audio clocks >> > accuracy. >> > >> > Signed-off-by: Olivier Bideau <olivier.bideau@st.com> >> > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org> >> > --- >> > .../devicetree/bindings/clock/st/st,flexgen.txt | 1 + >> > drivers/clk/st/clk-flexgen.c | 24 ++++++++++++++++++++++ >> > 2 files changed, 25 insertions(+) >> > >> > diff --git a/Documentation/devicetree/bindings/clock/st/st,flexgen.txt b/Documentation/devicetree/bindings/clock/st/st,flexgen.txt >> > index b7ee5c7..15b33c7 100644 >> > --- a/Documentation/devicetree/bindings/clock/st/st,flexgen.txt >> > +++ b/Documentation/devicetree/bindings/clock/st/st,flexgen.txt >> > @@ -60,6 +60,7 @@ This binding uses the common clock binding[2]. >> > Required properties: >> > - compatible : shall be: >> > "st,flexgen" >> > + "st,stih407-clkgend0", "st,flexgen" (enable clock propagation on parent) >> >> What do "d0" and "d2" refer to? >> >> This seems to indicate you have too much clock detail in the DT (with >> individual clocks described) or not enough with genericish compatible >> strings. What happens for the mext clock you need to adjust the flags >> on? You should be able to make these adjustments without DT updates. >> Perhaps you need a wider fixing of clock compatible strings. >> >> Rob > > Sorry i sent my response in html... > > Hi Rob, > > Thanks for reviewing. > > Can i remove > " > st,stih407-clkgend0" & " > st,stih407-clkgend2" compatible strings and add proprieties instead ? > I only need to activate 2 features and then we can keep generic > compatible strings. That is no different and suffers the same point I raised. It requires updating the DT for any clock configuration change or enhancement. Rob
On 05/26/2016 02:46 PM, Rob Herring wrote: > On Thu, May 26, 2016 at 4:49 AM, Gabriel Fernandez > <gabriel.fernandez@linaro.org> wrote: >> On 25 May 2016 at 19:24, Rob Herring <robh@kernel.org> wrote: >>> >>> On Wed, May 18, 2016 at 10:41:23AM +0200, Gabriel Fernandez wrote: >>>> This patch allows fine tuning of the quads FS for audio clocks >>>> accuracy. >>>> >>>> Signed-off-by: Olivier Bideau <olivier.bideau@st.com> >>>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org> >>>> --- >>>> .../devicetree/bindings/clock/st/st,flexgen.txt | 1 + >>>> drivers/clk/st/clk-flexgen.c | 24 ++++++++++++++++++++++ >>>> 2 files changed, 25 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/clock/st/st,flexgen.txt b/Documentation/devicetree/bindings/clock/st/st,flexgen.txt >>>> index b7ee5c7..15b33c7 100644 >>>> --- a/Documentation/devicetree/bindings/clock/st/st,flexgen.txt >>>> +++ b/Documentation/devicetree/bindings/clock/st/st,flexgen.txt >>>> @@ -60,6 +60,7 @@ This binding uses the common clock binding[2]. >>>> Required properties: >>>> - compatible : shall be: >>>> "st,flexgen" >>>> + "st,stih407-clkgend0", "st,flexgen" (enable clock propagation on parent) >>> >>> What do "d0" and "d2" refer to? >>> >>> This seems to indicate you have too much clock detail in the DT (with >>> individual clocks described) or not enough with genericish compatible >>> strings. What happens for the mext clock you need to adjust the flags >>> on? You should be able to make these adjustments without DT updates. >>> Perhaps you need a wider fixing of clock compatible strings. >>> >>> Rob >> >> Sorry i sent my response in html... >> >> Hi Rob, >> >> Thanks for reviewing. >> >> Can i remove >> " >> st,stih407-clkgend0" & " >> st,stih407-clkgend2" compatible strings and add proprieties instead ? >> I only need to activate 2 features and then we can keep generic >> compatible strings. > Hi Rob, > That is no different and suffers the same point I raised. It requires > updating the DT for any clock configuration change or enhancement. > Agree with you, DT update is needed as soon as a clock configuration should be changed. This is due to STiH clock driver design based on DT description of SoC clock tree. This clock driver was accepted 2 years ago. At the time being there was discussion about clock tree description location: driver or DT. Bad choice was done for this driver... If we decide to redesign STiH clock driver using in-driver clock tree description, this will modify STiH clock DT nodes description and so break DT backward compatibility. What's from your pov the best option? Regards, Loic > Rob > > _______________________________________________ > Kernel mailing list > Kernel@stlinux.com > http://www.stlinux.com/mailman/listinfo/kernel >
On Thu, May 26, 2016 at 8:05 AM, loic pallardy <loic.pallardy@st.com> wrote: > > > On 05/26/2016 02:46 PM, Rob Herring wrote: >> >> On Thu, May 26, 2016 at 4:49 AM, Gabriel Fernandez >> <gabriel.fernandez@linaro.org> wrote: >>> >>> On 25 May 2016 at 19:24, Rob Herring <robh@kernel.org> wrote: >>>> >>>> >>>> On Wed, May 18, 2016 at 10:41:23AM +0200, Gabriel Fernandez wrote: >>>>> >>>>> This patch allows fine tuning of the quads FS for audio clocks >>>>> accuracy. >>>>> >>>>> Signed-off-by: Olivier Bideau <olivier.bideau@st.com> >>>>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org> >>>>> --- >>>>> .../devicetree/bindings/clock/st/st,flexgen.txt | 1 + >>>>> drivers/clk/st/clk-flexgen.c | 24 >>>>> ++++++++++++++++++++++ >>>>> 2 files changed, 25 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/clock/st/st,flexgen.txt >>>>> b/Documentation/devicetree/bindings/clock/st/st,flexgen.txt >>>>> index b7ee5c7..15b33c7 100644 >>>>> --- a/Documentation/devicetree/bindings/clock/st/st,flexgen.txt >>>>> +++ b/Documentation/devicetree/bindings/clock/st/st,flexgen.txt >>>>> @@ -60,6 +60,7 @@ This binding uses the common clock binding[2]. >>>>> Required properties: >>>>> - compatible : shall be: >>>>> "st,flexgen" >>>>> + "st,stih407-clkgend0", "st,flexgen" (enable clock propagation on >>>>> parent) >>>> >>>> >>>> What do "d0" and "d2" refer to? >>>> >>>> This seems to indicate you have too much clock detail in the DT (with >>>> individual clocks described) or not enough with genericish compatible >>>> strings. What happens for the mext clock you need to adjust the flags >>>> on? You should be able to make these adjustments without DT updates. >>>> Perhaps you need a wider fixing of clock compatible strings. >>>> >>>> Rob >>> >>> >>> Sorry i sent my response in html... >>> >>> Hi Rob, >>> >>> Thanks for reviewing. >>> >>> Can i remove >>> " >>> st,stih407-clkgend0" & " >>> st,stih407-clkgend2" compatible strings and add proprieties instead ? >>> I only need to activate 2 features and then we can keep generic >>> compatible strings. >> >> > Hi Rob, >> >> That is no different and suffers the same point I raised. It requires >> updating the DT for any clock configuration change or enhancement. >> > Agree with you, DT update is needed as soon as a clock configuration should > be changed. This is due to STiH clock driver design based on DT description > of SoC clock tree. > > This clock driver was accepted 2 years ago. At the time being there was > discussion about clock tree description location: driver or DT. > Bad choice was done for this driver... > > If we decide to redesign STiH clock driver using in-driver clock tree > description, this will modify STiH clock DT nodes description and so break > DT backward compatibility. > > What's from your pov the best option? You can break it once or every time you need a clock change. I'd go with the former. Maybe more specific compatible strings throughout alone would be enough rather than a flag day changing the binding. Rob
On 05/26/2016 03:20 PM, Rob Herring wrote: > On Thu, May 26, 2016 at 8:05 AM, loic pallardy <loic.pallardy@st.com> wrote: >> >> >> On 05/26/2016 02:46 PM, Rob Herring wrote: >>> >>> On Thu, May 26, 2016 at 4:49 AM, Gabriel Fernandez >>> <gabriel.fernandez@linaro.org> wrote: >>>> >>>> On 25 May 2016 at 19:24, Rob Herring <robh@kernel.org> wrote: >>>>> >>>>> >>>>> On Wed, May 18, 2016 at 10:41:23AM +0200, Gabriel Fernandez wrote: >>>>>> >>>>>> This patch allows fine tuning of the quads FS for audio clocks >>>>>> accuracy. >>>>>> >>>>>> Signed-off-by: Olivier Bideau <olivier.bideau@st.com> >>>>>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org> >>>>>> --- >>>>>> .../devicetree/bindings/clock/st/st,flexgen.txt | 1 + >>>>>> drivers/clk/st/clk-flexgen.c | 24 >>>>>> ++++++++++++++++++++++ >>>>>> 2 files changed, 25 insertions(+) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/clock/st/st,flexgen.txt >>>>>> b/Documentation/devicetree/bindings/clock/st/st,flexgen.txt >>>>>> index b7ee5c7..15b33c7 100644 >>>>>> --- a/Documentation/devicetree/bindings/clock/st/st,flexgen.txt >>>>>> +++ b/Documentation/devicetree/bindings/clock/st/st,flexgen.txt >>>>>> @@ -60,6 +60,7 @@ This binding uses the common clock binding[2]. >>>>>> Required properties: >>>>>> - compatible : shall be: >>>>>> "st,flexgen" >>>>>> + "st,stih407-clkgend0", "st,flexgen" (enable clock propagation on >>>>>> parent) >>>>> >>>>> >>>>> What do "d0" and "d2" refer to? >>>>> >>>>> This seems to indicate you have too much clock detail in the DT (with >>>>> individual clocks described) or not enough with genericish compatible >>>>> strings. What happens for the mext clock you need to adjust the flags >>>>> on? You should be able to make these adjustments without DT updates. >>>>> Perhaps you need a wider fixing of clock compatible strings. >>>>> >>>>> Rob >>>> >>>> >>>> Sorry i sent my response in html... >>>> >>>> Hi Rob, >>>> >>>> Thanks for reviewing. >>>> >>>> Can i remove >>>> " >>>> st,stih407-clkgend0" & " >>>> st,stih407-clkgend2" compatible strings and add proprieties instead ? >>>> I only need to activate 2 features and then we can keep generic >>>> compatible strings. >>> >>> >> Hi Rob, >>> >>> That is no different and suffers the same point I raised. It requires >>> updating the DT for any clock configuration change or enhancement. >>> >> Agree with you, DT update is needed as soon as a clock configuration should >> be changed. This is due to STiH clock driver design based on DT description >> of SoC clock tree. >> >> This clock driver was accepted 2 years ago. At the time being there was >> discussion about clock tree description location: driver or DT. >> Bad choice was done for this driver... >> >> If we decide to redesign STiH clock driver using in-driver clock tree >> description, this will modify STiH clock DT nodes description and so break >> DT backward compatibility. >> >> What's from your pov the best option? > > You can break it once or every time you need a clock change. I'd go > with the former. Maybe more specific compatible strings throughout > alone would be enough rather than a flag day changing the binding. So if I understand you correctly, main issue is d0 and d2 signification. d0 and d2 are indeed location of the flexgen in the SoC. But that's right flexgen are dedicated to clocks generation for features (system, audio, video). What about "st,flexgen-audio" and "st,flexgen-video"? BR, Loic > > Rob >
On Fri, May 27, 2016 at 09:23:20AM +0200, loic pallardy wrote: > > > On 05/26/2016 03:20 PM, Rob Herring wrote: > >On Thu, May 26, 2016 at 8:05 AM, loic pallardy <loic.pallardy@st.com> wrote: > >> > >> > >>On 05/26/2016 02:46 PM, Rob Herring wrote: > >>> > >>>On Thu, May 26, 2016 at 4:49 AM, Gabriel Fernandez > >>><gabriel.fernandez@linaro.org> wrote: > >>>> > >>>>On 25 May 2016 at 19:24, Rob Herring <robh@kernel.org> wrote: > >>>>> > >>>>> > >>>>>On Wed, May 18, 2016 at 10:41:23AM +0200, Gabriel Fernandez wrote: > >>>>>> > >>>>>>This patch allows fine tuning of the quads FS for audio clocks > >>>>>>accuracy. > >>>>>> > >>> > >>>That is no different and suffers the same point I raised. It requires > >>>updating the DT for any clock configuration change or enhancement. > >>> > >>Agree with you, DT update is needed as soon as a clock configuration should > >>be changed. This is due to STiH clock driver design based on DT description > >>of SoC clock tree. > >> > >>This clock driver was accepted 2 years ago. At the time being there was > >>discussion about clock tree description location: driver or DT. > >>Bad choice was done for this driver... > >> > >>If we decide to redesign STiH clock driver using in-driver clock tree > >>description, this will modify STiH clock DT nodes description and so break > >>DT backward compatibility. > >> > >>What's from your pov the best option? > > > >You can break it once or every time you need a clock change. I'd go > >with the former. Maybe more specific compatible strings throughout > >alone would be enough rather than a flag day changing the binding. > > So if I understand you correctly, main issue is d0 and d2 signification. > d0 and d2 are indeed location of the flexgen in the SoC. But that's right > flexgen are dedicated to clocks generation for features (system, audio, > video). > What about "st,flexgen-audio" and "st,flexgen-video"? It is not so much the name of these 2, but whether there are other cases for different clock nodes that could need the same thing. If so, update them all now rather than 1 by 1. Rob
On 27 May 2016 at 17:41, Rob Herring <robh@kernel.org> wrote: > On Fri, May 27, 2016 at 09:23:20AM +0200, loic pallardy wrote: >> >> >> On 05/26/2016 03:20 PM, Rob Herring wrote: >> >On Thu, May 26, 2016 at 8:05 AM, loic pallardy <loic.pallardy@st.com> wrote: >> >> >> >> >> >>On 05/26/2016 02:46 PM, Rob Herring wrote: >> >>> >> >>>On Thu, May 26, 2016 at 4:49 AM, Gabriel Fernandez >> >>><gabriel.fernandez@linaro.org> wrote: >> >>>> >> >>>>On 25 May 2016 at 19:24, Rob Herring <robh@kernel.org> wrote: >> >>>>> >> >>>>> >> >>>>>On Wed, May 18, 2016 at 10:41:23AM +0200, Gabriel Fernandez wrote: >> >>>>>> >> >>>>>>This patch allows fine tuning of the quads FS for audio clocks >> >>>>>>accuracy. >> >>>>>> > >> >>> >> >>>That is no different and suffers the same point I raised. It requires >> >>>updating the DT for any clock configuration change or enhancement. >> >>> >> >>Agree with you, DT update is needed as soon as a clock configuration should >> >>be changed. This is due to STiH clock driver design based on DT description >> >>of SoC clock tree. >> >> >> >>This clock driver was accepted 2 years ago. At the time being there was >> >>discussion about clock tree description location: driver or DT. >> >>Bad choice was done for this driver... >> >> >> >>If we decide to redesign STiH clock driver using in-driver clock tree >> >>description, this will modify STiH clock DT nodes description and so break >> >>DT backward compatibility. >> >> >> >>What's from your pov the best option? >> > >> >You can break it once or every time you need a clock change. I'd go >> >with the former. Maybe more specific compatible strings throughout >> >alone would be enough rather than a flag day changing the binding. >> >> So if I understand you correctly, main issue is d0 and d2 signification. >> d0 and d2 are indeed location of the flexgen in the SoC. But that's right >> flexgen are dedicated to clocks generation for features (system, audio, >> video). >> What about "st,flexgen-audio" and "st,flexgen-video"? > > It is not so much the name of these 2, but whether there are other cases > for different clock nodes that could need the same thing. If so, update > them all now rather than 1 by 1. > > Rob Hi Rob, Ok i will send a V2 for that. Mike, the first patch "drivers: clk: st: Add fs660c32 synthesizer algorithm" can be taken into account regardless the remark of Rob. Can you have a look ? Thanks. Best Regards Gabriel
diff --git a/Documentation/devicetree/bindings/clock/st/st,flexgen.txt b/Documentation/devicetree/bindings/clock/st/st,flexgen.txt index b7ee5c7..15b33c7 100644 --- a/Documentation/devicetree/bindings/clock/st/st,flexgen.txt +++ b/Documentation/devicetree/bindings/clock/st/st,flexgen.txt @@ -60,6 +60,7 @@ This binding uses the common clock binding[2]. Required properties: - compatible : shall be: "st,flexgen" + "st,stih407-clkgend0", "st,flexgen" (enable clock propagation on parent) - #clock-cells : from common clock binding; shall be set to 1 (multiple clock outputs). diff --git a/drivers/clk/st/clk-flexgen.c b/drivers/clk/st/clk-flexgen.c index 627267c..3ff51ec 100644 --- a/drivers/clk/st/clk-flexgen.c +++ b/drivers/clk/st/clk-flexgen.c @@ -15,6 +15,10 @@ #include <linux/of.h> #include <linux/of_address.h> +struct clkgen_data { + unsigned long flags; +}; + struct flexgen { struct clk_hw hw; @@ -259,6 +263,18 @@ static const char ** __init flexgen_get_parents(struct device_node *np, return parents; } +static const struct clkgen_data clkgen_d0 = { + .flags = CLK_SET_RATE_PARENT, +}; + +static const struct of_device_id flexgen_of_match[] = { + { + .compatible = "st,stih407-clkgend0", + .data = &clkgen_d0, + }, + {} +}; + static void __init st_of_flexgen_setup(struct device_node *np) { struct device_node *pnode; @@ -267,6 +283,8 @@ static void __init st_of_flexgen_setup(struct device_node *np) const char **parents; int num_parents, i; spinlock_t *rlock = NULL; + const struct of_device_id *match; + struct clkgen_data *data = NULL; unsigned long flex_flags = 0; int ret; @@ -282,6 +300,12 @@ static void __init st_of_flexgen_setup(struct device_node *np) if (!parents) return; + match = of_match_node(flexgen_of_match, np); + if (match) { + data = (struct clkgen_data *)match->data; + flex_flags = data->flags; + } + clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL); if (!clk_data) goto err;