It is possible to mount multiple sub-devices on the mido bus.
The hardware power-on does not necessarily reset these devices.
The device may be in an uncertain state, causing the device's ID
to not be scanned.
So,before adding a reset to the scan, make sure the device is in
normal working mode.
Reported-by: kernel test robot <[email protected]>
Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Signed-off-by: Yan Wang <[email protected]>
---
v4:
- Get pulse width and settling time from the device tree
- Add logic for processing (PTR_ERR(reset) == -EPROBE_DEFER)
- included <linux/goio/consumer.h>
- fixed commit message
v3: https://lore.kernel.org/all/KL1PR01MB5448A33A549CDAD7D68945B9E6779@KL1PR01MB5448.apcprd01.prod.exchangelabs.com/
- fixed commit message
v2: https://lore.kernel.org/all/KL1PR01MB54482416A8BE0D80EA27223CE6779@KL1PR01MB5448.apcprd01.prod.exchangelabs.com/
- fixed commit message
- Using gpiod_ replace gpio_
v1: https://lore.kernel.org/all/KL1PR01MB5448631F2D6F71021602117FE6769@KL1PR01MB5448.apcprd01.prod.exchangelabs.com/
- Incorrect description of commit message.
- The gpio-api too old
---
drivers/net/mdio/fwnode_mdio.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 1183ef5e203e..9d7df6393059 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -11,6 +11,7 @@
#include <linux/of.h>
#include <linux/phy.h>
#include <linux/pse-pd/pse.h>
+#include <linux/gpio/consumer.h>
MODULE_AUTHOR("Calvin Johnson <[email protected]>");
MODULE_LICENSE("GPL");
@@ -57,6 +58,35 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
return register_mii_timestamper(arg.np, arg.args[0]);
}
+static void fwnode_mdiobus_pre_enable_phy(struct fwnode_handle *fwnode)
+{
+ struct gpio_desc *reset;
+ unsigned int reset_assert_delay;
+ unsigned int reset_deassert_delay;
+
+ reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_LOW, NULL);
+ if (IS_ERR(reset)) {
+ if (PTR_ERR(reset) == -EPROBE_DEFER)
+ pr_debug("%pOFn: %s: GPIOs not yet available, retry later\n",
+ to_of_node(fwnode), __func__);
+ else
+ pr_err("%pOFn: %s: Can't get reset line property\n",
+ to_of_node(fwnode), __func__);
+
+ return;
+ }
+ fwnode_property_read_u32(fwnode, "reset-assert-us",
+ &reset_assert_delay);
+ fwnode_property_read_u32(fwnode, "reset-deassert-us",
+ &reset_deassert_delay);
+ gpiod_set_value_cansleep(reset, 1);
+ fsleep(reset_assert_delay);
+ gpiod_set_value_cansleep(reset, 0);
+ fsleep(reset_deassert_delay);
+ /*Release phy's reset line, mdiobus_register_gpiod() need to request it*/
+ gpiod_put(reset);
+}
+
int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
struct phy_device *phy,
struct fwnode_handle *child, u32 addr)
@@ -119,6 +149,8 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
u32 phy_id;
int rc;
+ fwnode_mdiobus_pre_enable_phy(child);
+
psec = fwnode_find_pse_control(child);
if (IS_ERR(psec))
return PTR_ERR(psec);
--
2.17.1
On Thu, May 11, 2023 at 02:59:09PM +0800, Yan Wang wrote:
> It is possible to mount multiple sub-devices on the mido bus.
> The hardware power-on does not necessarily reset these devices.
> The device may be in an uncertain state, causing the device's ID
> to not be scanned.
>
> So,before adding a reset to the scan, make sure the device is in
> normal working mode.
>
> Reported-by: kernel test robot <[email protected]>
> Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> Signed-off-by: Yan Wang <[email protected]>
...
> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index 1183ef5e203e..9d7df6393059 100644
> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c
> @@ -11,6 +11,7 @@
> #include <linux/of.h>
> #include <linux/phy.h>
> #include <linux/pse-pd/pse.h>
> +#include <linux/gpio/consumer.h>
>
> MODULE_AUTHOR("Calvin Johnson <[email protected]>");
> MODULE_LICENSE("GPL");
> @@ -57,6 +58,35 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
> return register_mii_timestamper(arg.np, arg.args[0]);
> }
>
> +static void fwnode_mdiobus_pre_enable_phy(struct fwnode_handle *fwnode)
> +{
> + struct gpio_desc *reset;
> + unsigned int reset_assert_delay;
> + unsigned int reset_deassert_delay;
nit: Please arrange local variables for networking code in reverse xmas
tree order - longest line to shortest.
> +
> + reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_LOW, NULL);
> + if (IS_ERR(reset)) {
> + if (PTR_ERR(reset) == -EPROBE_DEFER)
> + pr_debug("%pOFn: %s: GPIOs not yet available, retry later\n",
> + to_of_node(fwnode), __func__);
> + else
> + pr_err("%pOFn: %s: Can't get reset line property\n",
> + to_of_node(fwnode), __func__);
> +
> + return;
> + }
> + fwnode_property_read_u32(fwnode, "reset-assert-us",
> + &reset_assert_delay);
> + fwnode_property_read_u32(fwnode, "reset-deassert-us",
> + &reset_deassert_delay);
Does the return value of fwnode_property_read_u32() need to be
checked for errors?
> + gpiod_set_value_cansleep(reset, 1);
> + fsleep(reset_assert_delay);
> + gpiod_set_value_cansleep(reset, 0);
> + fsleep(reset_deassert_delay);
> + /*Release phy's reset line, mdiobus_register_gpiod() need to request it*/
nit:
/* Release phy's reset line, mdiobus_register_gpiod() needs to
* request it.
*/
> + gpiod_put(reset);
> +}
> +
> int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
> struct phy_device *phy,
> struct fwnode_handle *child, u32 addr)
> @@ -119,6 +149,8 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> u32 phy_id;
> int rc;
>
> + fwnode_mdiobus_pre_enable_phy(child);
> +
> psec = fwnode_find_pse_control(child);
> if (IS_ERR(psec))
> return PTR_ERR(psec);
> --
> 2.17.1
>
>
On 5/11/2023 8:40 PM, Simon Horman wrote:
> On Thu, May 11, 2023 at 02:59:09PM +0800, Yan Wang wrote:
>> It is possible to mount multiple sub-devices on the mido bus.
>> The hardware power-on does not necessarily reset these devices.
>> The device may be in an uncertain state, causing the device's ID
>> to not be scanned.
>>
>> So,before adding a reset to the scan, make sure the device is in
>> normal working mode.
>>
>> Reported-by: kernel test robot <[email protected]>
>> Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>> Signed-off-by: Yan Wang <[email protected]>
> ...
>
>> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
>> index 1183ef5e203e..9d7df6393059 100644
>> --- a/drivers/net/mdio/fwnode_mdio.c
>> +++ b/drivers/net/mdio/fwnode_mdio.c
>> @@ -11,6 +11,7 @@
>> #include <linux/of.h>
>> #include <linux/phy.h>
>> #include <linux/pse-pd/pse.h>
>> +#include <linux/gpio/consumer.h>
>>
>> MODULE_AUTHOR("Calvin Johnson <[email protected]>");
>> MODULE_LICENSE("GPL");
>> @@ -57,6 +58,35 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
>> return register_mii_timestamper(arg.np, arg.args[0]);
>> }
>>
>> +static void fwnode_mdiobus_pre_enable_phy(struct fwnode_handle *fwnode)
>> +{
>> + struct gpio_desc *reset;
>> + unsigned int reset_assert_delay;
>> + unsigned int reset_deassert_delay;
> nit: Please arrange local variables for networking code in reverse xmas
> tree order - longest line to shortest.
>
>> +
>> + reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_LOW, NULL);
>> + if (IS_ERR(reset)) {
>> + if (PTR_ERR(reset) == -EPROBE_DEFER)
>> + pr_debug("%pOFn: %s: GPIOs not yet available, retry later\n",
>> + to_of_node(fwnode), __func__);
>> + else
>> + pr_err("%pOFn: %s: Can't get reset line property\n",
>> + to_of_node(fwnode), __func__);
>> +
>> + return;
>> + }
>> + fwnode_property_read_u32(fwnode, "reset-assert-us",
>> + &reset_assert_delay);
>> + fwnode_property_read_u32(fwnode, "reset-deassert-us",
>> + &reset_deassert_delay);
> Does the return value of fwnode_property_read_u32() need to be
> checked for errors?
>
>> + gpiod_set_value_cansleep(reset, 1);
>> + fsleep(reset_assert_delay);
>> + gpiod_set_value_cansleep(reset, 0);
>> + fsleep(reset_deassert_delay);
>> + /*Release phy's reset line, mdiobus_register_gpiod() need to request it*/
> nit:
>
> /* Release phy's reset line, mdiobus_register_gpiod() needs to
> * request it.
> */
Thank you for your suggestion.
Best Regards
>> + gpiod_put(reset);
>> +}
>> +
>> int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
>> struct phy_device *phy,
>> struct fwnode_handle *child, u32 addr)
>> @@ -119,6 +149,8 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
>> u32 phy_id;
>> int rc;
>>
>> + fwnode_mdiobus_pre_enable_phy(child);
>> +
>> psec = fwnode_find_pse_control(child);
>> if (IS_ERR(psec))
>> return PTR_ERR(psec);
>> --
>> 2.17.1
>>
>>
Am Donnerstag, 11. Mai 2023, 08:59:09 CEST schrieb Yan Wang:
> It is possible to mount multiple sub-devices on the mido bus.
> The hardware power-on does not necessarily reset these devices.
> The device may be in an uncertain state, causing the device's ID
> to not be scanned.
>
> So,before adding a reset to the scan, make sure the device is in
'So, before'. One space after ,
> normal working mode.
>
> Reported-by: kernel test robot <[email protected]>
> Link:
> https://lore.kernel.org/oe-kbuild-all/[email protected]/
> Signed-off-by: Yan Wang <[email protected]>
> ---
> v4:
> - Get pulse width and settling time from the device tree
> - Add logic for processing (PTR_ERR(reset) == -EPROBE_DEFER)
> - included <linux/goio/consumer.h>
> - fixed commit message
> v3:
> https://lore.kernel.org/all/KL1PR01MB5448A33A549CDAD7D68945B9E6779@KL1PR01M
> B5448.apcprd01.prod.exchangelabs.com/ - fixed commit message
> v2:
> https://lore.kernel.org/all/KL1PR01MB54482416A8BE0D80EA27223CE6779@KL1PR01M
> B5448.apcprd01.prod.exchangelabs.com/ - fixed commit message
> - Using gpiod_ replace gpio_
> v1:
> https://lore.kernel.org/all/KL1PR01MB5448631F2D6F71021602117FE6769@KL1PR01M
> B5448.apcprd01.prod.exchangelabs.com/ - Incorrect description of commit
> message.
> - The gpio-api too old
> ---
> drivers/net/mdio/fwnode_mdio.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index 1183ef5e203e..9d7df6393059 100644
> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c
> @@ -11,6 +11,7 @@
> #include <linux/of.h>
> #include <linux/phy.h>
> #include <linux/pse-pd/pse.h>
> +#include <linux/gpio/consumer.h>
Please sort alphabetically.
>
> MODULE_AUTHOR("Calvin Johnson <[email protected]>");
> MODULE_LICENSE("GPL");
> @@ -57,6 +58,35 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
> return register_mii_timestamper(arg.np, arg.args[0]);
> }
>
> +static void fwnode_mdiobus_pre_enable_phy(struct fwnode_handle *fwnode)
> +{
> + struct gpio_desc *reset;
> + unsigned int reset_assert_delay;
> + unsigned int reset_deassert_delay;
> +
> + reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_LOW,
NULL);
> + if (IS_ERR(reset)) {
> + if (PTR_ERR(reset) == -EPROBE_DEFER)
> + pr_debug("%pOFn: %s: GPIOs not yet available,
retry later\n",
> + to_of_node(fwnode), __func__);
You are not dealing with this error at all. pr_debug is (mostly) not even
printed in kernel log, despite the code registering the PHY ignoring the
deferral completely.
> + else
> + pr_err("%pOFn: %s: Can't get reset line
property\n",
> + to_of_node(fwnode), __func__);
> +
This error also gets ignored.
> + return;
> + }
> + fwnode_property_read_u32(fwnode, "reset-assert-us",
> + &reset_assert_delay);
> + fwnode_property_read_u32(fwnode, "reset-deassert-us",
> + &reset_deassert_delay);
These are currently read in fwnode_mdiobus_phy_device_register(). There is no
need to read them twice. It should be moved to a location where it just need
to be read once.
Regards,
Alexander
> + gpiod_set_value_cansleep(reset, 1);
> + fsleep(reset_assert_delay);
> + gpiod_set_value_cansleep(reset, 0);
> + fsleep(reset_deassert_delay);
> + /*Release phy's reset line, mdiobus_register_gpiod() need to request
it*/
> + gpiod_put(reset);
> +}
> +
> int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
> struct phy_device *phy,
> struct fwnode_handle *child,
u32 addr)
> @@ -119,6 +149,8 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> u32 phy_id;
> int rc;
>
> + fwnode_mdiobus_pre_enable_phy(child);
> +
> psec = fwnode_find_pse_control(child);
> if (IS_ERR(psec))
> return PTR_ERR(psec);
--
TQ-Systems GmbH | M?hlstra?e 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht M?nchen, HRB 105018
Gesch?ftsf?hrer: Detlef Schneider, R?diger Stahl, Stefan Schneider
http://www.tq-group.com/