| Message ID | 1392118632-11312-5-git-send-email-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 11, 2014 at 11:37:09AM +0000, Mark Rutland wrote: > - pl022->clk = devm_clk_get(&adev->dev, NULL); > + /* > + * For compatibility with old DTBs and platform data, fall back to the > + * first clock if there's not an explicitly named "sspclk" entry. > + */ > + pl022->clk = devm_clk_get(&adev->dev, "sspclk"); > + if (IS_ERR(pl022->clk)) > + pl022->clk = devm_clk_get(&adev->dev, NULL); > + I'll just have a bit of a grumble here and point out that this sort of stuff always worries me with the convention of using nameless clocks - it causes hassle adding further clocks. In any case you didn't CC me on the cover letter or any of the non-SPI patches so I'm not sure what the dependencies are here (if there are any), does the series need to go in as one?
On Tue, Feb 11, 2014 at 12:06:45PM +0000, Mark Brown wrote: > On Tue, Feb 11, 2014 at 11:37:09AM +0000, Mark Rutland wrote: > > > - pl022->clk = devm_clk_get(&adev->dev, NULL); > > + /* > > + * For compatibility with old DTBs and platform data, fall back to the > > + * first clock if there's not an explicitly named "sspclk" entry. > > + */ > > + pl022->clk = devm_clk_get(&adev->dev, "sspclk"); > > + if (IS_ERR(pl022->clk)) > > + pl022->clk = devm_clk_get(&adev->dev, NULL); > > + > > I'll just have a bit of a grumble here and point out that this sort of > stuff always worries me with the convention of using nameless clocks - > it causes hassle adding further clocks. > > In any case you didn't CC me on the cover letter or any of the non-SPI > patches so I'm not sure what the dependencies are here (if there are > any), does the series need to go in as one? Apologies for missing you for the cover letter (you can find a copy in the lakml archive [1]). The SPI patches don't depend on the rest of the series, but given Russell's comments on the cover [2] this will probably need a v2 anyway. I'll ensure you're Cc'd. Cheers, Mark. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/231572.html [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/231594.html
On Tuesday 11 February 2014, Mark Brown wrote: > On Tue, Feb 11, 2014 at 11:37:09AM +0000, Mark Rutland wrote: > > > - pl022->clk = devm_clk_get(&adev->dev, NULL); > > + /* > > + * For compatibility with old DTBs and platform data, fall back to the > > + * first clock if there's not an explicitly named "sspclk" entry. > > + */ > > + pl022->clk = devm_clk_get(&adev->dev, "sspclk"); > > + if (IS_ERR(pl022->clk)) > > + pl022->clk = devm_clk_get(&adev->dev, NULL); > > + > > I'll just have a bit of a grumble here and point out that this sort of > stuff always worries me with the convention of using nameless clocks - > it causes hassle adding further clocks. I think the best solution for this is to continue with anonymous clocks rather than adding names after the fact. This could be done (for DT-only drivers) using the of_clk_get() interface that takes an index, or we could add a generic dev_clk_get_index() or similar interface that has the same behavior but also works for clkdev. Arnd
On Tue, Feb 11, 2014 at 03:08:06PM +0100, Arnd Bergmann wrote: > On Tuesday 11 February 2014, Mark Brown wrote: > > On Tue, Feb 11, 2014 at 11:37:09AM +0000, Mark Rutland wrote: > > > > > - pl022->clk = devm_clk_get(&adev->dev, NULL); > > > + /* > > > + * For compatibility with old DTBs and platform data, fall back to the > > > + * first clock if there's not an explicitly named "sspclk" entry. > > > + */ > > > + pl022->clk = devm_clk_get(&adev->dev, "sspclk"); > > > + if (IS_ERR(pl022->clk)) > > > + pl022->clk = devm_clk_get(&adev->dev, NULL); > > > + > > > > I'll just have a bit of a grumble here and point out that this sort of > > stuff always worries me with the convention of using nameless clocks - > > it causes hassle adding further clocks. > > I think the best solution for this is to continue with anonymous clocks > rather than adding names after the fact. This could be done (for DT-only > drivers) using the of_clk_get() interface that takes an index, or > we could add a generic dev_clk_get_index() or similar interface that > has the same behavior but also works for clkdev. Mixing devm_* and non-devm_* interfaces doesn't work. If you want to do that, devm_of_clk_get() would be a prerequisit.
On Tuesday 11 February 2014 15:04:38 Russell King - ARM Linux wrote: > On Tue, Feb 11, 2014 at 03:08:06PM +0100, Arnd Bergmann wrote: > > On Tuesday 11 February 2014, Mark Brown wrote: > > > On Tue, Feb 11, 2014 at 11:37:09AM +0000, Mark Rutland wrote: > > > > > > > - pl022->clk = devm_clk_get(&adev->dev, NULL); > > > > + /* > > > > + * For compatibility with old DTBs and platform data, fall back to the > > > > + * first clock if there's not an explicitly named "sspclk" entry. > > > > + */ > > > > + pl022->clk = devm_clk_get(&adev->dev, "sspclk"); > > > > + if (IS_ERR(pl022->clk)) > > > > + pl022->clk = devm_clk_get(&adev->dev, NULL); > > > > + > > > > > > I'll just have a bit of a grumble here and point out that this sort of > > > stuff always worries me with the convention of using nameless clocks - > > > it causes hassle adding further clocks. > > > > I think the best solution for this is to continue with anonymous clocks > > rather than adding names after the fact. This could be done (for DT-only > > drivers) using the of_clk_get() interface that takes an index, or > > we could add a generic dev_clk_get_index() or similar interface that > > has the same behavior but also works for clkdev. > > Mixing devm_* and non-devm_* interfaces doesn't work. If you want to do > that, devm_of_clk_get() would be a prerequisit. Yes, good point. So if we want to do it, we would have to add a new function anyway, there is just the question whether it should be devm_of_clk_get() or devm_clk_get_index() if that can also work for non-DT devices. Do you think the latter actually makes sense in the clkdev interfaces? I'm not familiar enough with the code to tell how that would be implemented in a reasonable way. Arnd
On Tue, Feb 11, 2014 at 02:08:06PM +0000, Arnd Bergmann wrote: > On Tuesday 11 February 2014, Mark Brown wrote: > > On Tue, Feb 11, 2014 at 11:37:09AM +0000, Mark Rutland wrote: > > > > > - pl022->clk = devm_clk_get(&adev->dev, NULL); > > > + /* > > > + * For compatibility with old DTBs and platform data, fall back to the > > > + * first clock if there's not an explicitly named "sspclk" entry. > > > + */ > > > + pl022->clk = devm_clk_get(&adev->dev, "sspclk"); > > > + if (IS_ERR(pl022->clk)) > > > + pl022->clk = devm_clk_get(&adev->dev, NULL); > > > + > > > > I'll just have a bit of a grumble here and point out that this sort of > > stuff always worries me with the convention of using nameless clocks - > > it causes hassle adding further clocks. > > I think the best solution for this is to continue with anonymous clocks > rather than adding names after the fact. This could be done (for DT-only > drivers) using the of_clk_get() interface that takes an index, or > we could add a generic dev_clk_get_index() or similar interface that > has the same behavior but also works for clkdev. That works, and if taken alone patch 7 would codify that existing behaviour as the standard. To me it feels odd to require the last clock in the list (apb_pclk) to be named, and the rest to be in a particular order. For the dt case it seems saner to add new clocks with names as it allows arbitrary subsets of clocks to be wired up and described (though obviously in this case a missing sspclk would be problematic). For new bindings I'd really like to push people to always use named clocks as it makes things far more flexible, but I appreciate that here there are issues associated with modifying an existing binding. Mark, do you have specific issues that named clocks cause that I could look into? Cheers, Mark.
On Wed, Feb 12, 2014 at 10:33:29AM +0000, Mark Rutland wrote: > For new bindings I'd really like to push people to always use named > clocks as it makes things far more flexible, but I appreciate that here > there are issues associated with modifying an existing binding. > Mark, do you have specific issues that named clocks cause that I could > look into? None, I actively prefer naming them (though Russell's point about people picking names poorly pre-DT does make it clear why it makes sense that we weren't doing that). I was just grumbling about the fact that transitioning from unnamed to named is a bit ugly.
On Wednesday 12 February 2014, Mark Rutland wrote: > To me it feels odd to require the last clock in the list (apb_pclk) to > be named, and the rest to be in a particular order. For the dt case it > seems saner to add new clocks with names as it allows arbitrary subsets > of clocks to be wired up and described (though obviously in this case a > missing sspclk would be problematic). Yes, good point about the missing clocks, and I also agree mixing ordered and named clocks in one device is rather odd and can lead to trouble. I guess there isn't really a good way out here, and I certainly won't ask for it to be done one way or the other if someone else has a good argument which way it should be implemented. Arnd
On Wed, Feb 12, 2014 at 11:21:50AM +0000, Arnd Bergmann wrote: > On Wednesday 12 February 2014, Mark Rutland wrote: > > To me it feels odd to require the last clock in the list (apb_pclk) to > > be named, and the rest to be in a particular order. For the dt case it > > seems saner to add new clocks with names as it allows arbitrary subsets > > of clocks to be wired up and described (though obviously in this case a > > missing sspclk would be problematic). > > Yes, good point about the missing clocks, and I also agree mixing ordered > and named clocks in one device is rather odd and can lead to trouble. > > I guess there isn't really a good way out here, and I certainly won't > ask for it to be done one way or the other if someone else has a > good argument which way it should be implemented. Having thought about it, all dts that for the ssp_pclk must have some name for the sspclk (though the specific name is arbitrary). I could get the driver to try each of those before falling back to the index (perhaps with a helper that takes a list of known aliases), so all existing dts (that we are aware of) would work with the clock requested by name. I assume that for the non-dt case it's possible to name clock inputs to a device without the clock being associated with the name globally? If so we could get rid of the index usage entirely in this case. Does that sound workable? Thanks, Mark.
On Wednesday 12 February 2014 11:47:40 Mark Rutland wrote: > On Wed, Feb 12, 2014 at 11:21:50AM +0000, Arnd Bergmann wrote: > > On Wednesday 12 February 2014, Mark Rutland wrote: > > > To me it feels odd to require the last clock in the list (apb_pclk) to > > > be named, and the rest to be in a particular order. For the dt case it > > > seems saner to add new clocks with names as it allows arbitrary subsets > > > of clocks to be wired up and described (though obviously in this case a > > > missing sspclk would be problematic). > > > > Yes, good point about the missing clocks, and I also agree mixing ordered > > and named clocks in one device is rather odd and can lead to trouble. > > > > I guess there isn't really a good way out here, and I certainly won't > > ask for it to be done one way or the other if someone else has a > > good argument which way it should be implemented. > > Having thought about it, all dts that for the ssp_pclk must have some > name for the sspclk (though the specific name is arbitrary). I could get > the driver to try each of those before falling back to the index > (perhaps with a helper that takes a list of known aliases), so all > existing dts (that we are aware of) would work with the clock requested > by name. Strange. Why do the even have names in there? What are those strings? I noticed that ux500 has uses four different strings, one for each instance, which is obviously a bug and should just be fixed. There is no way this was intentional, and we can just rely on teh fallback if you want to have that anyway. > I assume that for the non-dt case it's possible to name clock inputs to > a device without the clock being associated with the name globally? If > so we could get rid of the index usage entirely in this case. Sorry, I don't understand the question. Arnd
On Wed, Feb 12, 2014 at 11:47:40AM +0000, Mark Rutland wrote: > I assume that for the non-dt case it's possible to name clock inputs to > a device without the clock being associated with the name globally? If > so we could get rid of the index usage entirely in this case. Yes, using clk_add_alias().
On Wed, Feb 12, 2014 at 01:03:26PM +0000, Arnd Bergmann wrote: > On Wednesday 12 February 2014 11:47:40 Mark Rutland wrote: > > On Wed, Feb 12, 2014 at 11:21:50AM +0000, Arnd Bergmann wrote: > > > On Wednesday 12 February 2014, Mark Rutland wrote: > > > > To me it feels odd to require the last clock in the list (apb_pclk) to > > > > be named, and the rest to be in a particular order. For the dt case it > > > > seems saner to add new clocks with names as it allows arbitrary subsets > > > > of clocks to be wired up and described (though obviously in this case a > > > > missing sspclk would be problematic). > > > > > > Yes, good point about the missing clocks, and I also agree mixing ordered > > > and named clocks in one device is rather odd and can lead to trouble. > > > > > > I guess there isn't really a good way out here, and I certainly won't > > > ask for it to be done one way or the other if someone else has a > > > good argument which way it should be implemented. > > > > Having thought about it, all dts that for the ssp_pclk must have some > > name for the sspclk (though the specific name is arbitrary). I could get > > the driver to try each of those before falling back to the index > > (perhaps with a helper that takes a list of known aliases), so all > > existing dts (that we are aware of) would work with the clock requested > > by name. > > Strange. Why do the even have names in there? What are those strings? I guess they're there as spacers to allow the AMBA bus code to get the right clock when it calls clk_get(&pcdev->dev, "apb_pclk"). Everyone seems to have worked around the binding rather than reporting the issue or fixing it. From a quick grep, for pl022's SSPCLK we currently have the strings: * ssp{0,1}clk * spi_clk * spi{0,1,2,3}clk Though I may have missed a string or two where nodes get amended in more specific files. A grep for apb_clk to find neighbours didn't highlight any obvious ones. > > I noticed that ux500 has uses four different strings, one for each > instance, which is obviously a bug and should just be fixed. There is > no way this was intentional, and we can just rely on teh fallback > if you want to have that anyway. Sure, I'll fix those up once we have a preferred name. I guess this would be SSPCLK by Russell's comments, I wasn't able to find a prior use in the git history, but it would be in keeping with KMIREFCLK as used by the pl050 driver. Given the general policy of trying to not break support for existing DTBs we'll need some fallback, either using the first clock or getting the driver to try the names above. > > > I assume that for the non-dt case it's possible to name clock inputs to > > a device without the clock being associated with the name globally? If > > so we could get rid of the index usage entirely in this case. > > Sorry, I don't understand the question. I thought one of the issues before dt was that clocks were in a global namespace. Mark's reply implies that's not necessarily the case, so I'll take a tour through clkdev to educate myself. Cheers, Mark.
On Wed, Feb 12, 2014 at 04:12:06PM +0000, Mark Rutland wrote: > I thought one of the issues before dt was that clocks were in a global > namespace. Mark's reply implies that's not necessarily the case, so I'll > take a tour through clkdev to educate myself. There's both a global namespace and a device local namespace, the most exact match is used - see the comment on clk_find(). Remember that in the past clock implementations didn't have to use clkdev at all so the implementations varied a lot (which is half the problem with drivers now). Lots of implementations just used global clock names and didn't pay any attention to dev.
On Wednesday 12 February 2014 16:12:06 Mark Rutland wrote: > On Wed, Feb 12, 2014 at 01:03:26PM +0000, Arnd Bergmann wrote: > > On Wednesday 12 February 2014 11:47:40 Mark Rutland wrote: > From a quick grep, for pl022's SSPCLK we currently have the strings: > > * ssp{0,1}clk > * spi_clk > * spi{0,1,2,3}clk > > Though I may have missed a string or two where nodes get amended in more > specific files. A grep for apb_clk to find neighbours didn't highlight > any obvious ones. Ok. Both ssp{0,1}clk and spi{0,1,2,3}clk /only/ appear in arch/arm/boot/dts/ste-dbx5x0.dtsi and are clearly a bug, so unless Linus Walleij has objections, I'd declare those to be bugs that should be fixed by changing the DT file to spi_clk. > > I noticed that ux500 has uses four different strings, one for each > > instance, which is obviously a bug and should just be fixed. There is > > no way this was intentional, and we can just rely on teh fallback > > if you want to have that anyway. > > Sure, I'll fix those up once we have a preferred name. I guess this > would be SSPCLK by Russell's comments, I wasn't able to find a prior use > in the git history, but it would be in keeping with KMIREFCLK as used by > the pl050 driver. We do have a few cases of spi_clk, so I'd use that one. Ideally it should be the string given in the data sheet for the IP block of course, possibly with capital letters and underscores turned converted to more regular strings. > > > I assume that for the non-dt case it's possible to name clock inputs to > > > a device without the clock being associated with the name globally? If > > > so we could get rid of the index usage entirely in this case. > > > > Sorry, I don't understand the question. > > I thought one of the issues before dt was that clocks were in a global > namespace. Mark's reply implies that's not necessarily the case, so I'll > take a tour through clkdev to educate myself. The whole point of clkdev is to create a local per-device namespace so drivers don't need to care about the global names, as far as I understand it. Arnd
On Wed, Feb 12, 2014 at 5:31 PM, Arnd Bergmann <arnd@arndb.de> wrote: > Ok. Both ssp{0,1}clk and spi{0,1,2,3}clk /only/ appear in > arch/arm/boot/dts/ste-dbx5x0.dtsi and are clearly a bug, so unless > Linus Walleij has objections, I'd declare those to be bugs that > should be fixed by changing the DT file to spi_clk. OK I'll fix. But didn't Mark just say that the preferred name should be SSPCLK? Here is the PL022 TRM: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0194g/index.html The clock name on the silicon side is clearly "SSPCLK" so let's stick with this. Yours, Linus Walleij
diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c index 2789b45..4b3941a 100644 --- a/drivers/spi/spi-pl022.c +++ b/drivers/spi/spi-pl022.c @@ -2176,7 +2176,14 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id) dev_info(&adev->dev, "mapped registers from %pa to %p\n", &adev->res.start, pl022->virtbase); - pl022->clk = devm_clk_get(&adev->dev, NULL); + /* + * For compatibility with old DTBs and platform data, fall back to the + * first clock if there's not an explicitly named "sspclk" entry. + */ + pl022->clk = devm_clk_get(&adev->dev, "sspclk"); + if (IS_ERR(pl022->clk)) + pl022->clk = devm_clk_get(&adev->dev, NULL); + if (IS_ERR(pl022->clk)) { status = PTR_ERR(pl022->clk); dev_err(&adev->dev, "could not retrieve SSP/SPI bus clock\n");
The primecell device tree binding (from which the pl022 binding is derived from) states that the apb_pclk clock input should be listed first for all primecell devices. The spi-pl022 driver requires the sspclk clock input to enable the SPI bus, but the way it currently grabs the clock means that it always gets the first clock (which should be apb_pclk). As the AMBA bus code grabs apb_pclk by name, some existing dts provide apb_pclk as the second clock in the clocks list to work around this, in violation of both the primecell binding. The pl022 binding does not mention clocks at all, so the first clock (SSPCLK) is given an arbitrary name. This patch attempts to fix the mess my having the spi-pl022 driver first attempt to get sspclk by name. If this fails, it falls back to the old behaviour of simply acquiring the first clock. This is compatible with any old dtb, whether it lists sspclk by name or not, and allows the driver to support dtbs which do not violate the bindings. Hopefully this will lead to future uniformity across dtbs. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Mark Brown <broonie@kernel.org> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Rob Herring <robh+dt@kernel.org> Cc: Pawel Moll <pawel.moll@arm.com> --- drivers/spi/spi-pl022.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)