Re: [U-Boot] [PATCH v2 2/6] dm: core: Allow multiple drivers to bind for a single node

[Simon Glass]

Hi,

On 20 March 2017 at 01:08, Maxime Ripard
 wrote:
> On Sun, Mar 12, 2017 at 02:21:35PM -0600, Simon Glass wrote:
>> Hi,
>>
>> On 3 March 2017 at 03:52, Dr. Philipp Tomsich
>>  wrote:
>> > Hi Simon,
>> >
>> > On 03 Mar 2017, at 05:52, Simon Glass  wrote:
>> >
>> > Hi Philipp,
>> >
>> > On 22 February 2017 at 13:47, Philipp Tomsich
>> >  wrote:
>> >
>> > Currently, driver binding stops once it encounters the first
>> > compatible driver that doesn't refuse to bind. However, there are
>> > cases where a single node will need to be handled by multiple driver
>> > classes. For those cases we provide a configurable option to continue
>> > to bind after the first driver has been found.
>> >
>> > The first use cases for this are from the DM conversion of the sunxi
>> > (Allwinner) architecture:
>> > * pinctrl (UCLASS_PINCTRL) and gpio (UCLASS_GPIO) drivers need to
>> >   bind against a single node
>> > * clock (UCLASS_CLK) and reset (UCLASS_RESET) drivers also need to
>> >   bind against a single node
>> >
>> >
>> > Does linux work this way? Another approach would be to have a separate
>> > MISC driver with two children, one pinctrl, one clk.
>> >
>> >
>> > The linux CLK driver creates and registers a reset-controller; the PINCTRL
>> > driver
>> > does the same with the gpio-controller. Similar code to do this is easily
>> > possible in
>> > U-Boot … see sunxi_pctrl_bind_gpio(…) in [PATCH v2 1/6] of this series.
>> >
>> > However, binding multiple times makes for much simpler code and allows to
>> > keep
>> > driver data in separate drivers.
>>
>> My question was more whether Linux registers multiple drivers with one
>> device node. It's just not something I expected.
>
> It does, but it's not really what we're doing in the linux driver. It
> has one driver, with one device, but registering into multiple
> frameworks.

I believe that the U-Boot equivalent of this is to have a parent
device with several children each in its own uclass.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/6] dm: core: Allow multiple drivers to bind for a single node

[Maxime Ripard]

On Sun, Mar 12, 2017 at 02:21:35PM -0600, Simon Glass wrote:
> Hi,
> 
> On 3 March 2017 at 03:52, Dr. Philipp Tomsich
>  wrote:
> > Hi Simon,
> >
> > On 03 Mar 2017, at 05:52, Simon Glass  wrote:
> >
> > Hi Philipp,
> >
> > On 22 February 2017 at 13:47, Philipp Tomsich
> >  wrote:
> >
> > Currently, driver binding stops once it encounters the first
> > compatible driver that doesn't refuse to bind. However, there are
> > cases where a single node will need to be handled by multiple driver
> > classes. For those cases we provide a configurable option to continue
> > to bind after the first driver has been found.
> >
> > The first use cases for this are from the DM conversion of the sunxi
> > (Allwinner) architecture:
> > * pinctrl (UCLASS_PINCTRL) and gpio (UCLASS_GPIO) drivers need to
> >   bind against a single node
> > * clock (UCLASS_CLK) and reset (UCLASS_RESET) drivers also need to
> >   bind against a single node
> >
> >
> > Does linux work this way? Another approach would be to have a separate
> > MISC driver with two children, one pinctrl, one clk.
> >
> >
> > The linux CLK driver creates and registers a reset-controller; the PINCTRL
> > driver
> > does the same with the gpio-controller. Similar code to do this is easily
> > possible in
> > U-Boot … see sunxi_pctrl_bind_gpio(…) in [PATCH v2 1/6] of this series.
> >
> > However, binding multiple times makes for much simpler code and allows to
> > keep
> > driver data in separate drivers.
> 
> My question was more whether Linux registers multiple drivers with one
> device node. It's just not something I expected.

It does, but it's not really what we're doing in the linux driver. It
has one driver, with one device, but registering into multiple
frameworks.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/6] dm: core: Allow multiple drivers to bind for a single node

[Heiko Stübner]

Hi Simon,

Am Sonntag, 12. März 2017, 14:21:35 CET schrieb Simon Glass:
> On 3 March 2017 at 03:52, Dr. Philipp Tomsich
>  wrote:
> > On 03 Mar 2017, at 05:52, Simon Glass  wrote:
> > On 22 February 2017 at 13:47, Philipp Tomsich
> >  wrote:
> > 
> > Currently, driver binding stops once it encounters the first
> > compatible driver that doesn't refuse to bind. However, there are
> > cases where a single node will need to be handled by multiple driver
> > classes. For those cases we provide a configurable option to continue
> > to bind after the first driver has been found.
> > 
> > The first use cases for this are from the DM conversion of the sunxi
> > (Allwinner) architecture:
> > * pinctrl (UCLASS_PINCTRL) and gpio (UCLASS_GPIO) drivers need to
> > 
> >   bind against a single node
> > 
> > * clock (UCLASS_CLK) and reset (UCLASS_RESET) drivers also need to
> > 
> >   bind against a single node
> > 
> > Does linux work this way? Another approach would be to have a separate
> > MISC driver with two children, one pinctrl, one clk.
> > 
> > 
> > The linux CLK driver creates and registers a reset-controller; the PINCTRL
> > driver
> > does the same with the gpio-controller. Similar code to do this is easily
> > possible in
> > U-Boot … see sunxi_pctrl_bind_gpio(…) in [PATCH v2 1/6] of this series.
> > 
> > However, binding multiple times makes for much simpler code and allows to
> > keep
> > driver data in separate drivers.
> 
> My question was more whether Linux registers multiple drivers with one
> device node. It's just not something I expected.
> 
> I'm not really convinced on this. It will break the current one-to-one
> relationship, and functions like device_get_child_by_of_offset(). I
> think it would be better to have a MISC driver with two children.

In the regular device model the Linux kernel also generally has a one-to-one 
model. There are special cases, like clock and timer init using special 
handling, or things like "syscon" which acts like more of a flag and can live 
next to a regular device.

Therefore things like the Rockchip clock controller create the reset 
controller included in the CRU block. A behaviour the uboot clk driver mimics.

Also doesn't allowing multiple drivers to sit on top of a node create 
contention on who gets access to registers? At least in the kernel multiple 
mappings of registers are generally not favoured.


Heiko
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/6] dm: core: Allow multiple drivers to bind for a single node

[Simon Glass]

Hi,

On 3 March 2017 at 03:52, Dr. Philipp Tomsich
 wrote:
> Hi Simon,
>
> On 03 Mar 2017, at 05:52, Simon Glass  wrote:
>
> Hi Philipp,
>
> On 22 February 2017 at 13:47, Philipp Tomsich
>  wrote:
>
> Currently, driver binding stops once it encounters the first
> compatible driver that doesn't refuse to bind. However, there are
> cases where a single node will need to be handled by multiple driver
> classes. For those cases we provide a configurable option to continue
> to bind after the first driver has been found.
>
> The first use cases for this are from the DM conversion of the sunxi
> (Allwinner) architecture:
> * pinctrl (UCLASS_PINCTRL) and gpio (UCLASS_GPIO) drivers need to
>   bind against a single node
> * clock (UCLASS_CLK) and reset (UCLASS_RESET) drivers also need to
>   bind against a single node
>
>
> Does linux work this way? Another approach would be to have a separate
> MISC driver with two children, one pinctrl, one clk.
>
>
> The linux CLK driver creates and registers a reset-controller; the PINCTRL
> driver
> does the same with the gpio-controller. Similar code to do this is easily
> possible in
> U-Boot … see sunxi_pctrl_bind_gpio(…) in [PATCH v2 1/6] of this series.
>
> However, binding multiple times makes for much simpler code and allows to
> keep
> driver data in separate drivers.

My question was more whether Linux registers multiple drivers with one
device node. It's just not something I expected.

I'm not really convinced on this. It will break the current one-to-one
relationship, and functions like device_get_child_by_of_offset(). I
think it would be better to have a MISC driver with two children.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/6] dm: core: Allow multiple drivers to bind for a single node

[Dr. Philipp Tomsich]

Hi Simon,

> On 03 Mar 2017, at 05:52, Simon Glass  wrote:
> 
> Hi Philipp,
> 
> On 22 February 2017 at 13:47, Philipp Tomsich
>  > wrote:
>> Currently, driver binding stops once it encounters the first
>> compatible driver that doesn't refuse to bind. However, there are
>> cases where a single node will need to be handled by multiple driver
>> classes. For those cases we provide a configurable option to continue
>> to bind after the first driver has been found.
>> 
>> The first use cases for this are from the DM conversion of the sunxi
>> (Allwinner) architecture:
>> * pinctrl (UCLASS_PINCTRL) and gpio (UCLASS_GPIO) drivers need to
>>   bind against a single node
>> * clock (UCLASS_CLK) and reset (UCLASS_RESET) drivers also need to
>>   bind against a single node
> 
> Does linux work this way? Another approach would be to have a separate
> MISC driver with two children, one pinctrl, one clk.

The linux CLK driver creates and registers a reset-controller; the PINCTRL 
driver
does the same with the gpio-controller. Similar code to do this is easily 
possible in
U-Boot … see sunxi_pctrl_bind_gpio(…) in [PATCH v2 1/6] of this series.

However, binding multiple times makes for much simpler code and allows to keep
driver data in separate drivers.

Regards,
Philipp.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/6] dm: core: Allow multiple drivers to bind for a single node

[Simon Glass]

Hi Philipp,

On 22 February 2017 at 13:47, Philipp Tomsich
 wrote:
> Currently, driver binding stops once it encounters the first
> compatible driver that doesn't refuse to bind. However, there are
> cases where a single node will need to be handled by multiple driver
> classes. For those cases we provide a configurable option to continue
> to bind after the first driver has been found.
>
> The first use cases for this are from the DM conversion of the sunxi
> (Allwinner) architecture:
>  * pinctrl (UCLASS_PINCTRL) and gpio (UCLASS_GPIO) drivers need to
>bind against a single node
>  * clock (UCLASS_CLK) and reset (UCLASS_RESET) drivers also need to
>bind against a single node

Does linux work this way? Another approach would be to have a separate
MISC driver with two children, one pinctrl, one clk.

>
> Signed-off-by: Philipp Tomsich 
> ---
>  drivers/core/Kconfig | 14 ++
>  drivers/core/lists.c | 12 +++-
>  2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
> index 8749561..913101c 100644
> --- a/drivers/core/Kconfig
> +++ b/drivers/core/Kconfig
> @@ -31,6 +31,20 @@ config DM_WARN
>   This will cause dm_warn() to be compiled out - it will do nothing
>   when called.
>
> +config DM_ALLOW_MULTIPLE_DRIVERS
> +bool "Allow multiple drivers to bind for one node"
> +   depends on DM
> +   default n

You should be able to drop this line.

> +   help
> + The driver model in U-Boot originally did not allow multiple
> + drivers to bind for a single device node.
> +
> + If enabled, multiple drivers can now bind for a single node
> + by using the same compatible string for matching: lists_bind_fdt()
> + will assume that binding multiple drivers is desirable, if the
> + caller does not request the pointer to the udevice structure to
> + be returned (i.e. if devp is NULL).

Please update the function documentation in the header file.

> +
>  config DM_DEVICE_REMOVE
> bool "Support device removal"
> depends on DM
> diff --git a/drivers/core/lists.c b/drivers/core/lists.c
> index 23b6ba7..52efe69 100644
> --- a/drivers/core/lists.c
> +++ b/drivers/core/lists.c
> @@ -166,7 +166,11 @@ int lists_bind_fdt(struct udevice *parent, const void 
> *blob, int offset,
> dm_dbg("   - attempt to match compatible string '%s'\n",
>compat);
>
> -   for (entry = driver; entry != driver + n_ents; entry++) {
> +   entry = driver;
> +#if defined(CONFIG_DM_ALLOW_MULTIPLE_DRIVERS)
> +   allow_more_matches:
> +#endif
> +   for (; entry != driver + n_ents; entry++) {
> ret = driver_check_compatible(entry->of_match, ,
>   compat);
> if (!ret)
> @@ -190,6 +194,12 @@ int lists_bind_fdt(struct udevice *parent, const void 
> *blob, int offset,
> found = true;
> if (devp)
> *devp = dev;
> +#if defined(CONFIG_DM_ALLOW_MULTIPLE_DRIVERS)

Can you make this a variable, e.g. with allow_multiple =
IS_ENABLED(DM_ALLOW_MULTIPLE_DRIVERS)? I'd prefer not to add #ifdefs
in this file.

> +   else {
> +   entry++;
> +   goto allow_more_matches;

Is it possible to loop without using goto?

> +   }
> +#endif
> }
> break;
> }
> --
> 1.9.1
>

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v2 2/6] dm: core: Allow multiple drivers to bind for a single node

[Philipp Tomsich]

Currently, driver binding stops once it encounters the first
compatible driver that doesn't refuse to bind. However, there are
cases where a single node will need to be handled by multiple driver
classes. For those cases we provide a configurable option to continue
to bind after the first driver has been found.

The first use cases for this are from the DM conversion of the sunxi
(Allwinner) architecture:
 * pinctrl (UCLASS_PINCTRL) and gpio (UCLASS_GPIO) drivers need to
   bind against a single node
 * clock (UCLASS_CLK) and reset (UCLASS_RESET) drivers also need to
   bind against a single node

Signed-off-by: Philipp Tomsich 
---
 drivers/core/Kconfig | 14 ++
 drivers/core/lists.c | 12 +++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
index 8749561..913101c 100644
--- a/drivers/core/Kconfig
+++ b/drivers/core/Kconfig
@@ -31,6 +31,20 @@ config DM_WARN
  This will cause dm_warn() to be compiled out - it will do nothing
  when called.
 
+config DM_ALLOW_MULTIPLE_DRIVERS
+bool "Allow multiple drivers to bind for one node"
+   depends on DM
+   default n
+   help
+ The driver model in U-Boot originally did not allow multiple
+ drivers to bind for a single device node.
+
+ If enabled, multiple drivers can now bind for a single node
+ by using the same compatible string for matching: lists_bind_fdt()
+ will assume that binding multiple drivers is desirable, if the
+ caller does not request the pointer to the udevice structure to
+ be returned (i.e. if devp is NULL).
+
 config DM_DEVICE_REMOVE
bool "Support device removal"
depends on DM
diff --git a/drivers/core/lists.c b/drivers/core/lists.c
index 23b6ba7..52efe69 100644
--- a/drivers/core/lists.c
+++ b/drivers/core/lists.c
@@ -166,7 +166,11 @@ int lists_bind_fdt(struct udevice *parent, const void 
*blob, int offset,
dm_dbg("   - attempt to match compatible string '%s'\n",
   compat);
 
-   for (entry = driver; entry != driver + n_ents; entry++) {
+   entry = driver;
+#if defined(CONFIG_DM_ALLOW_MULTIPLE_DRIVERS)
+   allow_more_matches:
+#endif
+   for (; entry != driver + n_ents; entry++) {
ret = driver_check_compatible(entry->of_match, ,
  compat);
if (!ret)
@@ -190,6 +194,12 @@ int lists_bind_fdt(struct udevice *parent, const void 
*blob, int offset,
found = true;
if (devp)
*devp = dev;
+#if defined(CONFIG_DM_ALLOW_MULTIPLE_DRIVERS)
+   else {
+   entry++;
+   goto allow_more_matches;
+   }
+#endif
}
break;
}
-- 
1.9.1

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot