On Thu, Feb 15, 2024 at 4:15 AM Conor Dooley <conor@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> > > > > --- > > > > .../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@xxxxxxxxxx> > > > > + > > > > +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
