Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1451842 > unrolled thread

Re: Why do we need reset_control_get_optional() ?

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Why do we need reset_control_get_optional() ? Philipp Zabel <p.zabel@pengutronix.de> - 2016-07-28 11:50 +0200
    Re: Why do we need reset_control_get_optional() ? Arnd Bergmann <arnd@arndb.de> - 2016-07-28 12:20 +0200
      Re: Why do we need reset_control_get_optional() ? Philipp Zabel <p.zabel@pengutronix.de> - 2016-07-28 13:00 +0200
      Re: Why do we need reset_control_get_optional() ? Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-07-28 13:00 +0200
        Re: Why do we need reset_control_get_optional() ? Philipp Zabel <p.zabel@pengutronix.de> - 2016-07-28 13:10 +0200
          Re: Why do we need reset_control_get_optional() ? Arnd Bergmann <arnd@arndb.de> - 2016-08-05 18:00 +0200
            Re: Why do we need reset_control_get_optional() ? Philipp Zabel <p.zabel@pengutronix.de> - 2016-08-08 18:40 +0200
              Re: Why do we need reset_control_get_optional() ? Arnd Bergmann <arnd@arndb.de> - 2016-08-08 23:50 +0200
    Re: Why do we need reset_control_get_optional() ? Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-07-28 12:40 +0200
      Re: Why do we need reset_control_get_optional() ? Philipp Zabel <p.zabel@pengutronix.de> - 2016-07-29 15:10 +0200
        Re: Why do we need reset_control_get_optional() ? Arnd Bergmann <arnd@arndb.de> - 2016-07-30 22:20 +0200
          Re: Why do we need reset_control_get_optional() ? Philipp Zabel <p.zabel@pengutronix.de> - 2016-08-05 11:00 +0200
            Re: Why do we need reset_control_get_optional() ? Arnd Bergmann <arnd@arndb.de> - 2016-08-05 17:40 +0200
              Re: Why do we need reset_control_get_optional() ? Philipp Zabel <p.zabel@pengutronix.de> - 2016-08-08 19:30 +0200

#1451842 — Re: Why do we need reset_control_get_optional() ?

Am Samstag, den 23.07.2016, 20:22 +0900 schrieb Masahiro Yamada:
> Hi.
> 
> 
> Now the reset subsystem provides
> a bunch of reset_control_get variants.
> 
> I am still wondering why we need to have _optional ones.
> 
> As far as I see, the difference is WARN_ON(1)
> when CONFIG_RESET_CONTROLLER is not defined.
> 
> 
> 
> [1] When the reset is mandatory,
> the code of the reset consumer is probably like follows:
> 
>   rst = devm_reset_control_get(dev, NULL);
>   if (IS_ERR(rst)) {
>           dev_err(dev, "failed to get reset\n");
>           return PTR_ERR(rst);
>   }
> 
>   ret = reset_control_deassert(rst);
>   if (ret) {
>           dev_err(dev, "failed to deassert reset\n");
>           return ret;
>   }
> 
>    ...
> 
> 
> 
> [2] When the reset is optional,
>   the code should be something like follows:
> 
>    rst = devm_reset_control_get(dev, NULL);
>    if (ERR_PTR(rst) == -EPROBE_DEFER)
>            return -EPROBE_DEFER;
> 
>    /* deassert reset if it is available */
>    if (!IS_ERR(rst)) {
>            ret = reset_control_deassert(rst);
>            if (ret) {
>                   dev_err(dev, "failed to deassert reset\n");
>                   return ret;
>            }
>     }
> 
> 
> 
> 
> What I mean is, we can write a driver in either way
> without using the _optional one.
> 
> No need to call WARN_ON(1).
> 
> 
> What does _optional buy us?

It will complain loudly with a backtrace if a driver requests a
non-optional reset on a kernel/platform with the reset framework
disabled.

> One more thing.
> WARN_ON(1) is only useful on run-time,
> but run-time test is more expensive than compile-time test.
> 
> If a driver really needs reset control,
> it should not be complied without CONFIG_RESET_CONTROLLER.
> So, the driver should have "depends on RESET_CONTROLLER" in Kconfig.

If we do that, we can't compile test those drivers anymore in
configurations without RESET_CONTROLLER enabled.

[...]
> I want to deprecate _optional variants in the following steps:
> 
> [1] Add "depends on RESET_CONTROLLER" to drivers
>     for which reset_control is mandatory.
> 
>     We can find those driver easily by grepping
>     the reference to non-optional reset_control_get().

Since we have the stubs, the RESET_CONTROLLER dependency is only at
runtime, not at build time.

I think Arnd wanted to move this in the opposite direction and remove
the configurable RESET_CONTROLLER symbol. Maybe we should let all
drivers that currently request non-optional resets have:
	depends on (ARCH_HAS_)RESET_CONTROLLER || COMPILE_TEST
?

regards
Philipp

[toc] | [next] | [standalone]

#1451859

On Thursday, July 28, 2016 11:43:00 AM CEST Philipp Zabel wrote:
> > I want to deprecate _optional variants in the following steps:
> > 
> > [1] Add "depends on RESET_CONTROLLER" to drivers
> >     for which reset_control is mandatory.
> > 
> >     We can find those driver easily by grepping
> >     the reference to non-optional reset_control_get().
> 
> Since we have the stubs, the RESET_CONTROLLER dependency is only at
> runtime, not at build time.
> 
> I think Arnd wanted to move this in the opposite direction and remove
> the configurable RESET_CONTROLLER symbol. Maybe we should let all
> drivers that currently request non-optional resets have:
>         depends on (ARCH_HAS_)RESET_CONTROLLER || COMPILE_TEST
> ?

There are various ways to improve the current situation.

I think it's important that a driver that has an optional
reset line behaves in exactly the same way whether the reset
subsystem is enabled or disabled when no reset line is
provided for a machine.

When a driver requires a reset line, we can either have a
build-time failure when the reset subsystem is disabled
(enforcing the Kconfig dependency), or cause a runtime
failure if either there is no reset line or the subsystem
is disabled.

In my experimental patch, I make the _optional functions
return NULL if no "resets" property is provided but return
an error if there are reset lines but the subsystem is
disabled, i.e. an optional reset must be used if it's in the
DT, but can be ignored otherwise.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1451874

Am Donnerstag, den 28.07.2016, 12:09 +0200 schrieb Arnd Bergmann:
> On Thursday, July 28, 2016 11:43:00 AM CEST Philipp Zabel wrote:
> > > I want to deprecate _optional variants in the following steps:
> > > 
> > > [1] Add "depends on RESET_CONTROLLER" to drivers
> > >     for which reset_control is mandatory.
> > > 
> > >     We can find those driver easily by grepping
> > >     the reference to non-optional reset_control_get().
> > 
> > Since we have the stubs, the RESET_CONTROLLER dependency is only at
> > runtime, not at build time.
> > 
> > I think Arnd wanted to move this in the opposite direction and remove
> > the configurable RESET_CONTROLLER symbol. Maybe we should let all
> > drivers that currently request non-optional resets have:
> >         depends on (ARCH_HAS_)RESET_CONTROLLER || COMPILE_TEST
> > ?
> 
> There are various ways to improve the current situation.
> 
> I think it's important that a driver that has an optional
> reset line behaves in exactly the same way whether the reset
> subsystem is enabled or disabled when no reset line is
> provided for a machine.

Agreed.

> When a driver requires a reset line, we can either have a
> build-time failure when the reset subsystem is disabled
> (enforcing the Kconfig dependency),

That would have been the case before commit 5bcd0b7f3c56 ("reset: Add
(devm_)reset_control_get stub functions"). Now that we allow compiling
all drivers with the stubs, I suppose option B is the way to go:

> or cause a runtime failure if either there is no reset line or
> the subsystem is disabled.

> In my experimental patch, I make the _optional functions
> return NULL if no "resets" property is provided but return
> an error if there are reset lines but the subsystem is
> disabled, i.e. an optional reset must be used if it's in the
> DT, but can be ignored otherwise.

I have to admit I don't understand the purpose of that.
reset_controller_get_optional returns -ENOENT if there's no reset in the
DT, and all drivers act accordingly.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1451875

Hi Arnd,


2016-07-28 19:09 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> On Thursday, July 28, 2016 11:43:00 AM CEST Philipp Zabel wrote:
>> > I want to deprecate _optional variants in the following steps:
>> >
>> > [1] Add "depends on RESET_CONTROLLER" to drivers
>> >     for which reset_control is mandatory.
>> >
>> >     We can find those driver easily by grepping
>> >     the reference to non-optional reset_control_get().
>>
>> Since we have the stubs, the RESET_CONTROLLER dependency is only at
>> runtime, not at build time.
>>
>> I think Arnd wanted to move this in the opposite direction and remove
>> the configurable RESET_CONTROLLER symbol. Maybe we should let all
>> drivers that currently request non-optional resets have:
>>         depends on (ARCH_HAS_)RESET_CONTROLLER || COMPILE_TEST
>> ?
>
> There are various ways to improve the current situation.
>
> I think it's important that a driver that has an optional
> reset line behaves in exactly the same way whether the reset
> subsystem is enabled or disabled when no reset line is
> provided for a machine.
>
> When a driver requires a reset line, we can either have a
> build-time failure when the reset subsystem is disabled
> (enforcing the Kconfig dependency), or cause a runtime
> failure if either there is no reset line or the subsystem
> is disabled.

Yes.  I am suggesting the "enforcing the Kconfig dependency".

"I will let you build this driver, but it would never work"
is not the right thing to do, I think.



> In my experimental patch, I make the _optional functions
> return NULL if no "resets" property is provided but return
> an error if there are reset lines but the subsystem is
> disabled, i.e. an optional reset must be used if it's in the
> DT, but can be ignored otherwise.

I do not like this idea.

reset_control_get() (or variants) should not return NULL, it is ambiguous.
It should return ERR_PTR(-ENOENT) if no "resets" property.

I only want two types for functions that return a pointer.

[1] return a valid pointer on success, or return NULL on failure
    (for example, kmalloc())
[2] return a valid pointer on success, or return error pointer on failure
   (many of _register() functions)

Mixing [1] and [2] will be a mess.



-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1451878

Am Donnerstag, den 28.07.2016, 19:52 +0900 schrieb Masahiro Yamada:
> Hi Arnd,
> 
> 
> 2016-07-28 19:09 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> > On Thursday, July 28, 2016 11:43:00 AM CEST Philipp Zabel wrote:
> >> > I want to deprecate _optional variants in the following steps:
> >> >
> >> > [1] Add "depends on RESET_CONTROLLER" to drivers
> >> >     for which reset_control is mandatory.
> >> >
> >> >     We can find those driver easily by grepping
> >> >     the reference to non-optional reset_control_get().
> >>
> >> Since we have the stubs, the RESET_CONTROLLER dependency is only at
> >> runtime, not at build time.
> >>
> >> I think Arnd wanted to move this in the opposite direction and remove
> >> the configurable RESET_CONTROLLER symbol. Maybe we should let all
> >> drivers that currently request non-optional resets have:
> >>         depends on (ARCH_HAS_)RESET_CONTROLLER || COMPILE_TEST
> >> ?
> >
> > There are various ways to improve the current situation.
> >
> > I think it's important that a driver that has an optional
> > reset line behaves in exactly the same way whether the reset
> > subsystem is enabled or disabled when no reset line is
> > provided for a machine.
> >
> > When a driver requires a reset line, we can either have a
> > build-time failure when the reset subsystem is disabled
> > (enforcing the Kconfig dependency), or cause a runtime
> > failure if either there is no reset line or the subsystem
> > is disabled.
> 
> Yes.  I am suggesting the "enforcing the Kconfig dependency".
> 
> "I will let you build this driver, but it would never work"
> is not the right thing to do, I think.

We'd loose randconfig build coverage though. I think allowing to build
unusable drivers in COMPILE_TEST scenarios is ok.

> > In my experimental patch, I make the _optional functions
> > return NULL if no "resets" property is provided but return
> > an error if there are reset lines but the subsystem is
> > disabled, i.e. an optional reset must be used if it's in the
> > DT, but can be ignored otherwise.
> 
> I do not like this idea.
> 
> reset_control_get() (or variants) should not return NULL, it is ambiguous.
> It should return ERR_PTR(-ENOENT) if no "resets" property.
> 
> I only want two types for functions that return a pointer.
> 
> [1] return a valid pointer on success, or return NULL on failure
>     (for example, kmalloc())
> [2] return a valid pointer on success, or return error pointer on failure
>    (many of _register() functions)
> 
> Mixing [1] and [2] will be a mess.

I too would prefer to keep that as-is. The reset_control_get_optional
stub could return -ENOENT if there is no resets device tree property.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1457186

On Thursday, July 28, 2016 1:00:49 PM CEST Philipp Zabel wrote:
> Am Donnerstag, den 28.07.2016, 19:52 +0900 schrieb Masahiro Yamada:

> > > In my experimental patch, I make the _optional functions
> > > return NULL if no "resets" property is provided but return
> > > an error if there are reset lines but the subsystem is
> > > disabled, i.e. an optional reset must be used if it's in the
> > > DT, but can be ignored otherwise.
> > 
> > I do not like this idea.
> > 
> > reset_control_get() (or variants) should not return NULL, it is ambiguous.
> > It should return ERR_PTR(-ENOENT) if no "resets" property.
> > 
> > I only want two types for functions that return a pointer.
> > 
> > [1] return a valid pointer on success, or return NULL on failure
> >     (for example, kmalloc())
> > [2] return a valid pointer on success, or return error pointer on failure
> >    (many of _register() functions)
> > 
> > Mixing [1] and [2] will be a mess.

Ah, right. I was thinking only of the case where the reset subsystem
is completely disabled here, so returning NULL could be considered
a valid return code that can in turn be passed into the other
functions.

However, I agree that returning NULL as a valid result from
..._get_optional() would be bad style, so let's drop my idea
there.

> I too would prefer to keep that as-is. The reset_control_get_optional
> stub could return -ENOENT if there is no resets device tree property.

Now I'm also confused about what we really need
reset_control_get_optional() for, and which error codes the callers
are supposed to check.

This is the matrix I think you mean for _get_optional:

CONFIG_RESET_CONTROLLER=y, dt entry present: valid pointer (or other error)
CONFIG_RESET_CONTROLLER=n, dt entry present: -EOPNOTSUPP
CONFIG_RESET_CONTROLLER=y, dt entry missing: -ENOENT
CONFIG_RESET_CONTROLLER=n, dt entry missing: -ENOENT

Is this what you had in mind? If so, what is the value of the
added runtime warning for reset_control_get? Any caller of that
function would already check for errors, the only difference
I see is that callers of _optional can ignore -ENOENT.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1457904

Am Freitag, den 05.08.2016, 17:50 +0200 schrieb Arnd Bergmann:
> On Thursday, July 28, 2016 1:00:49 PM CEST Philipp Zabel wrote:
> > Am Donnerstag, den 28.07.2016, 19:52 +0900 schrieb Masahiro Yamada:
> 
> > > > In my experimental patch, I make the _optional functions
> > > > return NULL if no "resets" property is provided but return
> > > > an error if there are reset lines but the subsystem is
> > > > disabled, i.e. an optional reset must be used if it's in the
> > > > DT, but can be ignored otherwise.
> > > 
> > > I do not like this idea.
> > > 
> > > reset_control_get() (or variants) should not return NULL, it is ambiguous.
> > > It should return ERR_PTR(-ENOENT) if no "resets" property.
> > > 
> > > I only want two types for functions that return a pointer.
> > > 
> > > [1] return a valid pointer on success, or return NULL on failure
> > >     (for example, kmalloc())
> > > [2] return a valid pointer on success, or return error pointer on failure
> > >    (many of _register() functions)
> > > 
> > > Mixing [1] and [2] will be a mess.
> 
> Ah, right. I was thinking only of the case where the reset subsystem
> is completely disabled here, so returning NULL could be considered
> a valid return code that can in turn be passed into the other
> functions.
> 
> However, I agree that returning NULL as a valid result from
> ..._get_optional() would be bad style, so let's drop my idea
> there.
> 
> > I too would prefer to keep that as-is. The reset_control_get_optional
> > stub could return -ENOENT if there is no resets device tree property.
> 
> Now I'm also confused about what we really need
> reset_control_get_optional() for, and which error codes the callers
> are supposed to check.
> 
> This is the matrix I think you mean for _get_optional:
> 
[...]
> CONFIG_RESET_CONTROLLER=n, dt entry present: -EOPNOTSUPP
> CONFIG_RESET_CONTROLLER=n, dt entry missing: -ENOENT

^^ I didn't consider this distiction.

> Is this what you had in mind? If so, what is the value of the
> added runtime warning for reset_control_get? Any caller of that
> function would already check for errors, the only difference
> I see is that callers of _optional can ignore -ENOENT.

My initial motivation was to make it as hard as possible to misconfigure
the kernel, which is why I initially didn't want stubs for the
non-optional variant. Of course that would cause build failures and/or
reduced compile test coverage, so we added the stubs and the warning to
make it obvious when a misconfigured kernel is running: on a kernel with
RESET_CONTROLLER=n drivers that use reset_control_get are expected to
build, but they are not expected to work. I suppose the same is the case
for _optional, if the dt entry is present, so maybe we should drop
reset_control_get_optional and add always a warning in case of
-EOPNOTSUPP.
I don't want all drivers to have to differentiate between -EOPNOTSUPP
and -ENOENT error codes, only current reset_control_get_optional users
have to do that.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1458265

On Monday, August 8, 2016 6:39:36 PM CEST Philipp Zabel wrote:
> > Now I'm also confused about what we really need
> > reset_control_get_optional() for, and which error codes the callers
> > are supposed to check.
> > 
> > This is the matrix I think you mean for _get_optional:
> > 
> [...]
> > CONFIG_RESET_CONTROLLER=n, dt entry present: -EOPNOTSUPP
> > CONFIG_RESET_CONTROLLER=n, dt entry missing: -ENOENT
> 
> ^^ I didn't consider this distiction.
> 
> > Is this what you had in mind? If so, what is the value of the
> > added runtime warning for reset_control_get? Any caller of that
> > function would already check for errors, the only difference
> > I see is that callers of _optional can ignore -ENOENT.
> 
> My initial motivation was to make it as hard as possible to misconfigure
> the kernel, which is why I initially didn't want stubs for the
> non-optional variant. Of course that would cause build failures and/or
> reduced compile test coverage, so we added the stubs and the warning to
> make it obvious when a misconfigured kernel is running: on a kernel with
> RESET_CONTROLLER=n drivers that use reset_control_get are expected to
> build, but they are not expected to work. I suppose the same is the case
> for _optional, if the dt entry is present, so maybe we should drop
> reset_control_get_optional and add always a warning in case of
> -EOPNOTSUPP.
> I don't want all drivers to have to differentiate between -EOPNOTSUPP
> and -ENOENT error codes, only current reset_control_get_optional users
> have to do that.

In almost all cases, I think drivers that require the reset line
wouldn't even check the failure code but just pass it down to
the caller (usually platform_device_probe()), so the really don't
need to care.

Some drivers might want to handle -EPROBE_DEFER (by not warning
about it before returning from probe), and -ENOENT can be handled
in a similar way (by continuing instead of failing).

	Arnd

[toc] | [prev] | [next] | [standalone]

#1451870

Hi Philipp,


2016-07-28 18:43 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Am Samstag, den 23.07.2016, 20:22 +0900 schrieb Masahiro Yamada:
>> Hi.
>>
>>
>> Now the reset subsystem provides
>> a bunch of reset_control_get variants.
>>
>> I am still wondering why we need to have _optional ones.
>>
>> As far as I see, the difference is WARN_ON(1)
>> when CONFIG_RESET_CONTROLLER is not defined.
>>
>>
>>
>> [1] When the reset is mandatory,
>> the code of the reset consumer is probably like follows:
>>
>>   rst = devm_reset_control_get(dev, NULL);
>>   if (IS_ERR(rst)) {
>>           dev_err(dev, "failed to get reset\n");
>>           return PTR_ERR(rst);
>>   }
>>
>>   ret = reset_control_deassert(rst);
>>   if (ret) {
>>           dev_err(dev, "failed to deassert reset\n");
>>           return ret;
>>   }
>>
>>    ...
>>
>>
>>
>> [2] When the reset is optional,
>>   the code should be something like follows:
>>
>>    rst = devm_reset_control_get(dev, NULL);
>>    if (ERR_PTR(rst) == -EPROBE_DEFER)
>>            return -EPROBE_DEFER;
>>
>>    /* deassert reset if it is available */
>>    if (!IS_ERR(rst)) {
>>            ret = reset_control_deassert(rst);
>>            if (ret) {
>>                   dev_err(dev, "failed to deassert reset\n");
>>                   return ret;
>>            }
>>     }
>>
>>
>>
>>
>> What I mean is, we can write a driver in either way
>> without using the _optional one.
>>
>> No need to call WARN_ON(1).
>>
>>
>> What does _optional buy us?
>
> It will complain loudly with a backtrace if a driver requests a
> non-optional reset on a kernel/platform with the reset framework
> disabled.

Right, but this situation will be solved with my suggestion.


>> One more thing.
>> WARN_ON(1) is only useful on run-time,
>> but run-time test is more expensive than compile-time test.
>>
>> If a driver really needs reset control,
>> it should not be complied without CONFIG_RESET_CONTROLLER.
>> So, the driver should have "depends on RESET_CONTROLLER" in Kconfig.
>
> If we do that, we can't compile test those drivers anymore in
> configurations without RESET_CONTROLLER enabled.
> [...]
>> I want to deprecate _optional variants in the following steps:
>>
>> [1] Add "depends on RESET_CONTROLLER" to drivers
>>     for which reset_control is mandatory.
>>
>>     We can find those driver easily by grepping
>>     the reference to non-optional reset_control_get().
>
> Since we have the stubs, the RESET_CONTROLLER dependency is only at
> runtime, not at build time.
>
> I think Arnd wanted to move this in the opposite direction and remove
> the configurable RESET_CONTROLLER symbol. Maybe we should let all
> drivers that currently request non-optional resets have:
>         depends on (ARCH_HAS_)RESET_CONTROLLER || COMPILE_TEST
> ?

No, I do not think we need to do that.

We should do
          depends on ARCH_<SOC_NAME> || COMPILE_TEST
because SOC_NAME is not a real dependency for the driver.


but,

      depends on RESET_CONTROLLER

is a genuine dependency, so it should not be OR'ed with COMPILE_TEST.




Currently, ARCH_HAS_RESET_CONTROLLER is only used to decide the default value
of RESET_CONTROLLER:


menuconfig RESET_CONTROLLER
        bool "Reset Controller Support"
        default y if ARCH_HAS_RESET_CONTROLLER
        help
          Generic Reset Controller support.



So, RESET_CONTROLLER can be enabled without any dependency,
i.e., COMPILE_TEST will be fine.



However, I think the following makes more sense:


menuconfig RESET_CONTROLLER
        bool "Reset Controller Support"
        depends on (ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST)
        default y
        help
          Generic Reset Controller support.





-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1452404

Hi Masahiro,

Am Donnerstag, den 28.07.2016, 19:29 +0900 schrieb Masahiro Yamada:
[...]
> However, I think the following makes more sense:
> 
> 
> menuconfig RESET_CONTROLLER
>         bool "Reset Controller Support"
>         depends on (ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST)
>         default y
>         help
>           Generic Reset Controller support.

That looks sensible to me. You'll only have to enable the reset
controller framework if either some enabled architecture has a reset
controller (in which case you want the driver for it to be activated by
default), or if you want to compile test some of the reset drivers.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1452743

On Friday, July 29, 2016 3:08:15 PM CEST Philipp Zabel wrote:
> Hi Masahiro,
> 
> Am Donnerstag, den 28.07.2016, 19:29 +0900 schrieb Masahiro Yamada:
> [...]
> > However, I think the following makes more sense:
> > 
> > 
> > menuconfig RESET_CONTROLLER
> >         bool "Reset Controller Support"
> >         depends on (ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST)
> >         default y
> >         help
> >           Generic Reset Controller support.
> 
> That looks sensible to me. You'll only have to enable the reset
> controller framework if either some enabled architecture has a reset
> controller (in which case you want the driver for it to be activated by
> default), or if you want to compile test some of the reset drivers.

This still doesn't let a platform 'select RESET_FOO', unless they
also select RESET_CONTROLLER and ARCH_HAS_RESET_CONTROLLER.

Why do we need to guard all drivers inside of two symbols?

	Arnd

[toc] | [prev] | [next] | [standalone]

#1456955

Am Samstag, den 30.07.2016, 22:13 +0200 schrieb Arnd Bergmann:
> On Friday, July 29, 2016 3:08:15 PM CEST Philipp Zabel wrote:
> > Hi Masahiro,
> > 
> > Am Donnerstag, den 28.07.2016, 19:29 +0900 schrieb Masahiro Yamada:
> > [...]
> > > However, I think the following makes more sense:
> > > 
> > > 
> > > menuconfig RESET_CONTROLLER
> > >         bool "Reset Controller Support"
> > >         depends on (ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST)
> > >         default y
> > >         help
> > >           Generic Reset Controller support.
> > 
> > That looks sensible to me. You'll only have to enable the reset
> > controller framework if either some enabled architecture has a reset
> > controller (in which case you want the driver for it to be activated by
> > default), or if you want to compile test some of the reset drivers.
> 
> This still doesn't let a platform 'select RESET_FOO', unless they
> also select RESET_CONTROLLER and ARCH_HAS_RESET_CONTROLLER.
> 
> Why do we need to guard all drivers inside of two symbols?

Does the platform have to select RESET_FOO at all? Wouldn't it be enough
for RESET_FOO to have "default ARCH_FOO" ?

Currently ARCH_HAS_RESET_CONTROLLER is used to default y the
RESET_CONTROLLER symbol. Maybe we should add another
ARCH_REQUIRE_RESET_CONTROLLER and have that select RESET_CONTROLLER,
similarly to how it is done for GPIOLIB?

config ARCH_HAS_RESET_CONTROLLER
	bool
	help
	  Selecting this option from the architecture Kconfig enables
	  the RESET_CONTROLLER framework by default but does not select
	  it. Use it for architectures that still work without reset
	  controller support and thus allow the user to disable it.

config ARCH_REQUIRE_RESET_CONTROLLER
	bool
	select RESET_CONTROLLER
	help
	  Selecting this option from the architecture Kconfig selects
	  the RESET_CONTROLLER framework. Use it for architectures that
	  should not be built without the reset controller framework
	  enabled.

menuconfig RESET_CONTROLLER
        bool "Reset Controller Support"
        default ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST
        help
          Generic Reset Controller support.

          This framework is designed to abstract reset handling of devices
          via GPIOs or SoC-internal reset controller modules.

          If unsure, say no.

The platforms could then select one of the ARCH_*_RESET_CONTROLLER
symbols and nobody would have to select RESET_CONTROLLER directly, for
example:

menuconfig ARCH_TEGRA                                                                                                                         
        bool "NVIDIA Tegra"
        depends on ARCH_MULTI_V7
        select ARCH_REQUIRE_GPIOLIB
        select ARCH_REQUIRE_RESET_CONTROLLER
        select ARCH_SUPPORTS_TRUSTED_FOUNDATIONS
        select ARM_AMBA
        select ARM_GIC
        select CLKSRC_MMIO
        select HAVE_ARM_SCU if SMP
        select HAVE_ARM_TWD if SMP
        select PINCTRL
        select PM_OPP
        select SOC_BUS
        help
          This enables support for NVIDIA Tegra based systems.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1457171

On Friday, August 5, 2016 10:55:58 AM CEST Philipp Zabel wrote:
> Am Samstag, den 30.07.2016, 22:13 +0200 schrieb Arnd Bergmann:
> > On Friday, July 29, 2016 3:08:15 PM CEST Philipp Zabel wrote:
> > > Hi Masahiro,
> > > 
> > > Am Donnerstag, den 28.07.2016, 19:29 +0900 schrieb Masahiro Yamada:
> > > [...]
> > > > However, I think the following makes more sense:
> > > > 
> > > > 
> > > > menuconfig RESET_CONTROLLER
> > > >         bool "Reset Controller Support"
> > > >         depends on (ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST)
> > > >         default y
> > > >         help
> > > >           Generic Reset Controller support.
> > > 
> > > That looks sensible to me. You'll only have to enable the reset
> > > controller framework if either some enabled architecture has a reset
> > > controller (in which case you want the driver for it to be activated by
> > > default), or if you want to compile test some of the reset drivers.
> > 
> > This still doesn't let a platform 'select RESET_FOO', unless they
> > also select RESET_CONTROLLER and ARCH_HAS_RESET_CONTROLLER.
> > 
> > Why do we need to guard all drivers inside of two symbols?
> 
> Does the platform have to select RESET_FOO at all? Wouldn't it be enough
> for RESET_FOO to have "default ARCH_FOO" ?

It depends on what you want to achieve. With a user-visible option
and "default ARCH_FOO", you can disable the driver manually, and
another driver that has "depends on ARCH_FOO" can not rely on this
one being present as it currently can.

If we do this as

config RESET_FOO
	bool "FOO reset controller" if COMPILE_TEST && !ARCH_FOO
	default ARCH_FOO

then I think we get both: you won't be able to turn it off
but also get the build testing.

> Currently ARCH_HAS_RESET_CONTROLLER is used to default y the
> RESET_CONTROLLER symbol. Maybe we should add another
> ARCH_REQUIRE_RESET_CONTROLLER and have that select RESET_CONTROLLER,
> similarly to how it is done for GPIOLIB?

GPIOLIB just stopped using it, there is now only CONFIG_GPIOLIB
that can get selected by platforms that need it.

> config ARCH_HAS_RESET_CONTROLLER
> 	bool
> 	help
> 	  Selecting this option from the architecture Kconfig enables
> 	  the RESET_CONTROLLER framework by default but does not select
> 	  it. Use it for architectures that still work without reset
> 	  controller support and thus allow the user to disable it.
> 
> config ARCH_REQUIRE_RESET_CONTROLLER
> 	bool
> 	select RESET_CONTROLLER
> 	help
> 	  Selecting this option from the architecture Kconfig selects
> 	  the RESET_CONTROLLER framework. Use it for architectures that
> 	  should not be built without the reset controller framework
> 	  enabled.
> 
> menuconfig RESET_CONTROLLER
>         bool "Reset Controller Support"
>         default ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST
>         help
>           Generic Reset Controller support.
> 
>           This framework is designed to abstract reset handling of devices
>           via GPIOs or SoC-internal reset controller modules.
> 
>           If unsure, say no.
> 
> The platforms could then select one of the ARCH_*_RESET_CONTROLLER
> symbols and nobody would have to select RESET_CONTROLLER directly, for
> example:
> 
> menuconfig ARCH_TEGRA                                                                                                                         
>         bool "NVIDIA Tegra"
>         depends on ARCH_MULTI_V7
>         select ARCH_REQUIRE_GPIOLIB
>         select ARCH_REQUIRE_RESET_CONTROLLER
>         select ARCH_SUPPORTS_TRUSTED_FOUNDATIONS

I never really like the way it was done for gpiolib. I think the
easiest way would be to have a menu for the reset controllers that
does not have any dependencies whatsoever, and make the individual
reset drivers select CONFIG_RESET_CONTROLLER, which then becomes
a hidden symbol that enables the core code.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1457934

Am Freitag, den 05.08.2016, 17:35 +0200 schrieb Arnd Bergmann:
> On Friday, August 5, 2016 10:55:58 AM CEST Philipp Zabel wrote:
> > Am Samstag, den 30.07.2016, 22:13 +0200 schrieb Arnd Bergmann:
> > > On Friday, July 29, 2016 3:08:15 PM CEST Philipp Zabel wrote:
> > > > Hi Masahiro,
> > > > 
> > > > Am Donnerstag, den 28.07.2016, 19:29 +0900 schrieb Masahiro Yamada:
> > > > [...]
> > > > > However, I think the following makes more sense:
> > > > > 
> > > > > 
> > > > > menuconfig RESET_CONTROLLER
> > > > >         bool "Reset Controller Support"
> > > > >         depends on (ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST)
> > > > >         default y
> > > > >         help
> > > > >           Generic Reset Controller support.
> > > > 
> > > > That looks sensible to me. You'll only have to enable the reset
> > > > controller framework if either some enabled architecture has a reset
> > > > controller (in which case you want the driver for it to be activated by
> > > > default), or if you want to compile test some of the reset drivers.
> > > 
> > > This still doesn't let a platform 'select RESET_FOO', unless they
> > > also select RESET_CONTROLLER and ARCH_HAS_RESET_CONTROLLER.
> > > 
> > > Why do we need to guard all drivers inside of two symbols?
> > 
> > Does the platform have to select RESET_FOO at all? Wouldn't it be enough
> > for RESET_FOO to have "default ARCH_FOO" ?
> 
> It depends on what you want to achieve. With a user-visible option
> and "default ARCH_FOO", you can disable the driver manually, and
> another driver that has "depends on ARCH_FOO" can not rely on this
> one being present as it currently can.
>
> If we do this as
> 
> config RESET_FOO
> 	bool "FOO reset controller" if COMPILE_TEST && !ARCH_FOO
> 	default ARCH_FOO
> 
> then I think we get both: you won't be able to turn it off
> but also get the build testing.

I like it. Automatically enable the mandatory reset controller driver
with its architecture, otherwise don't ask unless COMPILE_TEST is
enabled. For drivers that can be reasonably compiled as a module, it
could be

config RESET_BAR
	tristate "BAR reset controller" if COMPILE_TEST || ARCH_FOO
	default ARCH_FOO

> > Currently ARCH_HAS_RESET_CONTROLLER is used to default y the
> > RESET_CONTROLLER symbol. Maybe we should add another
> > ARCH_REQUIRE_RESET_CONTROLLER and have that select RESET_CONTROLLER,
> > similarly to how it is done for GPIOLIB?
> 
> GPIOLIB just stopped using it, there is now only CONFIG_GPIOLIB
> that can get selected by platforms that need it.

Ok.

[...]
> I never really like the way it was done for gpiolib. I think the
> easiest way would be to have a menu for the reset controllers that
> does not have any dependencies whatsoever, and make the individual
> reset drivers select CONFIG_RESET_CONTROLLER, which then becomes
> a hidden symbol that enables the core code.

I thought making all the driver config symbols visible by default is
problematic, but with "bool ... if COMPILE_TEST" that is addressed
already.

regards
Philipp

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web