Messages in this thread | | | Date | Wed, 22 Nov 2023 15:42:46 +0100 | From | Dragan Simic <> | Subject | Re: [PATCH v2] docs: dt-bindings: add DTS Coding Style document |
| |
On 2023-11-22 15:34, Rob Herring wrote: > On Wed, Nov 22, 2023 at 1:57 AM Michal Simek <michal.simek@amd.com> > wrote: >> On 11/22/23 09:53, Geert Uytterhoeven wrote: >> > On Wed, Nov 22, 2023 at 9:50 AM Michal Simek <michal.simek@amd.com> wrote: >> >> On 11/22/23 09:29, Dragan Simic wrote: >> >>> On 2023-11-22 09:21, Krzysztof Kozlowski wrote: >> >>>> On 22/11/2023 09:09, Chen-Yu Tsai wrote: >> >>>>> On Wed, Nov 22, 2023 at 4:05 PM Krzysztof Kozlowski >> >>>>> <krzysztof.kozlowski@linaro.org> wrote: >> >>>>>> >> >>>>>> On 21/11/2023 14:50, Rafał Miłecki wrote: >> >>>>>>>> +Order of Properties in Device Node >> >>>>>>>> +---------------------------------- >> >>>>>>>> + >> >>>>>>>> +Following order of properties in device nodes is preferred: >> >>>>>>>> + >> >>>>>>>> +1. compatible >> >>>>>>>> +2. reg >> >>>>>>>> +3. ranges >> >>>>>>>> +4. Standard/common properties (defined by common bindings, e.g. without >> >>>>>>>> + vendor-prefixes) >> >>>>>>>> +5. Vendor-specific properties >> >>>>>>>> +6. status (if applicable) >> >>>>>>>> +7. Child nodes, where each node is preceded with a blank line >> >>>>>>>> + >> >>>>>>>> +The "status" property is by default "okay", thus it can be omitted. >> >>>>>>> >> >>>>>>> I think it would really help to include position of #address-cells and >> >>>>>>> #size-cells here. In some files I saw them above "compatible" that seems >> >>>>>>> unintuitive. Some prefer putting them at end which I think makes sense >> >>>>>>> as they affect children nodes. >> >>>>>>> >> >>>>>>> Whatever you choose it'd be just nice to have things consistent. >> >>>>>> >> >>>>>> This is a standard/common property, thus it goes to (4) above. >> >>>>> >> >>>>> It's probably a mix, but AFAIK a lot of the device trees in tree have >> >>>>> #*-cells after "status". In some cases they are added in the board >> >>>>> .dts files, not the chip/module .dtsi files. >> >>>> >> >>>> Existing DTS is not a good example :) >> >>>> >> >>>>> >> >>>>> +1 that it makes sense at the end as they affect child nodes. >> >>>> >> >>>> I still insist that status must be the last, because: >> >>>> 1. Many SoC nodes have address/size cells but do not have any children >> >>>> (I2C, SPI), so we put useless information at the end. >> >>>> 2. Status should be the final information to say whether the node is >> >>>> ready or is not. I read the node, check properties and then look at the end: >> >>>> a. Lack of status means it is ready. >> >>>> b. status=disabled means device still needs board resources/customization >> >>> >> >>> I agree with the "status" belonging to the very end, because it's both logical >> >>> and much more readable. Also, "status" is expected to be modified in the >> >>> dependent DT files, which makes it kind of volatile and even more deserving to >> >>> be placed last. >> >> >> >> I am just curious if having status property at the end won't affect >> >> execution/boot up time. Not sure how it is done in Linux but in U-Boot at least >> >> (we want to have DTs in sync between Linux and U-Boot) of_find_property is >> >> pretty much big loop over all properties. And status property defined at the end >> >> means going over all of them to find it out to if device is present. >> >> Not sure if Linux works in the same way but at least of_get_property is done in >> >> the same way. >> > >> > As the default is "okay", you have to loop over all properties anyway. >> >> No doubt if you don't define status property that you need to loop >> over all of >> them. We normally describe the whole SOC with pretty much all IPs >> status = >> disabled and then in board file we are changing it to okay based on >> what it is >> actually wired out. >> It means on our systems all nodes have status properties. If you have >> it at >> first you don't need to go over all. > > Order in the source and order in the OS are independent. If checking > status needs to be optimized, then we could just put it first in the > property list or make the state a field in struct device_node. But > provide some data that it matters first.
That's exactly what I plan to do, i.e. to perform some benchmarks before and after, to see does it actually matter to the point where introducing the changes is worth it.
> I've had this idea to randomize the order nodes are processed so > there's no reliance on the DT order. Maybe I need the same on > properties...
| |