| Message ID | 1256894234-11264-1-git-send-email-w.sang@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Grant Likely |
| Headers | show |
On Fri, Oct 30, 2009 at 10:17:14AM +0100, Wolfram Sang wrote: > The matching logic returns a clock even if only the dev-part matches. This is > wrong as devices may utilize more than one clock, so the wrong clock may be > returned due to dev being not unique (noticed while working on the CAN driver). > The proposed new method will: > - require the id field (as _this_ is the unique identifier) NULL id fields are supposed to be supported in the cannonical clkdev API, unfortunately.
On Fri, Oct 30, 2009 at 10:54:14AM +0000, Mark Brown wrote: > On Fri, Oct 30, 2009 at 10:17:14AM +0100, Wolfram Sang wrote: > > The matching logic returns a clock even if only the dev-part matches. This is > > wrong as devices may utilize more than one clock, so the wrong clock may be > > returned due to dev being not unique (noticed while working on the CAN driver). > > The proposed new method will: > > > - require the id field (as _this_ is the unique identifier) > > NULL id fields are supposed to be supported in the cannonical clkdev > API, unfortunately. Hmm, ok, thanks for the hint. Where is this documented, I can't find it?
On Fri, Oct 30, 2009 at 12:36:44PM +0100, Wolfram Sang wrote: > On Fri, Oct 30, 2009 at 10:54:14AM +0000, Mark Brown wrote: > > > - require the id field (as _this_ is the unique identifier) > > NULL id fields are supposed to be supported in the cannonical clkdev > > API, unfortunately. > Hmm, ok, thanks for the hint. Where is this documented, I can't find it? I'm not aware of any particular documentation on it but searching for mailing list posts from rmk ought to turn up some posts on the issue.
On Fri, 2009-10-30 at 10:17 +0100, Wolfram Sang wrote: > The matching logic returns a clock even if only the dev-part matches. This is > wrong as devices may utilize more than one clock, so the wrong clock may be > returned due to dev being not unique (noticed while working on the CAN driver). > The proposed new method will: > > - require the id field (as _this_ is the unique identifier) > - dev need not be given; if NULL, it will match any device. > if given, it has to match the dev of the clock > - using the above rules, both fields need to match in order to claim the clock Have you considered switching to my proposed device-tree based clock reprentation ? Cheers, Ben. > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> > Cc: Wolfgang Denk <wd@denx.de> > Cc: Grant Likely <grant.likely@secretlab.ca> > --- > arch/powerpc/platforms/512x/clock.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > Index: .kernel/arch/powerpc/platforms/512x/clock.c > =================================================================== > --- .kernel.orig/arch/powerpc/platforms/512x/clock.c > +++ .kernel/arch/powerpc/platforms/512x/clock.c > @@ -53,19 +53,21 @@ static DEFINE_MUTEX(clocks_mutex); > static struct clk *mpc5121_clk_get(struct device *dev, const char *id) > { > struct clk *p, *clk = ERR_PTR(-ENOENT); > - int dev_match = 0; > - int id_match = 0; > + bool id_match = false; > + /* Match any device if no dev given */ > + bool dev_match = !dev; > > - if (dev == NULL || id == NULL) > + /* We need the unique identifier */ > + if (id == NULL) > return NULL; > > mutex_lock(&clocks_mutex); > list_for_each_entry(p, &clocks, node) { > if (dev == p->dev) > - dev_match++; > + dev_match = true; > if (strcmp(id, p->name) == 0) > - id_match++; > - if ((dev_match || id_match) && try_module_get(p->owner)) { > + id_match = true; > + if (dev_match && id_match && try_module_get(p->owner)) { > clk = p; > break; > } > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev
Hello Ben, > Have you considered switching to my proposed device-tree based clock > reprentation ? You mean this one? http://patchwork.ozlabs.org/patch/31551/ Sorry, I have missed it up to now :( While I think (after one glimpse) this moves things into the right direction, my priority at the moment is to get the mscan driver working on 512x (and then to mainline). For that, I needed the patch I posted. I hope I can have another look at your proposal later on, it looks really worth it. Until then I'd propose to consider this patch as it fixes a bug in clock assignment. But I leave the final decision to you maintainers, of course. Regards, Wolfram
On Sat, 2009-10-31 at 14:01 +0100, Wolfram Sang wrote: > Hello Ben, > > > Have you considered switching to my proposed device-tree based clock > > reprentation ? > > You mean this one? > > http://patchwork.ozlabs.org/patch/31551/ > > Sorry, I have missed it up to now :( > > While I think (after one glimpse) this moves things into the right direction, > my priority at the moment is to get the mscan driver working on 512x (and then > to mainline). For that, I needed the patch I posted. I hope I can have another > look at your proposal later on, it looks really worth it. > > Until then I'd propose to consider this patch as it fixes a bug in clock > assignment. But I leave the final decision to you maintainers, of course. Sure I don't have major objections against your patch (though who is formally mpc512x maintainer to ack it ?), I just wanted to make sure you had a look at my proposal since this is almost the only platform to use the clock API today in arch/powerpc. Cheers, Ben.
> Sure I don't have major objections against your patch (though who is > formally mpc512x maintainer to ack it ?), Grant is maintainer for MPC5xxx.
On Mon, Nov 2, 2009 at 7:22 AM, Wolfram Sang <w.sang@pengutronix.de> wrote: >> Sure I don't have major objections against your patch (though who is >> formally mpc512x maintainer to ack it ?), > > Grant is maintainer for MPC5xxx. Is this patch intended for mainline? I've received so many 5121 patches that only apply against a Denx tree that I'm starting to ignore them. Until the bulk of the out-of-tree 5121 support is merged, it would be helpful to specifically mention in the patch description whether or not it should be merged upstream. Cheers, g.
> Is this patch intended for mainline? I've received so many 5121
Yes, my branch is based on linus-git as of today. The CAN driver addition was
just posted to socketcan-devel. It is also independent of the denx-patches[1]
and will later be send to netdev, too.
[1] Of course, the FEC patches are still very handy to bring up a board ;)
Index: .kernel/arch/powerpc/platforms/512x/clock.c =================================================================== --- .kernel.orig/arch/powerpc/platforms/512x/clock.c +++ .kernel/arch/powerpc/platforms/512x/clock.c @@ -53,19 +53,21 @@ static DEFINE_MUTEX(clocks_mutex); static struct clk *mpc5121_clk_get(struct device *dev, const char *id) { struct clk *p, *clk = ERR_PTR(-ENOENT); - int dev_match = 0; - int id_match = 0; + bool id_match = false; + /* Match any device if no dev given */ + bool dev_match = !dev; - if (dev == NULL || id == NULL) + /* We need the unique identifier */ + if (id == NULL) return NULL; mutex_lock(&clocks_mutex); list_for_each_entry(p, &clocks, node) { if (dev == p->dev) - dev_match++; + dev_match = true; if (strcmp(id, p->name) == 0) - id_match++; - if ((dev_match || id_match) && try_module_get(p->owner)) { + id_match = true; + if (dev_match && id_match && try_module_get(p->owner)) { clk = p; break; }
The matching logic returns a clock even if only the dev-part matches. This is wrong as devices may utilize more than one clock, so the wrong clock may be returned due to dev being not unique (noticed while working on the CAN driver). The proposed new method will: - require the id field (as _this_ is the unique identifier) - dev need not be given; if NULL, it will match any device. if given, it has to match the dev of the clock - using the above rules, both fields need to match in order to claim the clock Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> Cc: Wolfgang Denk <wd@denx.de> Cc: Grant Likely <grant.likely@secretlab.ca> --- arch/powerpc/platforms/512x/clock.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)