LinuxLists.cc - [PATCH v3] net: mdiobus: Add a function to deassert reset

2023-05-10 08:28:04

Subject: [PATCH v3] net: mdiobus: Add a function to deassert reset

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.

I found that the subsequent drive registers the reset pin into the
structure of the sub-device to prevent conflicts, so release the
reset pin.

Signed-off-by: Yan Wang <[email protected]>
---
v3:
- 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 | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 1183ef5e203e..6695848b8ef2 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -57,6 +57,20 @@ 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;
+
+ reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_HIGH, NULL);
+ if (IS_ERR(reset) && PTR_ERR(reset) != -EPROBE_DEFER)
+ return;
+
+ usleep_range(100, 200);
+ gpiod_set_value_cansleep(reset, 0);
+ /*Release the reset pin,it needs to be registered with the PHY.*/
+ 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 +133,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

2023-05-10 09:55:27

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v3] net: mdiobus: Add a function to deassert reset

Hi Yan,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]
[also build test ERROR on net/main linus/master v6.4-rc1 next-20230510]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Yan-Wang/net-mdiobus-Add-a-function-to-deassert-reset/20230510-161736
base: net-next/main
patch link: https://lore.kernel.org/r/KL1PR01MB5448A33A549CDAD7D68945B9E6779%40KL1PR01MB5448.apcprd01.prod.exchangelabs.com
patch subject: [PATCH v3] net: mdiobus: Add a function to deassert reset
config: hexagon-randconfig-r045-20230509 (https://download.01.org/0day-ci/archive/20230510/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project b0fb98227c90adf2536c9ad644a74d5e92961111)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/f7ded94d887d1020adb4813c2b1025142288e882
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yan-Wang/net-mdiobus-Add-a-function-to-deassert-reset/20230510-161736
git checkout f7ded94d887d1020adb4813c2b1025142288e882
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/net/mdio/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

In file included from drivers/net/mdio/fwnode_mdio.c:10:
In file included from include/linux/fwnode_mdio.h:9:
In file included from include/linux/phy.h:16:
In file included from include/linux/ethtool.h:18:
In file included from include/linux/if_ether.h:19:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/net/mdio/fwnode_mdio.c:10:
In file included from include/linux/fwnode_mdio.h:9:
In file included from include/linux/phy.h:16:
In file included from include/linux/ethtool.h:18:
In file included from include/linux/if_ether.h:19:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/net/mdio/fwnode_mdio.c:10:
In file included from include/linux/fwnode_mdio.h:9:
In file included from include/linux/phy.h:16:
In file included from include/linux/ethtool.h:18:
In file included from include/linux/if_ether.h:19:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> drivers/net/mdio/fwnode_mdio.c:64:10: error: call to undeclared function 'fwnode_gpiod_get_index'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_HIGH, NULL);
^
>> drivers/net/mdio/fwnode_mdio.c:64:53: error: use of undeclared identifier 'GPIOD_OUT_HIGH'
reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_HIGH, NULL);
^
>> drivers/net/mdio/fwnode_mdio.c:69:2: error: call to undeclared function 'gpiod_set_value_cansleep'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
gpiod_set_value_cansleep(reset, 0);
^
>> drivers/net/mdio/fwnode_mdio.c:71:2: error: call to undeclared function 'gpiod_put'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
gpiod_put(reset);
^
6 warnings and 4 errors generated.

vim +/fwnode_gpiod_get_index +64 drivers/net/mdio/fwnode_mdio.c

59
60 static void fwnode_mdiobus_pre_enable_phy(struct fwnode_handle *fwnode)
61 {
62 struct gpio_desc *reset;
63
> 64 reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_HIGH, NULL);
65 if (IS_ERR(reset) && PTR_ERR(reset) != -EPROBE_DEFER)
66 return;
67
68 usleep_range(100, 200);
> 69 gpiod_set_value_cansleep(reset, 0);
70 /*Release the reset pin,it needs to be registered with the PHY.*/
> 71 gpiod_put(reset);
72 }
73

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

2023-05-10 10:14:48

by Simon Horman

[permalink] [raw]

Subject: Re: [PATCH v3] net: mdiobus: Add a function to deassert reset

2023-05-10 10:30:59

by Simon Horman

[permalink] [raw]

Subject: Re: [PATCH v3] net: mdiobus: Add a function to deassert reset

On Wed, May 10, 2023 at 04:15:22PM +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.
>
> I found that the subsequent drive registers the reset pin into the
> structure of the sub-device to prevent conflicts, so release the
> reset pin.
>
> Signed-off-by: Yan Wang <[email protected]>
> ---
> v3:
> - 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 | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index 1183ef5e203e..6695848b8ef2 100644
> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c
> @@ -57,6 +57,20 @@ 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;
> +
> + reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_HIGH, NULL);

Hi Yan,

As this calls fwnode_gpiod_get_index()
do you need to include linux/gpio/consumer.h ?

> + if (IS_ERR(reset) && PTR_ERR(reset) != -EPROBE_DEFER)
> + return;
> +
> + usleep_range(100, 200);
> + gpiod_set_value_cansleep(reset, 0);
> + /*Release the reset pin,it needs to be registered with the PHY.*/
> + gpiod_put(reset);
> +}
> +

...

2023-05-10 10:45:00

by Yan Wang

[permalink] [raw]

Subject: Re: [PATCH v3] net: mdiobus: Add a function to deassert reset

On 5/10/2023 6:22 PM, Simon Horman wrote:
> On Wed, May 10, 2023 at 04:15:22PM +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.
>>
>> I found that the subsequent drive registers the reset pin into the
>> structure of the sub-device to prevent conflicts, so release the
>> reset pin.
>>
>> Signed-off-by: Yan Wang <[email protected]>
>> ---
>> v3:
>> - 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 | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
>> index 1183ef5e203e..6695848b8ef2 100644
>> --- a/drivers/net/mdio/fwnode_mdio.c
>> +++ b/drivers/net/mdio/fwnode_mdio.c
>> @@ -57,6 +57,20 @@ 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;
>> +
>> + reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_HIGH, NULL);
> Hi Yan,
>
> As this calls fwnode_gpiod_get_index()
> do you need to include linux/gpio/consumer.h ?
Ok, I found and fixing
>> + if (IS_ERR(reset) && PTR_ERR(reset) != -EPROBE_DEFER)
>> + return;
>> +
>> + usleep_range(100, 200);
>> + gpiod_set_value_cansleep(reset, 0);
>> + /*Release the reset pin,it needs to be registered with the PHY.*/
>> + gpiod_put(reset);
>> +}
>> +
> ...

2023-05-10 11:44:37

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v3] net: mdiobus: Add a function to deassert reset

2023-05-10 22:50:16

by Andrew Lunn

[permalink] [raw]

Subject: Re: [PATCH v3] net: mdiobus: Add a function to deassert reset

> +static void fwnode_mdiobus_pre_enable_phy(struct fwnode_handle *fwnode)
> +{
> + struct gpio_desc *reset;
> +
> + reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_HIGH, NULL);
> + if (IS_ERR(reset) && PTR_ERR(reset) != -EPROBE_DEFER)
> + return;
> +
> + usleep_range(100, 200);
> + gpiod_set_value_cansleep(reset, 0);
> + /*Release the reset pin,it needs to be registered with the PHY.*/
> + gpiod_put(reset);

You are still putting it into reset, and then taking it out of
reset. This is what i said should not be done. Please only take it out
of reset if it is in reset.

Also, you ignored my comments about delay after reset.

Documentation/devicetree/bindings/net/ethernet-phy.yaml says:

reset-gpios:
maxItems: 1
description:
The GPIO phandle and specifier for the PHY reset signal.

reset-assert-us:
description:
Delay after the reset was asserted in microseconds. If this
property is missing the delay will be skipped.

reset-deassert-us:
description:
Delay after the reset was deasserted in microseconds. If
this property is missing the delay will be skipped.

You are deasserting the reset, so you should look for this property,
and if it is there, delay for that amount. Some PHYs take a while
before they will respond on the bus.

Andrew

2023-05-11 03:23:58

by Yan Wang

[permalink] [raw]

Subject: Re: [PATCH v3] net: mdiobus: Add a function to deassert reset

On 5/11/2023 6:15 AM, Andrew Lunn wrote:
>> +static void fwnode_mdiobus_pre_enable_phy(struct fwnode_handle *fwnode)
>> +{
>> + struct gpio_desc *reset;
>> +
>> + reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_HIGH, NULL);
>> + if (IS_ERR(reset) && PTR_ERR(reset) != -EPROBE_DEFER)
>> + return;
>> +
>> + usleep_range(100, 200);
>> + gpiod_set_value_cansleep(reset, 0);
>> + /*Release the reset pin,it needs to be registered with the PHY.*/
>> + gpiod_put(reset);
> You are still putting it into reset, and then taking it out of
> reset. This is what i said should not be done. Please only take it out
> of reset if it is in reset.
>
> Also, you ignored my comments about delay after reset.
>
> Documentation/devicetree/bindings/net/ethernet-phy.yaml says:
>
> reset-gpios:
> maxItems: 1
> description:
> The GPIO phandle and specifier for the PHY reset signal.
>
> reset-assert-us:
> description:
> Delay after the reset was asserted in microseconds. If this
> property is missing the delay will be skipped.
>
> reset-deassert-us:
> description:
> Delay after the reset was deasserted in microseconds. If
> this property is missing the delay will be skipped.
>
> You are deasserting the reset, so you should look for this property,
> and if it is there, delay for that amount. Some PHYs take a while
> before they will respond on the bus.
I'm very sorry, I forgot. Thank you
> Andrew