[PATCH 1/3] drivers: phy: Make NULL a valid phy reference

[PATCH 1/3] drivers: phy: Make NULL a valid phy reference

[Andrew Lunn]

The common clock framework considers NULL a valid clock
reference. This makes handling optional clocks simple, in that if the
optional clock is not available, a NULL reference can be used in the
place of a real clock, simplifying the clock consumer.

Extend this concept to the phy consumer API. A NULL can be passed to
the release calls, the phy_init() and phy_exit() calls, and
phy_power_on() and phy_power_off() and a NOP is performed.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 Documentation/phy.txt  |  6 ++++++
 drivers/phy/phy-core.c | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/Documentation/phy.txt b/Documentation/phy.txt
index 0103e4b15b0e..2e24b993e95f 100644
--- a/Documentation/phy.txt
+++ b/Documentation/phy.txt
@@ -84,6 +84,12 @@ The only difference between the two APIs is that devm_phy_get associates the
 device with the PHY using devres on successful PHY get. On driver detach,
 release function is invoked on the the devres data and devres data is freed.
 
+It should be noted that NULL is a valid phy reference. All phy
+consumer calls on the NULL phy become NOPs. That is the release calls,
+the phy_init() and phy_exit() calls, and phy_power_on() and
+phy_power_off() calls are all NOP when applied to a NULL phy. The NULL
+phy is useful in devices for handling optional phy devices.
+
 5. Releasing a reference to the PHY
 
 When the controller no longer needs the PHY, it has to release the reference
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 03cf8fb81554..74d34b5cc75c 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -150,6 +150,9 @@ int phy_init(struct phy *phy)
 {
 	int ret;
 
+	if (!phy)
+		return 0;
+
 	ret = phy_pm_runtime_get_sync(phy);
 	if (ret < 0 && ret != -ENOTSUPP)
 		return ret;
@@ -174,6 +177,9 @@ int phy_exit(struct phy *phy)
 {
 	int ret;
 
+	if (!phy)
+		return 0;
+
 	ret = phy_pm_runtime_get_sync(phy);
 	if (ret < 0 && ret != -ENOTSUPP)
 		return ret;
@@ -198,6 +204,9 @@ int phy_power_on(struct phy *phy)
 {
 	int ret = -ENOTSUPP;
 
+	if (!phy)
+		return 0;
+
 	ret = phy_pm_runtime_get_sync(phy);
 	if (ret < 0 && ret != -ENOTSUPP)
 		return ret;
@@ -222,6 +231,9 @@ int phy_power_off(struct phy *phy)
 {
 	int ret = -ENOTSUPP;
 
+	if (!phy)
+		return 0;
+
 	mutex_lock(&phy->mutex);
 	if (--phy->power_count == 0 && phy->ops->power_off) {
 		ret =  phy->ops->power_off(phy);
@@ -290,6 +302,9 @@ err0:
  */
 void phy_put(struct phy *phy)
 {
+	if (!phy)
+		return;
+
 	if (IS_ERR(phy))
 		return;
 
@@ -310,6 +325,9 @@ void devm_phy_put(struct device *dev, struct phy *phy)
 {
 	int r;
 
+	if (!phy)
+		return;
+
 	r = devres_destroy(dev, devm_phy_release, devm_phy_match, phy);
 	dev_WARN_ONCE(dev, r, "couldn't find PHY resource\n");
 }
-- 
1.8.5.2

[PATCH 2/3] drivers: phy: Add support for optional phys

[Andrew Lunn]

Add devm_phy_optional_get, which should be used when the phy is
optional. It does not return an error when the phy does not exist,
rather it returns NULL, which is considered as a valid phy, but
results in NOPs when used with the consumer API.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 Documentation/phy.txt   | 18 +++++++++++-------
 drivers/phy/phy-core.c  | 27 +++++++++++++++++++++++++--
 include/linux/phy/phy.h |  7 +++++++
 3 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/Documentation/phy.txt b/Documentation/phy.txt
index 2e24b993e95f..1f34a44975ba 100644
--- a/Documentation/phy.txt
+++ b/Documentation/phy.txt
@@ -76,13 +76,17 @@ it. This framework provides the following APIs to get a reference to the PHY.
 
 struct phy *phy_get(struct device *dev, const char *string);
 struct phy *devm_phy_get(struct device *dev, const char *string);
-
-phy_get and devm_phy_get can be used to get the PHY. In the case of dt boot,
-the string arguments should contain the phy name as given in the dt data and
-in the case of non-dt boot, it should contain the label of the PHY.
-The only difference between the two APIs is that devm_phy_get associates the
-device with the PHY using devres on successful PHY get. On driver detach,
-release function is invoked on the the devres data and devres data is freed.
+struct phy *devm_phy_optional_get(struct device *dev, const char *string);
+
+phy_get, devm_phy_get and devm_phy_optional_get can be used to get the
+PHY. In the case of dt boot, the string arguments should contain the
+phy name as given in the dt data and in the case of non-dt boot, it
+should contain the label of the PHY.  The two devm_phy_get associates
+the device with the PHY using devres on successful PHY get. On driver
+detach, release function is invoked on the the devres data and devres
+data is freed. devm_phy_optional_get should be used when the phy is
+optional. It does not return an error when the phy cannot be found, it
+returns NULL.
 
 It should be noted that NULL is a valid phy reference. All phy
 consumer calls on the NULL phy become NOPs. That is the release calls,
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 74d34b5cc75c..4992523111b5 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -390,13 +390,13 @@ struct phy *phy_get(struct device *dev, const char *string)
 			string);
 		phy = of_phy_get(dev, index);
 		if (IS_ERR(phy)) {
-			dev_err(dev, "unable to find phy\n");
+			dev_dbg(dev, "unable to find phy\n");
 			return phy;
 		}
 	} else {
 		phy = phy_lookup(dev, string);
 		if (IS_ERR(phy)) {
-			dev_err(dev, "unable to find phy\n");
+			dev_dbg(dev, "unable to find phy\n");
 			return phy;
 		}
 	}
@@ -441,6 +441,29 @@ struct phy *devm_phy_get(struct device *dev, const char *string)
 EXPORT_SYMBOL_GPL(devm_phy_get);
 
 /**
+ * devm_phy_optional_get() - lookup and obtain a reference to an optional phy.
+ * @dev: device that requests this phy
+ * @string: the phy name as given in the dt data or phy device name
+ * for non-dt case
+ *
+ * Gets the phy using phy_get(), and associates a device with it using
+ * devres. On driver detach, release function is invoked on the devres data,
+ * then, devres data is freed. This differs to devm_phy_get() in that if the
+ * phy does not exist, it is not considered an error. Instead the NULL phy
+ * is returned, which can be passed to all other phy consumer calls.
+ */
+struct phy *devm_phy_optional_get(struct device *dev, const char *string)
+{
+	struct phy *phy = devm_phy_get(dev, string);
+
+	if (PTR_ERR(phy) == -ENODEV)
+		phy = NULL;
+
+	return phy;
+}
+EXPORT_SYMBOL_GPL(devm_phy_optional_get);
+
+/**
  * phy_create() - create a new phy
  * @dev: device that is creating the new phy
  * @ops: function pointers for performing phy operations
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 6d722695e027..8a425533468c 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -129,6 +129,7 @@ int phy_power_on(struct phy *phy);
 int phy_power_off(struct phy *phy);
 struct phy *phy_get(struct device *dev, const char *string);
 struct phy *devm_phy_get(struct device *dev, const char *string);
+struct phy *devm_phy_optional_get(struct device *dev, const char *string);
 void phy_put(struct phy *phy);
 void devm_phy_put(struct device *dev, struct phy *phy);
 struct phy *of_phy_simple_xlate(struct device *dev,
@@ -209,6 +210,12 @@ static inline struct phy *devm_phy_get(struct device *dev, const char *string)
 	return ERR_PTR(-ENOSYS);
 }
 
+static inline struct phy *devm_phy_optional_get(struct device *dev,
+						const char *string)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 static inline void phy_put(struct phy *phy)
 {
 }
-- 
1.8.5.2

Re: [PATCH 2/3] drivers: phy: Add support for optional phys

[Kishon Vijay Abraham I]

Hi,

On Saturday 01 February 2014 07:45 PM, Andrew Lunn wrote:
> Add devm_phy_optional_get, which should be used when the phy is
> optional. It does not return an error when the phy does not exist,
> rather it returns NULL, which is considered as a valid phy, but
> results in NOPs when used with the consumer API.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  Documentation/phy.txt   | 18 +++++++++++-------
>  drivers/phy/phy-core.c  | 27 +++++++++++++++++++++++++--
>  include/linux/phy/phy.h |  7 +++++++
>  3 files changed, 43 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/phy.txt b/Documentation/phy.txt
> index 2e24b993e95f..1f34a44975ba 100644
> --- a/Documentation/phy.txt
> +++ b/Documentation/phy.txt
> @@ -76,13 +76,17 @@ it. This framework provides the following APIs to get a reference to the PHY.
>  
>  struct phy *phy_get(struct device *dev, const char *string);
>  struct phy *devm_phy_get(struct device *dev, const char *string);
> -
> -phy_get and devm_phy_get can be used to get the PHY. In the case of dt boot,
> -the string arguments should contain the phy name as given in the dt data and
> -in the case of non-dt boot, it should contain the label of the PHY.
> -The only difference between the two APIs is that devm_phy_get associates the
> -device with the PHY using devres on successful PHY get. On driver detach,
> -release function is invoked on the the devres data and devres data is freed.
> +struct phy *devm_phy_optional_get(struct device *dev, const char *string);
> +
> +phy_get, devm_phy_get and devm_phy_optional_get can be used to get the
> +PHY. In the case of dt boot, the string arguments should contain the
> +phy name as given in the dt data and in the case of non-dt boot, it
> +should contain the label of the PHY.  The two devm_phy_get associates
> +the device with the PHY using devres on successful PHY get. On driver
> +detach, release function is invoked on the the devres data and devres
> +data is freed. devm_phy_optional_get should be used when the phy is
> +optional. It does not return an error when the phy cannot be found, it
> +returns NULL.
>  
>  It should be noted that NULL is a valid phy reference. All phy
>  consumer calls on the NULL phy become NOPs. That is the release calls,
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 74d34b5cc75c..4992523111b5 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -390,13 +390,13 @@ struct phy *phy_get(struct device *dev, const char *string)
>  			string);
>  		phy = of_phy_get(dev, index);
>  		if (IS_ERR(phy)) {
> -			dev_err(dev, "unable to find phy\n");
> +			dev_dbg(dev, "unable to find phy\n");
>  			return phy;
>  		}
>  	} else {
>  		phy = phy_lookup(dev, string);
>  		if (IS_ERR(phy)) {
> -			dev_err(dev, "unable to find phy\n");
> +			dev_dbg(dev, "unable to find phy\n");
>  			return phy;
>  		}

Hans de Goede has a patch [1] that deals with this. You might want to have a
look at it.
[1] -> https://groups.google.com/forum/#!msg/linux-sunxi/NfWi6rWcGsA/nw2jUw8KVOEJ
>  	}
> @@ -441,6 +441,29 @@ struct phy *devm_phy_get(struct device *dev, const char *string)
>  EXPORT_SYMBOL_GPL(devm_phy_get);
>  
>  /**
> + * devm_phy_optional_get() - lookup and obtain a reference to an optional phy.
> + * @dev: device that requests this phy
> + * @string: the phy name as given in the dt data or phy device name
> + * for non-dt case
> + *
> + * Gets the phy using phy_get(), and associates a device with it using
> + * devres. On driver detach, release function is invoked on the devres data,
> + * then, devres data is freed. This differs to devm_phy_get() in that if the
> + * phy does not exist, it is not considered an error. Instead the NULL phy
> + * is returned, which can be passed to all other phy consumer calls.

It doesn't explain how devm_phy_optional_get is different from phy_get :-s
> + */
> +struct phy *devm_phy_optional_get(struct device *dev, const char *string)
> +{
> +	struct phy *phy = devm_phy_get(dev, string);
> +
> +	if (PTR_ERR(phy) == -ENODEV)
> +		phy = NULL;

Do we need an API in phy-core to handle this? Can't this be done in the
controller driver itself?

Cheers
Kishon

Re: [PATCH 2/3] drivers: phy: Add support for optional phys

[Andrew Lunn]

> >  /**
> > + * devm_phy_optional_get() - lookup and obtain a reference to an optional phy.
> > + * @dev: device that requests this phy
> > + * @string: the phy name as given in the dt data or phy device name
> > + * for non-dt case
> > + *
> > + * Gets the phy using phy_get(), and associates a device with it using
> > + * devres. On driver detach, release function is invoked on the devres data,
> > + * then, devres data is freed. This differs to devm_phy_get() in that if the
> > + * phy does not exist, it is not considered an error. Instead the NULL phy
> > + * is returned, which can be passed to all other phy consumer calls.
> 
> It doesn't explain how devm_phy_optional_get is different from phy_get :-s

How about i append, 

, and so will never return -ENODEV.

to the next but last sentence?

> > + */
> > +struct phy *devm_phy_optional_get(struct device *dev, const char *string)
> > +{
> > +	struct phy *phy = devm_phy_get(dev, string);
> > +
> > +	if (PTR_ERR(phy) == -ENODEV)
> > +		phy = NULL;
> 
> Do we need an API in phy-core to handle this? Can't this be done in the
> controller driver itself?

Sure it can be done in the consumer, but the code is ugly. Take a look
at:

http://marc.info/?l=linux-ide&m=139116894403306&w=2

	Andrew

Re: [PATCH 2/3] drivers: phy: Add support for optional phys

[Kishon Vijay Abraham I]

On Monday 03 February 2014 03:06 PM, Andrew Lunn wrote:
>>>  /**
>>> + * devm_phy_optional_get() - lookup and obtain a reference to an optional phy.
>>> + * @dev: device that requests this phy
>>> + * @string: the phy name as given in the dt data or phy device name
>>> + * for non-dt case
>>> + *
>>> + * Gets the phy using phy_get(), and associates a device with it using
>>> + * devres. On driver detach, release function is invoked on the devres data,
>>> + * then, devres data is freed. This differs to devm_phy_get() in that if the
>>> + * phy does not exist, it is not considered an error. Instead the NULL phy
>>> + * is returned, which can be passed to all other phy consumer calls.
>>
>> It doesn't explain how devm_phy_optional_get is different from phy_get :-s

ah.. it actually explained. Sorry.
> 
> How about i append, 
> 
> , and so will never return -ENODEV.
> 
> to the next but last sentence?

Yeah, that can be added too.
> 
>>> + */
>>> +struct phy *devm_phy_optional_get(struct device *dev, const char *string)
>>> +{
>>> +	struct phy *phy = devm_phy_get(dev, string);
>>> +
>>> +	if (PTR_ERR(phy) == -ENODEV)
>>> +		phy = NULL;
>>
>> Do we need an API in phy-core to handle this? Can't this be done in the
>> controller driver itself?
> 
> Sure it can be done in the consumer, but the code is ugly. Take a look
> at:
> 
> http://marc.info/?l=linux-ide&m=139116894403306&w=2

Ok. Just one comment though. Implement phy_get_optional too so that we have
both the devres and the other variant.

Thanks
Kishon

[patch v2 1/3] drivers: phy: Make NULL a valid phy reference

[Andrew Lunn]

The common clock framework considers NULL a valid clock
reference. This makes handling optional clocks simple, in that if the
optional clock is not available, a NULL reference can be used in the
place of a real clock, simplifying the clock consumer.

Extend this concept to the phy consumer API. A NULL can be passed to
the release calls, the phy_init() and phy_exit() calls, and
phy_power_on() and phy_power_off() and a NOP is performed.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v2
No change.
---
 Documentation/phy.txt  |  6 ++++++
 drivers/phy/phy-core.c | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/Documentation/phy.txt b/Documentation/phy.txt
index 0103e4b15b0e..2e24b993e95f 100644
--- a/Documentation/phy.txt
+++ b/Documentation/phy.txt
@@ -84,6 +84,12 @@ The only difference between the two APIs is that devm_phy_get associates the
 device with the PHY using devres on successful PHY get. On driver detach,
 release function is invoked on the the devres data and devres data is freed.
 
+It should be noted that NULL is a valid phy reference. All phy
+consumer calls on the NULL phy become NOPs. That is the release calls,
+the phy_init() and phy_exit() calls, and phy_power_on() and
+phy_power_off() calls are all NOP when applied to a NULL phy. The NULL
+phy is useful in devices for handling optional phy devices.
+
 5. Releasing a reference to the PHY
 
 When the controller no longer needs the PHY, it has to release the reference
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 645c867c1257..56535253e50c 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -162,6 +162,9 @@ int phy_init(struct phy *phy)
 {
 	int ret;
 
+	if (!phy)
+		return 0;
+
 	ret = phy_pm_runtime_get_sync(phy);
 	if (ret < 0 && ret != -ENOTSUPP)
 		return ret;
@@ -187,6 +190,9 @@ int phy_exit(struct phy *phy)
 {
 	int ret;
 
+	if (!phy)
+		return 0;
+
 	ret = phy_pm_runtime_get_sync(phy);
 	if (ret < 0 && ret != -ENOTSUPP)
 		return ret;
@@ -212,6 +218,9 @@ int phy_power_on(struct phy *phy)
 {
 	int ret;
 
+	if (!phy)
+		return 0;
+
 	ret = phy_pm_runtime_get_sync(phy);
 	if (ret < 0 && ret != -ENOTSUPP)
 		return ret;
@@ -240,6 +249,9 @@ int phy_power_off(struct phy *phy)
 {
 	int ret;
 
+	if (!phy)
+		return 0;
+
 	mutex_lock(&phy->mutex);
 	if (phy->power_count == 1 && phy->ops->power_off) {
 		ret =  phy->ops->power_off(phy);
@@ -308,6 +320,9 @@ err0:
  */
 void phy_put(struct phy *phy)
 {
+	if (!phy)
+		return;
+
 	if (IS_ERR(phy))
 		return;
 
@@ -328,6 +343,9 @@ void devm_phy_put(struct device *dev, struct phy *phy)
 {
 	int r;
 
+	if (!phy)
+		return;
+
 	r = devres_destroy(dev, devm_phy_release, devm_phy_match, phy);
 	dev_WARN_ONCE(dev, r, "couldn't find PHY resource\n");
 }
-- 
1.8.5.2

[patch v2 2/3] drivers: phy: Add support for optional phys

[Andrew Lunn]

Add devm_phy_optional_get and phy_optioanl_get, which should be used
when the phy is optional. They does not return an error when the phy
does not exist, rather they returns NULL, which is considered as a valid
phy, but results in NOPs when used with the consumer API.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v2
Add phy_optional_get()
Improve the comments about the optional functions.
Drop the dev_err()->dev_dbg() changes.
---
 Documentation/phy.txt   | 20 +++++++++++++-------
 drivers/phy/phy-core.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/phy/phy.h | 14 ++++++++++++++
 3 files changed, 72 insertions(+), 7 deletions(-)

diff --git a/Documentation/phy.txt b/Documentation/phy.txt
index 2e24b993e95f..ebff6ee52441 100644
--- a/Documentation/phy.txt
+++ b/Documentation/phy.txt
@@ -75,14 +75,20 @@ Before the controller can make use of the PHY, it has to get a reference to
 it. This framework provides the following APIs to get a reference to the PHY.
 
 struct phy *phy_get(struct device *dev, const char *string);
+struct phy *phy_optional_get(struct device *dev, const char *string);
 struct phy *devm_phy_get(struct device *dev, const char *string);
-
-phy_get and devm_phy_get can be used to get the PHY. In the case of dt boot,
-the string arguments should contain the phy name as given in the dt data and
-in the case of non-dt boot, it should contain the label of the PHY.
-The only difference between the two APIs is that devm_phy_get associates the
-device with the PHY using devres on successful PHY get. On driver detach,
-release function is invoked on the the devres data and devres data is freed.
+struct phy *devm_phy_optional_get(struct device *dev, const char *string);
+
+phy_get, phy_optional_get, devm_phy_get and devm_phy_optional_get can
+be used to get the PHY. In the case of dt boot, the string arguments
+should contain the phy name as given in the dt data and in the case of
+non-dt boot, it should contain the label of the PHY.  The two
+devm_phy_get associates the device with the PHY using devres on
+successful PHY get. On driver detach, release function is invoked on
+the the devres data and devres data is freed. phy_optional_get and
+devm_phy_optional_get should be used when the phy is optional. These
+two functions will never return -ENODEV, but instead returns NULL when
+the phy cannot be found.
 
 It should be noted that NULL is a valid phy reference. All phy
 consumer calls on the NULL phy become NOPs. That is the release calls,
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 56535253e50c..5fedc606f5c0 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -429,6 +429,27 @@ struct phy *phy_get(struct device *dev, const char *string)
 EXPORT_SYMBOL_GPL(phy_get);
 
 /**
+ * phy_optional_get() - lookup and obtain a reference to an optional phy.
+ * @dev: device that requests this phy
+ * @string: the phy name as given in the dt data or the name of the controller
+ * port for non-dt case
+ *
+ * Returns the phy driver, after getting a refcount to it; or
+ * NULL if there is no such phy.  The caller is responsible for
+ * calling phy_put() to release that count.
+ */
+struct phy *phy_optional_get(struct device *dev, const char *string)
+{
+	struct phy *phy = phy_get(dev, string);
+
+	if (PTR_ERR(phy) == -ENODEV)
+		phy = NULL;
+
+	return phy;
+}
+EXPORT_SYMBOL_GPL(phy_optional_get);
+
+/**
  * devm_phy_get() - lookup and obtain a reference to a phy.
  * @dev: device that requests this phy
  * @string: the phy name as given in the dt data or phy device name
@@ -459,6 +480,30 @@ struct phy *devm_phy_get(struct device *dev, const char *string)
 EXPORT_SYMBOL_GPL(devm_phy_get);
 
 /**
+ * devm_phy_optional_get() - lookup and obtain a reference to an optional phy.
+ * @dev: device that requests this phy
+ * @string: the phy name as given in the dt data or phy device name
+ * for non-dt case
+ *
+ * Gets the phy using phy_get(), and associates a device with it using
+ * devres. On driver detach, release function is invoked on the devres
+ * data, then, devres data is freed. This differs to devm_phy_get() in
+ * that if the phy does not exist, it is not considered an error and
+ * -ENODEV will not be returned. Instead the NULL phy is returned,
+ * which can be passed to all other phy consumer calls.
+ */
+struct phy *devm_phy_optional_get(struct device *dev, const char *string)
+{
+	struct phy *phy = devm_phy_get(dev, string);
+
+	if (PTR_ERR(phy) == -ENODEV)
+		phy = NULL;
+
+	return phy;
+}
+EXPORT_SYMBOL_GPL(devm_phy_optional_get);
+
+/**
  * phy_create() - create a new phy
  * @dev: device that is creating the new phy
  * @ops: function pointers for performing phy operations
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index e273e5ac19c9..3f83459dbb20 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -146,7 +146,9 @@ static inline void phy_set_bus_width(struct phy *phy, int bus_width)
 	phy->attrs.bus_width = bus_width;
 }
 struct phy *phy_get(struct device *dev, const char *string);
+struct phy *phy_optional_get(struct device *dev, const char *string);
 struct phy *devm_phy_get(struct device *dev, const char *string);
+struct phy *devm_phy_optional_get(struct device *dev, const char *string);
 void phy_put(struct phy *phy);
 void devm_phy_put(struct device *dev, struct phy *phy);
 struct phy *of_phy_simple_xlate(struct device *dev,
@@ -232,11 +234,23 @@ static inline struct phy *phy_get(struct device *dev, const char *string)
 	return ERR_PTR(-ENOSYS);
 }
 
+static inline struct phy *phy_optional_get(struct device *dev,
+					   const char *string)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 static inline struct phy *devm_phy_get(struct device *dev, const char *string)
 {
 	return ERR_PTR(-ENOSYS);
 }
 
+static inline struct phy *devm_phy_optional_get(struct device *dev,
+						const char *string)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 static inline void phy_put(struct phy *phy)
 {
 }
-- 
1.8.5.2

[patch v2 3/3] ata: sata_mv: Fix probe failures with optional phys

[Andrew Lunn]

Make use of devm_phy_optional() in order to fix probe failures on
Armada 370, XP and others, when there is no phy driver available.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v2
No change
---
 drivers/ata/sata_mv.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 20a7517bd339..52b8181ddafd 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -4126,12 +4126,14 @@ static int mv_platform_probe(struct platform_device *pdev)
 			clk_prepare_enable(hpriv->port_clks[port]);
 
 		sprintf(port_number, "port%d", port);
-		hpriv->port_phys[port] = devm_phy_get(&pdev->dev, port_number);
+		hpriv->port_phys[port] = devm_phy_optional_get(&pdev->dev,
+							       port_number);
 		if (IS_ERR(hpriv->port_phys[port])) {
 			rc = PTR_ERR(hpriv->port_phys[port]);
 			hpriv->port_phys[port] = NULL;
-			if ((rc != -EPROBE_DEFER) && (rc != -ENODEV))
-				dev_warn(&pdev->dev, "error getting phy");
+			if (rc != -EPROBE_DEFER)
+				dev_warn(&pdev->dev, "error getting phy %d",
+					rc);
 			goto err;
 		} else
 			phy_power_on(hpriv->port_phys[port]);
-- 
1.8.5.2

Re: [patch v2 3/3] ata: sata_mv: Fix probe failures with optional phys

[Tejun Heo]

On Mon, Feb 03, 2014 at 07:00:46PM +0100, Andrew Lunn wrote:
> Make use of devm_phy_optional() in order to fix probe failures on
> Armada 370, XP and others, when there is no phy driver available.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

 Acked-by: Tejun Heo <tj@kernel.org>

Please feel free to route any way you see fit.

Thanks!

-- 
tejun

Re: [patch v2 3/3] ata: sata_mv: Fix probe failures with optional phys

[Sergei Shtylyov]

Hello.

On 02/03/2014 09:00 PM, Andrew Lunn wrote:

> Make use of devm_phy_optional() in order to fix probe failures on

    devm_phy_optional_get() instead.

> Armada 370, XP and others, when there is no phy driver available.

> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> v2
> No change
> ---
>   drivers/ata/sata_mv.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)

> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 20a7517bd339..52b8181ddafd 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -4126,12 +4126,14 @@ static int mv_platform_probe(struct platform_device *pdev)
>   			clk_prepare_enable(hpriv->port_clks[port]);
>
>   		sprintf(port_number, "port%d", port);
> -		hpriv->port_phys[port] = devm_phy_get(&pdev->dev, port_number);
> +		hpriv->port_phys[port] = devm_phy_optional_get(&pdev->dev,
> +							       port_number);

WBR, Sergei

[patch v3 3/3] ata: sata_mv: Fix probe failures with optional phys

[Andrew Lunn]

Make use of devm_optional_get() in order to fix probe failures on
Armada 370, XP and others, when there is no phy driver available.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v2
No change
v3
Fix commit log devm_get_optional->devm_option_get.
---
 drivers/ata/sata_mv.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 20a7517bd339..52b8181ddafd 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -4126,12 +4126,14 @@ static int mv_platform_probe(struct platform_device *pdev)
 			clk_prepare_enable(hpriv->port_clks[port]);
 
 		sprintf(port_number, "port%d", port);
-		hpriv->port_phys[port] = devm_phy_get(&pdev->dev, port_number);
+		hpriv->port_phys[port] = devm_phy_optional_get(&pdev->dev,
+							       port_number);
 		if (IS_ERR(hpriv->port_phys[port])) {
 			rc = PTR_ERR(hpriv->port_phys[port]);
 			hpriv->port_phys[port] = NULL;
-			if ((rc != -EPROBE_DEFER) && (rc != -ENODEV))
-				dev_warn(&pdev->dev, "error getting phy");
+			if (rc != -EPROBE_DEFER)
+				dev_warn(&pdev->dev, "error getting phy %d",
+					rc);
 			goto err;
 		} else
 			phy_power_on(hpriv->port_phys[port]);
-- 
1.8.5.2

Re: [patch v3 3/3] ata: sata_mv: Fix probe failures with optional phys

[Sergei Shtylyov]

Hello.

On 02/03/2014 09:17 PM, Andrew Lunn wrote:

> Make use of devm_optional_get() in order to fix probe failures on

    It's devm_phy_optional_get() according to your own source.

> Armada 370, XP and others, when there is no phy driver available.

> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> v2
> No change
> v3
> Fix commit log devm_get_optional->devm_option_get.
> ---
>   drivers/ata/sata_mv.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)

> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 20a7517bd339..52b8181ddafd 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -4126,12 +4126,14 @@ static int mv_platform_probe(struct platform_device *pdev)
>   			clk_prepare_enable(hpriv->port_clks[port]);
>
>   		sprintf(port_number, "port%d", port);
> -		hpriv->port_phys[port] = devm_phy_get(&pdev->dev, port_number);
> +		hpriv->port_phys[port] = devm_phy_optional_get(&pdev->dev,
> +							       port_number);

WBR, Sergei

Re: [patch v2 1/3] drivers: phy: Make NULL a valid phy reference

[Sergei Shtylyov]

Hello.

On 02/03/2014 09:00 PM, Andrew Lunn wrote:

> The common clock framework considers NULL a valid clock
> reference. This makes handling optional clocks simple, in that if the
> optional clock is not available, a NULL reference can be used in the
> place of a real clock, simplifying the clock consumer.

> Extend this concept to the phy consumer API. A NULL can be passed to
> the release calls, the phy_init() and phy_exit() calls, and
> phy_power_on() and phy_power_off() and a NOP is performed.

> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
[...]

>   When the controller no longer needs the PHY, it has to release the reference
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 645c867c1257..56535253e50c 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
[...]
> @@ -308,6 +320,9 @@ err0:
>    */
>   void phy_put(struct phy *phy)
>   {
> +	if (!phy)
> +		return;
> +
>   	if (IS_ERR(phy))
>   		return;

    Why not merge these two?

WBR, Sergei

Re: [patch v2 1/3] drivers: phy: Make NULL a valid phy reference

[Andrew Lunn]

On Mon, Feb 03, 2014 at 10:06:09PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 02/03/2014 09:00 PM, Andrew Lunn wrote:
> 
> >The common clock framework considers NULL a valid clock
> >reference. This makes handling optional clocks simple, in that if the
> >optional clock is not available, a NULL reference can be used in the
> >place of a real clock, simplifying the clock consumer.
> 
> >Extend this concept to the phy consumer API. A NULL can be passed to
> >the release calls, the phy_init() and phy_exit() calls, and
> >phy_power_on() and phy_power_off() and a NOP is performed.
> 
> >Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> [...]
> 
> >  When the controller no longer needs the PHY, it has to release the reference
> >diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> >index 645c867c1257..56535253e50c 100644
> >--- a/drivers/phy/phy-core.c
> >+++ b/drivers/phy/phy-core.c
> [...]
> >@@ -308,6 +320,9 @@ err0:
> >   */
> >  void phy_put(struct phy *phy)
> >  {
> >+	if (!phy)
> >+		return;
> >+
> >  	if (IS_ERR(phy))
> >  		return;
> 
>    Why not merge these two?

Hi Sergei

Have you heard Russell King talk about IS_ERR_NULL()?

     Andrew

Re: [patch v2 1/3] drivers: phy: Make NULL a valid phy reference

[Sergei Shtylyov]

On 02/03/2014 09:13 PM, Andrew Lunn wrote:

>>> The common clock framework considers NULL a valid clock
>>> reference. This makes handling optional clocks simple, in that if the
>>> optional clock is not available, a NULL reference can be used in the
>>> place of a real clock, simplifying the clock consumer.

>>> Extend this concept to the phy consumer API. A NULL can be passed to
>>> the release calls, the phy_init() and phy_exit() calls, and
>>> phy_power_on() and phy_power_off() and a NOP is performed.

>>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>> [...]

>>>   When the controller no longer needs the PHY, it has to release the reference
>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>> index 645c867c1257..56535253e50c 100644
>>> --- a/drivers/phy/phy-core.c
>>> +++ b/drivers/phy/phy-core.c
>> [...]
>>> @@ -308,6 +320,9 @@ err0:
>>>    */
>>>   void phy_put(struct phy *phy)
>>>   {
>>> +	if (!phy)
>>> +		return;
>>> +
>>>   	if (IS_ERR(phy))
>>>   		return;

>>     Why not merge these two?

> Hi Sergei

> Have you heard Russell King talk about IS_ERR_NULL()?

    Probably not but I didn't think of it actually, just:

	if (!phy || IS_ERR(phy))
		return;

PS: BTW, I'm not sure why phy_put() has to handle error pointers...

>       Andrew

WBR, Sergei

Re: [patch v2 1/3] drivers: phy: Make NULL a valid phy reference

[Gregory CLEMENT]

On 03/02/2014 19:00, Andrew Lunn wrote:
> The common clock framework considers NULL a valid clock
> reference. This makes handling optional clocks simple, in that if the
> optional clock is not available, a NULL reference can be used in the
> place of a real clock, simplifying the clock consumer.
> 
> Extend this concept to the phy consumer API. A NULL can be passed to
> the release calls, the phy_init() and phy_exit() calls, and
> phy_power_on() and phy_power_off() and a NOP is performed.
> 
Hi Andrew,

I have tested these 3 patches on severals Armada 370/XP boards:
 - Mirabox
 - Armada 370 DB
 - OpenBlocks AX3
 - Armada XP GP

For all this boards without these series the kernel crash
during boot on v3.14-rc1.
With this series (and "ATA: SATA_MV: Add missing Kconfig
select statememnt" patch), the kernel boot fine on all
these boards.

You can add my

Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

for the whole series.


Thanks,

Gregory




> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> v2
> No change.
> ---
>  Documentation/phy.txt  |  6 ++++++
>  drivers/phy/phy-core.c | 18 ++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/phy.txt b/Documentation/phy.txt
> index 0103e4b15b0e..2e24b993e95f 100644
> --- a/Documentation/phy.txt
> +++ b/Documentation/phy.txt
> @@ -84,6 +84,12 @@ The only difference between the two APIs is that devm_phy_get associates the
>  device with the PHY using devres on successful PHY get. On driver detach,
>  release function is invoked on the the devres data and devres data is freed.
>  
> +It should be noted that NULL is a valid phy reference. All phy
> +consumer calls on the NULL phy become NOPs. That is the release calls,
> +the phy_init() and phy_exit() calls, and phy_power_on() and
> +phy_power_off() calls are all NOP when applied to a NULL phy. The NULL
> +phy is useful in devices for handling optional phy devices.
> +
>  5. Releasing a reference to the PHY
>  
>  When the controller no longer needs the PHY, it has to release the reference
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 645c867c1257..56535253e50c 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -162,6 +162,9 @@ int phy_init(struct phy *phy)
>  {
>  	int ret;
>  
> +	if (!phy)
> +		return 0;
> +
>  	ret = phy_pm_runtime_get_sync(phy);
>  	if (ret < 0 && ret != -ENOTSUPP)
>  		return ret;
> @@ -187,6 +190,9 @@ int phy_exit(struct phy *phy)
>  {
>  	int ret;
>  
> +	if (!phy)
> +		return 0;
> +
>  	ret = phy_pm_runtime_get_sync(phy);
>  	if (ret < 0 && ret != -ENOTSUPP)
>  		return ret;
> @@ -212,6 +218,9 @@ int phy_power_on(struct phy *phy)
>  {
>  	int ret;
>  
> +	if (!phy)
> +		return 0;
> +
>  	ret = phy_pm_runtime_get_sync(phy);
>  	if (ret < 0 && ret != -ENOTSUPP)
>  		return ret;
> @@ -240,6 +249,9 @@ int phy_power_off(struct phy *phy)
>  {
>  	int ret;
>  
> +	if (!phy)
> +		return 0;
> +
>  	mutex_lock(&phy->mutex);
>  	if (phy->power_count == 1 && phy->ops->power_off) {
>  		ret =  phy->ops->power_off(phy);
> @@ -308,6 +320,9 @@ err0:
>   */
>  void phy_put(struct phy *phy)
>  {
> +	if (!phy)
> +		return;
> +
>  	if (IS_ERR(phy))
>  		return;
>  
> @@ -328,6 +343,9 @@ void devm_phy_put(struct device *dev, struct phy *phy)
>  {
>  	int r;
>  
> +	if (!phy)
> +		return;
> +
>  	r = devres_destroy(dev, devm_phy_release, devm_phy_match, phy);
>  	dev_WARN_ONCE(dev, r, "couldn't find PHY resource\n");
>  }
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 3/3] ata: sata_mv: Fix probe failures with optional phys

[Andrew Lunn]

Make use of devm_phy_optional() in order to fix probe failures on
Armada 370, XP and others, when there is no phy driver available.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/ata/sata_mv.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index eaa21eddbe70..26021cf077a8 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -4111,12 +4111,14 @@ static int mv_platform_probe(struct platform_device *pdev)
 			clk_prepare_enable(hpriv->port_clks[port]);
 
 		sprintf(port_number, "port%d", port);
-		hpriv->port_phys[port] = devm_phy_get(&pdev->dev, port_number);
+		hpriv->port_phys[port] = devm_phy_optional_get(&pdev->dev,
+							       port_number);
 		if (IS_ERR(hpriv->port_phys[port])) {
 			rc = PTR_ERR(hpriv->port_phys[port]);
 			hpriv->port_phys[port] = NULL;
-			if ((rc != -EPROBE_DEFER) && (rc != -ENODEV))
-				dev_warn(&pdev->dev, "error getting phy");
+			if (rc != -EPROBE_DEFER)
+				dev_warn(&pdev->dev, "error getting phy %d",
+					rc);
 			goto err;
 		} else
 			phy_power_on(hpriv->port_phys[port]);
-- 
1.8.5.2

Re: [PATCH 3/3] ata: sata_mv: Fix probe failures with optional phys

[Ezequiel Garcia]

On Sat, Feb 01, 2014 at 03:15:57PM +0100, Andrew Lunn wrote:
> Make use of devm_phy_optional() in order to fix probe failures on
> Armada 370, XP and others, when there is no phy driver available.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/ata/sata_mv.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index eaa21eddbe70..26021cf077a8 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -4111,12 +4111,14 @@ static int mv_platform_probe(struct platform_device *pdev)
>  			clk_prepare_enable(hpriv->port_clks[port]);
>  
>  		sprintf(port_number, "port%d", port);
> -		hpriv->port_phys[port] = devm_phy_get(&pdev->dev, port_number);
> +		hpriv->port_phys[port] = devm_phy_optional_get(&pdev->dev,
> +							       port_number);
>  		if (IS_ERR(hpriv->port_phys[port])) {
>  			rc = PTR_ERR(hpriv->port_phys[port]);
>  			hpriv->port_phys[port] = NULL;
> -			if ((rc != -EPROBE_DEFER) && (rc != -ENODEV))
> -				dev_warn(&pdev->dev, "error getting phy");
> +			if (rc != -EPROBE_DEFER)
> +				dev_warn(&pdev->dev, "error getting phy %d",
> +					rc);
>  			goto err;

IMHO, this new series look much better. However, I still think the above code
is highly confusing (took me some time to see why you don't print the warning
on PROBE_DEFER, but do the goto in all cases).

Would it be too much to ask to add some comments to it? Your previous
explanation about why we need to fail on EPROBE_DEFER, to allow the phy
driver to load, was great. Adding some of that here would be nice.

I'll test this next week.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 3/3] ata: sata_mv: Fix probe failures with optional phys

[Andrew Lunn]

> IMHO, this new series look much better. However, I still think the above code
> is highly confusing (took me some time to see why you don't print the warning
> on PROBE_DEFER, but do the goto in all cases).
> 
> Would it be too much to ask to add some comments to it? Your previous
> explanation about why we need to fail on EPROBE_DEFER, to allow the phy
> driver to load, was great. Adding some of that here would be nice.

Hi Ezequiel

Anybody writing device drivers should know about EPROBE_DEFER.  If
they don't they are writing broken drivers. So putting in a comment
here would be just pointing out the obvious.

What i can however do is add a comment that devm_phy_get_optional()
returns a valid phy if there is no error. Might that help with the
confusion?

Thanks
	Andrew

Re: [PATCH 3/3] ata: sata_mv: Fix probe failures with optional phys

[Ezequiel Garcia]

On Sat, Feb 01, 2014 at 03:56:59PM +0100, Andrew Lunn wrote:
> > IMHO, this new series look much better. However, I still think the above code
> > is highly confusing (took me some time to see why you don't print the warning
> > on PROBE_DEFER, but do the goto in all cases).
> > 
> > Would it be too much to ask to add some comments to it? Your previous
> > explanation about why we need to fail on EPROBE_DEFER, to allow the phy
> > driver to load, was great. Adding some of that here would be nice.
> 
> Anybody writing device drivers should know about EPROBE_DEFER.  If
> they don't they are writing broken drivers. So putting in a comment
> here would be just pointing out the obvious.
> 

Maybe you're right. It wasn't obvious to me, though.

> What i can however do is add a comment that devm_phy_get_optional()
> returns a valid phy if there is no error. Might that help with the
> confusion?
> 

Well, In don't think devm_phy_get_optional() brings any confusion.
The name itself is pretty self-explanatory, and in that case you can always
go look at the function implementation and documentation.

So, I'd say no.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 3/3] ata: sata_mv: Fix probe failures with optional phys

[Tejun Heo]

On Sat, Feb 01, 2014 at 03:15:57PM +0100, Andrew Lunn wrote:
> Make use of devm_phy_optional() in order to fix probe failures on
> Armada 370, XP and others, when there is no phy driver available.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Looks good to me from the ata side.  How should these be routed?  I
can take them through libata/for-3.14-fixes once phy people ack the
changes or the other way around is fine too.

Thanks.

-- 
tejun

Re: [PATCH 3/3] ata: sata_mv: Fix probe failures with optional phys

[Andrew Lunn]

On Mon, Feb 03, 2014 at 11:03:15AM -0500, Tejun Heo wrote:
> On Sat, Feb 01, 2014 at 03:15:57PM +0100, Andrew Lunn wrote:
> > Make use of devm_phy_optional() in order to fix probe failures on
> > Armada 370, XP and others, when there is no phy driver available.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> Looks good to me from the ata side.  How should these be routed?  I
> can take them through libata/for-3.14-fixes once phy people ack the
> changes or the other way around is fine too.

Hi Tejun

There should be a v2 today. I don't care too much how it goes in, but
we should aim to get them in by -rc2 since we have quite a few broken
platforms at the moment.

	  Andrew

Re: [PATCH 3/3] ata: sata_mv: Fix probe failures with optional phys

[Tejun Heo]

On Mon, Feb 03, 2014 at 05:20:10PM +0100, Andrew Lunn wrote:
> On Mon, Feb 03, 2014 at 11:03:15AM -0500, Tejun Heo wrote:
> > On Sat, Feb 01, 2014 at 03:15:57PM +0100, Andrew Lunn wrote:
> > > Make use of devm_phy_optional() in order to fix probe failures on
> > > Armada 370, XP and others, when there is no phy driver available.
> > > 
> > > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > 
> > Looks good to me from the ata side.  How should these be routed?  I
> > can take them through libata/for-3.14-fixes once phy people ack the
> > changes or the other way around is fine too.
> 
> Hi Tejun
> 
> There should be a v2 today. I don't care too much how it goes in, but
> we should aim to get them in by -rc2 since we have quite a few broken
> platforms at the moment.

I'm okay either way.  If the phy tree wants to take it, please feel
free to add my Acked-by when posting v2.

Thanks.

-- 
tejun

Re: [PATCH 3/3] ata: sata_mv: Fix probe failures with optional phys

[Gregory CLEMENT]

On 03/02/2014 17:03, Tejun Heo wrote:
> On Sat, Feb 01, 2014 at 03:15:57PM +0100, Andrew Lunn wrote:
>> Make use of devm_phy_optional() in order to fix probe failures on
>> Armada 370, XP and others, when there is no phy driver available.
>>
>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> Looks good to me from the ata side.  How should these be routed?  I
> can take them through libata/for-3.14-fixes once phy people ack the
> changes or the other way around is fine too.
> 
> Thanks.
> 

Hi Tejun, Andrew,

I have just tested it on both Armada 370 and Armada XP and it failed
at the end of the mv_platform_probe function while trying to do a
clk_put on the clocks on the second port.

Indeed the devm_phy_optional_get failed on the 1st port, so the clock
of the second port was not allocated. What I don't get is why the
IS_ERR check don't work here.

Here the log of the crash:

Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
d0012000.serial: ttyS0 at MMIO 0xd0012000 (irq = 20, base_baud = 15625000) is a 16550A
console [ttyS0] enabled
d0012100.serial: ttyS1 at MMIO 0xd0012100 (irq = 21, base_baud = 15625000) is a 16550A
sata_mv d00a0000.sata: error getting phy -38
Unable to handle kernel NULL pointer dereference at virtual address 00000054
pgd = c0004000
[00000054] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.0-rc1-00007-g5787958c945a #989
task: ed82bb80 ti: ed83e000 task.ti: ed83e000
PC is at __clk_put+0x1c/0x8c
LR is at clk_prepare_lock+0xc/0xd8
pc : [<c02c02e4>]    lr : [<c02bdac8>]    psr: 60000113
sp : ed83fe18  ip : 00000000  fp : ed807b80
r10: ed908200  r9 : ed234290  r8 : ffffffda
r7 : ed908210  r6 : 00000000  r5 : eda5ced0  r4 : 00000000
r3 : ed82bb80  r2 : 00000001  r1 : ed83fe08  r0 : 00000054
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c53c7d  Table: 0000406a  DAC: 00000015
Process swapper/0 (pid: 1, stack limit = 0xed83e240)
Stack: (0xed83fe18 to 0xed840000)
fe00:                                                       00000001 c01f7390
fe20: ed90ee10 00000001 c04d658c 0000001f ed90ee10 00000002 c03b4e20 00000000
fe40: 74726f70 ed900030 00000001 ed232ca8 ed83e000 ed908210 c07d11c8 00000000
fe60: c07d11c8 c04d658c ed83e000 c04c2510 00000000 c01c2f38 c01c2f20 ed908210
fe80: c0809d88 c01c1ba8 00000000 ed908210 c07d11c8 ed908244 00000000 c01c1d50
fea0: 00000000 c07d11c8 c01c1cc4 c01c03b8 ed85b7dc ed85f1b4 c07d11c8 ed234300
fec0: c07cff90 c01c1380 c045e658 c04e4bf8 c07d11c8 c07d11c8 00000006 c04e4bf8
fee0: c07e6d80 c01c2390 00000000 00000000 00000006 c04d65b4 c04eb3c4 c0008874
ff00: 00000086 ef7fcd48 c04459a8 00000066 00000000 c049fd4c 00000063 ed83ff30
ff20: c0034d48 c0034d4c 00000113 ffffffff ef7fcd55 c03ae82c 00000086 c0034f34
ff40: c049f6ec 00000006 ef7fcd60 00000006 c07bfb70 c04eb3c4 00000006 c04e4bf8
ff60: c07e6d80 00000086 c04e4c04 c04c2510 00000000 c04c2bec 00000006 00000006
ff80: c04c2510 4c011108 00000000 c0393808 00000000 00000000 00000000 00000000
ffa0: 00000000 c0393810 00000000 c000e378 00000000 00000000 00000000 00000000
ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
ffe0: 00000000 00000000 00000000 00000000 00000013 00000000 8041106c 11928242
[<c02c02e4>] (__clk_put) from [<c01f7390>] (mv_platform_probe+0x29c/0x410)
[<c01f7390>] (mv_platform_probe) from [<c01c2f38>] (platform_drv_probe+0x18/0x48)
[<c01c2f38>] (platform_drv_probe) from [<c01c1ba8>] (driver_probe_device+0x104/0x220)
[<c01c1ba8>] (driver_probe_device) from [<c01c1d50>] (__driver_attach+0x8c/0x90)
[<c01c1d50>] (__driver_attach) from [<c01c03b8>] (bus_for_each_dev+0x54/0x88)
[<c01c03b8>] (bus_for_each_dev) from [<c01c1380>] (bus_add_driver+0xd4/0x1d0)
[<c01c1380>] (bus_add_driver) from [<c01c2390>] (driver_register+0x78/0xf4)
[<c01c2390>] (driver_register) from [<c04d65b4>] (mv_init+0x28/0x4c)
[<c04d65b4>] (mv_init) from [<c0008874>] (do_one_initcall+0xe4/0x140)
[<c0008874>] (do_one_initcall) from [<c04c2bec>] (kernel_init_freeable+0xfc/0x1c8)
[<c04c2bec>] (kernel_init_freeable) from [<c0393810>] (kernel_init+0x8/0x118)
[<c0393810>] (kernel_init) from [<c000e378>] (ret_from_fork+0x14/0x3c)
Code: 8a000012 ebfff5f7 e2840054 f57ff05b (e1903f9f)
---[ end trace 31531e36499d4923 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b



Thanks,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 3/3] ata: sata_mv: Fix probe failures with optional phys

[Gregory CLEMENT]

On 03/02/2014 17:21, Gregory CLEMENT wrote:
> On 03/02/2014 17:03, Tejun Heo wrote:
>> On Sat, Feb 01, 2014 at 03:15:57PM +0100, Andrew Lunn wrote:
>>> Make use of devm_phy_optional() in order to fix probe failures on
>>> Armada 370, XP and others, when there is no phy driver available.
>>>
>>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>>
>> Looks good to me from the ata side.  How should these be routed?  I
>> can take them through libata/for-3.14-fixes once phy people ack the
>> changes or the other way around is fine too.
>>
>> Thanks.
>>
> 
> Hi Tejun, Andrew,
> 
> I have just tested it on both Armada 370 and Armada XP and it failed
> at the end of the mv_platform_probe function while trying to do a
> clk_put on the clocks on the second port.
> 
> Indeed the devm_phy_optional_get failed on the 1st port, so the clock
> of the second port was not allocated. What I don't get is why the
> IS_ERR check don't work here.


I found the root of my issue!

Actually devm_phy_optional_get failed but by returning -ENOSYS.
The phy lib was not selected because I have just applied the 3 patches of the series
and not the other one "ATA: SATA_MV: Add missing Kconfig select statememnt"

Could you confirm that it will be part of the 3.14-rc2 too?


Thanks,

Gregory


> 
> Here the log of the crash:
> 
> Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> d0012000.serial: ttyS0 at MMIO 0xd0012000 (irq = 20, base_baud = 15625000) is a 16550A
> console [ttyS0] enabled
> d0012100.serial: ttyS1 at MMIO 0xd0012100 (irq = 21, base_baud = 15625000) is a 16550A
> sata_mv d00a0000.sata: error getting phy -38
> Unable to handle kernel NULL pointer dereference at virtual address 00000054
> pgd = c0004000
> [00000054] *pgd=00000000
> Internal error: Oops: 5 [#1] SMP ARM
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.0-rc1-00007-g5787958c945a #989
> task: ed82bb80 ti: ed83e000 task.ti: ed83e000
> PC is at __clk_put+0x1c/0x8c
> LR is at clk_prepare_lock+0xc/0xd8
> pc : [<c02c02e4>]    lr : [<c02bdac8>]    psr: 60000113
> sp : ed83fe18  ip : 00000000  fp : ed807b80
> r10: ed908200  r9 : ed234290  r8 : ffffffda
> r7 : ed908210  r6 : 00000000  r5 : eda5ced0  r4 : 00000000
> r3 : ed82bb80  r2 : 00000001  r1 : ed83fe08  r0 : 00000054
> Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
> Control: 10c53c7d  Table: 0000406a  DAC: 00000015
> Process swapper/0 (pid: 1, stack limit = 0xed83e240)
> Stack: (0xed83fe18 to 0xed840000)
> fe00:                                                       00000001 c01f7390
> fe20: ed90ee10 00000001 c04d658c 0000001f ed90ee10 00000002 c03b4e20 00000000
> fe40: 74726f70 ed900030 00000001 ed232ca8 ed83e000 ed908210 c07d11c8 00000000
> fe60: c07d11c8 c04d658c ed83e000 c04c2510 00000000 c01c2f38 c01c2f20 ed908210
> fe80: c0809d88 c01c1ba8 00000000 ed908210 c07d11c8 ed908244 00000000 c01c1d50
> fea0: 00000000 c07d11c8 c01c1cc4 c01c03b8 ed85b7dc ed85f1b4 c07d11c8 ed234300
> fec0: c07cff90 c01c1380 c045e658 c04e4bf8 c07d11c8 c07d11c8 00000006 c04e4bf8
> fee0: c07e6d80 c01c2390 00000000 00000000 00000006 c04d65b4 c04eb3c4 c0008874
> ff00: 00000086 ef7fcd48 c04459a8 00000066 00000000 c049fd4c 00000063 ed83ff30
> ff20: c0034d48 c0034d4c 00000113 ffffffff ef7fcd55 c03ae82c 00000086 c0034f34
> ff40: c049f6ec 00000006 ef7fcd60 00000006 c07bfb70 c04eb3c4 00000006 c04e4bf8
> ff60: c07e6d80 00000086 c04e4c04 c04c2510 00000000 c04c2bec 00000006 00000006
> ff80: c04c2510 4c011108 00000000 c0393808 00000000 00000000 00000000 00000000
> ffa0: 00000000 c0393810 00000000 c000e378 00000000 00000000 00000000 00000000
> ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> ffe0: 00000000 00000000 00000000 00000000 00000013 00000000 8041106c 11928242
> [<c02c02e4>] (__clk_put) from [<c01f7390>] (mv_platform_probe+0x29c/0x410)
> [<c01f7390>] (mv_platform_probe) from [<c01c2f38>] (platform_drv_probe+0x18/0x48)
> [<c01c2f38>] (platform_drv_probe) from [<c01c1ba8>] (driver_probe_device+0x104/0x220)
> [<c01c1ba8>] (driver_probe_device) from [<c01c1d50>] (__driver_attach+0x8c/0x90)
> [<c01c1d50>] (__driver_attach) from [<c01c03b8>] (bus_for_each_dev+0x54/0x88)
> [<c01c03b8>] (bus_for_each_dev) from [<c01c1380>] (bus_add_driver+0xd4/0x1d0)
> [<c01c1380>] (bus_add_driver) from [<c01c2390>] (driver_register+0x78/0xf4)
> [<c01c2390>] (driver_register) from [<c04d65b4>] (mv_init+0x28/0x4c)
> [<c04d65b4>] (mv_init) from [<c0008874>] (do_one_initcall+0xe4/0x140)
> [<c0008874>] (do_one_initcall) from [<c04c2bec>] (kernel_init_freeable+0xfc/0x1c8)
> [<c04c2bec>] (kernel_init_freeable) from [<c0393810>] (kernel_init+0x8/0x118)
> [<c0393810>] (kernel_init) from [<c000e378>] (ret_from_fork+0x14/0x3c)
> Code: 8a000012 ebfff5f7 e2840054 f57ff05b (e1903f9f)
> ---[ end trace 31531e36499d4923 ]---
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> 
> 
> 
> Thanks,
> 
> Gregory
> 
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 3/3] ata: sata_mv: Fix probe failures with optional phys

[Andrew Lunn]

> I have just tested it on both Armada 370 and Armada XP and it failed
> at the end of the mv_platform_probe function while trying to do a
> clk_put on the clocks on the second port.

That is odd

drivers/clk/clkdev.c_
void clk_put(struct clk *clk)
{
	__clk_put(clk);
}

and arch/arm/include/asm/clkdev.h

#define __clk_put(clk)  do { } while (0)

How can that cause a crash.

However, PC is at __clk_put+0x1c/0x8c

0x8c is a lot of instructions for what should be NOP.

Could you please take a look at this.

> Indeed the devm_phy_optional_get failed on the 1st port, so the clock
> of the second port was not allocated. What I don't get is why the
> IS_ERR check don't work here.

Remember that NULL is not an error. I'm assuming it is somehow
dereferencing NULL.

	 Andrew