[PATCH v4 0/2] dm: core: drivers: add function uclass_probe_all()

[PATCH v4 0/2] dm: core: drivers: add function uclass_probe_all()

[Vabhav Sharma]

From: Vabhav Sharma <vabhav.sharma@nxp.com>

- Add common method to probe devices belonging to same uclass 
- Add config in serial uclass to support optional inclusion of uclass_probe_all
- Enable support for available serial devices probe

Changes for v4:
Incorporated review comments from Simon  
  - Removed if (dev)..  conditional check in function uclass_probe_all()

Changes for v3:
  Incorporated Simon and Stephan review comment
  - Define generic function uclass_probe_all(enum uclass_id)
    in drivers/core/uclass.c
  - Added the function in caller of serial_find_console_or_panic()
  - Removed repeated sequence with generic function call uclass_probe_all()

Changes for v2:
  Incorporated Stefan review comment,Update #ifdef with macro if (IS_ENABLED).

Vabhav Sharma (2):
  dm: core: add function uclass_probe_all() to probe all devices
  drivers: serial: probe all uart devices

 drivers/core/uclass.c          | 16 ++++++++++++++++
 drivers/serial/Kconfig         | 17 +++++++++++++++++
 drivers/serial/serial-uclass.c |  4 ++++
 include/dm/uclass.h            | 12 ++++++++++++
 4 files changed, 49 insertions(+)

-- 
2.7.4

[PATCH v4 1/2] dm: core: add function uclass_probe_all() to probe all devices

[Vabhav Sharma]

From: Vabhav Sharma <vabhav.sharma@nxp.com>

Support a common method to probe all devices associated with uclass.

This includes data structures and code for finding the first device and
looping for remaining devices associated with uclasses (groups of devices
with the same purpose, e.g. all SERIAL ports will be in the same uclass).

An example is SBSA compliant PL011 UART IP, where firmware does the serial
port initialization and prepare uart device to let the kernel use it for
sending and reveiving the characters.SERIAL uclass will use this function
to initialize PL011 UART ports.

The feature is enabled with CONFIG_DM.

Signed-off-by: Vabhav Sharma <vabhav.sharma@nxp.com>
Reviewed-by: Stefan Roese <sr@denx.de>
Reviewed-by: Simon Glass <sjg@chromium.org>
--
  v4:
  Incorporated review comments of Simon
  Removed if (dev)..  conditional check

  v3:
  Incorporated review comments of Stephan,Simon
  Related discussion https://patchwork.ozlabs.org/project/uboot/patch/1601400
385-11854-1-git-send-email-vabhav.sharma at oss.nxp.com/
---
 drivers/core/uclass.c | 16 ++++++++++++++++
 include/dm/uclass.h   | 12 ++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
index c3f1b73..a1dc8bb 100644
--- a/drivers/core/uclass.c
+++ b/drivers/core/uclass.c
@@ -792,6 +792,22 @@ int uclass_pre_remove_device(struct udevice *dev)
 }
 #endif
 
+int uclass_probe_all(enum uclass_id id)
+{
+	struct udevice *dev;
+	int ret;
+
+	ret = uclass_first_device(id, &dev);
+	if (ret || !dev)
+		return ret;
+
+	/* Scanning uclass to probe all devices */
+	for (; dev; uclass_next_device(&dev))
+		;
+
+	return 0;
+}
+
 UCLASS_DRIVER(nop) = {
 	.id		= UCLASS_NOP,
 	.name		= "nop",
diff --git a/include/dm/uclass.h b/include/dm/uclass.h
index 7188304..7ac0aaa 100644
--- a/include/dm/uclass.h
+++ b/include/dm/uclass.h
@@ -381,6 +381,18 @@ int uclass_first_device_drvdata(enum uclass_id id, ulong driver_data,
 int uclass_resolve_seq(struct udevice *dev);
 
 /**
+ * uclass_probe_all() - Probe all devices based on an uclass ID
+ *
+ * Every uclass is identified by an ID, a number from 0 to n-1 where n is
+ * the number of uclasses. This function probe all devices asocciated with
+ * a uclass by looking its ID.
+ *
+ * @id: uclass ID to look up
+ * @return 0 if OK, other -ve on error
+ */
+int uclass_probe_all(enum uclass_id id);
+
+/**
  * uclass_id_foreach_dev() - Helper function to iteration through devices
  *
  * This creates a for() loop which works through the available devices in
-- 
2.7.4

[PATCH v4 2/2] drivers: serial: probe all uart devices

[Vabhav Sharma]

From: Vabhav Sharma <vabhav.sharma@nxp.com>

U-Boot DM model probe only single device at a time
which is enabled and configured using device tree
or platform data method.

PL011 UART IP is SBSA compliant and firmware does the
serial port set-up, initialization and let the kernel use
UART port for sending and receiving characters.

Normally software talk to one serial port time but some
LayerScape platform require all the UART devices enabled
in Linux for various use case.

Adding support to probe all enabled serial devices like SBSA
compliant PL011 UART ports probe and initialization by firmware.

Signed-off-by: Vabhav Sharma <vabhav.sharma@nxp.com>
Reviewed-by: Stefan Roese <sr@denx.de>
Reviewed-by: Simon Glass <sjg@chromium.org>
--
v3:
  Incorporated Simon and Stephan review comment
  - Define generic function uclass_probe_all(enum uclass_id)
    in drivers/core/uclass.c
  - Added the function in caller of serial_find_console_or_panic()
  - Removed repeated sequence with generic function call uclass_probe_all()
  - Dependent on other patch [PATCH] dm: core: add function uclass_probe_all()
    to probe all devices

v2:
  Incorporated Stefan review comment, Update #ifdef with macro
  if (IS_ENABLED)..
---
 drivers/serial/Kconfig         | 17 +++++++++++++++++
 drivers/serial/serial-uclass.c |  4 ++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index b4805a2..af8779b 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -134,6 +134,23 @@ config SERIAL_SEARCH_ALL
 
 	  If unsure, say N.
 
+config SERIAL_PROBE_ALL
+	bool "Probe all available serial devices"
+	depends on DM_SERIAL
+	default n
+	help
+	  The serial subsystem only probe for single serial device,
+	  but does not probe for other remaining serial devices.
+	  With this option set,we make probing and searching for
+	  all available devices optional.
+	  Normally, U-Boot talk to one serial port at a time but SBSA
+	  compliant UART devices like PL011 require initialization
+	  by firmware and let the kernel use serial port for sending
+	  and receiving the characters.
+
+	  If probing is not required for all remaining available
+	  devices other than default current console device, say N.
+
 config SPL_DM_SERIAL
 	bool "Enable Driver Model for serial drivers in SPL"
 	depends on DM_SERIAL && SPL_DM
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index f3c25d4..417d3af 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -172,6 +172,10 @@ int serial_init(void)
 /* Called after relocation */
 int serial_initialize(void)
 {
+	/* Scanning uclass to probe devices */
+	if (IS_ENABLED(CONFIG_SERIAL_PROBE_ALL))
+		uclass_probe_all(UCLASS_SERIAL);
+
 	return serial_init();
 }
 
-- 
2.7.4

[PATCH v4 2/2] drivers: serial: probe all uart devices

[Sean Anderson]

On 11/17/20 10:00 AM, Vabhav Sharma wrote:
> From: Vabhav Sharma <vabhav.sharma@nxp.com>
> 
> U-Boot DM model probe only single device at a time
> which is enabled and configured using device tree
> or platform data method.
> 
> PL011 UART IP is SBSA compliant and firmware does the
> serial port set-up, initialization and let the kernel use
> UART port for sending and receiving characters.
> 
> Normally software talk to one serial port time but some
> LayerScape platform require all the UART devices enabled
> in Linux for various use case.
> 
> Adding support to probe all enabled serial devices like SBSA
> compliant PL011 UART ports probe and initialization by firmware.
> 
> Signed-off-by: Vabhav Sharma <vabhav.sharma@nxp.com>
> Reviewed-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> --
> v3:
>   Incorporated Simon and Stephan review comment
>   - Define generic function uclass_probe_all(enum uclass_id)
>     in drivers/core/uclass.c
>   - Added the function in caller of serial_find_console_or_panic()
>   - Removed repeated sequence with generic function call uclass_probe_all()
>   - Dependent on other patch [PATCH] dm: core: add function uclass_probe_all()
>     to probe all devices
> 
> v2:
>   Incorporated Stefan review comment, Update #ifdef with macro
>   if (IS_ENABLED)..
> ---
>  drivers/serial/Kconfig         | 17 +++++++++++++++++
>  drivers/serial/serial-uclass.c |  4 ++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index b4805a2..af8779b 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -134,6 +134,23 @@ config SERIAL_SEARCH_ALL
>  
>  	  If unsure, say N.
>  
> +config SERIAL_PROBE_ALL
> +	bool "Probe all available serial devices"
> +	depends on DM_SERIAL
> +	default n
> +	help
> +	  The serial subsystem only probe for single serial device,

nit: probes for a

> +	  but does not probe for other remaining serial devices.
> +	  With this option set,we make probing and searching for

nit: set, we

> +	  all available devices optional.
> +	  Normally, U-Boot talk to one serial port at a time but SBSA

nit: talks
nit: time, but

> +	  compliant UART devices like PL011 require initialization
> +	  by firmware and let the kernel use serial port for sending

nit: to let the kernel use the

> +	  and receiving the characters.
> +
> +	  If probing is not required for all remaining available
> +	  devices other than default current console device, say N.

Perhaps use the "If unsure, say N." wording like other Kconfigs.

> +
>  config SPL_DM_SERIAL
>  	bool "Enable Driver Model for serial drivers in SPL"
>  	depends on DM_SERIAL && SPL_DM
> diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> index f3c25d4..417d3af 100644
> --- a/drivers/serial/serial-uclass.c
> +++ b/drivers/serial/serial-uclass.c
> @@ -172,6 +172,10 @@ int serial_init(void)
>  /* Called after relocation */
>  int serial_initialize(void)
>  {
> +	/* Scanning uclass to probe devices */
> +	if (IS_ENABLED(CONFIG_SERIAL_PROBE_ALL))
> +		uclass_probe_all(UCLASS_SERIAL);
> +
>  	return serial_init();
>  }
>  
> 

[PATCH v4 1/2] dm: core: add function uclass_probe_all() to probe all devices

[Sean Anderson]

On 11/17/20 10:00 AM, Vabhav Sharma wrote:
> From: Vabhav Sharma <vabhav.sharma@nxp.com>
> 
> Support a common method to probe all devices associated with uclass.
> 
> This includes data structures and code for finding the first device and
> looping for remaining devices associated with uclasses (groups of devices
> with the same purpose, e.g. all SERIAL ports will be in the same uclass).
> 
> An example is SBSA compliant PL011 UART IP, where firmware does the serial
> port initialization and prepare uart device to let the kernel use it for
> sending and reveiving the characters.SERIAL uclass will use this function
> to initialize PL011 UART ports.
> 
> The feature is enabled with CONFIG_DM.
> 
> Signed-off-by: Vabhav Sharma <vabhav.sharma@nxp.com>
> Reviewed-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> --
>   v4:
>   Incorporated review comments of Simon
>   Removed if (dev)..  conditional check
> 
>   v3:
>   Incorporated review comments of Stephan,Simon
>   Related discussion https://patchwork.ozlabs.org/project/uboot/patch/1601400
> 385-11854-1-git-send-email-vabhav.sharma at oss.nxp.com/
> ---
>  drivers/core/uclass.c | 16 ++++++++++++++++
>  include/dm/uclass.h   | 12 ++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
> index c3f1b73..a1dc8bb 100644
> --- a/drivers/core/uclass.c
> +++ b/drivers/core/uclass.c
> @@ -792,6 +792,22 @@ int uclass_pre_remove_device(struct udevice *dev)
>  }
>  #endif
>  
> +int uclass_probe_all(enum uclass_id id)
> +{
> +	struct udevice *dev;
> +	int ret;
> +
> +	ret = uclass_first_device(id, &dev);
> +	if (ret || !dev)
> +		return ret;
> +
> +	/* Scanning uclass to probe all devices */
> +	for (; dev; uclass_next_device(&dev))

You must check the return value of this function.

Also, I would suggest using a while loop instead of an empty for loop.

> +		;
> +
> +	return 0;
> +}
> +
>  UCLASS_DRIVER(nop) = {
>  	.id		= UCLASS_NOP,
>  	.name		= "nop",
> diff --git a/include/dm/uclass.h b/include/dm/uclass.h
> index 7188304..7ac0aaa 100644
> --- a/include/dm/uclass.h
> +++ b/include/dm/uclass.h
> @@ -381,6 +381,18 @@ int uclass_first_device_drvdata(enum uclass_id id, ulong driver_data,
>  int uclass_resolve_seq(struct udevice *dev);
>  
>  /**
> + * uclass_probe_all() - Probe all devices based on an uclass ID
> + *
> + * Every uclass is identified by an ID, a number from 0 to n-1 where n is
> + * the number of uclasses. This function probe all devices asocciated with

nit: probes associated

> + * a uclass by looking its ID.

nit: for its

AFAICT uclass_find_next_device walks the linked-list of devices in a
uclass, and does not care about the ID. So this documentation is
incorrect.

> + *
> + * @id: uclass ID to look up
> + * @return 0 if OK, other -ve on error
> + */
> +int uclass_probe_all(enum uclass_id id);
> +
> +/**
>   * uclass_id_foreach_dev() - Helper function to iteration through devices
>   *
>   * This creates a for() loop which works through the available devices in
> 

[PATCH v4 2/2] drivers: serial: probe all uart devices

[Vabhav Sharma]

> -----Original Message-----
> From: Sean Anderson <seanga2@gmail.com>
> Sent: Thursday, November 19, 2020 9:01 AM
> To: Vabhav Sharma (OSS) <vabhav.sharma@oss.nxp.com>;
> sjg at chromium.org; sr at denx.de
> Cc: u-boot at lists.denx.de; Varun Sethi <V.Sethi@nxp.com>;
> andre.przywara at arm.com; Vabhav Sharma <vabhav.sharma@nxp.com>
> Subject: Re: [PATCH v4 2/2] drivers: serial: probe all uart devices
> 
> On 11/17/20 10:00 AM, Vabhav Sharma wrote:
> > From: Vabhav Sharma <vabhav.sharma@nxp.com>
> >
> > U-Boot DM model probe only single device at a time which is enabled
> > and configured using device tree or platform data method.
> >
> > PL011 UART IP is SBSA compliant and firmware does the serial port
> > set-up, initialization and let the kernel use UART port for sending
> > and receiving characters.
> >
> > Normally software talk to one serial port time but some LayerScape
> > platform require all the UART devices enabled in Linux for various use
> > case.
> >
> > Adding support to probe all enabled serial devices like SBSA compliant
> > PL011 UART ports probe and initialization by firmware.
> >
> > Signed-off-by: Vabhav Sharma <vabhav.sharma@nxp.com>
> > Reviewed-by: Stefan Roese <sr@denx.de>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > --
> > v3:
> >   Incorporated Simon and Stephan review comment
> >   - Define generic function uclass_probe_all(enum uclass_id)
> >     in drivers/core/uclass.c
> >   - Added the function in caller of serial_find_console_or_panic()
> >   - Removed repeated sequence with generic function call uclass_probe_all()
> >   - Dependent on other patch [PATCH] dm: core: add function
> uclass_probe_all()
> >     to probe all devices
> >
> > v2:
> >   Incorporated Stefan review comment, Update #ifdef with macro
> >   if (IS_ENABLED)..
> > ---
> >  drivers/serial/Kconfig         | 17 +++++++++++++++++
> >  drivers/serial/serial-uclass.c |  4 ++++
> >  2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig index
> > b4805a2..af8779b 100644
> > --- a/drivers/serial/Kconfig
> > +++ b/drivers/serial/Kconfig
> > @@ -134,6 +134,23 @@ config SERIAL_SEARCH_ALL
> >
> >  	  If unsure, say N.
> >
> > +config SERIAL_PROBE_ALL
> > +	bool "Probe all available serial devices"
> > +	depends on DM_SERIAL
> > +	default n
> > +	help
> > +	  The serial subsystem only probe for single serial device,
> 
> nit: probes for a
Ok
> 
> > +	  but does not probe for other remaining serial devices.
> > +	  With this option set,we make probing and searching for
> 
> nit: set, we
Sure
>
> > +	  all available devices optional.
> > +	  Normally, U-Boot talk to one serial port at a time but SBSA
> 
> nit: talks
> nit: time, but
Agree
> 
> > +	  compliant UART devices like PL011 require initialization
> > +	  by firmware and let the kernel use serial port for sending
> 
> nit: to let the kernel use the
I understand
> 
> > +	  and receiving the characters.
> > +
> > +	  If probing is not required for all remaining available
> > +	  devices other than default current console device, say N.
> 
> Perhaps use the "If unsure, say N." wording like other Kconfigs.
Got it
> 
> > +
> >  config SPL_DM_SERIAL
> >  	bool "Enable Driver Model for serial drivers in SPL"
> >  	depends on DM_SERIAL && SPL_DM
> > diff --git a/drivers/serial/serial-uclass.c
> > b/drivers/serial/serial-uclass.c index f3c25d4..417d3af 100644
> > --- a/drivers/serial/serial-uclass.c
> > +++ b/drivers/serial/serial-uclass.c
> > @@ -172,6 +172,10 @@ int serial_init(void)
> >  /* Called after relocation */
> >  int serial_initialize(void)
> >  {
> > +	/* Scanning uclass to probe devices */
> > +	if (IS_ENABLED(CONFIG_SERIAL_PROBE_ALL))
> > +		uclass_probe_all(UCLASS_SERIAL);
> > +
> >  	return serial_init();
> >  }
> >
> >

[PATCH v4 1/2] dm: core: add function uclass_probe_all() to probe all devices

[Vabhav Sharma]

> -----Original Message-----
> From: Sean Anderson <seanga2@gmail.com>
> Sent: Thursday, November 19, 2020 9:05 AM
> To: Vabhav Sharma (OSS) <vabhav.sharma@oss.nxp.com>;
> sjg at chromium.org; sr at denx.de
> Cc: u-boot at lists.denx.de; Varun Sethi <V.Sethi@nxp.com>;
> andre.przywara at arm.com; Vabhav Sharma <vabhav.sharma@nxp.com>
> Subject: Re: [PATCH v4 1/2] dm: core: add function uclass_probe_all() to
> probe all devices
> 
> On 11/17/20 10:00 AM, Vabhav Sharma wrote:
> > From: Vabhav Sharma <vabhav.sharma@nxp.com>
> >
> > Support a common method to probe all devices associated with uclass.
> >
> > This includes data structures and code for finding the first device
> > and looping for remaining devices associated with uclasses (groups of
> > devices with the same purpose, e.g. all SERIAL ports will be in the same
> uclass).
> >
> > An example is SBSA compliant PL011 UART IP, where firmware does the
> > serial port initialization and prepare uart device to let the kernel
> > use it for sending and reveiving the characters.SERIAL uclass will use
> > this function to initialize PL011 UART ports.
> >
> > The feature is enabled with CONFIG_DM.
> >
> > Signed-off-by: Vabhav Sharma <vabhav.sharma@nxp.com>
> > Reviewed-by: Stefan Roese <sr@denx.de>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > --
> >   v4:
> >   Incorporated review comments of Simon
> >   Removed if (dev)..  conditional check
> >
> >   v3:
> >   Incorporated review comments of Stephan,Simon
> >   Related discussion
> > https://patchwork.ozlabs.org/project/uboot/patch/1601400
> > 385-11854-1-git-send-email-vabhav.sharma at oss.nxp.com/
> > ---
> >  drivers/core/uclass.c | 16 ++++++++++++++++
> >  include/dm/uclass.h   | 12 ++++++++++++
> >  2 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index
> > c3f1b73..a1dc8bb 100644
> > --- a/drivers/core/uclass.c
> > +++ b/drivers/core/uclass.c
> > @@ -792,6 +792,22 @@ int uclass_pre_remove_device(struct udevice
> *dev)
> > }  #endif
> >
> > +int uclass_probe_all(enum uclass_id id) {
> > +	struct udevice *dev;
> > +	int ret;
> > +
> > +	ret = uclass_first_device(id, &dev);
> > +	if (ret || !dev)
> > +		return ret;
> > +
> > +	/* Scanning uclass to probe all devices */
> > +	for (; dev; uclass_next_device(&dev))
> 
> You must check the return value of this function.
Error check is done for first device before passing the device to uclass_next_device(),
I think of other implementation is to combine the first device check and iterating through device list of u-class as 
for (ret = uclass_first_device(id, &dev); dev && !ret;  ret = uclass_next_device(&dev))
	;
But iteration is not required if first device is not found and current changes seems to be ok
Please share valuable feedback
> 
> Also, I would suggest using a while loop instead of an empty for loop.
Please elaborate, Found for loop best suitable to use here
> 
> > +		;
> > +
> > +	return 0;
> > +}
> > +
> >  UCLASS_DRIVER(nop) = {
> >  	.id		= UCLASS_NOP,
> >  	.name		= "nop",
> > diff --git a/include/dm/uclass.h b/include/dm/uclass.h index
> > 7188304..7ac0aaa 100644
> > --- a/include/dm/uclass.h
> > +++ b/include/dm/uclass.h
> > @@ -381,6 +381,18 @@ int uclass_first_device_drvdata(enum uclass_id
> > id, ulong driver_data,  int uclass_resolve_seq(struct udevice *dev);
> >
> >  /**
> > + * uclass_probe_all() - Probe all devices based on an uclass ID
> > + *
> > + * Every uclass is identified by an ID, a number from 0 to n-1 where
> > + n is
> > + * the number of uclasses. This function probe all devices asocciated
> > + with
> 
> nit: probes associated
Ok
> 
> > + * a uclass by looking its ID.
> 
> nit: for its
Sure
> 
> AFAICT uclass_find_next_device walks the linked-list of devices in a uclass,
> and does not care about the ID. So this documentation is incorrect.
This documentation is for new function uclass_probe_all() and understand each Uclass is identified by enum ID e.g. UCLASS_SERIAL for serial devices.
According used the statement  "Every uclass is identified by an ID"

Please suggest.
> 
> > + *
> > + * @id: uclass ID to look up
> > + * @return 0 if OK, other -ve on error  */ int uclass_probe_all(enum
> > +uclass_id id);
> > +
> > +/**
> >   * uclass_id_foreach_dev() - Helper function to iteration through devices
> >   *
> >   * This creates a for() loop which works through the available
> > devices in
> >

[PATCH v4 1/2] dm: core: add function uclass_probe_all() to probe all devices

[Sean Anderson]

On 11/23/20 3:06 AM, Vabhav Sharma (OSS) wrote:
> 
> 
>> -----Original Message-----
>> From: Sean Anderson <seanga2@gmail.com>
>> Sent: Thursday, November 19, 2020 9:05 AM
>> To: Vabhav Sharma (OSS) <vabhav.sharma@oss.nxp.com>;
>> sjg at chromium.org; sr at denx.de
>> Cc: u-boot at lists.denx.de; Varun Sethi <V.Sethi@nxp.com>;
>> andre.przywara at arm.com; Vabhav Sharma <vabhav.sharma@nxp.com>
>> Subject: Re: [PATCH v4 1/2] dm: core: add function uclass_probe_all() to
>> probe all devices
>>
>> On 11/17/20 10:00 AM, Vabhav Sharma wrote:
>>> From: Vabhav Sharma <vabhav.sharma@nxp.com>
>>>
>>> Support a common method to probe all devices associated with uclass.
>>>
>>> This includes data structures and code for finding the first device
>>> and looping for remaining devices associated with uclasses (groups of
>>> devices with the same purpose, e.g. all SERIAL ports will be in the same
>> uclass).
>>>
>>> An example is SBSA compliant PL011 UART IP, where firmware does the
>>> serial port initialization and prepare uart device to let the kernel
>>> use it for sending and reveiving the characters.SERIAL uclass will use
>>> this function to initialize PL011 UART ports.
>>>
>>> The feature is enabled with CONFIG_DM.
>>>
>>> Signed-off-by: Vabhav Sharma <vabhav.sharma@nxp.com>
>>> Reviewed-by: Stefan Roese <sr@denx.de>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>> --
>>>    v4:
>>>    Incorporated review comments of Simon
>>>    Removed if (dev)..  conditional check
>>>
>>>    v3:
>>>    Incorporated review comments of Stephan,Simon
>>>    Related discussion
>>> https://patchwork.ozlabs.org/project/uboot/patch/1601400
>>> 385-11854-1-git-send-email-vabhav.sharma at oss.nxp.com/
>>> ---
>>>   drivers/core/uclass.c | 16 ++++++++++++++++
>>>   include/dm/uclass.h   | 12 ++++++++++++
>>>   2 files changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index
>>> c3f1b73..a1dc8bb 100644
>>> --- a/drivers/core/uclass.c
>>> +++ b/drivers/core/uclass.c
>>> @@ -792,6 +792,22 @@ int uclass_pre_remove_device(struct udevice
>> *dev)
>>> }  #endif
>>>
>>> +int uclass_probe_all(enum uclass_id id) {
>>> +	struct udevice *dev;
>>> +	int ret;
>>> +
>>> +	ret = uclass_first_device(id, &dev);
>>> +	if (ret || !dev)
>>> +		return ret;
>>> +
>>> +	/* Scanning uclass to probe all devices */
>>> +	for (; dev; uclass_next_device(&dev))
>>
>> You must check the return value of this function.
> Error check is done for first device before passing the device to uclass_next_device(),
> I think of other implementation is to combine the first device check and iterating through device list of u-class as
> for (ret = uclass_first_device(id, &dev); dev && !ret;  ret = uclass_next_device(&dev))
> 	;
> But iteration is not required if first device is not found and current changes seems to be ok
> Please share valuable feedback


If the second (or any device after the first) fails to init, then the
error will be silently ignored.

>>
>> Also, I would suggest using a while loop instead of an empty for loop.
> Please elaborate, Found for loop best suitable to use here

In terms of original functionality,

while (dev)
	uclass_next_device(&dev)

However, I suggest

while (dev) {
	ret = uclass_next_device(&dev)
	if (ret)
		return ret;
}

So that errors are handled properly.

>>
>>> +		;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   UCLASS_DRIVER(nop) = {
>>>   	.id		= UCLASS_NOP,
>>>   	.name		= "nop",
>>> diff --git a/include/dm/uclass.h b/include/dm/uclass.h index
>>> 7188304..7ac0aaa 100644
>>> --- a/include/dm/uclass.h
>>> +++ b/include/dm/uclass.h
>>> @@ -381,6 +381,18 @@ int uclass_first_device_drvdata(enum uclass_id
>>> id, ulong driver_data,  int uclass_resolve_seq(struct udevice *dev);
>>>
>>>   /**
>>> + * uclass_probe_all() - Probe all devices based on an uclass ID
>>> + *
>>> + * Every uclass is identified by an ID, a number from 0 to n-1 where
>>> + n is
>>> + * the number of uclasses. This function probe all devices asocciated
>>> + with
>>
>> nit: probes associated
> Ok
>>
>>> + * a uclass by looking its ID.
>>
>> nit: for its
> Sure
>>
>> AFAICT uclass_find_next_device walks the linked-list of devices in a uclass,
>> and does not care about the ID. So this documentation is incorrect.
> This documentation is for new function uclass_probe_all() and understand each Uclass is identified by enum ID e.g. UCLASS_SERIAL for serial devices.
> According used the statement  "Every uclass is identified by an ID"
> 
> Please suggest.

Ok, this documentation is confusing because the idea of uclasses being
identified by their UCLASS_XXX id is a very common idea in U-Boot
already. On my first reading, I thought you were instead referring to
udevices being identified by id, when instead you were using
udevice_get_next to get the device. I suggest documenting the method
used to initialize device, and refrain from (re)documenting the method
used to identify the uclass. If someone is confused about that, they
need only refer to the definition of enum uclass_id.

>>
>>> + *
>>> + * @id: uclass ID to look up
>>> + * @return 0 if OK, other -ve on error  */ int uclass_probe_all(enum
>>> +uclass_id id);
>>> +
>>> +/**
>>>    * uclass_id_foreach_dev() - Helper function to iteration through devices
>>>    *
>>>    * This creates a for() loop which works through the available
>>> devices in
>>>
> 

[PATCH v4 1/2] dm: core: add function uclass_probe_all() to probe all devices

[Vabhav Sharma]

> -----Original Message-----
> From: Sean Anderson <seanga2@gmail.com>
> Sent: Monday, November 23, 2020 6:54 PM
> To: Vabhav Sharma (OSS) <vabhav.sharma@oss.nxp.com>;
> sjg at chromium.org; sr at denx.de
> Cc: u-boot at lists.denx.de; Varun Sethi <V.Sethi@nxp.com>;
> andre.przywara at arm.com
> Subject: Re: [PATCH v4 1/2] dm: core: add function uclass_probe_all() to
> probe all devices
> 
> On 11/23/20 3:06 AM, Vabhav Sharma (OSS) wrote:
> >
> >
> >> -----Original Message-----
> >> From: Sean Anderson <seanga2@gmail.com>
> >> Sent: Thursday, November 19, 2020 9:05 AM
> >> To: Vabhav Sharma (OSS) <vabhav.sharma@oss.nxp.com>;
> >> sjg at chromium.org; sr at denx.de
> >> Cc: u-boot at lists.denx.de; Varun Sethi <V.Sethi@nxp.com>;
> >> andre.przywara at arm.com; Vabhav Sharma <vabhav.sharma@nxp.com>
> >> Subject: Re: [PATCH v4 1/2] dm: core: add function uclass_probe_all()
> >> to probe all devices
> >>
> >> On 11/17/20 10:00 AM, Vabhav Sharma wrote:
> >>> From: Vabhav Sharma <vabhav.sharma@nxp.com>
> >>>
> >>> Support a common method to probe all devices associated with uclass.
> >>>
> >>> This includes data structures and code for finding the first device
> >>> and looping for remaining devices associated with uclasses (groups
> >>> of devices with the same purpose, e.g. all SERIAL ports will be in
> >>> the same
> >> uclass).
> >>>
> >>> An example is SBSA compliant PL011 UART IP, where firmware does the
> >>> serial port initialization and prepare uart device to let the kernel
> >>> use it for sending and reveiving the characters.SERIAL uclass will
> >>> use this function to initialize PL011 UART ports.
> >>>
> >>> The feature is enabled with CONFIG_DM.
> >>>
> >>> Signed-off-by: Vabhav Sharma <vabhav.sharma@nxp.com>
> >>> Reviewed-by: Stefan Roese <sr@denx.de>
> >>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>> --
> >>>    v4:
> >>>    Incorporated review comments of Simon
> >>>    Removed if (dev)..  conditional check
> >>>
> >>>    v3:
> >>>    Incorporated review comments of Stephan,Simon
> >>>    Related discussion
> >>> https://patchwork.ozlabs.org/project/uboot/patch/1601400
> >>> 385-11854-1-git-send-email-vabhav.sharma at oss.nxp.com/
> >>> ---
> >>>   drivers/core/uclass.c | 16 ++++++++++++++++
> >>>   include/dm/uclass.h   | 12 ++++++++++++
> >>>   2 files changed, 28 insertions(+)
> >>>
> >>> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index
> >>> c3f1b73..a1dc8bb 100644
> >>> --- a/drivers/core/uclass.c
> >>> +++ b/drivers/core/uclass.c
> >>> @@ -792,6 +792,22 @@ int uclass_pre_remove_device(struct udevice
> >> *dev)
> >>> }  #endif
> >>>
> >>> +int uclass_probe_all(enum uclass_id id) {
> >>> +	struct udevice *dev;
> >>> +	int ret;
> >>> +
> >>> +	ret = uclass_first_device(id, &dev);
> >>> +	if (ret || !dev)
> >>> +		return ret;
> >>> +
> >>> +	/* Scanning uclass to probe all devices */
> >>> +	for (; dev; uclass_next_device(&dev))
> >>
> >> You must check the return value of this function.
> > Error check is done for first device before passing the device to
> > uclass_next_device(), I think of other implementation is to combine
> > the first device check and iterating through device list of u-class as for (ret =
> uclass_first_device(id, &dev); dev && !ret;  ret = uclass_next_device(&dev))
> > 	;
> > But iteration is not required if first device is not found and current
> > changes seems to be ok Please share valuable feedback
> 
> 
> If the second (or any device after the first) fails to init, then the error will be
> silently ignored.
I see
> 
> >>
> >> Also, I would suggest using a while loop instead of an empty for loop.
> > Please elaborate, Found for loop best suitable to use here
> 
> In terms of original functionality,
> 
> while (dev)
> 	uclass_next_device(&dev)
> 
> However, I suggest
> 
> while (dev) {
> 	ret = uclass_next_device(&dev)
> 	if (ret)
> 		return ret;
> }
> 
> So that errors are handled properly.
> 
Sure, I will verify the changes to push next version 
> >>
> >>> +		;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>>   UCLASS_DRIVER(nop) = {
> >>>   	.id		= UCLASS_NOP,
> >>>   	.name		= "nop",
> >>> diff --git a/include/dm/uclass.h b/include/dm/uclass.h index
> >>> 7188304..7ac0aaa 100644
> >>> --- a/include/dm/uclass.h
> >>> +++ b/include/dm/uclass.h
> >>> @@ -381,6 +381,18 @@ int uclass_first_device_drvdata(enum uclass_id
> >>> id, ulong driver_data,  int uclass_resolve_seq(struct udevice *dev);
> >>>
> >>>   /**
> >>> + * uclass_probe_all() - Probe all devices based on an uclass ID
> >>> + *
> >>> + * Every uclass is identified by an ID, a number from 0 to n-1
> >>> + where n is
> >>> + * the number of uclasses. This function probe all devices
> >>> + asocciated with
> >>
> >> nit: probes associated
> > Ok
> >>
> >>> + * a uclass by looking its ID.
> >>
> >> nit: for its
> > Sure
> >>
> >> AFAICT uclass_find_next_device walks the linked-list of devices in a
> >> uclass, and does not care about the ID. So this documentation is incorrect.
> > This documentation is for new function uclass_probe_all() and understand
> each Uclass is identified by enum ID e.g. UCLASS_SERIAL for serial devices.
> > According used the statement  "Every uclass is identified by an ID"
> >
> > Please suggest.
> 
> Ok, this documentation is confusing because the idea of uclasses being
> identified by their UCLASS_XXX id is a very common idea in U-Boot already.
> On my first reading, I thought you were instead referring to udevices being
> identified by id, when instead you were using udevice_get_next to get the
> device. I suggest documenting the method used to initialize device, and
> refrain from (re)documenting the method used to identify the uclass. If
> someone is confused about that, they need only refer to the definition of
> enum uclass_id.
Thank you, Will rephrase it
> 
> >>
> >>> + *
> >>> + * @id: uclass ID to look up
> >>> + * @return 0 if OK, other -ve on error  */ int
> >>> +uclass_probe_all(enum uclass_id id);
> >>> +
> >>> +/**
> >>>    * uclass_id_foreach_dev() - Helper function to iteration through
> devices
> >>>    *
> >>>    * This creates a for() loop which works through the available
> >>> devices in
> >>>
> >

[PATCH v4 2/2] drivers: serial: probe all uart devices

[Sean Anderson]

On 11/23/20 2:14 AM, Vabhav Sharma (OSS) wrote:
> 
> 
>> -----Original Message-----
>> From: Sean Anderson <seanga2@gmail.com>
>> Sent: Thursday, November 19, 2020 9:01 AM
>> To: Vabhav Sharma (OSS) <vabhav.sharma@oss.nxp.com>;
>> sjg at chromium.org; sr at denx.de
>> Cc: u-boot at lists.denx.de; Varun Sethi <V.Sethi@nxp.com>;
>> andre.przywara at arm.com; Vabhav Sharma <vabhav.sharma@nxp.com>
>> Subject: Re: [PATCH v4 2/2] drivers: serial: probe all uart devices
>>
>> On 11/17/20 10:00 AM, Vabhav Sharma wrote:
>>> From: Vabhav Sharma <vabhav.sharma@nxp.com>
>>>
>>> U-Boot DM model probe only single device at a time which is enabled
>>> and configured using device tree or platform data method.
>>>
>>> PL011 UART IP is SBSA compliant and firmware does the serial port
>>> set-up, initialization and let the kernel use UART port for sending
>>> and receiving characters.
>>>
>>> Normally software talk to one serial port time but some LayerScape
>>> platform require all the UART devices enabled in Linux for various use
>>> case.
>>>
>>> Adding support to probe all enabled serial devices like SBSA compliant
>>> PL011 UART ports probe and initialization by firmware.
>>>
>>> Signed-off-by: Vabhav Sharma <vabhav.sharma@nxp.com>
>>> Reviewed-by: Stefan Roese <sr@denx.de>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>> --
>>> v3:
>>>    Incorporated Simon and Stephan review comment
>>>    - Define generic function uclass_probe_all(enum uclass_id)
>>>      in drivers/core/uclass.c
>>>    - Added the function in caller of serial_find_console_or_panic()
>>>    - Removed repeated sequence with generic function call uclass_probe_all()
>>>    - Dependent on other patch [PATCH] dm: core: add function
>> uclass_probe_all()
>>>      to probe all devices
>>>
>>> v2:
>>>    Incorporated Stefan review comment, Update #ifdef with macro
>>>    if (IS_ENABLED)..
>>> ---
>>>   drivers/serial/Kconfig         | 17 +++++++++++++++++
>>>   drivers/serial/serial-uclass.c |  4 ++++
>>>   2 files changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig index
>>> b4805a2..af8779b 100644
>>> --- a/drivers/serial/Kconfig
>>> +++ b/drivers/serial/Kconfig
>>> @@ -134,6 +134,23 @@ config SERIAL_SEARCH_ALL
>>>
>>>   	  If unsure, say N.
>>>
>>> +config SERIAL_PROBE_ALL
>>> +	bool "Probe all available serial devices"
>>> +	depends on DM_SERIAL
>>> +	default n
>>> +	help
>>> +	  The serial subsystem only probe for single serial device,
>>
>> nit: probes for a
> Ok
>>
>>> +	  but does not probe for other remaining serial devices.
>>> +	  With this option set,we make probing and searching for
>>
>> nit: set, we
> Sure
>>
>>> +	  all available devices optional.
>>> +	  Normally, U-Boot talk to one serial port at a time but SBSA
>>
>> nit: talks
>> nit: time, but
> Agree
>>
>>> +	  compliant UART devices like PL011 require initialization
>>> +	  by firmware and let the kernel use serial port for sending
>>
>> nit: to let the kernel use the
> I understand
>>
>>> +	  and receiving the characters.
>>> +
>>> +	  If probing is not required for all remaining available
>>> +	  devices other than default current console device, say N.
>>
>> Perhaps use the "If unsure, say N." wording like other Kconfigs.
> Got it
>>
>>> +
>>>   config SPL_DM_SERIAL
>>>   	bool "Enable Driver Model for serial drivers in SPL"
>>>   	depends on DM_SERIAL && SPL_DM
>>> diff --git a/drivers/serial/serial-uclass.c
>>> b/drivers/serial/serial-uclass.c index f3c25d4..417d3af 100644
>>> --- a/drivers/serial/serial-uclass.c
>>> +++ b/drivers/serial/serial-uclass.c
>>> @@ -172,6 +172,10 @@ int serial_init(void)
>>>   /* Called after relocation */
>>>   int serial_initialize(void)
>>>   {
>>> +	/* Scanning uclass to probe devices */
>>> +	if (IS_ENABLED(CONFIG_SERIAL_PROBE_ALL))
>>> +		uclass_probe_all(UCLASS_SERIAL);
>>> +

Note that you will also need to check the return value here. E.g.

if (IS_ENABLED(CONFIG_SERIAL_PROBE_ALL))
	int ret = uclass_probe_all(UCLASS_SERIAL);
	if (ret)
		return ret;

>>>   	return serial_init();
>>>   }
>>>
>>>
>