Date:   Thu, 28 Oct 2021 11:19:33 -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, devicetree@...r.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: reset: Add generic GPIO reset binding

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

>>
>> 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.

>> +%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.

[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(!)). 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).

--Sean

[2] Documentation/devicetree/bindings/net/ethernet-phy.yaml
[3] Documentation/devicetree/bindings/net/mdio.yaml
[4] Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml

>> +
>> +required:
>> +  - '#reset-cells'
>> +  - compatible
>> +  - reset-gpios
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    pcs_reset: reset-pcs {
>> +        #reset-cells = <1>;
>> +        compatible = "gpio-reset";
>> +        reset-gpios = <&gpio 0 GPIO_ACTIVE_LOW>,
>> +                      <&gpio 1 GPIO_ACTIVE_LOW>,
>> +                      <&gpio 2 GPIO_ACTIVE_LOW>,
>> +                      <&gpio 3 GPIO_ACTIVE_LOW>;
>> +        done-gpios = <&gpio 4 GPIO_ACTIVE_HIGH>,
>> +                     <&gpio 5 GPIO_ACTIVE_HIGH>,
>> +                     <&gpio 6 GPIO_ACTIVE_HIGH>,
>> +                     <&gpio 7 GPIO_ACTIVE_HIGH>;
>> +        post-deassert-us = <100>;
>> +    };
>> --
>> 2.25.1
>>
>>
>

Powered by blists - more mailing lists