Message-ID: <322faa05-240e-0fd4-8ceb-68f77e871cf6@seco.com>
Date:   Mon, 1 Nov 2021 12:24:30 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Rob Herring <robh@...nel.org>
Cc:     Philipp Zabel <p.zabel@...gutronix.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        devicetree@...r.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: reset: Add generic GPIO reset binding



On 10/28/21 9:35 PM, Rob Herring wrote:
> On Thu, Oct 28, 2021 at 10:19 AM Sean Anderson <sean.anderson@...o.com> wrote:
>>
>> Hi Rob,
>>
>> On 10/26/21 10:27 PM, Rob Herring wrote:
>> > On Mon, Oct 18, 2021 at 07:49:21PM -0400, Sean Anderson wrote:
>> >> This adds a binding for a generic GPIO reset driver. This driver is
>> >> designed to easily add a GPIO-based reset to a driver which expected a
>> >> reset controller. It offers greater flexibility than a reset-gpios
>> >> property, and allows for one code path to be shared for GPIO resets and
>> >> MMIO-based resets.
>> >
>> > I would like to do this last part, but not requiring a binding change.
>> > IOW, be able to register any 'reset-gpios' property as a reset provider
>> > directly without this added level of indirection.
>>
>> That would be nice, but it seems like someone would have to go through
>> every driver with a reset-gpios property and convert them. Since the
>> reset GPIOs are
> 
> All that has to happen is when a driver requests a reset, the reset
> subsystem can check for a 'reset-gpios' when there is not a 'resets'
> property. If it finds one, then it can either instantiate a reset
> provider or add that GPIO to an existing provider. Then you can
> convert drivers one by one, or not.

I will have a stab at this.

>> >>
>> >> Signed-off-by: Sean Anderson <sean.anderson@...o.com>
>> >> ---
>> >>
>> >>  .../devicetree/bindings/reset/gpio-reset.yaml | 93 +++++++++++++++++++
>> >>  1 file changed, 93 insertions(+)
>> >>  create mode 100644 Documentation/devicetree/bindings/reset/gpio-reset.yaml
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.yaml b/Documentation/devicetree/bindings/reset/gpio-reset.yaml
>> >> new file mode 100644
>> >> index 000000000000..de2ab074cea3
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/reset/gpio-reset.yaml
>> >> @@ -0,0 +1,93 @@
>> >> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause)
>> >
>> > GPL-2.0-only not GPL-2.0+
>>
>> GPL-2.0+ is a strict superset. And bindings are required to be BSD
>> anyway. I don't see the issue.
> 
> Not everyone agrees with GPLv3. What about GPLv4, v5, etc.? You're
> okay with them no matter what they say?

So you would rather have GPL-2.0-only OR GPL-3.0-only OR BSD-2-Clause :)

> The issue is many people pay no attention. Just copy whatever they
> started from, or put whatever they want. The dts files are a mess. The
> binding docs all defaulted to GPL2. So I'm fixing the mess with
> bindings and that means dictating the license.

Ok

>> >> +%YAML 1.2
>> >> +---
>> >> +$id: http://devicetree.org/schemas/reset/gpio-reset.yaml#
>> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> >> +
>> >> +title: Generic GPIO reset driver
>> >> +
>> >> +maintainers:
>> >> +  - Sean Anderson <seanga2@...il.com>
>> >> +
>> >> +description: |
>> >> +  This is a generic GPIO reset driver which can provide a reset-controller
>> >> +  interface for GPIO-based reset lines. This driver always operates with
>> >> +  logical GPIO values; to invert the polarity, specify GPIO_ACTIVE_LOW in the
>> >> +  GPIO's flags.
>> >> +
>> >> +properties:
>> >> +  compatible:
>> >> +    const: gpio-reset
>> >> +
>> >> +  '#reset-cells':
>> >> +    const: 1
>> >> +
>> >> +  reset-gpios:
>> >> +    description: |
>> >> +      GPIOs to assert when asserting a reset. There is a one-to-one mapping
>> >> +      between the reset specifier and the index of the GPIO in this list to
>> >> +      assert.
>> >> +
>> >> +  done-gpios:
>> >> +    description: |
>> >> +      GPIOs which indicate that the device controlled by the GPIO has exited
>> >> +      reset. There must be one done GPIO for each reset GPIO, or no done GPIOs
>> >> +      at all. The driver will wait for up to done-timeout-us for the
>> >> +      corresponding done GPIO to assert before returning.
>> >
>> > This is odd. Do you have some examples of h/w needing this done signal?
>> > It certainly doesn't seem like something we have a generic need for.
>>
>> Yes [1]. This device has a "reset done" signal, but no reset timings
>> specified in the datasheet. I don't know if this is truly needed,
>> because we can read the ID register, but it is nice when bringing up the
>> device. I left it in because I was using it.
> 
> Okay, but done-gpios belongs in the device node that has a done
> signal. Your binding pretty assumes you always have one because you
> need equal numbers of reset and done gpios.

Have two devices, one with done GPIOs, and one without.

> Anyways, I don't think this binding is going anywhere.
> 
>>
>> [1] https://lore.kernel.org/netdev/20211004191527.1610759-16-sean.anderson@seco.com/
>>
>> >> +
>> >> +  pre-assert-us:
>> >> +    default: 0
>> >> +    description: |
>> >> +      Microseconds to delay between when the reset was requested to be
>> >> +      asserted, and asserting the reset GPIO
>> >> +
>> >> +  post-assert-us:
>> >> +    default: 0
>> >> +    description: |
>> >> +      Microseconds to delay after asserting the reset GPIO and before returning
>> >> +      to the caller.
>> >> +
>> >> +  pre-deassert-us:
>> >> +    default: 0
>> >> +    description: |
>> >> +      Microseconds to delay between when the reset was requested to be
>> >> +      deasserted, and asserting the reset GPIO
>> >> +
>> >> +  post-deassert-us:
>> >> +    default: 0
>> >> +    description: |
>> >> +      Microseconds to delay after deasserting the reset GPIO and before
>> >> +      returning to the caller. This delay is always present, even if the done
>> >> +      GPIO goes high earlier.
>> >> +
>> >> +  done-timeout-us:
>> >> +    default: 1000
>> >> +    description:
>> >> +      Microseconds to wait for the done GPIO to assert after deasserting the
>> >> +      reset GPIO. If post-deassert-us is present, this property defaults to 10
>> >> +      times that delay. The timeout starts after waiting for the post deassert
>> >> +      delay.
>> >
>> > There's a reason we don't have all these timing values in DT. The timing
>> > requirements are defined by each device (being reset) and implied by
>> > their compatible strings. If we wanted a macro language for power
>> > sequence timings of regulators, clocks, resets, enables, etc., then we
>> > would have designed such a thing already.
>>
>> Well, there are already things like reset-assert-us and
>> reset-deassert-us in [2, 3, 4] (with different names(!)).
> 
> Yes, things evolve poorly. What's just one more property added at a time.
> 
>> Part of what I
>> want to address with this device is that there are several existing
>> properties which specify various aspects of the above timings. I think
>> it would be good to standardize on these. Maybe this should be a
>> property which applies to the reset consumer? Analogously, we also
>> have assigned-clocks so that not every driver has to know what the
>> correct frequency/parent is (especially when they can vary among
>> different hardware variations).
> 
> Yes, there are some examples, but you won't find many new examples.
> The rule is power sequencing requirements/timing is implied by the
> device's compatible string.
> 
> You are looking at just reset. What about timing WRT regulators and
> clocks for starters?

*shrug*

This just seems like a very common area which needs to customized.

--Sean

Powered by blists - more mailing lists