| Message ID | 20220723090942.1637676-1-dmitry.baryshkov@linaro.org |
|---|---|
| Headers | show |
| Series | dt-bindings: arm: qcom: define schema, not devices | expand |
On Sat, 23 Jul 2022 at 20:48, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 23/07/2022 11:09, Dmitry Baryshkov wrote: > > Describing each compatible board in DT schema seems wrong to me. It > > means that each new board is incompatible by default, until added to the DT > > schema. s/incompatible/non-valid/ > > The bindings do not document something because it is or it is no > compatible. They document the compatible. Your patch essentially removes > the documentation, so there is no point in having compatibles in board > at all... I do not quite agree here. Please correct me if I'm wrong. Schema defines which DT files are correct and which are not. Which properties are required, which values are valid, etc. So far so good. For the device nodes it declares (or is willing to declare) all possible compatibility strings. There is a sensible number of on-chip IP blocks, external chips, etc. Each and every new chip (or new IP block) we are going to have a yaml file describing its usage. Perfect. For the machine compatibility lists the arm,qcom schema declares which machine compat strings are valid. And this looks like a problem. Now for the DT to be fully valid against DT schema, we have to define its machine compat string. For each and every phone/sbc/evb we have to name it in schema. I think this is an overkill. It feels like we are putting DT information (mapping form machine compat to SoC) into the DT schema. For qcs404 we already have a schema which uses three items: {}, qcom,qcs404-evb, qcom,qcs404. This sounds like a perfect idea to me. We allow any board, created by Qualcomm, Google or any other random vendor, named Foo, Bar or Baz, as long as it declares that it is compatible with qcom,qcs404-evb. To go even further. We now have the qrb5165-rb5.dts, declaring compatible = "qcom,qrb5165-rb5", "qcom,sm8250". If at some point I add a navigation/communication/whatever mezz on top of it. It would make sense to use compatible = "qcom,qrb5165-rb5-navi-comm", "qcom,qrb5165-rb5", "qcom,sm8250". Adding this to the growing list inside arm,qcom.yaml sounds like a nightmare. I can only hope that at some point JSON schema gains postfixItems support (as proposed) to be able to change e.g. arm,qcom.yaml to list just our qcom,something platforms as possible postfixItems for the schema. Regarding having compatibles in board files at all. I think that they are somehow misused nowadays. Instead of declaring that the Dragonboard 845c is compatible with "thundercomm,db845c", "qcom,sdm845-sbc", "96boards,ce-board", "96boards,iot-board", "qcom,sdm845", the DT file declares nearly useless "thundercomm,db845c", "qcom,sdm845". Thus, if we (mostly) use machine compatible array to just define Vendor+device name and the underlying Qualcomm SoC, I propose to leave just a sensible part (SoC) in DT schema, while allowing any string in the first part. > >Adding support for more and more devices would grow this file > > indefinitely. Drop most of individual device-specific compatibility > > strings leaving just list of platforms in place. All entries which > > differ from two-item string array are left inplace.
On 24/07/2022 00:50, Dmitry Baryshkov wrote: > On Sat, 23 Jul 2022 at 20:48, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 23/07/2022 11:09, Dmitry Baryshkov wrote: >>> Describing each compatible board in DT schema seems wrong to me. It >>> means that each new board is incompatible by default, until added to the DT >>> schema. > > s/incompatible/non-valid/ > >> >> The bindings do not document something because it is or it is no >> compatible. They document the compatible. Your patch essentially removes >> the documentation, so there is no point in having compatibles in board >> at all... > > I do not quite agree here. Please correct me if I'm wrong. > Schema defines which DT files are correct and which are not. Which > properties are required, which values are valid, etc. So far so good. Schema is a tool, we create here bindings. The bindings document what you wrote above, plus compatibles and any other pieces mentioned in DT spec. > For the device nodes it declares (or is willing to declare) all > possible compatibility strings. There is a sensible number of on-chip > IP blocks, external chips, etc. Each and every new chip (or new IP > block) we are going to have a yaml file describing its usage. Perfect. > For the machine compatibility lists the arm,qcom schema declares which > machine compat strings are valid. And this looks like a problem. Now > for the DT to be fully valid against DT schema, we have to define its > machine compat string. Although one of goals is to have schema compliance, that is not the reason why we document compatibles. Compatibles were documented long time ago before DT schema came, because the bindings require it. Bindings define the interface between the DTS and software which uses it. SW is kernel, bootloaders, user-space and some more. > For each and every phone/sbc/evb we have to name it in schema. I think > this is an overkill. Qualcomm is rather moderate, nothing big here so definitely it is not an overkill. We almost do not have there SoMs. Just take a pick at Freescale - this is much more complex than Qualcomm, so any changes should start with that. Qualcomm's "complexity" is not a reason to do anything... > It feels like we are putting DT information > (mapping form machine compat to SoC) into the DT schema. No, we are documenting the compatible in bindings. Just like we always did and we always had to. > For qcs404 we already have a schema which uses three items: {}, > qcom,qcs404-evb, qcom,qcs404. This sounds like a perfect idea to me. > We allow any board, created by Qualcomm, Google or any other random > vendor, named Foo, Bar or Baz, as long as it declares that it is > compatible with qcom,qcs404-evb. > > To go even further. We now have the qrb5165-rb5.dts, declaring > compatible = "qcom,qrb5165-rb5", "qcom,sm8250". > If at some point I add a navigation/communication/whatever mezz on top > of it. It would make sense to use compatible = > "qcom,qrb5165-rb5-navi-comm", "qcom,qrb5165-rb5", "qcom,sm8250". > Adding this to the growing list inside arm,qcom.yaml sounds like a > nightmare. I can only hope that at some point JSON schema gains > postfixItems support (as proposed) to be able to change e.g. > arm,qcom.yaml to list just our qcom,something platforms as possible > postfixItems for the schema. Again, Qualcomm complexity is nothing compared to Freescale. :) > > Regarding having compatibles in board files at all. I think that they > are somehow misused nowadays. Instead of declaring that the > Dragonboard 845c is compatible with "thundercomm,db845c", > "qcom,sdm845-sbc", "96boards,ce-board", "96boards,iot-board", > "qcom,sdm845", the DT file declares nearly useless > "thundercomm,db845c", "qcom,sdm845". > > Thus, if we (mostly) use machine compatible array to just define > Vendor+device name and the underlying Qualcomm SoC, I propose to leave > just a sensible part (SoC) in DT schema, while allowing any string in > the first part. No, because you miss then the purpose of bindings - to document the compatible which is the important piece of interface between DTS/bootloader/kernel/other OS/user-space. To summarize, you propose to not document board compatibles. This is not what we want, because then the next step is - let's don't document SoCs. If you do not document it, means anyone can uniliteraly change it, e.g. in kernel DTS, which will break all other users (e.g. user-space or bootloaders) parsing the compatibles. And before you say - no one parses the board compatibles - let me just say that several user-space (embedded/closed) parse them, bootloaders parse them (U-Boot, Google's Chromebooks and others) Best regards, Krzysztof
Hi, On Sat, Jul 23, 2022 at 3:51 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Sat, 23 Jul 2022 at 20:48, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > > > On 23/07/2022 11:09, Dmitry Baryshkov wrote: > > > Describing each compatible board in DT schema seems wrong to me. It > > > means that each new board is incompatible by default, until added to the DT > > > schema. > > s/incompatible/non-valid/ > > > > > The bindings do not document something because it is or it is no > > compatible. They document the compatible. Your patch essentially removes > > the documentation, so there is no point in having compatibles in board > > at all... > > I do not quite agree here. Please correct me if I'm wrong. > Schema defines which DT files are correct and which are not. Which > properties are required, which values are valid, etc. So far so good. > For the device nodes it declares (or is willing to declare) all > possible compatibility strings. There is a sensible number of on-chip > IP blocks, external chips, etc. Each and every new chip (or new IP > block) we are going to have a yaml file describing its usage. Perfect. > For the machine compatibility lists the arm,qcom schema declares which > machine compat strings are valid. And this looks like a problem. Now > for the DT to be fully valid against DT schema, we have to define its > machine compat string. > For each and every phone/sbc/evb we have to name it in schema. I think > this is an overkill. It feels like we are putting DT information > (mapping form machine compat to SoC) into the DT schema. > For qcs404 we already have a schema which uses three items: {}, > qcom,qcs404-evb, qcom,qcs404. This sounds like a perfect idea to me. > We allow any board, created by Qualcomm, Google or any other random > vendor, named Foo, Bar or Baz, as long as it declares that it is > compatible with qcom,qcs404-evb. I assume the above was supposed to be "as long as it declares that it is compatible with qcom-qcs404" since everywhere else you're declaring that only the SoC compatible is important. In any case, I would tend to agree that the most useful thing that the schema here is doing is validating that we didn't have a typo in the SoC field. I already tried to argue that adding every board into this yaml file seemed like extra overhead without a lot of benefit but I finally ceded to do the busy work instead of continuing to argue. As a side note, at one point Stephen Boyd was on a quest to get the "SoC" removed from the top-level compatible and moved to the "SoC" node. I dunno if that's still something he's pursuing. > To go even further. We now have the qrb5165-rb5.dts, declaring > compatible = "qcom,qrb5165-rb5", "qcom,sm8250". > If at some point I add a navigation/communication/whatever mezz on top > of it. It would make sense to use compatible = > "qcom,qrb5165-rb5-navi-comm", "qcom,qrb5165-rb5", "qcom,sm8250". > Adding this to the growing list inside arm,qcom.yaml sounds like a > nightmare. I can only hope that at some point JSON schema gains > postfixItems support (as proposed) to be able to change e.g. > arm,qcom.yaml to list just our qcom,something platforms as possible > postfixItems for the schema. > > Regarding having compatibles in board files at all. I think that they > are somehow misused nowadays. Instead of declaring that the > Dragonboard 845c is compatible with "thundercomm,db845c", > "qcom,sdm845-sbc", "96boards,ce-board", "96boards,iot-board", > "qcom,sdm845", the DT file declares nearly useless > "thundercomm,db845c", "qcom,sdm845". Again a little bit of an aside, but one point I've tried (unsuccessfully) to make in the past is that, at least for the ChromeOS bootloader (and presumably anyone else using a FIT image), the top-level compatible works almost completely opposite of all the rest of the compatibles. For most compatible strings, you start with a known device tree file which declares the compatibles for a peripheral. You then search for a driver that can handle that peripheral. You start with the first string and try to find a driver for that. If you can't, you go onto the second string which is supposed to work, just not as well. Great. The top level compatible string, at least in the case of ChromeOS, is used by the bootloader to pick among all the device tree files it has. That's a critical difference. Let me try to be concrete. Let's say you have two boards that are exactly identical but one has LTE components stuffed and one doesn't (it's WiFi only). It's fine to treat the LTE board as a WiFi-only board but not OK to treat the WiFi-only board as an LTE board (it'll crash). The bootloader knows (through its own means) whether you're on LTE or WiFi-only and is given a pile of device trees. Let's look specifically at the device tree file for the LTE board. One way to look at it is that the dts for the LTE board should have compatibles: compatible = "lte", "wifi-only" The above matches the normal device tree mentality. It says: "hey, if you've got a lte driver for this board then use it; otherwise use the wifi-only driver". However, the above is actually broken for the bootloader use case. The bootloader is trying to pick a device tree and, to the bootloader, the above says "you can use this dts for either an lte board or a wifi-only board". That's bad. If the bootloader picks this device tree for a wifi-only board then the OS will try to initialize lte and things will crash. To go further, if you think about it things actually work fine if the wifi-only device tree says it's compatible with the LTE board. This is why I say it's opposite... ;-) > Thus, if we (mostly) use machine compatible array to just define > Vendor+device name and the underlying Qualcomm SoC, I propose to leave > just a sensible part (SoC) in DT schema, while allowing any string in > the first part. > > > >Adding support for more and more devices would grow this file > > > indefinitely. Drop most of individual device-specific compatibility > > > strings leaving just list of platforms in place. All entries which > > > differ from two-item string array are left inplace. > > -- > With best wishes > Dmitry
On 25/07/2022 18:25, Doug Anderson wrote: > Let's look specifically at the device tree file for the LTE board. One > way to look at it is that the dts for the LTE board should have > compatibles: > compatible = "lte", "wifi-only" > > The above matches the normal device tree mentality. It says: "hey, if > you've got a lte driver for this board then use it; otherwise use the > wifi-only driver". > > However, the above is actually broken for the bootloader use case. The > bootloader is trying to pick a device tree and, to the bootloader, the > above says "you can use this dts for either an lte board or a > wifi-only board". That's bad. If the bootloader picks this device tree > for a wifi-only board then the OS will try to initialize lte and > things will crash. To go further, if you think about it things > actually work fine if the wifi-only device tree says it's compatible > with the LTE board. This is why I say it's opposite... ;-) This is not specific to "bootloaders" but your specific implementation of entire chain. How you described it, you have dependent pieces - user-space must use the same DTB as bootloader chosen, but bootloader makes different choices than user-space. It's perfectly fine to make these choices different, but then user-space should not depend on something which was/was not initialized in bootloader. IOW, if bootloader picked up generic WiFi compatible and user-space will crash if picking up specific comaptible, you have a dependency and user-space should probably bind to modified DTB, where LTE comaptible is removed. Other systems - I would say most of them - are independent, IOW, we try to make kernel and user-space independent of what bootloader did, because we are never sure what bootloader actually did and what DTS it received. Best regards, Krzysztof
Hi, On Mon, Jul 25, 2022 at 9:41 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 25/07/2022 18:25, Doug Anderson wrote: > > Let's look specifically at the device tree file for the LTE board. One > > way to look at it is that the dts for the LTE board should have > > compatibles: > > compatible = "lte", "wifi-only" > > > > The above matches the normal device tree mentality. It says: "hey, if > > you've got a lte driver for this board then use it; otherwise use the > > wifi-only driver". > > > > However, the above is actually broken for the bootloader use case. The > > bootloader is trying to pick a device tree and, to the bootloader, the > > above says "you can use this dts for either an lte board or a > > wifi-only board". That's bad. If the bootloader picks this device tree > > for a wifi-only board then the OS will try to initialize lte and > > things will crash. To go further, if you think about it things > > actually work fine if the wifi-only device tree says it's compatible > > with the LTE board. This is why I say it's opposite... ;-) > > This is not specific to "bootloaders" but your specific implementation > of entire chain. How you described it, you have dependent pieces - > user-space must use the same DTB as bootloader chosen, but bootloader > makes different choices than user-space. It's perfectly fine to make > these choices different, but then user-space should not depend on > something which was/was not initialized in bootloader. I think there's a misunderstanding here. Currently the ChromeOS bootloader doesn't use the device tree to control its flow at all. ...but the ChromeOS bootloader is in charge of picking the device tree to give to the kernel. Specifically I'm not aware of any mechanism in the kernel where you can give it a pile of device tree files and have it pick the right one. I believe that the official ABI says that it's up to the bootloader to provide the device tree to the kernel. This is right out of `Documentation/arm64/booting.rst` A FIT image is, as far as I'm aware, a standard way to bundle a kernel together with many device trees. The idea here is that the bootloader should grab the kernel out and whichever device tree it decides is the right one. It should then boot the kernel and give it the correct device tree. -Doug
On 25/07/2022 18:41, Krzysztof Kozlowski wrote: > On 25/07/2022 18:25, Doug Anderson wrote: >> Let's look specifically at the device tree file for the LTE board. One >> way to look at it is that the dts for the LTE board should have >> compatibles: >> compatible = "lte", "wifi-only" >> >> The above matches the normal device tree mentality. It says: "hey, if >> you've got a lte driver for this board then use it; otherwise use the >> wifi-only driver". >> >> However, the above is actually broken for the bootloader use case. The >> bootloader is trying to pick a device tree and, to the bootloader, the >> above says "you can use this dts for either an lte board or a >> wifi-only board". That's bad. If the bootloader picks this device tree >> for a wifi-only board then the OS will try to initialize lte and >> things will crash. To go further, if you think about it things >> actually work fine if the wifi-only device tree says it's compatible >> with the LTE board. This is why I say it's opposite... ;-) > > This is not specific to "bootloaders" but your specific implementation > of entire chain. How you described it, you have dependent pieces - > user-space must use the same DTB as bootloader chosen, but bootloader > makes different choices than user-space. It's perfectly fine to make > these choices different, but then user-space should not depend on > something which was/was not initialized in bootloader. > > IOW, if bootloader picked up generic WiFi compatible and user-space will > crash if picking up specific comaptible, you have a dependency and > user-space should probably bind to modified DTB, where LTE comaptible is > removed. > > Other systems - I would say most of them - are independent, IOW, we try > to make kernel and user-space independent of what bootloader did, > because we are never sure what bootloader actually did and what DTS it > received. You can BTW compare it nicely to Linux device driver binding. If a driver binds to more generic (WiFi) compatible, it is not allowed to use any features/code related to more specific compatible (LTE). Your case breaks this rule. Bootloader bound to generic (WiFi) compatible, but it passed entire DTB/FDT to kernel/user-space which can then run code for more specific compatible. Although I understand the point the board compatibles by themself provide little help for such use case. Best regards, Krzysztof
On 25/07/2022 18:54, Doug Anderson wrote: > Hi, > > On Mon, Jul 25, 2022 at 9:41 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 25/07/2022 18:25, Doug Anderson wrote: >>> Let's look specifically at the device tree file for the LTE board. One >>> way to look at it is that the dts for the LTE board should have >>> compatibles: >>> compatible = "lte", "wifi-only" >>> >>> The above matches the normal device tree mentality. It says: "hey, if >>> you've got a lte driver for this board then use it; otherwise use the >>> wifi-only driver". >>> >>> However, the above is actually broken for the bootloader use case. The >>> bootloader is trying to pick a device tree and, to the bootloader, the >>> above says "you can use this dts for either an lte board or a >>> wifi-only board". That's bad. If the bootloader picks this device tree >>> for a wifi-only board then the OS will try to initialize lte and >>> things will crash. To go further, if you think about it things >>> actually work fine if the wifi-only device tree says it's compatible >>> with the LTE board. This is why I say it's opposite... ;-) >> >> This is not specific to "bootloaders" but your specific implementation >> of entire chain. How you described it, you have dependent pieces - >> user-space must use the same DTB as bootloader chosen, but bootloader >> makes different choices than user-space. It's perfectly fine to make >> these choices different, but then user-space should not depend on >> something which was/was not initialized in bootloader. > > I think there's a misunderstanding here. > > Currently the ChromeOS bootloader doesn't use the device tree to > control its flow at all. ...but the ChromeOS bootloader is in charge > of picking the device tree to give to the kernel. > > Specifically I'm not aware of any mechanism in the kernel where you > can give it a pile of device tree files and have it pick the right > one. I believe that the official ABI says that it's up to the > bootloader to provide the device tree to the kernel. This is right out > of `Documentation/arm64/booting.rst` > > A FIT image is, as far as I'm aware, a standard way to bundle a kernel > together with many device trees. The idea here is that the bootloader > should grab the kernel out and whichever device tree it decides is the > right one. It should then boot the kernel and give it the correct > device tree. So that's the same case if you had a device called "Foo" (google,foo) with a Qualcomm sdm845 processor (qcom,sdm845) and have a DTS like: "other-company,bar", "qcom,sdm845" Bootloader on Foo sees "bar", ignores it. Then it sees "qcom,sdm845" compatible and is all happy! It loads the DTS and passes to the kernel... You cannot do that... Best regards, Krzysztof
Hi, On Mon, Jul 25, 2022 at 10:08 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 25/07/2022 18:54, Doug Anderson wrote: > > Hi, > > > > On Mon, Jul 25, 2022 at 9:41 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 25/07/2022 18:25, Doug Anderson wrote: > >>> Let's look specifically at the device tree file for the LTE board. One > >>> way to look at it is that the dts for the LTE board should have > >>> compatibles: > >>> compatible = "lte", "wifi-only" > >>> > >>> The above matches the normal device tree mentality. It says: "hey, if > >>> you've got a lte driver for this board then use it; otherwise use the > >>> wifi-only driver". > >>> > >>> However, the above is actually broken for the bootloader use case. The > >>> bootloader is trying to pick a device tree and, to the bootloader, the > >>> above says "you can use this dts for either an lte board or a > >>> wifi-only board". That's bad. If the bootloader picks this device tree > >>> for a wifi-only board then the OS will try to initialize lte and > >>> things will crash. To go further, if you think about it things > >>> actually work fine if the wifi-only device tree says it's compatible > >>> with the LTE board. This is why I say it's opposite... ;-) > >> > >> This is not specific to "bootloaders" but your specific implementation > >> of entire chain. How you described it, you have dependent pieces - > >> user-space must use the same DTB as bootloader chosen, but bootloader > >> makes different choices than user-space. It's perfectly fine to make > >> these choices different, but then user-space should not depend on > >> something which was/was not initialized in bootloader. > > > > I think there's a misunderstanding here. > > > > Currently the ChromeOS bootloader doesn't use the device tree to > > control its flow at all. ...but the ChromeOS bootloader is in charge > > of picking the device tree to give to the kernel. > > > > Specifically I'm not aware of any mechanism in the kernel where you > > can give it a pile of device tree files and have it pick the right > > one. I believe that the official ABI says that it's up to the > > bootloader to provide the device tree to the kernel. This is right out > > of `Documentation/arm64/booting.rst` > > > > A FIT image is, as far as I'm aware, a standard way to bundle a kernel > > together with many device trees. The idea here is that the bootloader > > should grab the kernel out and whichever device tree it decides is the > > right one. It should then boot the kernel and give it the correct > > device tree. > > So that's the same case if you had a device called "Foo" (google,foo) > with a Qualcomm sdm845 processor (qcom,sdm845) and have a DTS like: > "other-company,bar", "qcom,sdm845" > > Bootloader on Foo sees "bar", ignores it. Then it sees "qcom,sdm845" > compatible and is all happy! It loads the DTS and passes to the kernel... > > You cannot do that... The bootloader never falls back to just the SoC name. -Doug
On Mon, 25 Jul 2022 at 19:18, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 24/07/2022 00:50, Dmitry Baryshkov wrote: > > On Sat, 23 Jul 2022 at 20:48, Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 23/07/2022 11:09, Dmitry Baryshkov wrote: > >>> Describing each compatible board in DT schema seems wrong to me. It > >>> means that each new board is incompatible by default, until added to the DT > >>> schema. > > > > s/incompatible/non-valid/ > > > >> > >> The bindings do not document something because it is or it is no > >> compatible. They document the compatible. Your patch essentially removes > >> the documentation, so there is no point in having compatibles in board > >> at all... > > > > I do not quite agree here. Please correct me if I'm wrong. > > Schema defines which DT files are correct and which are not. Which > > properties are required, which values are valid, etc. So far so good. > > Schema is a tool, we create here bindings. The bindings document what > you wrote above, plus compatibles and any other pieces mentioned in DT spec. > > > For the device nodes it declares (or is willing to declare) all > > possible compatibility strings. There is a sensible number of on-chip > > IP blocks, external chips, etc. Each and every new chip (or new IP > > block) we are going to have a yaml file describing its usage. Perfect. > > For the machine compatibility lists the arm,qcom schema declares which > > machine compat strings are valid. And this looks like a problem. Now > > for the DT to be fully valid against DT schema, we have to define its > > machine compat string. > > Although one of goals is to have schema compliance, that is not the > reason why we document compatibles. Compatibles were documented long > time ago before DT schema came, because the bindings require it. > > Bindings define the interface between the DTS and software which uses > it. SW is kernel, bootloaders, user-space and some more. I completely agree here.
On 25/07/2022 19:09, Doug Anderson wrote: > Hi, > > On Mon, Jul 25, 2022 at 10:08 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 25/07/2022 18:54, Doug Anderson wrote: >>> Hi, >>> >>> On Mon, Jul 25, 2022 at 9:41 AM Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>> On 25/07/2022 18:25, Doug Anderson wrote: >>>>> Let's look specifically at the device tree file for the LTE board. One >>>>> way to look at it is that the dts for the LTE board should have >>>>> compatibles: >>>>> compatible = "lte", "wifi-only" >>>>> >>>>> The above matches the normal device tree mentality. It says: "hey, if >>>>> you've got a lte driver for this board then use it; otherwise use the >>>>> wifi-only driver". >>>>> >>>>> However, the above is actually broken for the bootloader use case. The >>>>> bootloader is trying to pick a device tree and, to the bootloader, the >>>>> above says "you can use this dts for either an lte board or a >>>>> wifi-only board". That's bad. If the bootloader picks this device tree >>>>> for a wifi-only board then the OS will try to initialize lte and >>>>> things will crash. To go further, if you think about it things >>>>> actually work fine if the wifi-only device tree says it's compatible >>>>> with the LTE board. This is why I say it's opposite... ;-) >>>> >>>> This is not specific to "bootloaders" but your specific implementation >>>> of entire chain. How you described it, you have dependent pieces - >>>> user-space must use the same DTB as bootloader chosen, but bootloader >>>> makes different choices than user-space. It's perfectly fine to make >>>> these choices different, but then user-space should not depend on >>>> something which was/was not initialized in bootloader. >>> >>> I think there's a misunderstanding here. >>> >>> Currently the ChromeOS bootloader doesn't use the device tree to >>> control its flow at all. ...but the ChromeOS bootloader is in charge >>> of picking the device tree to give to the kernel. >>> >>> Specifically I'm not aware of any mechanism in the kernel where you >>> can give it a pile of device tree files and have it pick the right >>> one. I believe that the official ABI says that it's up to the >>> bootloader to provide the device tree to the kernel. This is right out >>> of `Documentation/arm64/booting.rst` >>> >>> A FIT image is, as far as I'm aware, a standard way to bundle a kernel >>> together with many device trees. The idea here is that the bootloader >>> should grab the kernel out and whichever device tree it decides is the >>> right one. It should then boot the kernel and give it the correct >>> device tree. >> >> So that's the same case if you had a device called "Foo" (google,foo) >> with a Qualcomm sdm845 processor (qcom,sdm845) and have a DTS like: >> "other-company,bar", "qcom,sdm845" >> >> Bootloader on Foo sees "bar", ignores it. Then it sees "qcom,sdm845" >> compatible and is all happy! It loads the DTS and passes to the kernel... >> >> You cannot do that... > > The bootloader never falls back to just the SoC name. There is no "SoC" part of compatible list. Only by convenience we put it that way, but DT spec does not define first compatible to be for platform because they all are[1], therefore following your earlier description - bootloader should load BAR DTS on FOO device just because qcom,sdm845 matches. [1] https://devicetree-specification.readthedocs.io/en/latest/chapter3-devicenodes.html#root-node Best regards, Krzysztof
Hi, On Mon, Jul 25, 2022 at 10:14 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 25/07/2022 19:09, Doug Anderson wrote: > > Hi, > > > > On Mon, Jul 25, 2022 at 10:08 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 25/07/2022 18:54, Doug Anderson wrote: > >>> Hi, > >>> > >>> On Mon, Jul 25, 2022 at 9:41 AM Krzysztof Kozlowski > >>> <krzysztof.kozlowski@linaro.org> wrote: > >>>> > >>>> On 25/07/2022 18:25, Doug Anderson wrote: > >>>>> Let's look specifically at the device tree file for the LTE board. One > >>>>> way to look at it is that the dts for the LTE board should have > >>>>> compatibles: > >>>>> compatible = "lte", "wifi-only" > >>>>> > >>>>> The above matches the normal device tree mentality. It says: "hey, if > >>>>> you've got a lte driver for this board then use it; otherwise use the > >>>>> wifi-only driver". > >>>>> > >>>>> However, the above is actually broken for the bootloader use case. The > >>>>> bootloader is trying to pick a device tree and, to the bootloader, the > >>>>> above says "you can use this dts for either an lte board or a > >>>>> wifi-only board". That's bad. If the bootloader picks this device tree > >>>>> for a wifi-only board then the OS will try to initialize lte and > >>>>> things will crash. To go further, if you think about it things > >>>>> actually work fine if the wifi-only device tree says it's compatible > >>>>> with the LTE board. This is why I say it's opposite... ;-) > >>>> > >>>> This is not specific to "bootloaders" but your specific implementation > >>>> of entire chain. How you described it, you have dependent pieces - > >>>> user-space must use the same DTB as bootloader chosen, but bootloader > >>>> makes different choices than user-space. It's perfectly fine to make > >>>> these choices different, but then user-space should not depend on > >>>> something which was/was not initialized in bootloader. > >>> > >>> I think there's a misunderstanding here. > >>> > >>> Currently the ChromeOS bootloader doesn't use the device tree to > >>> control its flow at all. ...but the ChromeOS bootloader is in charge > >>> of picking the device tree to give to the kernel. > >>> > >>> Specifically I'm not aware of any mechanism in the kernel where you > >>> can give it a pile of device tree files and have it pick the right > >>> one. I believe that the official ABI says that it's up to the > >>> bootloader to provide the device tree to the kernel. This is right out > >>> of `Documentation/arm64/booting.rst` > >>> > >>> A FIT image is, as far as I'm aware, a standard way to bundle a kernel > >>> together with many device trees. The idea here is that the bootloader > >>> should grab the kernel out and whichever device tree it decides is the > >>> right one. It should then boot the kernel and give it the correct > >>> device tree. > >> > >> So that's the same case if you had a device called "Foo" (google,foo) > >> with a Qualcomm sdm845 processor (qcom,sdm845) and have a DTS like: > >> "other-company,bar", "qcom,sdm845" > >> > >> Bootloader on Foo sees "bar", ignores it. Then it sees "qcom,sdm845" > >> compatible and is all happy! It loads the DTS and passes to the kernel... > >> > >> You cannot do that... > > > > The bootloader never falls back to just the SoC name. > > There is no "SoC" part of compatible list. Only by convenience we put it > that way, but DT spec does not define first compatible to be for > platform because they all are[1], therefore following your earlier > description - bootloader should load BAR DTS on FOO device just because > qcom,sdm845 matches. As documented in "Documentation/arm/google/chromebook-boot-flow.rst", the bootloader creates a match list to pick a device tree file. It never creates a match list that has just the SoC. It always picks based on the board name and never falls back to just the SoC name. -Doug
On 25/07/2022 19:13, Dmitry Baryshkov wrote: > On Mon, 25 Jul 2022 at 19:18, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 24/07/2022 00:50, Dmitry Baryshkov wrote: >>> On Sat, 23 Jul 2022 at 20:48, Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>> On 23/07/2022 11:09, Dmitry Baryshkov wrote: >>>>> Describing each compatible board in DT schema seems wrong to me. It >>>>> means that each new board is incompatible by default, until added to the DT >>>>> schema. >>> >>> s/incompatible/non-valid/ >>> >>>> >>>> The bindings do not document something because it is or it is no >>>> compatible. They document the compatible. Your patch essentially removes >>>> the documentation, so there is no point in having compatibles in board >>>> at all... >>> >>> I do not quite agree here. Please correct me if I'm wrong. >>> Schema defines which DT files are correct and which are not. Which >>> properties are required, which values are valid, etc. So far so good. >> >> Schema is a tool, we create here bindings. The bindings document what >> you wrote above, plus compatibles and any other pieces mentioned in DT spec. >> >>> For the device nodes it declares (or is willing to declare) all >>> possible compatibility strings. There is a sensible number of on-chip >>> IP blocks, external chips, etc. Each and every new chip (or new IP >>> block) we are going to have a yaml file describing its usage. Perfect. >>> For the machine compatibility lists the arm,qcom schema declares which >>> machine compat strings are valid. And this looks like a problem. Now >>> for the DT to be fully valid against DT schema, we have to define its >>> machine compat string. >> >> Although one of goals is to have schema compliance, that is not the >> reason why we document compatibles. Compatibles were documented long >> time ago before DT schema came, because the bindings require it. >> >> Bindings define the interface between the DTS and software which uses >> it. SW is kernel, bootloaders, user-space and some more. > > I completely agree here. > > From the point of HW/SW interfaces maybe the following compat lists > would make more sense: > - "xiaomi,msm8996-phone", "qcom,msm8996" > - "qcom,qrb5165-iot-platform", "qcom,sm8250" > - "qcom,sda845-iot-platform", "qcom,sdm845" > - "inforce,sda660-sbc", "qcom,sda660" > > etc. IOW, describing the kind of the device, not the exact name/model of it. With a specific compatible (covered by pattern), this could work, although not sure what would be the benefit. Other components, like some user-space or bootloader might still need the specific compatible. And if anyone needs a specific compatible, it means it should be documented. >> To summarize, you propose to not document board compatibles. This is not >> what we want, because then the next step is - let's don't document SoCs. >> If you do not document it, means anyone can uniliteraly change it, e.g. >> in kernel DTS, which will break all other users (e.g. user-space or >> bootloaders) parsing the compatibles. And before you say - no one parses >> the board compatibles - let me just say that several user-space >> (embedded/closed) parse them, bootloaders parse them (U-Boot, Google's >> Chromebooks and others) > > Yes, I know. And in the case of e.g. Google ChromeOS bootloader it > might make sense to declare compatibles using patterns rather than > enumeration. Patterns would simplify here a lot but also would allow (thus not validate) incorrect combinations. Consider Ssytem-on-Modules: oneOf: - items: - {} - foo,som-msm8996 - qcom,msm8996 - items: - {} - qcom,msm8996 what stops anyone to have a DTS: "xiaomi,msm8996-phone", "foo,som-msm8996", "qcom,msm8996" ? even though it does not contain that SoM from company Foo? For DT schema it would be a perfectly valid combination, even though it would not make any sense. We document valid machines/compatibles as documentation of the interface but the schema also allows us - with that documentation - to actually check if DTS is correct. One more use case are the the qcom,board-id and msm-id properties. They might once be restricted to specific board compatibles, because maybe only the closed-bootloader platforms need them. We don't want these properties in general, thus we might decide to prohibit to use them in open platforms (e.g. using open bootloader). Without board compatibles we won't be able to do it. > Anyway, thank you for your comments, let's continue with documenting > all the devices until something changes. > Best regards, Krzysztof
On 25/07/2022 19:22, Doug Anderson wrote: >>>> You cannot do that... >>> >>> The bootloader never falls back to just the SoC name. >> >> There is no "SoC" part of compatible list. Only by convenience we put it >> that way, but DT spec does not define first compatible to be for >> platform because they all are[1], therefore following your earlier >> description - bootloader should load BAR DTS on FOO device just because >> qcom,sdm845 matches. > > As documented in "Documentation/arm/google/chromebook-boot-flow.rst", > the bootloader creates a match list to pick a device tree file. It > never creates a match list that has just the SoC. It always picks > based on the board name and never falls back to just the SoC name. So this means you embedded some custom-interpretation of board compatibles in bootloader. What stops you to customize it even more, that the bootloader must always pick the most specific compatible? IOW, it cannot load DTB from more generic compatible (just like it should not load qcom,sdm845 DTS)? I understand the limitation of board compatibles for your case, but I still believe it is wrong usage of it. If the usage - by principle - was correct, then you would not need to add the restriction you mentioned above ("never creates a match list that has just the SoC"). Because when Linux drivers match, they do not have such restriction... Best regards, Krzysztof
On Sat, Jul 23, 2022 at 4:51 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Sat, 23 Jul 2022 at 20:48, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > > > On 23/07/2022 11:09, Dmitry Baryshkov wrote: > > > Describing each compatible board in DT schema seems wrong to me. It > > > means that each new board is incompatible by default, until added to the DT > > > schema. > > s/incompatible/non-valid/ > > > > > The bindings do not document something because it is or it is no > > compatible. They document the compatible. Your patch essentially removes > > the documentation, so there is no point in having compatibles in board > > at all... > > I do not quite agree here. Please correct me if I'm wrong. > Schema defines which DT files are correct and which are not. Which > properties are required, which values are valid, etc. So far so good. > For the device nodes it declares (or is willing to declare) all > possible compatibility strings. There is a sensible number of on-chip > IP blocks, external chips, etc. Each and every new chip (or new IP > block) we are going to have a yaml file describing its usage. Perfect. > For the machine compatibility lists the arm,qcom schema declares which > machine compat strings are valid. And this looks like a problem. Now > for the DT to be fully valid against DT schema, we have to define its > machine compat string. If we don't document all the compatible strings, how do we know if there are name collisions (same compatible string used for 2 different boards)? I'm not in favor of not documenting some compatibles. I'd be more inclined to allow invalid combinations if it reduced duplicating compatible strings than that. > For each and every phone/sbc/evb we have to name it in schema. I think > this is an overkill. It feels like we are putting DT information > (mapping form machine compat to SoC) into the DT schema. > For qcs404 we already have a schema which uses three items: {}, > qcom,qcs404-evb, qcom,qcs404. This sounds like a perfect idea to me. > We allow any board, created by Qualcomm, Google or any other random > vendor, named Foo, Bar or Baz, as long as it declares that it is > compatible with qcom,qcs404-evb. > > To go even further. We now have the qrb5165-rb5.dts, declaring > compatible = "qcom,qrb5165-rb5", "qcom,sm8250". > If at some point I add a navigation/communication/whatever mezz on top > of it. It would make sense to use compatible = > "qcom,qrb5165-rb5-navi-comm", "qcom,qrb5165-rb5", "qcom,sm8250". > Adding this to the growing list inside arm,qcom.yaml sounds like a > nightmare. The combination of SoMs and/or add-on boards is not a QCom specific issue. So if there's going to be any change for how we do things, it needs to be for everyone. > I can only hope that at some point JSON schema gains > postfixItems support (as proposed) to be able to change e.g. > arm,qcom.yaml to list just our qcom,something platforms as possible > postfixItems for the schema. I'm not really sure we'd want to support that. I'm not thrilled that 'items' has changed to 'prefixItems' in newer versions. The name is confusing for how we use 'items'. So my current thought is we'll stick with 'items' even if we replace it with 'prefixItems' in the processed schema. Rob
Hi, On Mon, Jul 25, 2022 at 10:29 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 25/07/2022 19:22, Doug Anderson wrote: > >>>> You cannot do that... > >>> > >>> The bootloader never falls back to just the SoC name. > >> > >> There is no "SoC" part of compatible list. Only by convenience we put it > >> that way, but DT spec does not define first compatible to be for > >> platform because they all are[1], therefore following your earlier > >> description - bootloader should load BAR DTS on FOO device just because > >> qcom,sdm845 matches. > > > > As documented in "Documentation/arm/google/chromebook-boot-flow.rst", > > the bootloader creates a match list to pick a device tree file. It > > never creates a match list that has just the SoC. It always picks > > based on the board name and never falls back to just the SoC name. > > So this means you embedded some custom-interpretation of board > compatibles in bootloader. What stops you to customize it even more, > that the bootloader must always pick the most specific compatible? IOW, > it cannot load DTB from more generic compatible (just like it should not > load qcom,sdm845 DTS)? > > I understand the limitation of board compatibles for your case, but I > still believe it is wrong usage of it. > If the usage - by principle - was correct, then you would not need to > add the restriction you mentioned above ("never creates a match list > that has just the SoC"). > > Because when Linux drivers match, they do not have such restriction... I think we're essentially re-hashing a previous discussion that led me writing the documentation of how depthcharge boots. It's probably not worth rehashing. Depthcharge isn't going to change unless we find a compelling way to solve all the use cases we described last time we talked about this. I honestly think the "backwardness" stems from the fact that in the normal case we start with a dts and pick a driver and in the FIT image / bootloader case we are picking a dts. Those are fundamentally different needs. -Doug