diff mbox series

[v2,3/4] dt-bindings: Add post-init-supplier property

Message ID 20240212213147.489377-4-saravanak@google.com
State New
Headers show
Series Add post-init-supplier binding to improve suspend/resume stability | expand

Commit Message

Saravana Kannan Feb. 12, 2024, 9:31 p.m. UTC
The post-init-supplier property can be used to break a dependency cycle by
marking some supplier(s) as a post device initialization supplier(s). This
allows an OS to do a better job at ordering initialization and
suspend/resume of the devices in a dependency cycle.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 .../bindings/post-init-supplier.yaml          | 101 ++++++++++++++++++
 MAINTAINERS                                   |  13 +--
 2 files changed, 108 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/post-init-supplier.yaml

Comments

Conor Dooley Feb. 14, 2024, 6:48 p.m. UTC | #1
On Mon, Feb 12, 2024 at 01:31:44PM -0800, Saravana Kannan wrote:
> The post-init-supplier property can be used to break a dependency cycle by
> marking some supplier(s) as a post device initialization supplier(s). This
> allows an OS to do a better job at ordering initialization and
> suspend/resume of the devices in a dependency cycle.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  .../bindings/post-init-supplier.yaml          | 101 ++++++++++++++++++
>  MAINTAINERS                                   |  13 +--
>  2 files changed, 108 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/post-init-supplier.yaml
> 
> diff --git a/Documentation/devicetree/bindings/post-init-supplier.yaml b/Documentation/devicetree/bindings/post-init-supplier.yaml
> new file mode 100644
> index 000000000000..aab75b667259
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/post-init-supplier.yaml
> @@ -0,0 +1,101 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2020, Google LLC. All rights reserved.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/post-init-supplier.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Post device initialization supplier
> +
> +maintainers:
> +  - Saravana Kannan <saravanak@google.com>
> +
> +description: |
> +  This property is used to indicate that the device(s) pointed to by the
> +  property are not needed for the initialization of the device that lists this
> +  property.

> This property is meaningful only when pointing to direct suppliers
> +  of a device that are pointed to by other properties in the device.

I don't think this sentence makes sense, or at least it is not easy to
parse. It implies that it can "point to" other properties too - but
that's not the case. It is only valid to "point to" these suppliers.
I'd drop this entirely.

> +
> +  A device can list its suppliers in devicetree using one or more of the
> +  standard devicetree bindings. By default, it would be safe to assume the
> +  supplier device can be initialized before the consumer device is initialized.

"it would be safe to assume" seems odd wording to me - I feel like the
default is stronger than "safe to assume". I'd just drop the "would be
safe to assume and replace with "is assumed".

> +
> +  However, that assumption cannot be made when there are cyclic dependencies
> +  between devices. Since each device is a supplier (directly or indirectly) of
> +  the others in the cycle, there is no guaranteed safe order for initializing
> +  the devices in a cycle. We can try to initialize them in an arbitrary order
> +  and eventually successfully initialize all of them, but that doesn't always
> +  work well.
> +
> +  For example, say,
> +  * The device tree has the following cyclic dependency X -> Y -> Z -> X (where
> +    -> denotes "depends on").
> +  * But X is not needed to fully initialize Z (X might be needed only when a
> +    specific functionality is requested post initialization).
> +
> +  If all the other -> are mandatory initialization dependencies, then trying to
> +  initialize the devices in a loop (or arbitrarily) will always eventually end
> +  up with the devices being initialized in the order Z, Y and X.
> +
> +  However, if Y is an optional supplier for X (where X provides limited
> +  functionality when Y is not initialized and providing its services), then
> +  trying to initialize the devices in a loop (or arbitrarily) could end up with
> +  the devices being initialized in the following order:
> +
> +  * Z, Y and X - All devices provide full functionality
> +  * Z, X and Y - X provides partial functionality
> +  * X, Z and Y - X provides partial functionality
> +
> +  However, we always want to initialize the devices in the order Z, Y and X
> +  since that provides the full functionality without interruptions.
> +
> +  One alternate option that might be suggested is to have the driver for X
> +  notice that Y became available at a later point and adjust the functionality
> +  it provides. However, other userspace applications could have started using X
> +  with the limited functionality before Y was available and it might not be
> +  possible to transparently transition X or the users of X to full
> +  functionality while X is in use.
> +
> +  Similarly, when it comes to suspend (resume) ordering, it's unclear which
> +  device in a dependency cycle needs to be suspended/resumed first and trying
> +  arbitrary orders can result in system crashes or instability.
> +
> +  Explicitly calling out which link in a cycle needs to be broken when
> +  determining the order, simplifies things a lot, improves efficiency, makes
> +  the behavior more deterministic and maximizes the functionality that can be
> +  provided without interruption.
> +
> +  This property is used to provide this additional information between devices
> +  in a cycle by telling which supplier(s) is not needed for initializing the
> +  device that lists this property.
> +
> +  In the example above, Z would list X as a post-init-supplier and the
> +  initialization dependency would become X -> Y -> Z -/-> X. So the best order
> +  to initialize them become clear: Z, Y and then X.

Otherwise, I think this is a great description, describing the use case
well :)

> +
> +select: true
> +properties:
> +  post-init-supplier:
> +    # One or more suppliers can be marked as post initialization supplier
> +    description:
> +      List of phandles to suppliers that are not needed for initializing or
> +      resuming this device.
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +      items:
> +        maxItems: 1

Rob's bot rightfully complains here about invalid syntax. What you
actually want to enforce here is any number of device phandles, but
these phandles all contain only the label and no indices etc, right?

> +
> +examples:
> +  - |
> +    gcc: clock-controller@1000 {
> +        compatible = "vendor,soc4-gcc", "vendor,soc1-gcc";
> +        reg = <0x1000 0x80>;
> +        clocks = <&dispcc 0x1>

This clearly was never tested, Rob's bot warnings aside. You're missing
a ; at EOL here and with the other clock below. 

Cheers,
Conor.

> +        #clock-cells = <1>;
> +        post-init-supplier = <&dispcc>;
> +    };
> +    dispcc: clock-controller@2000 {
> +        compatible = "vendor,soc4-dispcc", "vendor,soc1-dispcc";
> +        reg = <0x2000 0x80>;
> +        clocks = <&gcc 0xdd>
> +        #clock-cells = <1>;
> +    };
Saravana Kannan Feb. 14, 2024, 11:32 p.m. UTC | #2
Hi Conon,

On Wed, Feb 14, 2024 at 10:49 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Mon, Feb 12, 2024 at 01:31:44PM -0800, Saravana Kannan wrote:
> > The post-init-supplier property can be used to break a dependency cycle by
> > marking some supplier(s) as a post device initialization supplier(s). This
> > allows an OS to do a better job at ordering initialization and
> > suspend/resume of the devices in a dependency cycle.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  .../bindings/post-init-supplier.yaml          | 101 ++++++++++++++++++
> >  MAINTAINERS                                   |  13 +--
> >  2 files changed, 108 insertions(+), 6 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/post-init-supplier.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/post-init-supplier.yaml b/Documentation/devicetree/bindings/post-init-supplier.yaml
> > new file mode 100644
> > index 000000000000..aab75b667259
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/post-init-supplier.yaml
> > @@ -0,0 +1,101 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (c) 2020, Google LLC. All rights reserved.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/post-init-supplier.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Post device initialization supplier
> > +
> > +maintainers:
> > +  - Saravana Kannan <saravanak@google.com>
> > +
> > +description: |
> > +  This property is used to indicate that the device(s) pointed to by the
> > +  property are not needed for the initialization of the device that lists this
> > +  property.
>
> > This property is meaningful only when pointing to direct suppliers
> > +  of a device that are pointed to by other properties in the device.
>
> I don't think this sentence makes sense, or at least it is not easy to
> parse. It implies that it can "point to" other properties too

I don't see how this sentence implies this. But open to suggestions on
how to reword it. I don't want to drop this line entirely though
because I'm trying to make it clear that this doesn't make a device
(that's not previously a supplier) into a supplier. It only down
grades an existing supplier to a post device initialization supplier.

> - but
> that's not the case. It is only valid to "point to" these suppliers.
> I'd drop this entirely.

>
> > +
> > +  A device can list its suppliers in devicetree using one or more of the
> > +  standard devicetree bindings. By default, it would be safe to assume the
> > +  supplier device can be initialized before the consumer device is initialized.
>
> "it would be safe to assume" seems odd wording to me - I feel like the
> default is stronger than "safe to assume". I'd just drop the "would be
> safe to assume and replace with "is assumed".

Sounds good.

>
> > +
> > +  However, that assumption cannot be made when there are cyclic dependencies
> > +  between devices. Since each device is a supplier (directly or indirectly) of
> > +  the others in the cycle, there is no guaranteed safe order for initializing
> > +  the devices in a cycle. We can try to initialize them in an arbitrary order
> > +  and eventually successfully initialize all of them, but that doesn't always
> > +  work well.
> > +
> > +  For example, say,
> > +  * The device tree has the following cyclic dependency X -> Y -> Z -> X (where
> > +    -> denotes "depends on").
> > +  * But X is not needed to fully initialize Z (X might be needed only when a
> > +    specific functionality is requested post initialization).
> > +
> > +  If all the other -> are mandatory initialization dependencies, then trying to
> > +  initialize the devices in a loop (or arbitrarily) will always eventually end
> > +  up with the devices being initialized in the order Z, Y and X.
> > +
> > +  However, if Y is an optional supplier for X (where X provides limited
> > +  functionality when Y is not initialized and providing its services), then
> > +  trying to initialize the devices in a loop (or arbitrarily) could end up with
> > +  the devices being initialized in the following order:
> > +
> > +  * Z, Y and X - All devices provide full functionality
> > +  * Z, X and Y - X provides partial functionality
> > +  * X, Z and Y - X provides partial functionality
> > +
> > +  However, we always want to initialize the devices in the order Z, Y and X
> > +  since that provides the full functionality without interruptions.
> > +
> > +  One alternate option that might be suggested is to have the driver for X
> > +  notice that Y became available at a later point and adjust the functionality
> > +  it provides. However, other userspace applications could have started using X
> > +  with the limited functionality before Y was available and it might not be
> > +  possible to transparently transition X or the users of X to full
> > +  functionality while X is in use.
> > +
> > +  Similarly, when it comes to suspend (resume) ordering, it's unclear which
> > +  device in a dependency cycle needs to be suspended/resumed first and trying
> > +  arbitrary orders can result in system crashes or instability.
> > +
> > +  Explicitly calling out which link in a cycle needs to be broken when
> > +  determining the order, simplifies things a lot, improves efficiency, makes
> > +  the behavior more deterministic and maximizes the functionality that can be
> > +  provided without interruption.
> > +
> > +  This property is used to provide this additional information between devices
> > +  in a cycle by telling which supplier(s) is not needed for initializing the
> > +  device that lists this property.
> > +
> > +  In the example above, Z would list X as a post-init-supplier and the
> > +  initialization dependency would become X -> Y -> Z -/-> X. So the best order
> > +  to initialize them become clear: Z, Y and then X.
>
> Otherwise, I think this is a great description, describing the use case
> well :)

Thanks! I always spend more time writing documentation and commit text
than the time I spend writing code.

>
> > +
> > +select: true
> > +properties:
> > +  post-init-supplier:

[Merging your other email here]

> Also, this should likely be pluralised, to match "clocks" "resets"
> "interrupts" etc.

Good point. Done.

> > +    # One or more suppliers can be marked as post initialization supplier
> > +    description:
> > +      List of phandles to suppliers that are not needed for initializing or
> > +      resuming this device.
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +      items:
> > +        maxItems: 1
>
> Rob's bot rightfully complains here about invalid syntax.

I added these two lines based on Rob's feedback. Is the indentation
that's wrong?

Yeah, I'm trying to run the dts checker, but I haven't be able to get
it to work on my end. See my email to Rob on the v1 series about this.

$ make DT_CHECKER_FLAGS=-m dt_binding_check

The best I could get out of it is a bunch of error reports on other
files and then:
...
<snip>/Documentation/devicetree/bindings/post-init-suppliers.yaml:
ignoring, error parsing file
...

I also tried to use DT_SCHEMA_FILES so I can only test this one file,
but that wasn't working either:

$ make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/post-init-suppliers.yaml
or
$ make DT_CHECKER_FLAGS=-m dt_binding_check DT_SCHEMA_FILES=<path to
the .patch file>

Results in this error early on in the output:
...
usage: yamllint [-h] [-] [-c CONFIG_FILE | -d CONFIG_DATA]
[--list-files] [-f {parsable,standard,colored,github,auto}] [-s]
[--no-warnings] [-v] [FILE_OR_DIR ...]
yamllint: error: one of the arguments FILE_OR_DIR - is required
...
/mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-suppliers.yaml:
ignoring, error parsing file
...

> What you
> actually want to enforce here is any number of device phandles, but
> these phandles all contain only the label and no indices etc, right?

Correct.

>
> > +
> > +examples:
> > +  - |
> > +    gcc: clock-controller@1000 {
> > +        compatible = "vendor,soc4-gcc", "vendor,soc1-gcc";
> > +        reg = <0x1000 0x80>;
> > +        clocks = <&dispcc 0x1>
>
> This clearly was never tested, Rob's bot warnings aside. You're missing
> a ; at EOL here and with the other clock below.

Yup. I'm unable to get the test to run.

Thanks,
Saravana
Krzysztof Kozlowski Feb. 17, 2024, 10:27 a.m. UTC | #3
On 15/02/2024 00:32, Saravana Kannan wrote:
> 
> Good point. Done.
> 
>>> +    # One or more suppliers can be marked as post initialization supplier
>>> +    description:
>>> +      List of phandles to suppliers that are not needed for initializing or
>>> +      resuming this device.
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +      items:
>>> +        maxItems: 1
>>
>> Rob's bot rightfully complains here about invalid syntax.
> 
> I added these two lines based on Rob's feedback. Is the indentation
> that's wrong?
> 
> Yeah, I'm trying to run the dts checker, but I haven't be able to get
> it to work on my end. See my email to Rob on the v1 series about this.
> 
> $ make DT_CHECKER_FLAGS=-m dt_binding_check
> 
> The best I could get out of it is a bunch of error reports on other
> files and then:
> ...
> <snip>/Documentation/devicetree/bindings/post-init-suppliers.yaml:
> ignoring, error parsing file
> ...
> 
> I also tried to use DT_SCHEMA_FILES so I can only test this one file,
> but that wasn't working either:

I see the errors immediately during testing, no special arguments needed:

crosc64_dt_binding_check post-init-supplier.yaml
make[1]: Entering directory '/home/krzk/dev/linux/linux/out'
  LINT    Documentation/devicetree/bindings
  DTEX    Documentation/devicetree/bindings/post-init-supplier.example.dts
../Documentation/devicetree/bindings/post-init-supplier.yaml:84:12:
[error] syntax error: mapping values are not allowed here (syntax)
  CHKDT   Documentation/devicetree/bindings/processed-schema.json
../Documentation/devicetree/bindings/post-init-supplier.yaml:84:12:
mapping values are not allowed in this context
make[3]: *** [../Documentation/devicetree/bindings/Makefile:26:
Documentation/devicetree/bindings/post-init-supplier.example.dts] Error 1
make[3]: *** Deleting file
'Documentation/devicetree/bindings/post-init-supplier.example.dts'
make[3]: *** Waiting for unfinished jobs....
../Documentation/devicetree/bindings/post-init-supplier.yaml:84:12:
mapping values are not allowed in this context
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
/home/krzk/dev/linux/linux/Documentation/devicetree/bindings/post-init-supplier.yaml:
ignoring, error parsing file
make[2]: *** [/home/krzk/dev/linux/linux/Makefile:1424:
dt_binding_check] Error 2
make[1]: *** [/home/krzk/dev/linux/linux/Makefile:240: __sub-make] Error 2
make[1]: Leaving directory '/home/krzk/dev/linux/linux/out'
make: *** [Makefile:240: __sub-make] Error 2


https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/

I assume you develop on some older trees, because both next and v6.8-rc1
work... or standard issues: old dtschema, old yamllint.

I am afraid you do it for some old Android kernel... :(

Best regards,
Krzysztof
Saravana Kannan Feb. 21, 2024, 4:07 a.m. UTC | #4
On Sat, Feb 17, 2024 at 2:27 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/02/2024 00:32, Saravana Kannan wrote:
> >
> > Good point. Done.
> >
> >>> +    # One or more suppliers can be marked as post initialization supplier
> >>> +    description:
> >>> +      List of phandles to suppliers that are not needed for initializing or
> >>> +      resuming this device.
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> >>> +      items:
> >>> +        maxItems: 1
> >>
> >> Rob's bot rightfully complains here about invalid syntax.
> >
> > I added these two lines based on Rob's feedback. Is the indentation
> > that's wrong?
> >
> > Yeah, I'm trying to run the dts checker, but I haven't be able to get
> > it to work on my end. See my email to Rob on the v1 series about this.
> >
> > $ make DT_CHECKER_FLAGS=-m dt_binding_check
> >
> > The best I could get out of it is a bunch of error reports on other
> > files and then:
> > ...
> > <snip>/Documentation/devicetree/bindings/post-init-suppliers.yaml:
> > ignoring, error parsing file
> > ...
> >
> > I also tried to use DT_SCHEMA_FILES so I can only test this one file,
> > but that wasn't working either:
>
> I see the errors immediately during testing, no special arguments needed:
>
> crosc64_dt_binding_check post-init-supplier.yaml
> make[1]: Entering directory '/home/krzk/dev/linux/linux/out'
>   LINT    Documentation/devicetree/bindings
>   DTEX    Documentation/devicetree/bindings/post-init-supplier.example.dts
> ../Documentation/devicetree/bindings/post-init-supplier.yaml:84:12:
> [error] syntax error: mapping values are not allowed here (syntax)
>   CHKDT   Documentation/devicetree/bindings/processed-schema.json
> ../Documentation/devicetree/bindings/post-init-supplier.yaml:84:12:
> mapping values are not allowed in this context
> make[3]: *** [../Documentation/devicetree/bindings/Makefile:26:
> Documentation/devicetree/bindings/post-init-supplier.example.dts] Error 1
> make[3]: *** Deleting file
> 'Documentation/devicetree/bindings/post-init-supplier.example.dts'
> make[3]: *** Waiting for unfinished jobs....
> ../Documentation/devicetree/bindings/post-init-supplier.yaml:84:12:
> mapping values are not allowed in this context
>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
> /home/krzk/dev/linux/linux/Documentation/devicetree/bindings/post-init-supplier.yaml:
> ignoring, error parsing file
> make[2]: *** [/home/krzk/dev/linux/linux/Makefile:1424:
> dt_binding_check] Error 2
> make[1]: *** [/home/krzk/dev/linux/linux/Makefile:240: __sub-make] Error 2
> make[1]: Leaving directory '/home/krzk/dev/linux/linux/out'
> make: *** [Makefile:240: __sub-make] Error 2

I think I was just getting overwhelmed with the sea of error logs I
saw (for unrelated files). If I don't use the flags it's way too noisy
and it's not always the first thing that's reported.

This is what I see now and I think I now understand what to look for.

$ make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/post-init-suppliers.yaml
  LINT    Documentation/devicetree/bindings
./Documentation/devicetree/bindings/post-init-suppliers.yaml:84:12:
[error] syntax error: mapping values are not allowed here (syntax)
  CHKDT   Documentation/devicetree/bindings/processed-schema.json
./Documentation/devicetree/bindings/post-init-suppliers.yaml:84:12:
mapping values are not allowed in this context
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
/mnt/android/linus-tree/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml:
ignoring, error in schema: properties
/mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-suppliers.yaml:
ignoring, error parsing file
/mnt/android/linus-tree/Documentation/devicetree/bindings/soc/tegra/nvidia,tegra20-pmc.yaml:
ignoring, error in schema: allOf: 0: then: properties: pinmux
/mnt/android/linus-tree/Documentation/devicetree/bindings/net/lantiq,pef2256.yaml:
ignoring, error in schema: properties: lantiq,data-rate-bps
/mnt/android/linus-tree/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml:
ignoring, error in schema: properties: honeywell,pmin-pascal
/mnt/android/linus-tree/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:
ignoring, error in schema: properties: honeywell,pmax-pascal
  DTEX    Documentation/devicetree/bindings/post-init-suppliers.example.dts
Documentation/devicetree/bindings/post-init-suppliers.yaml:84:12:
mapping values are not allowed in this context
make[2]: *** [Documentation/devicetree/bindings/Makefile:26:
Documentation/devicetree/bindings/post-init-suppliers.example.dts]
Error 1
make[2]: *** Deleting file
'Documentation/devicetree/bindings/post-init-suppliers.example.dts'
make[1]: *** [/mnt/android/linus-tree/Makefile:1432: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

>
>
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
>
> I assume you develop on some older trees, because both next and v6.8-rc1
> work... or standard issues: old dtschema, old yamllint.
>
> I am afraid you do it for some old Android kernel... :(

No, I always develop on Linus's tree and test it on an android kernel
that's behind Linus's tree by a month or so.

My yamllint version is 1.32.0, but until 2 weeks ago the latest
yamllint version was 1.33.0.

And dt-schema is  2022.08.2-5 and I had to revert this from Linus's
tree to get it to work:
b32dcf23a03e dt-bindings: Drop kernel copy of common reserved-memory bindings

Unfortunately, AFAIK, I don't have permissions to change the package
repo, so can't really install a newer version.

Thanks for the tips.

-Saravana



-Saravana
Saravana Kannan Feb. 21, 2024, 4:13 a.m. UTC | #5
On Thu, Feb 15, 2024 at 4:15 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Feb 14, 2024 at 03:32:31PM -0800, Saravana Kannan wrote:
> > Hi Conon,
> >
> > On Wed, Feb 14, 2024 at 10:49 AM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Mon, Feb 12, 2024 at 01:31:44PM -0800, Saravana Kannan wrote:
> > > > The post-init-supplier property can be used to break a dependency cycle by
> > > > marking some supplier(s) as a post device initialization supplier(s). This
> > > > allows an OS to do a better job at ordering initialization and
> > > > suspend/resume of the devices in a dependency cycle.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  .../bindings/post-init-supplier.yaml          | 101 ++++++++++++++++++
> > > >  MAINTAINERS                                   |  13 +--
> > > >  2 files changed, 108 insertions(+), 6 deletions(-)
> > > >  create mode 100644 Documentation/devicetree/bindings/post-init-supplier.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/post-init-supplier.yaml b/Documentation/devicetree/bindings/post-init-supplier.yaml
> > > > new file mode 100644
> > > > index 000000000000..aab75b667259
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/post-init-supplier.yaml
> > > > @@ -0,0 +1,101 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +# Copyright (c) 2020, Google LLC. All rights reserved.
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/post-init-supplier.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Post device initialization supplier
> > > > +
> > > > +maintainers:
> > > > +  - Saravana Kannan <saravanak@google.com>
> > > > +
> > > > +description: |
> > > > +  This property is used to indicate that the device(s) pointed to by the
> > > > +  property are not needed for the initialization of the device that lists this
> > > > +  property.
> > >
> > > > This property is meaningful only when pointing to direct suppliers
> > > > +  of a device that are pointed to by other properties in the device.
> > >
> > > I don't think this sentence makes sense, or at least it is not easy to
> > > parse. It implies that it can "point to" other properties too
> >
> > I don't see how this sentence implies this.
>
> Because, to me, it reads as if you can put extra stuff in here that will
> be ignored if not "pointed to" by another property. The word
> "meaningful" is what implies that you can.
>
> > But open to suggestions on
> > how to reword it. I don't want to drop this line entirely though
> > because I'm trying to make it clear that this doesn't make a device
> > (that's not previously a supplier) into a supplier. It only down
> > grades an existing supplier to a post device initialization supplier.
>
> If you wanna keep it, I would just go for what you said in this
> response - that this property does not make devices into suppliers and
> is only to mark existing suppliers as post-init. I think that rules out
> putting other devices in there.

Sounds good.

> > > - but
> > > that's not the case. It is only valid to "point to" these suppliers.
> > > I'd drop this entirely.
> >
> > >
> > > > +
> > > > +  A device can list its suppliers in devicetree using one or more of the
> > > > +  standard devicetree bindings. By default, it would be safe to assume the
> > > > +  supplier device can be initialized before the consumer device is initialized.
> > >
> > > "it would be safe to assume" seems odd wording to me - I feel like the
> > > default is stronger than "safe to assume". I'd just drop the "would be
> > > safe to assume and replace with "is assumed".
> >
> > Sounds good.
> >
> > >
> > > > +
> > > > +  However, that assumption cannot be made when there are cyclic dependencies
> > > > +  between devices. Since each device is a supplier (directly or indirectly) of
> > > > +  the others in the cycle, there is no guaranteed safe order for initializing
> > > > +  the devices in a cycle. We can try to initialize them in an arbitrary order
> > > > +  and eventually successfully initialize all of them, but that doesn't always
> > > > +  work well.
> > > > +
> > > > +  For example, say,
> > > > +  * The device tree has the following cyclic dependency X -> Y -> Z -> X (where
> > > > +    -> denotes "depends on").
> > > > +  * But X is not needed to fully initialize Z (X might be needed only when a
> > > > +    specific functionality is requested post initialization).
> > > > +
> > > > +  If all the other -> are mandatory initialization dependencies, then trying to
> > > > +  initialize the devices in a loop (or arbitrarily) will always eventually end
> > > > +  up with the devices being initialized in the order Z, Y and X.
> > > > +
> > > > +  However, if Y is an optional supplier for X (where X provides limited
> > > > +  functionality when Y is not initialized and providing its services), then
> > > > +  trying to initialize the devices in a loop (or arbitrarily) could end up with
> > > > +  the devices being initialized in the following order:
> > > > +
> > > > +  * Z, Y and X - All devices provide full functionality
> > > > +  * Z, X and Y - X provides partial functionality
> > > > +  * X, Z and Y - X provides partial functionality
> > > > +
> > > > +  However, we always want to initialize the devices in the order Z, Y and X
> > > > +  since that provides the full functionality without interruptions.
> > > > +
> > > > +  One alternate option that might be suggested is to have the driver for X
> > > > +  notice that Y became available at a later point and adjust the functionality
> > > > +  it provides. However, other userspace applications could have started using X
> > > > +  with the limited functionality before Y was available and it might not be
> > > > +  possible to transparently transition X or the users of X to full
> > > > +  functionality while X is in use.
> > > > +
> > > > +  Similarly, when it comes to suspend (resume) ordering, it's unclear which
> > > > +  device in a dependency cycle needs to be suspended/resumed first and trying
> > > > +  arbitrary orders can result in system crashes or instability.
> > > > +
> > > > +  Explicitly calling out which link in a cycle needs to be broken when
> > > > +  determining the order, simplifies things a lot, improves efficiency, makes
> > > > +  the behavior more deterministic and maximizes the functionality that can be
> > > > +  provided without interruption.
> > > > +
> > > > +  This property is used to provide this additional information between devices
> > > > +  in a cycle by telling which supplier(s) is not needed for initializing the
> > > > +  device that lists this property.
> > > > +
> > > > +  In the example above, Z would list X as a post-init-supplier and the
> > > > +  initialization dependency would become X -> Y -> Z -/-> X. So the best order
> > > > +  to initialize them become clear: Z, Y and then X.
> > >
> > > Otherwise, I think this is a great description, describing the use case
> > > well :)
> >
> > Thanks! I always spend more time writing documentation and commit text
> > than the time I spend writing code.
> >
> > >
> > > > +
> > > > +select: true
> > > > +properties:
> > > > +  post-init-supplier:
> >
> > [Merging your other email here]
> >
> > > Also, this should likely be pluralised, to match "clocks" "resets"
> > > "interrupts" etc.
> >
> > Good point. Done.
> >
> > > > +    # One or more suppliers can be marked as post initialization supplier
> > > > +    description:
> > > > +      List of phandles to suppliers that are not needed for initializing or
> > > > +      resuming this device.
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > > > +      items:
> > > > +        maxItems: 1
> > >
> > > Rob's bot rightfully complains here about invalid syntax.
> >
> > I added these two lines based on Rob's feedback. Is the indentation
> > that's wrong?
>
> Aye, both items: and maxItems: need to lose a level of indent. That
> said, its not actually restricting anything. I fixed it up locally and
> you can put as many elements as you like into each phandle and it does
> not care. Maybe Rob can tell what is going wrong there..

I made that fix and now I'm getting this:
$ make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/post-init-suppliers.yaml
  DTEX    Documentation/devicetree/bindings/post-init-suppliers.example.dts
  LINT    Documentation/devicetree/bindings
  CHKDT   Documentation/devicetree/bindings/processed-schema.json
/mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-suppliers.yaml:
'oneOf' conditional failed, one must be fixed:
        'unevaluatedProperties' is a required property
        'additionalProperties' is a required property
        hint: Either unevaluatedProperties or additionalProperties
must be present
        from schema $id: http://devicetree.org/meta-schemas/core.yaml#
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
/mnt/android/linus-tree/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml:
ignoring, error in schema: properties
/mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-suppliers.yaml:
ignoring, error in schema:
/mnt/android/linus-tree/Documentation/devicetree/bindings/soc/tegra/nvidia,tegra20-pmc.yaml:
ignoring, error in schema: allOf: 0: then: properties: pinmux
/mnt/android/linus-tree/Documentation/devicetree/bindings/net/lantiq,pef2256.yaml:
ignoring, error in schema: properties: lantiq,data-rate-bps
/mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-supplier.yaml:
ignoring, error in schema:
/mnt/android/linus-tree/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml:
ignoring, error in schema: properties: honeywell,pmax-pascal
/mnt/android/linus-tree/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:
ignoring, error in schema: properties: honeywell,pmax-pascal
  DTC_CHK Documentation/devicetree/bindings/post-init-suppliers.example.dtb
Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
/example-0/clock-controller@1000: failed to match any schema with
compatible: ['vendor,soc4-gcc', 'vendor,soc1-gcc']
Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
/example-0/clock-controller@1000: failed to match any schema with
compatible: ['vendor,soc4-gcc', 'vendor,soc1-gcc']
Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
/example-0/clock-controller@2000: failed to match any schema with
compatible: ['vendor,soc4-dispcc', 'vendor,soc1-dispcc']
Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
/example-0/clock-controller@2000: failed to match any schema with
compatible: ['vendor,soc4-dispcc', 'vendor,soc1-dispcc']

But I guess the "oneOf" error is because the yaml is being treated as
a description of a DT node and not a schema?

Rob,

Can you let me know how to move ahead with this? I'll do the fixes
that Conor suggested in v3.

-Saravana

>
> >
> > Yeah, I'm trying to run the dts checker, but I haven't be able to get
> > it to work on my end. See my email to Rob on the v1 series about this.
> >
> > $ make DT_CHECKER_FLAGS=-m dt_binding_check
> >
> > The best I could get out of it is a bunch of error reports on other
> > files and then:
> > ...
> > <snip>/Documentation/devicetree/bindings/post-init-suppliers.yaml:
> > ignoring, error parsing file
> > ...
>
> Yup, that is about right, although you snipped out the actual complaint.
>
> >
> > I also tried to use DT_SCHEMA_FILES so I can only test this one file,
> > but that wasn't working either:
> >
> > $ make DT_CHECKER_FLAGS=-m dt_binding_check
> > DT_SCHEMA_FILES=Documentation/devicetree/bindings/post-init-suppliers.yaml
> > or
> > $ make DT_CHECKER_FLAGS=-m dt_binding_check DT_SCHEMA_FILES=<path to
> > the .patch file>
> >
> > Results in this error early on in the output:
> > ...
> > usage: yamllint [-h] [-] [-c CONFIG_FILE | -d CONFIG_DATA]
> > [--list-files] [-f {parsable,standard,colored,github,auto}] [-s]
> > [--no-warnings] [-v] [FILE_OR_DIR ...]
> > yamllint: error: one of the arguments FILE_OR_DIR - is required
> > ...
> > /mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-suppliers.yaml:
> > ignoring, error parsing file
> > ...
>
> That is part of the actual complaint:
>
> make dt_binding_check W=1 -j 30 DT_SCHEMA_FILES=post-init-supplier.yaml
>   LINT    Documentation/devicetree/bindings
>   DTEX    Documentation/devicetree/bindings/post-init-supplier.example.dts
> Documentation/devicetree/bindings/post-init-supplier.yaml:84:12: mapping values are not allowed here
> make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/post-init-supplier.example.dts] Error 1
> make[2]: *** Deleting file 'Documentation/devicetree/bindings/post-init-supplier.example.dts'
> make[2]: *** Waiting for unfinished jobs....
> ./Documentation/devicetree/bindings/post-init-supplier.yaml:84:12: [error] syntax error: mapping values are not allowed here (syntax)
>   CHKDT   Documentation/devicetree/bindings/processed-schema.json
> ./Documentation/devicetree/bindings/post-init-supplier.yaml:84:12: mapping values are not allowed here
>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
> /stuff/linux-dt/Documentation/devicetree/bindings/post-init-supplier.yaml: ignoring, error parsing file
> make[1]: *** [/stuff/linux-dt/Makefile:1432: dt_binding_check] Error 2
> make: *** [Makefile:240: __sub-make] Error 2
Krzysztof Kozlowski Feb. 21, 2024, 7:21 a.m. UTC | #6
On 21/02/2024 05:07, Saravana Kannan wrote:
>>
>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
>>
>> I assume you develop on some older trees, because both next and v6.8-rc1
>> work... or standard issues: old dtschema, old yamllint.
>>
>> I am afraid you do it for some old Android kernel... :(
> 
> No, I always develop on Linus's tree and test it on an android kernel
> that's behind Linus's tree by a month or so.
> 
> My yamllint version is 1.32.0, but until 2 weeks ago the latest
> yamllint version was 1.33.0.
> 
> And dt-schema is  2022.08.2-5 and I had to revert this from Linus's
> tree to get it to work:
> b32dcf23a03e dt-bindings: Drop kernel copy of common reserved-memory bindings
> 
> Unfortunately, AFAIK, I don't have permissions to change the package
> repo, so can't really install a newer version.

pip packages are by default per user, so why you cannot install updated
dtschema?

Best regards,
Krzysztof
Rob Herring (Arm) Feb. 21, 2024, 2:32 p.m. UTC | #7
On Mon, Feb 12, 2024 at 01:31:44PM -0800, Saravana Kannan wrote:
> The post-init-supplier property can be used to break a dependency cycle by
> marking some supplier(s) as a post device initialization supplier(s). This
> allows an OS to do a better job at ordering initialization and
> suspend/resume of the devices in a dependency cycle.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  .../bindings/post-init-supplier.yaml          | 101 ++++++++++++++++++
>  MAINTAINERS                                   |  13 +--
>  2 files changed, 108 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/post-init-supplier.yaml
> 
> diff --git a/Documentation/devicetree/bindings/post-init-supplier.yaml b/Documentation/devicetree/bindings/post-init-supplier.yaml
> new file mode 100644
> index 000000000000..aab75b667259
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/post-init-supplier.yaml
> @@ -0,0 +1,101 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2020, Google LLC. All rights reserved.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/post-init-supplier.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Post device initialization supplier
> +
> +maintainers:
> +  - Saravana Kannan <saravanak@google.com>
> +
> +description: |
> +  This property is used to indicate that the device(s) pointed to by the
> +  property are not needed for the initialization of the device that lists this
> +  property. This property is meaningful only when pointing to direct suppliers
> +  of a device that are pointed to by other properties in the device.
> +
> +  A device can list its suppliers in devicetree using one or more of the
> +  standard devicetree bindings. By default, it would be safe to assume the
> +  supplier device can be initialized before the consumer device is initialized.
> +
> +  However, that assumption cannot be made when there are cyclic dependencies
> +  between devices. Since each device is a supplier (directly or indirectly) of
> +  the others in the cycle, there is no guaranteed safe order for initializing
> +  the devices in a cycle. We can try to initialize them in an arbitrary order
> +  and eventually successfully initialize all of them, but that doesn't always
> +  work well.
> +
> +  For example, say,
> +  * The device tree has the following cyclic dependency X -> Y -> Z -> X (where
> +    -> denotes "depends on").
> +  * But X is not needed to fully initialize Z (X might be needed only when a
> +    specific functionality is requested post initialization).
> +
> +  If all the other -> are mandatory initialization dependencies, then trying to
> +  initialize the devices in a loop (or arbitrarily) will always eventually end
> +  up with the devices being initialized in the order Z, Y and X.
> +
> +  However, if Y is an optional supplier for X (where X provides limited
> +  functionality when Y is not initialized and providing its services), then
> +  trying to initialize the devices in a loop (or arbitrarily) could end up with
> +  the devices being initialized in the following order:
> +
> +  * Z, Y and X - All devices provide full functionality
> +  * Z, X and Y - X provides partial functionality
> +  * X, Z and Y - X provides partial functionality
> +
> +  However, we always want to initialize the devices in the order Z, Y and X
> +  since that provides the full functionality without interruptions.
> +
> +  One alternate option that might be suggested is to have the driver for X
> +  notice that Y became available at a later point and adjust the functionality
> +  it provides. However, other userspace applications could have started using X
> +  with the limited functionality before Y was available and it might not be
> +  possible to transparently transition X or the users of X to full
> +  functionality while X is in use.
> +
> +  Similarly, when it comes to suspend (resume) ordering, it's unclear which
> +  device in a dependency cycle needs to be suspended/resumed first and trying
> +  arbitrary orders can result in system crashes or instability.
> +
> +  Explicitly calling out which link in a cycle needs to be broken when
> +  determining the order, simplifies things a lot, improves efficiency, makes
> +  the behavior more deterministic and maximizes the functionality that can be
> +  provided without interruption.
> +
> +  This property is used to provide this additional information between devices
> +  in a cycle by telling which supplier(s) is not needed for initializing the
> +  device that lists this property.
> +
> +  In the example above, Z would list X as a post-init-supplier and the
> +  initialization dependency would become X -> Y -> Z -/-> X. So the best order
> +  to initialize them become clear: Z, Y and then X.
> +
> +select: true

blank line

> +properties:
> +  post-init-supplier:

'supply' is already used for regulators. Let's make it 
'post-init-providers'.

Rob
Conor Dooley Feb. 21, 2024, 7:34 p.m. UTC | #8
On Tue, Feb 20, 2024 at 08:13:31PM -0800, Saravana Kannan wrote:
> I made that fix and now I'm getting this:
> $ make DT_CHECKER_FLAGS=-m dt_binding_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/post-init-suppliers.yaml
>   DTEX    Documentation/devicetree/bindings/post-init-suppliers.example.dts
>   LINT    Documentation/devicetree/bindings
>   CHKDT   Documentation/devicetree/bindings/processed-schema.json
> /mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-suppliers.yaml:
> 'oneOf' conditional failed, one must be fixed:
>         'unevaluatedProperties' is a required property
>         'additionalProperties' is a required property
>         hint: Either unevaluatedProperties or additionalProperties
> must be present
>         from schema $id: http://devicetree.org/meta-schemas/core.yaml#
>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
> /mnt/android/linus-tree/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml:
> ignoring, error in schema: properties
> /mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-suppliers.yaml:
> ignoring, error in schema:
> /mnt/android/linus-tree/Documentation/devicetree/bindings/soc/tegra/nvidia,tegra20-pmc.yaml:
> ignoring, error in schema: allOf: 0: then: properties: pinmux
> /mnt/android/linus-tree/Documentation/devicetree/bindings/net/lantiq,pef2256.yaml:
> ignoring, error in schema: properties: lantiq,data-rate-bps
> /mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-supplier.yaml:
> ignoring, error in schema:
> /mnt/android/linus-tree/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml:
> ignoring, error in schema: properties: honeywell,pmax-pascal
> /mnt/android/linus-tree/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:
> ignoring, error in schema: properties: honeywell,pmax-pascal

>   DTC_CHK Documentation/devicetree/bindings/post-init-suppliers.example.dtb
> Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
> /example-0/clock-controller@1000: failed to match any schema with
> compatible: ['vendor,soc4-gcc', 'vendor,soc1-gcc']
> Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
> /example-0/clock-controller@1000: failed to match any schema with
> compatible: ['vendor,soc4-gcc', 'vendor,soc1-gcc']
> Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
> /example-0/clock-controller@2000: failed to match any schema with
> compatible: ['vendor,soc4-dispcc', 'vendor,soc1-dispcc']
> Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
> /example-0/clock-controller@2000: failed to match any schema with
> compatible: ['vendor,soc4-dispcc', 'vendor,soc1-dispcc']

FWIW, I don't see these or the other errors you see above. You really
need to get yourself a newer version of dt-schema, or else avoid
working on this using whatever castrated system google provides you with!

> But I guess the "oneOf" error is because the yaml is being treated as
> a description of a DT node and not a schema?

The oneOf is due to missing "additionalProperties: true" - As far as I
understand you need that regardless of whether this is going into
dt-schema or the kernel.
Saravana Kannan Feb. 21, 2024, 10:47 p.m. UTC | #9
On Wed, Feb 21, 2024 at 11:34 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Tue, Feb 20, 2024 at 08:13:31PM -0800, Saravana Kannan wrote:
> > I made that fix and now I'm getting this:
> > $ make DT_CHECKER_FLAGS=-m dt_binding_check
> > DT_SCHEMA_FILES=Documentation/devicetree/bindings/post-init-suppliers.yaml
> >   DTEX    Documentation/devicetree/bindings/post-init-suppliers.example.dts
> >   LINT    Documentation/devicetree/bindings
> >   CHKDT   Documentation/devicetree/bindings/processed-schema.json
> > /mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-suppliers.yaml:
> > 'oneOf' conditional failed, one must be fixed:
> >         'unevaluatedProperties' is a required property
> >         'additionalProperties' is a required property
> >         hint: Either unevaluatedProperties or additionalProperties
> > must be present
> >         from schema $id: http://devicetree.org/meta-schemas/core.yaml#
> >   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
> > /mnt/android/linus-tree/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml:
> > ignoring, error in schema: properties
> > /mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-suppliers.yaml:
> > ignoring, error in schema:
> > /mnt/android/linus-tree/Documentation/devicetree/bindings/soc/tegra/nvidia,tegra20-pmc.yaml:
> > ignoring, error in schema: allOf: 0: then: properties: pinmux
> > /mnt/android/linus-tree/Documentation/devicetree/bindings/net/lantiq,pef2256.yaml:
> > ignoring, error in schema: properties: lantiq,data-rate-bps
> > /mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-supplier.yaml:
> > ignoring, error in schema:
> > /mnt/android/linus-tree/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml:
> > ignoring, error in schema: properties: honeywell,pmax-pascal
> > /mnt/android/linus-tree/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:
> > ignoring, error in schema: properties: honeywell,pmax-pascal
>
> >   DTC_CHK Documentation/devicetree/bindings/post-init-suppliers.example.dtb
> > Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
> > /example-0/clock-controller@1000: failed to match any schema with
> > compatible: ['vendor,soc4-gcc', 'vendor,soc1-gcc']
> > Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
> > /example-0/clock-controller@1000: failed to match any schema with
> > compatible: ['vendor,soc4-gcc', 'vendor,soc1-gcc']
> > Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
> > /example-0/clock-controller@2000: failed to match any schema with
> > compatible: ['vendor,soc4-dispcc', 'vendor,soc1-dispcc']
> > Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
> > /example-0/clock-controller@2000: failed to match any schema with
> > compatible: ['vendor,soc4-dispcc', 'vendor,soc1-dispcc']
>
> FWIW, I don't see these or the other errors you see above. You really
> need to get yourself a newer version of dt-schema, or else avoid
> working on this using whatever castrated system google provides you with!

Ok, finally found the workaround to updating these packages and the
output is a lot cleaner now.

> > But I guess the "oneOf" error is because the yaml is being treated as
> > a description of a DT node and not a schema?
>
> The oneOf is due to missing "additionalProperties: true" - As far as I
> understand you need that regardless of whether this is going into
> dt-schema or the kernel.

Ok, I added that and the errors go away. I'll send out a v3 and
hopefully Rob can pick it up.

-Saravana
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/post-init-supplier.yaml b/Documentation/devicetree/bindings/post-init-supplier.yaml
new file mode 100644
index 000000000000..aab75b667259
--- /dev/null
+++ b/Documentation/devicetree/bindings/post-init-supplier.yaml
@@ -0,0 +1,101 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2020, Google LLC. All rights reserved.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/post-init-supplier.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Post device initialization supplier
+
+maintainers:
+  - Saravana Kannan <saravanak@google.com>
+
+description: |
+  This property is used to indicate that the device(s) pointed to by the
+  property are not needed for the initialization of the device that lists this
+  property. This property is meaningful only when pointing to direct suppliers
+  of a device that are pointed to by other properties in the device.
+
+  A device can list its suppliers in devicetree using one or more of the
+  standard devicetree bindings. By default, it would be safe to assume the
+  supplier device can be initialized before the consumer device is initialized.
+
+  However, that assumption cannot be made when there are cyclic dependencies
+  between devices. Since each device is a supplier (directly or indirectly) of
+  the others in the cycle, there is no guaranteed safe order for initializing
+  the devices in a cycle. We can try to initialize them in an arbitrary order
+  and eventually successfully initialize all of them, but that doesn't always
+  work well.
+
+  For example, say,
+  * The device tree has the following cyclic dependency X -> Y -> Z -> X (where
+    -> denotes "depends on").
+  * But X is not needed to fully initialize Z (X might be needed only when a
+    specific functionality is requested post initialization).
+
+  If all the other -> are mandatory initialization dependencies, then trying to
+  initialize the devices in a loop (or arbitrarily) will always eventually end
+  up with the devices being initialized in the order Z, Y and X.
+
+  However, if Y is an optional supplier for X (where X provides limited
+  functionality when Y is not initialized and providing its services), then
+  trying to initialize the devices in a loop (or arbitrarily) could end up with
+  the devices being initialized in the following order:
+
+  * Z, Y and X - All devices provide full functionality
+  * Z, X and Y - X provides partial functionality
+  * X, Z and Y - X provides partial functionality
+
+  However, we always want to initialize the devices in the order Z, Y and X
+  since that provides the full functionality without interruptions.
+
+  One alternate option that might be suggested is to have the driver for X
+  notice that Y became available at a later point and adjust the functionality
+  it provides. However, other userspace applications could have started using X
+  with the limited functionality before Y was available and it might not be
+  possible to transparently transition X or the users of X to full
+  functionality while X is in use.
+
+  Similarly, when it comes to suspend (resume) ordering, it's unclear which
+  device in a dependency cycle needs to be suspended/resumed first and trying
+  arbitrary orders can result in system crashes or instability.
+
+  Explicitly calling out which link in a cycle needs to be broken when
+  determining the order, simplifies things a lot, improves efficiency, makes
+  the behavior more deterministic and maximizes the functionality that can be
+  provided without interruption.
+
+  This property is used to provide this additional information between devices
+  in a cycle by telling which supplier(s) is not needed for initializing the
+  device that lists this property.
+
+  In the example above, Z would list X as a post-init-supplier and the
+  initialization dependency would become X -> Y -> Z -/-> X. So the best order
+  to initialize them become clear: Z, Y and then X.
+
+select: true
+properties:
+  post-init-supplier:
+    # One or more suppliers can be marked as post initialization supplier
+    description:
+      List of phandles to suppliers that are not needed for initializing or
+      resuming this device.
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+      items:
+        maxItems: 1
+
+examples:
+  - |
+    gcc: clock-controller@1000 {
+        compatible = "vendor,soc4-gcc", "vendor,soc1-gcc";
+        reg = <0x1000 0x80>;
+        clocks = <&dispcc 0x1>
+        #clock-cells = <1>;
+        post-init-supplier = <&dispcc>;
+    };
+    dispcc: clock-controller@2000 {
+        compatible = "vendor,soc4-dispcc", "vendor,soc1-dispcc";
+        reg = <0x2000 0x80>;
+        clocks = <&gcc 0xdd>
+        #clock-cells = <1>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 3dfe7ea25320..79719af714be 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6055,12 +6055,6 @@  S:	Maintained
 F:	drivers/base/devcoredump.c
 F:	include/linux/devcoredump.h
 
-DEVICE DEPENDENCY HELPER SCRIPT
-M:	Saravana Kannan <saravanak@google.com>
-L:	linux-kernel@vger.kernel.org
-S:	Maintained
-F:	scripts/dev-needs.sh
-
 DEVICE DIRECT ACCESS (DAX)
 M:	Dan Williams <dan.j.williams@intel.com>
 M:	Vishal Verma <vishal.l.verma@intel.com>
@@ -8295,6 +8289,13 @@  F:	include/linux/firewire.h
 F:	include/uapi/linux/firewire*.h
 F:	tools/firewire/
 
+FIRMWARE DEVICE LINK (fw_devlink)
+M:	Saravana Kannan <saravanak@google.com>
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/post-init-supplier.yaml
+F:	scripts/dev-needs.sh
+
 FIRMWARE FRAMEWORK FOR ARMV8-A
 M:	Sudeep Holla <sudeep.holla@arm.com>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)