| Message ID | 20250116225521.2688224-2-sean.anderson@linux.dev |
|---|---|
| State | New |
| Headers | show |
| Series | spi: zynqmp-gqspi: Improve error recovery by resetting | expand |
On 1/16/25 23:55, Sean Anderson wrote: > Add a reset to help recover from cancelled operations. > > Signed-off-by: Sean Anderson <sean.anderson@linux.dev> > --- > > Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml > index 04d4d3b4916d..901e15fcce2d 100644 > --- a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml > +++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml > @@ -36,12 +36,16 @@ properties: > power-domains: > maxItems: 1 > > + resets: > + maxItems: 1 > + > required: > - compatible > - reg > - interrupts > - clock-names > - clocks > + - resets In 2/5 you are calling devm_reset_control_get_optional_exclusive() that's why I expect reset is not really required property. M
On 1/17/25 02:14, Michal Simek wrote: > > > On 1/16/25 23:55, Sean Anderson wrote: >> Add a reset to help recover from cancelled operations. >> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev> >> --- >> >> Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml >> index 04d4d3b4916d..901e15fcce2d 100644 >> --- a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml >> +++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml >> @@ -36,12 +36,16 @@ properties: >> power-domains: >> maxItems: 1 >> + resets: >> + maxItems: 1 >> + >> required: >> - compatible >> - reg >> - interrupts >> - clock-names >> - clocks >> + - resets > > In 2/5 you are calling devm_reset_control_get_optional_exclusive() that's why I expect reset is not really required property. It's optional for the driver for backwards compatibility. But for the devicetree we make it mandatory since it should be included in all new devicetrees. --Sean
On 1/23/25 17:45, Rob Herring wrote: > On Fri, Jan 17, 2025 at 11:12:15AM -0500, Sean Anderson wrote: >> On 1/17/25 02:14, Michal Simek wrote: >> > >> > >> > On 1/16/25 23:55, Sean Anderson wrote: >> >> Add a reset to help recover from cancelled operations. >> >> >> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev> >> >> --- >> >> >> >> Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml | 6 ++++++ >> >> 1 file changed, 6 insertions(+) >> >> >> >> diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml >> >> index 04d4d3b4916d..901e15fcce2d 100644 >> >> --- a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml >> >> +++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml >> >> @@ -36,12 +36,16 @@ properties: >> >> power-domains: >> >> maxItems: 1 >> >> + resets: >> >> + maxItems: 1 >> >> + >> >> required: >> >> - compatible >> >> - reg >> >> - interrupts >> >> - clock-names >> >> - clocks >> >> + - resets >> > >> > In 2/5 you are calling devm_reset_control_get_optional_exclusive() that's why I expect reset is not really required property. >> >> It's optional for the driver for backwards compatibility. But for the >> devicetree we make it mandatory since it should be included in all new >> devicetrees. > > Generally, we discourage new required properties as that's an ABI > change. The exception is really when optional was a mistake. That's > arguably the case here if the h/w always has a reset. This device has a reset on ZynqMP and Versal. The driver still considers this property optional, so it's not an ABI break. But I made it required in the schema to help out the folks at AMD when they get around to upstreaming the Versal devicetree :) > Unfortunately, there's not a way to distinguish 'required' from > 'required for new users'. I will add a note to the commit message about this situation. --Sean
On 1/23/25 23:57, Sean Anderson wrote: > On 1/23/25 17:45, Rob Herring wrote: >> On Fri, Jan 17, 2025 at 11:12:15AM -0500, Sean Anderson wrote: >>> On 1/17/25 02:14, Michal Simek wrote: >>>> >>>> >>>> On 1/16/25 23:55, Sean Anderson wrote: >>>>> Add a reset to help recover from cancelled operations. >>>>> >>>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev> >>>>> --- >>>>> >>>>> Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml >>>>> index 04d4d3b4916d..901e15fcce2d 100644 >>>>> --- a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml >>>>> +++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml >>>>> @@ -36,12 +36,16 @@ properties: >>>>> power-domains: >>>>> maxItems: 1 >>>>> + resets: >>>>> + maxItems: 1 >>>>> + >>>>> required: >>>>> - compatible >>>>> - reg >>>>> - interrupts >>>>> - clock-names >>>>> - clocks >>>>> + - resets >>>> >>>> In 2/5 you are calling devm_reset_control_get_optional_exclusive() that's why I expect reset is not really required property. >>> >>> It's optional for the driver for backwards compatibility. But for the >>> devicetree we make it mandatory since it should be included in all new >>> devicetrees. >> >> Generally, we discourage new required properties as that's an ABI >> change. The exception is really when optional was a mistake. That's >> arguably the case here if the h/w always has a reset. > > This device has a reset on ZynqMP and Versal. > > The driver still considers this property optional, so it's not an ABI break. > But I made it required in the schema to help out the folks at AMD when they > get around to upstreaming the Versal devicetree :) Pretty much every IP block has hardware reset wired. It is just user decision if that reset is going to be handled or if low level firmware allows to handle it. The same logic applies to clocks too. >> Unfortunately, there's not a way to distinguish 'required' from >> 'required for new users'. > > I will add a note to the commit message about this situation. Again even new users don't need to have an access to this feature. Not sure what's the right way to go here but in our arm64 based chip reset is provided via firmware interface and I have no issue to say we are expecting reset property to be present but it is up to firmware implementation if a reset is actually happening on HW level. Thanks, Michal
On 1/27/25 12:57, Rob Herring wrote: > On Thu, Jan 23, 2025 at 05:57:41PM -0500, Sean Anderson wrote: >> On 1/23/25 17:45, Rob Herring wrote: >> > On Fri, Jan 17, 2025 at 11:12:15AM -0500, Sean Anderson wrote: >> >> On 1/17/25 02:14, Michal Simek wrote: >> >> > >> >> > >> >> > On 1/16/25 23:55, Sean Anderson wrote: >> >> >> Add a reset to help recover from cancelled operations. >> >> >> >> >> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev> >> >> >> --- >> >> >> >> >> >> Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml | 6 ++++++ >> >> >> 1 file changed, 6 insertions(+) >> >> >> >> >> >> diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml >> >> >> index 04d4d3b4916d..901e15fcce2d 100644 >> >> >> --- a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml >> >> >> +++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml >> >> >> @@ -36,12 +36,16 @@ properties: >> >> >> power-domains: >> >> >> maxItems: 1 >> >> >> + resets: >> >> >> + maxItems: 1 >> >> >> + >> >> >> required: >> >> >> - compatible >> >> >> - reg >> >> >> - interrupts >> >> >> - clock-names >> >> >> - clocks >> >> >> + - resets >> >> > >> >> > In 2/5 you are calling devm_reset_control_get_optional_exclusive() that's why I expect reset is not really required property. >> >> >> >> It's optional for the driver for backwards compatibility. But for the >> >> devicetree we make it mandatory since it should be included in all new >> >> devicetrees. >> > >> > Generally, we discourage new required properties as that's an ABI >> > change. The exception is really when optional was a mistake. That's >> > arguably the case here if the h/w always has a reset. >> >> This device has a reset on ZynqMP and Versal. >> >> The driver still considers this property optional, so it's not an ABI break. >> But I made it required in the schema to help out the folks at AMD when they >> get around to upstreaming the Versal devicetree :) > > Not 'the driver', but 'a driver'. You can't say what *all* drivers do. > If I write a new driver and read the schema, then I can say 'resets is > required so I'll make it required in my new driver'. But then my new > driver doesn't work with an older DT that didn't have resets which was > valid at the time. OK, I'll add a description to this effect. The humans can read that, and the machines won't care. --Sean
diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml index 04d4d3b4916d..901e15fcce2d 100644 --- a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml +++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml @@ -36,12 +36,16 @@ properties: power-domains: maxItems: 1 + resets: + maxItems: 1 + required: - compatible - reg - interrupts - clock-names - clocks + - resets unevaluatedProperties: false @@ -66,6 +70,7 @@ allOf: examples: - | #include <dt-bindings/clock/xlnx-zynqmp-clk.h> + #include <dt-bindings/reset/xlnx-zynqmp-resets.h> soc { #address-cells = <2>; #size-cells = <2>; @@ -76,6 +81,7 @@ examples: clock-names = "ref_clk", "pclk"; interrupts = <0 15 4>; interrupt-parent = <&gic>; + resets = <&zynqmp_reset ZYNQMP_RESET_QSPI>; reg = <0x0 0xff0f0000 0x0 0x1000>, <0x0 0xc0000000 0x0 0x8000000>; };
Add a reset to help recover from cancelled operations. Signed-off-by: Sean Anderson <sean.anderson@linux.dev> --- Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml | 6 ++++++ 1 file changed, 6 insertions(+)