[PATCH] Fix returns of some CLK API calls, if !CONFIG_HAVE_CLOCK

[PATCH] Fix returns of some CLK API calls, if !CONFIG_HAVE_CLOCK

[Thomas Bogendoerfer]

If CONFIG_HAVE_CLOCK is not set, return values of clk_get(),
devm_clk_get(), devm_get_clk_from_child(), clk_get_parent()
and clk_get_sys() are wrong. According to spec these functions
should either return a pointer to a struct clk or a valid IS_ERR
condition. NULL is neither, so returning ERR_PTR(-ENODEV) makes
more sense.

Without this change serial console on SNI RM400 machines (MIPS arch)
is broken, because sccnxp driver doesn't get a valid clock rate.

Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---
 include/linux/clk.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index e9d36b3..b844a65 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -442,18 +442,18 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id);
 
 static inline struct clk *clk_get(struct device *dev, const char *id)
 {
-	return NULL;
+	return ERR_PTR(-ENODEV);
 }
 
 static inline struct clk *devm_clk_get(struct device *dev, const char *id)
 {
-	return NULL;
+	return ERR_PTR(-ENODEV);
 }
 
 static inline struct clk *devm_get_clk_from_child(struct device *dev,
 				struct device_node *np, const char *con_id)
 {
-	return NULL;
+	return ERR_PTR(-ENODEV);
 }
 
 static inline void clk_put(struct clk *clk) {}
@@ -494,12 +494,12 @@ static inline int clk_set_parent(struct clk *clk, struct clk *parent)
 
 static inline struct clk *clk_get_parent(struct clk *clk)
 {
-	return NULL;
+	return ERR_PTR(-ENODEV);
 }
 
 static inline struct clk *clk_get_sys(const char *dev_id, const char *con_id)
 {
-	return NULL;
+	return ERR_PTR(-ENODEV);
 }
 #endif
 
-- 
2.1.4

Re: [PATCH] Fix returns of some CLK API calls, if !CONFIG_HAVE_CLOCK

[Russell King - ARM Linux]

On Tue, Apr 25, 2017 at 02:30:07PM +0200, Thomas Bogendoerfer wrote:
> If CONFIG_HAVE_CLOCK is not set, return values of clk_get(),
> devm_clk_get(), devm_get_clk_from_child(), clk_get_parent()
> and clk_get_sys() are wrong. According to spec these functions
> should either return a pointer to a struct clk or a valid IS_ERR
> condition. NULL is neither, so returning ERR_PTR(-ENODEV) makes
> more sense.

That's wrong.  When the clk API is disabled, the expected behaviour is
that drivers will not fail.  Returning ERR_PTR(-ENODEV) will cause them
to fail, so will break platforms.

NAK.

> Without this change serial console on SNI RM400 machines (MIPS arch)
> is broken, because sccnxp driver doesn't get a valid clock rate.

So the driver needs to depend on HAVE_CLOCK.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] Fix returns of some CLK API calls, if !CONFIG_HAVE_CLOCK

[Tom Bogendoerfer]

On Tue, Apr 25, 2017 at 05:31:37PM +0100, Russell King - ARM Linux wrote:
> On Tue, Apr 25, 2017 at 02:30:07PM +0200, Thomas Bogendoerfer wrote:
> > If CONFIG_HAVE_CLOCK is not set, return values of clk_get(),
> > devm_clk_get(), devm_get_clk_from_child(), clk_get_parent()
> > and clk_get_sys() are wrong. According to spec these functions
> > should either return a pointer to a struct clk or a valid IS_ERR
> > condition. NULL is neither, so returning ERR_PTR(-ENODEV) makes
> > more sense.
> 
> That's wrong.  When the clk API is disabled, the expected behaviour is
> that drivers will not fail.  Returning ERR_PTR(-ENODEV) will cause them
> to fail, so will break platforms.
>
> NAK.
> 

then we have to go through all drivers, which could work with and
without HAVE_CLK and replace the IS_ERR() checks with something
like IS_ERR_OR_NULL().  Easy for the part I'm interested in the first place.

> > Without this change serial console on SNI RM400 machines (MIPS arch)
> > is broken, because sccnxp driver doesn't get a valid clock rate.
> 
> So the driver needs to depeond on HAVE_CLOCK.

the driver worked without HAVE_CLOCK before just fine, and while it
got invaded by CLK API it got broken.

No problem to fix that, but just looking at include/linux/clk.h:

/**
 * devm_clk_get - lookup and obtain a managed reference to a clock producer.
 * @dev: device for clock "consumer"
 * @id: clock consumer ID
 *
 * Returns a struct clk corresponding to the clock producer, or
 * valid IS_ERR() condition containing errno.  The implementation

I would expect either no replacement for that, if !HAVE_CLOCK
or simple a sane result code... and NULL isn't sane at least
with the description above...

But still it's your call.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] Fix returns of some CLK API calls, if !CONFIG_HAVE_CLOCK

[Russell King - ARM Linux]

On Tue, Apr 25, 2017 at 10:54:35PM +0200, Tom Bogendoerfer wrote:
> On Tue, Apr 25, 2017 at 05:31:37PM +0100, Russell King - ARM Linux wrote:
> > On Tue, Apr 25, 2017 at 02:30:07PM +0200, Thomas Bogendoerfer wrote:
> > > If CONFIG_HAVE_CLOCK is not set, return values of clk_get(),
> > > devm_clk_get(), devm_get_clk_from_child(), clk_get_parent()
> > > and clk_get_sys() are wrong. According to spec these functions
> > > should either return a pointer to a struct clk or a valid IS_ERR
> > > condition. NULL is neither, so returning ERR_PTR(-ENODEV) makes
> > > more sense.
> > 
> > That's wrong.  When the clk API is disabled, the expected behaviour is
> > that drivers will not fail.  Returning ERR_PTR(-ENODEV) will cause them
> > to fail, so will break platforms.
> >
> > NAK.
> > 
> 
> then we have to go through all drivers, which could work with and
> without HAVE_CLK and replace the IS_ERR() checks with something
> like IS_ERR_OR_NULL().  Easy for the part I'm interested in the
> first place.

What you describe is exactly what you're proposing to happen if your
patch gets merged - we go from a situation where drivers that do this
today:

	clk = devm_clk_get();
	if (IS_ERR(clk))
		return PTR_ERR(clk);

start failing to probe, whereas the current situation is that they
work.

It's well established that the NULL clk means that there is no clock
available (eg, because the architecture doesn't support the clk API)
and this allows drivers to work across different architectures today.
provided they aren't reliant on obtaining the current clock rate.

> > > Without this change serial console on SNI RM400 machines (MIPS arch)
> > > is broken, because sccnxp driver doesn't get a valid clock rate.
> > 
> > So the driver needs to depeond on HAVE_CLOCK.
> 
> the driver worked without HAVE_CLOCK before just fine, and while it
> got invaded by CLK API it got broken.
> 
> No problem to fix that, but just looking at include/linux/clk.h:
> 
> /**
>  * devm_clk_get - lookup and obtain a managed reference to a clock producer.
>  * @dev: device for clock "consumer"
>  * @id: clock consumer ID
>  *
>  * Returns a struct clk corresponding to the clock producer, or
>  * valid IS_ERR() condition containing errno.  The implementation
> 
> I would expect either no replacement for that, if !HAVE_CLOCK
> or simple a sane result code... and NULL isn't sane at least
> with the description above...

As far as drivers are concerned, any value returned that IS_ERR()
tests false must not be assumed to be a failure.

However, looking at commit 90efa75f7ab0, there's one point that's
definitely wrong, and another that can be improved to avoid your
problem.

1. clk_get_rate() on a disabled clock:

 * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
 *                This is only valid once the clock source has been enabled.

   The clock is expected to be enabled before clk_get_rate() is called,
   and the driver is not doing that.

2. if uartclk is zero after enabling, then there's clearly no clock
   rate available, and the driver really ought to fallback to using
   the old method.

So, I'd suggest that the driver should be coded:

	clk = devm_clk_get(&pdev->dev, NULL);
	if (IS_ERR(clk)) {
		ret = PTR_ERR(clk);
		if (ret == -EPROBE_DEFER)
			goto err_out;
		uartclk = 0;
	} else {
		uartclk = clk_get_rate(clk);
	}

	if (!uartclk) {
		dev_notice(&pdev->dev, "Using default clock frequency\n");
		uartclk = s->freq_std;
	}

	/* Check input frequency */
	...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.