Re: [PATCH v5] reset: add support for non-DT systems
On Tue, 2018-02-27 at 19:07 +0100, Bartosz Golaszewski wrote:
> 2018-02-27 17:10 GMT+01:00 Philipp Zabel :
> > Hi Bartosz,
> >
> > thank you for the update.
> >
> > On Fri, 2018-02-23 at 12:39 +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski
> > >
> > > The reset framework only supports device-tree. There are some platforms
> > > however, which need to use it even in legacy, board-file based mode.
> > >
> > > An example of such architecture is the DaVinci family of SoCs which
> > > supports both device tree and legacy boot modes and we don't want to
> > > introduce any regressions.
> > >
> > > We're currently working on converting the platform from its hand-crafted
> > > clock API to using the common clock framework. Part of the overhaul will
> > > be representing the chip's power sleep controller's reset lines using
> > > the reset framework.
> > >
> > > This changeset extends the core reset code with a new reset lookup
> > > entry structure. It contains data allowing the reset core to associate
> > > reset lines with devices by comparing the dev_id and con_id strings.
> > >
> > > It also provides a function allowing drivers to register lookup entries
> > > with the framework.
> > >
> > > The new lookup function is only called as a fallback in case the
> > > of_node field is NULL and doesn't change anything for current users.
> > >
> > > Tested with a dummy reset driver with several lookup entries.
> > >
> > > An example lookup table registration from a driver can be found below:
> > >
> > > static struct reset_control_lookup foobar_reset_lookup[] = {
> > > RESET_LOOKUP("foo.0", "foo", 15),
> > > RESET_LOOKUP("bar.0", NULL, 5),
> > > };
> > >
> > > foobar_probe()
> > > {
> > > ...
> > >
> > > reset_controller_add_lookup(&rcdev, foobar_reset_lookup,
> > > ARRAY_SIZE(foobar_reset_lookup));
> > >
> > > ...
> > > }
> > >
> > > Cc: Sekhar Nori
> > > Cc: Kevin Hilman
> > > Cc: David Lechner
> > > Signed-off-by: Bartosz Golaszewski
> > > ---
> > > v1 -> v2:
> > > - renamed the new function to __reset_control_get_from_lookup()
> > > - added a missing break; when a matching entry is found
> > > - rearranged the code in __reset_control_get() - we can no longer get to
> > > the
> > > return at the bottom, so remove it and return from
> > > __reset_control_get_from_lookup() if __of_reset_control_get() fails
> > > - return -ENOENT from reset_contol_get() if we can't find a matching
> > > entry,
> > > prevously returned -EINVAL referred to the fact that we passed a device
> > > without the of_node which is no longer an error condition
> > > - add a comment about needing a sentinel in the lookup table
> > >
> > > v2 -> v3:
> > > - added the reset id number field to the lookup struct so that we don't
> > > need
> > > to rely on the array index
> > >
> > > v3 -> v4:
> > > - separated the driver and lookup table registration logic by adding a
> > > function meant to be called by machine-specific code that adds a lookup
> > > table to the internal list
> > > - the correct reset controller is now found by first finding the lookup
> > > table associated with it, then finding the actual reset controller by
> > > the associated device
> > >
> > > v4 -> v5:
> > > - since the first user of this will be the davinci clk driver and it
> > > already registers clock lookup from within the driver code - allow
> > > drivers to register lookups with the assumption that the code can be
> > > extended to make it possible to register entries from machine code as
> > > well
> >
> > How do you imagine this may be extended? By adding an rddev_devid field
> > to the lookup, similarly to the pwm_lookup?
> > I suppose reset_controller_add_lookup could then be called with a NULL
> > rcdev to register lookups by id.
> >
>
> Yes, this is what I was thinking about more or less.
Ok. I just want to avoid having to change all users when somebody needs
that functionality, even though hopefully there won't be that many.
> > > - simplify the code - only expose a single lookup structure and a simply
> > > registration function
> > > - add the RESET_LOOKUP macro for brevity
> > >
> > > drivers/reset/core.c | 65
> > > +++-
> > > include/linux/reset-controller.h | 28 +
> > > 2 files changed, 92 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > > index da4292e9de97..75e54a05147a 100644
> > > --- a/drivers/reset/core.c
> > > +++ b/drivers/reset/core.c
> > > @@ -23,6 +23,9 @@
> > > static DEFINE_MUTEX(reset_list_mutex);
> > > static LIST_HEAD(reset_controller_list);
> > >
> > > +static DEFINE_MUTEX(reset_lookup_mutex);
> > > +static LIST_HEAD(reset_lookup_list);
> > > +
> > > /**
> > > * struct reset_control - a reset control
> > > * @rcdev: a pointer to the reset controller device
> > > @@ -148,6 +151,3
Re: [PATCH v5] reset: add support for non-DT systems
2018-02-27 17:10 GMT+01:00 Philipp Zabel :
> Hi Bartosz,
>
> thank you for the update.
>
> On Fri, 2018-02-23 at 12:39 +0100, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski
>>
>> The reset framework only supports device-tree. There are some platforms
>> however, which need to use it even in legacy, board-file based mode.
>>
>> An example of such architecture is the DaVinci family of SoCs which
>> supports both device tree and legacy boot modes and we don't want to
>> introduce any regressions.
>>
>> We're currently working on converting the platform from its hand-crafted
>> clock API to using the common clock framework. Part of the overhaul will
>> be representing the chip's power sleep controller's reset lines using
>> the reset framework.
>>
>> This changeset extends the core reset code with a new reset lookup
>> entry structure. It contains data allowing the reset core to associate
>> reset lines with devices by comparing the dev_id and con_id strings.
>>
>> It also provides a function allowing drivers to register lookup entries
>> with the framework.
>>
>> The new lookup function is only called as a fallback in case the
>> of_node field is NULL and doesn't change anything for current users.
>>
>> Tested with a dummy reset driver with several lookup entries.
>>
>> An example lookup table registration from a driver can be found below:
>>
>> static struct reset_control_lookup foobar_reset_lookup[] = {
>> RESET_LOOKUP("foo.0", "foo", 15),
>> RESET_LOOKUP("bar.0", NULL, 5),
>> };
>>
>> foobar_probe()
>> {
>> ...
>>
>> reset_controller_add_lookup(&rcdev, foobar_reset_lookup,
>> ARRAY_SIZE(foobar_reset_lookup));
>>
>> ...
>> }
>>
>> Cc: Sekhar Nori
>> Cc: Kevin Hilman
>> Cc: David Lechner
>> Signed-off-by: Bartosz Golaszewski
>> ---
>> v1 -> v2:
>> - renamed the new function to __reset_control_get_from_lookup()
>> - added a missing break; when a matching entry is found
>> - rearranged the code in __reset_control_get() - we can no longer get to the
>> return at the bottom, so remove it and return from
>> __reset_control_get_from_lookup() if __of_reset_control_get() fails
>> - return -ENOENT from reset_contol_get() if we can't find a matching entry,
>> prevously returned -EINVAL referred to the fact that we passed a device
>> without the of_node which is no longer an error condition
>> - add a comment about needing a sentinel in the lookup table
>>
>> v2 -> v3:
>> - added the reset id number field to the lookup struct so that we don't need
>> to rely on the array index
>>
>> v3 -> v4:
>> - separated the driver and lookup table registration logic by adding a
>> function meant to be called by machine-specific code that adds a lookup
>> table to the internal list
>> - the correct reset controller is now found by first finding the lookup
>> table associated with it, then finding the actual reset controller by
>> the associated device
>>
>> v4 -> v5:
>> - since the first user of this will be the davinci clk driver and it
>> already registers clock lookup from within the driver code - allow
>> drivers to register lookups with the assumption that the code can be
>> extended to make it possible to register entries from machine code as
>> well
>
> How do you imagine this may be extended? By adding an rddev_devid field
> to the lookup, similarly to the pwm_lookup?
> I suppose reset_controller_add_lookup could then be called with a NULL
> rcdev to register lookups by id.
>
Yes, this is what I was thinking about more or less.
>> - simplify the code - only expose a single lookup structure and a simply
>> registration function
>> - add the RESET_LOOKUP macro for brevity
>>
>> drivers/reset/core.c | 65
>> +++-
>> include/linux/reset-controller.h | 28 +
>> 2 files changed, 92 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
>> index da4292e9de97..75e54a05147a 100644
>> --- a/drivers/reset/core.c
>> +++ b/drivers/reset/core.c
>> @@ -23,6 +23,9 @@
>> static DEFINE_MUTEX(reset_list_mutex);
>> static LIST_HEAD(reset_controller_list);
>>
>> +static DEFINE_MUTEX(reset_lookup_mutex);
>> +static LIST_HEAD(reset_lookup_list);
>> +
>> /**
>> * struct reset_control - a reset control
>> * @rcdev: a pointer to the reset controller device
>> @@ -148,6 +151,30 @@ int devm_reset_controller_register(struct device *dev,
>> }
>> EXPORT_SYMBOL_GPL(devm_reset_controller_register);
>>
>> +/**
>> + * reset_controller_add_lookup - register a set of lookup entries
>> + * @rcdev: initialized reset controller device owning the reset line
>> + * @lookup: array of reset lookup entries
>> + * @num_entries: number of entries in the lookup array
>> + */
>> +void reset_controller_add_lookup(struct reset_controller_dev *rcdev,
>> + struct reset_control_lookup *lookup,
>> + unsigned int
Re: [PATCH v5] reset: add support for non-DT systems
Hi Bartosz,
thank you for the update.
On Fri, 2018-02-23 at 12:39 +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski
>
> The reset framework only supports device-tree. There are some platforms
> however, which need to use it even in legacy, board-file based mode.
>
> An example of such architecture is the DaVinci family of SoCs which
> supports both device tree and legacy boot modes and we don't want to
> introduce any regressions.
>
> We're currently working on converting the platform from its hand-crafted
> clock API to using the common clock framework. Part of the overhaul will
> be representing the chip's power sleep controller's reset lines using
> the reset framework.
>
> This changeset extends the core reset code with a new reset lookup
> entry structure. It contains data allowing the reset core to associate
> reset lines with devices by comparing the dev_id and con_id strings.
>
> It also provides a function allowing drivers to register lookup entries
> with the framework.
>
> The new lookup function is only called as a fallback in case the
> of_node field is NULL and doesn't change anything for current users.
>
> Tested with a dummy reset driver with several lookup entries.
>
> An example lookup table registration from a driver can be found below:
>
> static struct reset_control_lookup foobar_reset_lookup[] = {
> RESET_LOOKUP("foo.0", "foo", 15),
> RESET_LOOKUP("bar.0", NULL, 5),
> };
>
> foobar_probe()
> {
> ...
>
> reset_controller_add_lookup(&rcdev, foobar_reset_lookup,
> ARRAY_SIZE(foobar_reset_lookup));
>
> ...
> }
>
> Cc: Sekhar Nori
> Cc: Kevin Hilman
> Cc: David Lechner
> Signed-off-by: Bartosz Golaszewski
> ---
> v1 -> v2:
> - renamed the new function to __reset_control_get_from_lookup()
> - added a missing break; when a matching entry is found
> - rearranged the code in __reset_control_get() - we can no longer get to the
> return at the bottom, so remove it and return from
> __reset_control_get_from_lookup() if __of_reset_control_get() fails
> - return -ENOENT from reset_contol_get() if we can't find a matching entry,
> prevously returned -EINVAL referred to the fact that we passed a device
> without the of_node which is no longer an error condition
> - add a comment about needing a sentinel in the lookup table
>
> v2 -> v3:
> - added the reset id number field to the lookup struct so that we don't need
> to rely on the array index
>
> v3 -> v4:
> - separated the driver and lookup table registration logic by adding a
> function meant to be called by machine-specific code that adds a lookup
> table to the internal list
> - the correct reset controller is now found by first finding the lookup
> table associated with it, then finding the actual reset controller by
> the associated device
>
> v4 -> v5:
> - since the first user of this will be the davinci clk driver and it
> already registers clock lookup from within the driver code - allow
> drivers to register lookups with the assumption that the code can be
> extended to make it possible to register entries from machine code as
> well
How do you imagine this may be extended? By adding an rddev_devid field
to the lookup, similarly to the pwm_lookup?
I suppose reset_controller_add_lookup could then be called with a NULL
rcdev to register lookups by id.
> - simplify the code - only expose a single lookup structure and a simply
> registration function
> - add the RESET_LOOKUP macro for brevity
>
> drivers/reset/core.c | 65
> +++-
> include/linux/reset-controller.h | 28 +
> 2 files changed, 92 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index da4292e9de97..75e54a05147a 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -23,6 +23,9 @@
> static DEFINE_MUTEX(reset_list_mutex);
> static LIST_HEAD(reset_controller_list);
>
> +static DEFINE_MUTEX(reset_lookup_mutex);
> +static LIST_HEAD(reset_lookup_list);
> +
> /**
> * struct reset_control - a reset control
> * @rcdev: a pointer to the reset controller device
> @@ -148,6 +151,30 @@ int devm_reset_controller_register(struct device *dev,
> }
> EXPORT_SYMBOL_GPL(devm_reset_controller_register);
>
> +/**
> + * reset_controller_add_lookup - register a set of lookup entries
> + * @rcdev: initialized reset controller device owning the reset line
> + * @lookup: array of reset lookup entries
> + * @num_entries: number of entries in the lookup array
> + */
> +void reset_controller_add_lookup(struct reset_controller_dev *rcdev,
> + struct reset_control_lookup *lookup,
> + unsigned int num_entries)
> +{
> + struct reset_control_lookup *entry;
> + unsigned int i;
> +
> + mutex_lock(&reset_lookup_mutex);
> + for (i = 0; i < num_entries; i++) {
> + entry = &lookup
Re: [PATCH v5] reset: add support for non-DT systems
2018-02-23 12:39 GMT+01:00 Bartosz Golaszewski :
> From: Bartosz Golaszewski
>
> The reset framework only supports device-tree. There are some platforms
> however, which need to use it even in legacy, board-file based mode.
>
> An example of such architecture is the DaVinci family of SoCs which
> supports both device tree and legacy boot modes and we don't want to
> introduce any regressions.
>
> We're currently working on converting the platform from its hand-crafted
> clock API to using the common clock framework. Part of the overhaul will
> be representing the chip's power sleep controller's reset lines using
> the reset framework.
>
> This changeset extends the core reset code with a new reset lookup
> entry structure. It contains data allowing the reset core to associate
> reset lines with devices by comparing the dev_id and con_id strings.
>
> It also provides a function allowing drivers to register lookup entries
> with the framework.
>
> The new lookup function is only called as a fallback in case the
> of_node field is NULL and doesn't change anything for current users.
>
> Tested with a dummy reset driver with several lookup entries.
>
> An example lookup table registration from a driver can be found below:
>
> static struct reset_control_lookup foobar_reset_lookup[] = {
> RESET_LOOKUP("foo.0", "foo", 15),
> RESET_LOOKUP("bar.0", NULL, 5),
> };
>
> foobar_probe()
> {
> ...
>
> reset_controller_add_lookup(&rcdev, foobar_reset_lookup,
> ARRAY_SIZE(foobar_reset_lookup));
>
> ...
> }
>
> Cc: Sekhar Nori
> Cc: Kevin Hilman
> Cc: David Lechner
> Signed-off-by: Bartosz Golaszewski
> ---
> v1 -> v2:
> - renamed the new function to __reset_control_get_from_lookup()
> - added a missing break; when a matching entry is found
> - rearranged the code in __reset_control_get() - we can no longer get to the
> return at the bottom, so remove it and return from
> __reset_control_get_from_lookup() if __of_reset_control_get() fails
> - return -ENOENT from reset_contol_get() if we can't find a matching entry,
> prevously returned -EINVAL referred to the fact that we passed a device
> without the of_node which is no longer an error condition
> - add a comment about needing a sentinel in the lookup table
>
> v2 -> v3:
> - added the reset id number field to the lookup struct so that we don't need
> to rely on the array index
>
> v3 -> v4:
> - separated the driver and lookup table registration logic by adding a
> function meant to be called by machine-specific code that adds a lookup
> table to the internal list
> - the correct reset controller is now found by first finding the lookup
> table associated with it, then finding the actual reset controller by
> the associated device
>
> v4 -> v5:
> - since the first user of this will be the davinci clk driver and it
> already registers clock lookup from within the driver code - allow
> drivers to register lookups with the assumption that the code can be
> extended to make it possible to register entries from machine code as
> well
> - simplify the code - only expose a single lookup structure and a simply
> registration function
> - add the RESET_LOOKUP macro for brevity
>
> drivers/reset/core.c | 65
> +++-
> include/linux/reset-controller.h | 28 +
> 2 files changed, 92 insertions(+), 1 deletion(-)
Also: I decided not to go the phy way of allocating memory dynamically
for lookup entries - I believe this is overkill and makes the code
more complicated with all the error checking required.
Instead, the lookup entries structs should be provided by the caller.
Bart
