| Message ID | 1373914074-20889-2-git-send-email-gsi@denx.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Mon, Jul 15, 2013 at 08:47:30PM +0200, Gerhard Sittig wrote: > clocks need to get prepared before they can get enabled, > fix the MPC512x PSC SPI master's initialization > Signed-off-by: Gerhard Sittig <gsi@denx.de> > --- > drivers/spi/spi-mpc512x-psc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-mpc512x-psc.c b/drivers/spi/spi-mpc512x-psc.c > index 29fce6a..76b20ea 100644 > --- a/drivers/spi/spi-mpc512x-psc.c > +++ b/drivers/spi/spi-mpc512x-psc.c > @@ -395,7 +395,7 @@ static int mpc512x_psc_spi_port_config(struct spi_master *master, > > sprintf(name, "psc%d_mclk", master->bus_num); > spiclk = clk_get(&master->dev, name); > - clk_enable(spiclk); > + clk_prepare_enable(spiclk); > mps->mclk = clk_get_rate(spiclk); > clk_put(spiclk); This code is *clearly* buggy and should be fixed rather than papered over. Not only is there no matching put the driver is also dropping the reference to the clock rather than holding on to it while it's in use.
On Mon, Jul 15, 2013 at 21:17 +0100, Mark Brown wrote: > > On Mon, Jul 15, 2013 at 08:47:30PM +0200, Gerhard Sittig wrote: > > clocks need to get prepared before they can get enabled, > > fix the MPC512x PSC SPI master's initialization > > > Signed-off-by: Gerhard Sittig <gsi@denx.de> > > --- > > drivers/spi/spi-mpc512x-psc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/spi/spi-mpc512x-psc.c b/drivers/spi/spi-mpc512x-psc.c > > index 29fce6a..76b20ea 100644 > > --- a/drivers/spi/spi-mpc512x-psc.c > > +++ b/drivers/spi/spi-mpc512x-psc.c > > @@ -395,7 +395,7 @@ static int mpc512x_psc_spi_port_config(struct spi_master *master, > > > > sprintf(name, "psc%d_mclk", master->bus_num); > > spiclk = clk_get(&master->dev, name); > > - clk_enable(spiclk); > > + clk_prepare_enable(spiclk); > > mps->mclk = clk_get_rate(spiclk); > > clk_put(spiclk); > > This code is *clearly* buggy and should be fixed rather than papered > over. Not only is there no matching put the driver is also dropping the > reference to the clock rather than holding on to it while it's in use. "This code" refers to the driver's original condition, right? I agree that the driver has been suffering from incomplete clock handling since it was born three years ago. And that this issue shall get addressed. The question is just whether it needs to be part of this series which has a different focus. What the above patch addresses is prevention of an immediate and fatal breakage, when common clock gets used but clocks aren't prepared before getting enabled. So I consider it appropriate as a step in preparation before introducing support for common clock on the platform. (It's funny that I had a comment in this spirit in the commit message, but trimmed it to not be overly verbose.) What the patch does not address is to fix any other deficiencies in the driver which might have been lurking there for ages. This would be the scope of a different patch or series, as addressing it here as well would mix orthogonal issues within one series, and would complicate review and test (and would delay or even potentially kill the introduction of desirable support for common infrastructure just because other legacy non-fatal issues aren't addressed as well in bypassing). I will look into what I can do to address your concerns about proper general clock handling in this specific driver. But I'm unable to do this for all other drivers as well which I happen to pass by as I work on platform code (partially due to lack of knowledge). In any case I won't be able to test all of these changes in all subsystems (mostly due to lack of hardware and test setups). So I suggest to leave these general cleanup activities for a separate series. The current series certainly improves general platform code, transparently brings new drivers into successful operation, and keeps the quality of existing code (doesn't break anything, does even actively prevent breakage). So it shall be acceptable despite its not being perfect (like addressing each and every other aspect which may arise elsewhere). Where would I stop then? Sorry for the lengthy reply, but I guess it's about just one general aspect which equally applies to other parts of the series, not just the specific SPI driver which the feedback was provided for. And I do appreciate your looking at the submission. virtually yours Gerhard Sittig
On Wed, Jul 17, 2013 at 01:22:29PM +0200, Gerhard Sittig wrote: > On Mon, Jul 15, 2013 at 21:17 +0100, Mark Brown wrote: > > On Mon, Jul 15, 2013 at 08:47:30PM +0200, Gerhard Sittig wrote: > > > sprintf(name, "psc%d_mclk", master->bus_num); > > > spiclk = clk_get(&master->dev, name); > > > - clk_enable(spiclk); > > > + clk_prepare_enable(spiclk); > > > mps->mclk = clk_get_rate(spiclk); > > > clk_put(spiclk); > > This code is *clearly* buggy and should be fixed rather than papered > > over. Not only is there no matching put the driver is also dropping the > > reference to the clock rather than holding on to it while it's in use. > "This code" refers to the driver's original condition, right? I > agree that the driver has been suffering from incomplete clock > handling since it was born three years ago. And that this issue > shall get addressed. The question is just whether it needs to be > part of this series which has a different focus. This is a pretty long e-mail. It'd probably have taken less time to fix the issues than to reply to the e-mail... but anyway. A big part of the issue with the state of the driver is that there's obvious clock API abuse going on that isn't corrected here - the main one is that the sprintf() for the clock name is a fairly clear warning sign, the driver should be using a fixed value there. This raises a warning flag for me about the quality of the common clock API implementation that's being sent and/or the potential for bugs to be noticed with the common clock framework. I'd not expect this code to actually work, and looking at the rest of the series I don't see how it does since I can't see what forces the name.
On Wed, Jul 17, 2013 at 13:07 +0100, Mark Brown wrote: > > On Wed, Jul 17, 2013 at 01:22:29PM +0200, Gerhard Sittig wrote: > > On Mon, Jul 15, 2013 at 21:17 +0100, Mark Brown wrote: > > > On Mon, Jul 15, 2013 at 08:47:30PM +0200, Gerhard Sittig wrote: > > > > > sprintf(name, "psc%d_mclk", master->bus_num); > > > > spiclk = clk_get(&master->dev, name); > > > > - clk_enable(spiclk); > > > > + clk_prepare_enable(spiclk); > > > > mps->mclk = clk_get_rate(spiclk); > > > > clk_put(spiclk); > > > > This code is *clearly* buggy and should be fixed rather than papered > > > over. Not only is there no matching put the driver is also dropping the > > > reference to the clock rather than holding on to it while it's in use. > > > "This code" refers to the driver's original condition, right? I > > agree that the driver has been suffering from incomplete clock > > handling since it was born three years ago. And that this issue > > shall get addressed. The question is just whether it needs to be > > part of this series which has a different focus. > > This is a pretty long e-mail. It'd probably have taken less time to > fix the issues than to reply to the e-mail... but anyway. [ this is meta stuff, technical details are after the next quotation ] Not quite. Please consider that careful submission is as expensive as thorough review is, and that a lot of work is done before v1 gets submitted, which you may not always notice from looking at a concentrated series that may no longer suggest how much it took to get there (especially when prepared carefully). As mentioned before I did not question the need to fix issues as they get identified. I just asked whether reworking the serial driver is in the scope of this series which provides a different clock driver implementation for the platform. To me these two things are orthogonal, while I did promise to see how I can address your concerns. Let's provide the technical information you need to judge the quality of the submission and see why it shall be acceptable: > A big part of the issue with the state of the driver is that there's > obvious clock API abuse going on that isn't corrected here - the main > one is that the sprintf() for the clock name is a fairly clear warning > sign, the driver should be using a fixed value there. This raises a > warning flag for me about the quality of the common clock API > implementation that's being sent and/or the potential for bugs to be > noticed with the common clock framework. I'd not expect this code to > actually work, and looking at the rest of the series I don't see how it > does since I can't see what forces the name. Why the code does work: OK, here is why the driver keeps its state throughout the set and is operational as good or as bad as it was before: The sprintf() in the SPI driver used to construct a name which includes the PSC index. This applies to the UART driver as well, as they both use the PSC controller hardware to implement their serial communication. The corresponding clock name was found in the previous PPC_CLOCK implementation since the clock driver provided those names which include the PSC index (fixed strings in a table). The initial COMMON_CLK implementation in part 09/24 registers clkdev items for compatibility during migration. The string isn't greppable since it's constructed by the preprocessor in the MCLK data setup, see the MCLK_SETUP_DATA_PSC() macro. Part 13/24 adjusts the SPI driver to no longer construct a name, but instead use a fixed string after OF based clock lookup became available. This shall meet your expectation and simplifies the client side. Part 15/24 does the same for the UART driver. Part 16/24 removes the clkdev registration in the clock driver after both the UART and SPI clients switched to OF clock lookups. At this stage things are the way you would expect: The client uses a fixed string in the lookup, while lookup occurs via device tree, and instances are supported without the clients' constructing something based on a component index. Remaining issues are not with the common clock support of the platform, but in the serial driver and are identical to the previous (actually: the initial) status. While we agree on the existance of the remaining issue and the desire to address it. Just not on which context that fix shall be done in. The series' status and perspective: The UART and SPI drivers did work before, while it's true that clock handling wasn't complete (in non-fatal ways). This has been the case in the past, and has been identified just now. The serial drivers are kept operational during migration, and still are operational after the series. But now they use common clock and OF lookups, which is an improvement for the platform in my eyes. It's true that the status of the serial driver isn't improved with the series -- but it isn't degraded either, while (additional) breakage actively gets prevented. Now let me see how I can improve the SPI driver with regard to overall clock handling and beyond mere operation by coincidence in the absence of errors. But please understand that I don't want to stall the common clock support for a whole platform just because some of the drivers it needs to touch to keep them operational have other issues that weren't addressed yet, while these issues aren't introduced with the series but preceed it. virtually yours Gerhard Sittig
On Wed, Jul 17, 2013 at 04:26:28PM +0200, Gerhard Sittig wrote: > On Wed, Jul 17, 2013 at 13:07 +0100, Mark Brown wrote: > > This is a pretty long e-mail. It'd probably have taken less time to > > fix the issues than to reply to the e-mail... but anyway. > Not quite. Please consider that careful submission is as > expensive as thorough review is, and that a lot of work is done > before v1 gets submitted, which you may not always notice from > looking at a concentrated series that may no longer suggest how > much it took to get there (especially when prepared carefully). This is rather undermined the more time and effort gets spent pushing back against doing trival fixes, of course... besides, the issues here are all really simple to fix and test. It's not a major or risky rewrite of the driver or anything like that - most of this can be tested by just probing the driver. > As mentioned before I did not question the need to fix issues as > they get identified. I just asked whether reworking the serial OK, so send patches then. > The initial COMMON_CLK implementation in part 09/24 registers > clkdev items for compatibility during migration. The string > isn't greppable since it's constructed by the preprocessor in the > MCLK data setup, see the MCLK_SETUP_DATA_PSC() macro. Ugh, right - it didn't show up in searches due to being hidden by the macro. > Now let me see how I can improve the SPI driver with regard to > overall clock handling and beyond mere operation by coincidence > in the absence of errors. But please understand that I don't > want to stall the common clock support for a whole platform just > because some of the drivers it needs to touch to keep them > operational have other issues that weren't addressed yet, while > these issues aren't introduced with the series but preceed it. Again, you're not being asked to implement substantial new functionality here. From my point of view I can't test this at all so I'm looking at code that's obviously not good for the standard clock API and wondering if it even works or how robust it's going to be going forward as the common clock code changes which makes me relucatant to say it'll be OK. The fact that you're switching over to use generic code is itself a reason to make sure that the API usage is sane, dodgy API usage against open coded clock implementations is less risky than against the common code.
diff --git a/drivers/spi/spi-mpc512x-psc.c b/drivers/spi/spi-mpc512x-psc.c index 29fce6a..76b20ea 100644 --- a/drivers/spi/spi-mpc512x-psc.c +++ b/drivers/spi/spi-mpc512x-psc.c @@ -395,7 +395,7 @@ static int mpc512x_psc_spi_port_config(struct spi_master *master, sprintf(name, "psc%d_mclk", master->bus_num); spiclk = clk_get(&master->dev, name); - clk_enable(spiclk); + clk_prepare_enable(spiclk); mps->mclk = clk_get_rate(spiclk); clk_put(spiclk);
clocks need to get prepared before they can get enabled, fix the MPC512x PSC SPI master's initialization Signed-off-by: Gerhard Sittig <gsi@denx.de> --- drivers/spi/spi-mpc512x-psc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)