| Message ID | 1471933609-8456-2-git-send-email-mw@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 23, 2016 at 08:26:48AM +0200, Marcin Wojtas wrote: > Armada CP110 system controller comprise its own routine responsble > for registering gate clocks. Among others 'flags' field in > struct clk_init_data was not set, using a random values, which > may cause an unpredicted behavior. > > This patch fixes the problem by setting CLK_IS_BASIC flag for > all gated clocks of Armada 7k/8k SoCs family. > > Fixes: d3da3eaef7f4 ("clk: mvebu: new driver for Armada CP110 system ...") > > Signed-off-by: Marcin Wojtas <mw@semihalf.com> > --- > drivers/clk/mvebu/cp110-system-controller.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c > index 7fa42d6..0835e1d 100644 > --- a/drivers/clk/mvebu/cp110-system-controller.c > +++ b/drivers/clk/mvebu/cp110-system-controller.c > @@ -144,6 +144,7 @@ static struct clk *cp110_register_gate(const char *name, > > init.name = name; > init.ops = &cp110_gate_ops; > + init.flags = CLK_IS_BASIC; > init.parent_names = &parent_name; > init.num_parents = 1; Hi Marcin How about adding a memset for init? That would also help if new fields every get added to clk_init_data. Andrew
HI Andrew, 2016-08-23 16:16 GMT+02:00 Andrew Lunn <andrew@lunn.ch>: > On Tue, Aug 23, 2016 at 08:26:48AM +0200, Marcin Wojtas wrote: >> Armada CP110 system controller comprise its own routine responsble >> for registering gate clocks. Among others 'flags' field in >> struct clk_init_data was not set, using a random values, which >> may cause an unpredicted behavior. >> >> This patch fixes the problem by setting CLK_IS_BASIC flag for >> all gated clocks of Armada 7k/8k SoCs family. >> >> Fixes: d3da3eaef7f4 ("clk: mvebu: new driver for Armada CP110 system ...") >> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com> >> --- >> drivers/clk/mvebu/cp110-system-controller.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c >> index 7fa42d6..0835e1d 100644 >> --- a/drivers/clk/mvebu/cp110-system-controller.c >> +++ b/drivers/clk/mvebu/cp110-system-controller.c >> @@ -144,6 +144,7 @@ static struct clk *cp110_register_gate(const char *name, >> >> init.name = name; >> init.ops = &cp110_gate_ops; >> + init.flags = CLK_IS_BASIC; >> init.parent_names = &parent_name; >> init.num_parents = 1; > > Hi Marcin > > How about adding a memset for init? That would also help if new fields > every get added to clk_init_data. > Sure, it can be added. Best regards, Marcin
On 08/23, Marcin Wojtas wrote: > Armada CP110 system controller comprise its own routine responsble > for registering gate clocks. Among others 'flags' field in > struct clk_init_data was not set, using a random values, which > may cause an unpredicted behavior. > > This patch fixes the problem by setting CLK_IS_BASIC flag for > all gated clocks of Armada 7k/8k SoCs family. > > Fixes: d3da3eaef7f4 ("clk: mvebu: new driver for Armada CP110 system ...") > > Signed-off-by: Marcin Wojtas <mw@semihalf.com> > --- > drivers/clk/mvebu/cp110-system-controller.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c > index 7fa42d6..0835e1d 100644 > --- a/drivers/clk/mvebu/cp110-system-controller.c > +++ b/drivers/clk/mvebu/cp110-system-controller.c > @@ -144,6 +144,7 @@ static struct clk *cp110_register_gate(const char *name, > > init.name = name; > init.ops = &cp110_gate_ops; > + init.flags = CLK_IS_BASIC; Please don't use CLK_IS_BASIC unless you need it (so far only TI clks seem to want it?). Just set it to 0 if possible. > init.parent_names = &parent_name; > init.num_parents = 1; >
Hello, On Tue, 23 Aug 2016 08:26:48 +0200, Marcin Wojtas wrote: > diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c > index 7fa42d6..0835e1d 100644 > --- a/drivers/clk/mvebu/cp110-system-controller.c > +++ b/drivers/clk/mvebu/cp110-system-controller.c > @@ -144,6 +144,7 @@ static struct clk *cp110_register_gate(const char *name, > > init.name = name; > init.ops = &cp110_gate_ops; > + init.flags = CLK_IS_BASIC; Is this really correct? The documentation for CLK_IS_BASIC is pretty slim, but it says: #define CLK_IS_BASIC BIT(5) /* Basic clk, can't do a to_clk_foo() */ However, we *do* have a to_clk_*() macro in this driver: struct cp110_gate_clk { struct clk_hw hw; struct regmap *regmap; u8 bit_idx; }; #define to_cp110_gate_clk(clk) container_of(clk, struct cp110_gate_clk, hw) If you read the commit log of commit f7d8caadfd2813cbada82ce9041b13c38e8e5282, which introduced the flag, it says: clk: Add CLK_IS_BASIC flag to identify basic clocks Most platforms end up using a mix of basic clock types and some which use clk_hw_foo struct for filling in custom platform information when the clocks don't fit into basic types supported. In platform code, its useful to know if a clock is using a basic type or clk_hw_foo, which helps platforms know if they can safely use to_clk_hw_foo to derive the clk_hw_foo pointer from clk_hw. Mark all basic clocks with a CLK_IS_BASIC flag. Signed-off-by: Rajendra Nayak <rnayak@ti.com> Signed-off-by: Mike Turquette <mturquette@linaro.org> We are in the case where we have our own clk_hw_foo structure, and a to_clk_hw_foo macro to derive the clk_hw_foo from clk_hw. According to this, the CP110 clocks are *not* basic clocks, and therefore we shouldn't have this flag. Perhaps just the memset() is missing. Thanks, Thomas
Hi Thomas, 2016-08-30 15:10 GMT+02:00 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>: > Hello, > > On Tue, 23 Aug 2016 08:26:48 +0200, Marcin Wojtas wrote: > >> diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c >> index 7fa42d6..0835e1d 100644 >> --- a/drivers/clk/mvebu/cp110-system-controller.c >> +++ b/drivers/clk/mvebu/cp110-system-controller.c >> @@ -144,6 +144,7 @@ static struct clk *cp110_register_gate(const char *name, >> >> init.name = name; >> init.ops = &cp110_gate_ops; >> + init.flags = CLK_IS_BASIC; > > Is this really correct? > > The documentation for CLK_IS_BASIC is pretty slim, but it says: > > #define CLK_IS_BASIC BIT(5) /* Basic clk, can't do a to_clk_foo() */ > > However, we *do* have a to_clk_*() macro in this driver: > > struct cp110_gate_clk { > struct clk_hw hw; > struct regmap *regmap; > u8 bit_idx; > }; > > #define to_cp110_gate_clk(clk) container_of(clk, struct cp110_gate_clk, hw) > > If you read the commit log of commit > f7d8caadfd2813cbada82ce9041b13c38e8e5282, which introduced the flag, it > says: > > clk: Add CLK_IS_BASIC flag to identify basic clocks > > Most platforms end up using a mix of basic clock types and > some which use clk_hw_foo struct for filling in custom platform > information when the clocks don't fit into basic types supported. > > In platform code, its useful to know if a clock is using a basic > type or clk_hw_foo, which helps platforms know if they can > safely use to_clk_hw_foo to derive the clk_hw_foo pointer from > clk_hw. > > Mark all basic clocks with a CLK_IS_BASIC flag. > > Signed-off-by: Rajendra Nayak <rnayak@ti.com> > Signed-off-by: Mike Turquette <mturquette@linaro.org> > > We are in the case where we have our own clk_hw_foo structure, and a > to_clk_hw_foo macro to derive the clk_hw_foo from clk_hw. > > According to this, the CP110 clocks are *not* basic clocks, and > therefore we shouldn't have this flag. Perhaps just the memset() is > missing. > I agree, from functional point of view and considering also not exact fit to CLK_IS_BASIC definition, memset should be sufficient. Best regards, Marcin
diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c index 7fa42d6..0835e1d 100644 --- a/drivers/clk/mvebu/cp110-system-controller.c +++ b/drivers/clk/mvebu/cp110-system-controller.c @@ -144,6 +144,7 @@ static struct clk *cp110_register_gate(const char *name, init.name = name; init.ops = &cp110_gate_ops; + init.flags = CLK_IS_BASIC; init.parent_names = &parent_name; init.num_parents = 1;
Armada CP110 system controller comprise its own routine responsble for registering gate clocks. Among others 'flags' field in struct clk_init_data was not set, using a random values, which may cause an unpredicted behavior. This patch fixes the problem by setting CLK_IS_BASIC flag for all gated clocks of Armada 7k/8k SoCs family. Fixes: d3da3eaef7f4 ("clk: mvebu: new driver for Armada CP110 system ...") Signed-off-by: Marcin Wojtas <mw@semihalf.com> --- drivers/clk/mvebu/cp110-system-controller.c | 1 + 1 file changed, 1 insertion(+)