| Message ID | 20220712021144.7068-2-william.zhang@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm: bcmbca: Update BCM4908 dts for BCMBCA merge | expand |
On 12/07/2022 04:11, William Zhang wrote: > Merge BCM4908 SoC device tree description into BCMBCA and combined > all BCM4908 chip variants into the same BCM4908 chip family item. Merge means you combine some entries, so I would expect to see the removal here as well. > > Each compatible string represent the whole chip family. The board > variants and chip varints go into the first and second enum in the > compatible string item list. > > Signed-off-by: William Zhang <william.zhang@broadcom.com> > --- > > .../bindings/arm/bcm/brcm,bcmbca.yaml | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/bcm/brcm,bcmbca.yaml b/Documentation/devicetree/bindings/arm/bcm/brcm,bcmbca.yaml > index d9dc4f22f4a5..906c3e1de372 100644 > --- a/Documentation/devicetree/bindings/arm/bcm/brcm,bcmbca.yaml > +++ b/Documentation/devicetree/bindings/arm/bcm/brcm,bcmbca.yaml > @@ -28,6 +28,23 @@ properties: > - const: brcm,bcm47622 > - const: brcm,bcmbca > > + - description: BCM4908 Family based boards > + items: > + - enum: > + # BCM4908 SoC based boards > + - brcm,bcm94908 > + - asus,gt-ac5300 > + - netgear,raxe500 > + # BCM4906 SoC based boards > + - brcm,bcm94906 > + - netgear,r8000p > + - tplink,archer-c2300-v1 > + - enum: > + - brcm,bcm4908 > + - brcm,bcm4906 > + - brcm,bcm49408 This is wrong. brcm,bcm94908 followed by brcm,bcm4906 does not look like valid list of compatibles. > + - const: brcm,bcmbca > + > - description: BCM4912 based boards > items: > - enum: Best regards, Krzysztof
On 7/12/22 00:45, Krzysztof Kozlowski wrote: > On 12/07/2022 04:11, William Zhang wrote: >> Merge BCM4908 SoC device tree description into BCMBCA and combined >> all BCM4908 chip variants into the same BCM4908 chip family item. > > Merge means you combine some entries, so I would expect to see the > removal here as well. > Will combine with the removal patch as you pointed out >> >> Each compatible string represent the whole chip family. The board >> variants and chip varints go into the first and second enum in the >> compatible string item list. >> >> Signed-off-by: William Zhang <william.zhang@broadcom.com> >> --- >> >> .../bindings/arm/bcm/brcm,bcmbca.yaml | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/bcm/brcm,bcmbca.yaml b/Documentation/devicetree/bindings/arm/bcm/brcm,bcmbca.yaml >> index d9dc4f22f4a5..906c3e1de372 100644 >> --- a/Documentation/devicetree/bindings/arm/bcm/brcm,bcmbca.yaml >> +++ b/Documentation/devicetree/bindings/arm/bcm/brcm,bcmbca.yaml >> @@ -28,6 +28,23 @@ properties: >> - const: brcm,bcm47622 >> - const: brcm,bcmbca >> >> + - description: BCM4908 Family based boards >> + items: >> + - enum: >> + # BCM4908 SoC based boards >> + - brcm,bcm94908 >> + - asus,gt-ac5300 >> + - netgear,raxe500 >> + # BCM4906 SoC based boards >> + - brcm,bcm94906 >> + - netgear,r8000p >> + - tplink,archer-c2300-v1 >> + - enum: >> + - brcm,bcm4908 >> + - brcm,bcm4906 >> + - brcm,bcm49408 > > This is wrong. brcm,bcm94908 followed by brcm,bcm4906 does not look > like valid list of compatibles. > For 4908 board variant, it will need to be followed by 4908 chip. Sorry for the basic question but is there any requirement to enforce this kind of rule? I would assume dts writer know what he/she is doing and select the right combination. >> + - const: brcm,bcmbca >> + >> - description: BCM4912 based boards >> items: >> - enum: > > > Best regards, > Krzysztof
On 12/07/2022 19:37, William Zhang wrote: >>> + - description: BCM4908 Family based boards >>> + items: >>> + - enum: >>> + # BCM4908 SoC based boards >>> + - brcm,bcm94908 >>> + - asus,gt-ac5300 >>> + - netgear,raxe500 >>> + # BCM4906 SoC based boards >>> + - brcm,bcm94906 >>> + - netgear,r8000p >>> + - tplink,archer-c2300-v1 >>> + - enum: >>> + - brcm,bcm4908 >>> + - brcm,bcm4906 >>> + - brcm,bcm49408 >> >> This is wrong. brcm,bcm94908 followed by brcm,bcm4906 does not look >> like valid list of compatibles. >> > For 4908 board variant, it will need to be followed by 4908 chip. Sorry > for the basic question but is there any requirement to enforce this kind > of rule? I would assume dts writer know what he/she is doing and select > the right combination. The entire point of DT schema is to validate DTS. Combination like above prevents that goal. Best regards, Krzysztof
On 7/12/22 11:18, Krzysztof Kozlowski wrote: > On 12/07/2022 19:37, William Zhang wrote: >>>> + - description: BCM4908 Family based boards >>>> + items: >>>> + - enum: >>>> + # BCM4908 SoC based boards >>>> + - brcm,bcm94908 >>>> + - asus,gt-ac5300 >>>> + - netgear,raxe500 >>>> + # BCM4906 SoC based boards >>>> + - brcm,bcm94906 >>>> + - netgear,r8000p >>>> + - tplink,archer-c2300-v1 >>>> + - enum: >>>> + - brcm,bcm4908 >>>> + - brcm,bcm4906 >>>> + - brcm,bcm49408 >>> >>> This is wrong. brcm,bcm94908 followed by brcm,bcm4906 does not look >>> like valid list of compatibles. >>> >> For 4908 board variant, it will need to be followed by 4908 chip. Sorry >> for the basic question but is there any requirement to enforce this kind >> of rule? I would assume dts writer know what he/she is doing and select >> the right combination. > > The entire point of DT schema is to validate DTS. Combination like above > prevents that goal. > > Best regards, > Krzysztof Understand the DT schema purpose. But items property allows multiple enums in the list which gives a lot of flexibility but make it hard to validate. I am not familiar with DT schema, is there any directive to specify one enum value depending on another so dts validation tool can report error if combination is wrong? This is our preferred format of all bcmbca compatible string especially when we could have more than 10 chip variants for the same chip family and we really want to work on the chip family id. We will make sure they are in the right combination in our own patch and patch from other contributors. Would this work? If not, I will probably have to revert the change of 4908(maybe append brcm,bcmbca as this chip belongs to the same bca group) and use "enum board variant", "const main chip id", "brcm,bca" for all other chips as our secondary choice.
On 2022-07-13 02:57, William Zhang wrote: > On 7/12/22 11:18, Krzysztof Kozlowski wrote: >> On 12/07/2022 19:37, William Zhang wrote: >>>>> + - description: BCM4908 Family based boards >>>>> + items: >>>>> + - enum: >>>>> + # BCM4908 SoC based boards >>>>> + - brcm,bcm94908 >>>>> + - asus,gt-ac5300 >>>>> + - netgear,raxe500 >>>>> + # BCM4906 SoC based boards >>>>> + - brcm,bcm94906 >>>>> + - netgear,r8000p >>>>> + - tplink,archer-c2300-v1 >>>>> + - enum: >>>>> + - brcm,bcm4908 >>>>> + - brcm,bcm4906 >>>>> + - brcm,bcm49408 >>>> >>>> This is wrong. brcm,bcm94908 followed by brcm,bcm4906 does not look >>>> like valid list of compatibles. >>>> >>> For 4908 board variant, it will need to be followed by 4908 chip. >>> Sorry >>> for the basic question but is there any requirement to enforce this >>> kind >>> of rule? I would assume dts writer know what he/she is doing and >>> select >>> the right combination. >> >> The entire point of DT schema is to validate DTS. Combination like >> above >> prevents that goal. >> >> Best regards, >> Krzysztof > Understand the DT schema purpose. But items property allows multiple > enums in the list which gives a lot of flexibility but make it hard to > validate. I am not familiar with DT schema, is there any directive to > specify one enum value depending on another so dts validation tool can > report error if combination is wrong? > > This is our preferred format of all bcmbca compatible string > especially when we could have more than 10 chip variants for the same > chip family and we really want to work on the chip family id. We will > make sure they are in the right combination in our own patch and patch > from other contributors. Would this work? If not, I will probably have > to revert the change of 4908(maybe append brcm,bcmbca as this chip > belongs to the same bca group) and use "enum board variant", "const > main chip id", "brcm,bca" for all other chips as our secondary choice. I'm not sure why I didn't even receive 1/3 and half of discussion e-mails. You can't just put all strings into a single bag and allow mixing them in any combos. Please check how it's properly handled in the current existing binding: Documentation/devicetree/bindings/arm/bcm/brcm,bcm4908.yaml Above binding enforces that non-matching compatible strings are not used together.
On 2022-07-13 12:50, Rafał Miłecki wrote: > On 2022-07-13 02:57, William Zhang wrote: >> On 7/12/22 11:18, Krzysztof Kozlowski wrote: >>> On 12/07/2022 19:37, William Zhang wrote: >>>>>> + - description: BCM4908 Family based boards >>>>>> + items: >>>>>> + - enum: >>>>>> + # BCM4908 SoC based boards >>>>>> + - brcm,bcm94908 >>>>>> + - asus,gt-ac5300 >>>>>> + - netgear,raxe500 >>>>>> + # BCM4906 SoC based boards >>>>>> + - brcm,bcm94906 >>>>>> + - netgear,r8000p >>>>>> + - tplink,archer-c2300-v1 >>>>>> + - enum: >>>>>> + - brcm,bcm4908 >>>>>> + - brcm,bcm4906 >>>>>> + - brcm,bcm49408 >>>>> >>>>> This is wrong. brcm,bcm94908 followed by brcm,bcm4906 does not >>>>> look >>>>> like valid list of compatibles. >>>>> >>>> For 4908 board variant, it will need to be followed by 4908 chip. >>>> Sorry >>>> for the basic question but is there any requirement to enforce this >>>> kind >>>> of rule? I would assume dts writer know what he/she is doing and >>>> select >>>> the right combination. >>> >>> The entire point of DT schema is to validate DTS. Combination like >>> above >>> prevents that goal. >>> >>> Best regards, >>> Krzysztof >> Understand the DT schema purpose. But items property allows multiple >> enums in the list which gives a lot of flexibility but make it hard to >> validate. I am not familiar with DT schema, is there any directive to >> specify one enum value depending on another so dts validation tool can >> report error if combination is wrong? >> >> This is our preferred format of all bcmbca compatible string >> especially when we could have more than 10 chip variants for the same >> chip family and we really want to work on the chip family id. We will >> make sure they are in the right combination in our own patch and patch >> from other contributors. Would this work? If not, I will probably have >> to revert the change of 4908(maybe append brcm,bcmbca as this chip >> belongs to the same bca group) and use "enum board variant", "const >> main chip id", "brcm,bca" for all other chips as our secondary choice. > > I'm not sure why I didn't even receive 1/3 and half of discussion > e-mails. > > You can't just put all strings into a single bag and allow mixing them > in any combos. Please check how it's properly handled in the current > existing binding: > Documentation/devicetree/bindings/arm/bcm/brcm,bcm4908.yaml > > Above binding enforces that non-matching compatible strings are not > used > together. I just noticed you're actually removing brcm,bcm4908.yaml in the 2/3 so you must be aware of that file. So you see a cleanly working binding in the brcm,bcm4908.yaml but instead copying it you decided to wrote your own one from scratch. Incorrectly. This smells of NIH (not invented here). Please just use that binding I wrote and move if it needed.
On 7/13/22 03:50, Rafał Miłecki wrote: > On 2022-07-13 02:57, William Zhang wrote: >> On 7/12/22 11:18, Krzysztof Kozlowski wrote: >>> On 12/07/2022 19:37, William Zhang wrote: >>>>>> + - description: BCM4908 Family based boards >>>>>> + items: >>>>>> + - enum: >>>>>> + # BCM4908 SoC based boards >>>>>> + - brcm,bcm94908 >>>>>> + - asus,gt-ac5300 >>>>>> + - netgear,raxe500 >>>>>> + # BCM4906 SoC based boards >>>>>> + - brcm,bcm94906 >>>>>> + - netgear,r8000p >>>>>> + - tplink,archer-c2300-v1 >>>>>> + - enum: >>>>>> + - brcm,bcm4908 >>>>>> + - brcm,bcm4906 >>>>>> + - brcm,bcm49408 >>>>> >>>>> This is wrong. brcm,bcm94908 followed by brcm,bcm4906 does not look >>>>> like valid list of compatibles. >>>>> >>>> For 4908 board variant, it will need to be followed by 4908 chip. Sorry >>>> for the basic question but is there any requirement to enforce this >>>> kind >>>> of rule? I would assume dts writer know what he/she is doing and >>>> select >>>> the right combination. >>> >>> The entire point of DT schema is to validate DTS. Combination like above >>> prevents that goal. >>> >>> Best regards, >>> Krzysztof >> Understand the DT schema purpose. But items property allows multiple >> enums in the list which gives a lot of flexibility but make it hard to >> validate. I am not familiar with DT schema, is there any directive to >> specify one enum value depending on another so dts validation tool can >> report error if combination is wrong? >> >> This is our preferred format of all bcmbca compatible string >> especially when we could have more than 10 chip variants for the same >> chip family and we really want to work on the chip family id. We will >> make sure they are in the right combination in our own patch and patch >> from other contributors. Would this work? If not, I will probably have >> to revert the change of 4908(maybe append brcm,bcmbca as this chip >> belongs to the same bca group) and use "enum board variant", "const >> main chip id", "brcm,bca" for all other chips as our secondary choice. > > I'm not sure why I didn't even receive 1/3 and half of discussion > e-mails. You are copied on all 4 emails (including cover letter).
Hi Rafal, On 7/13/22 03:58, Rafał Miłecki wrote: > On 2022-07-13 12:50, Rafał Miłecki wrote: >> On 2022-07-13 02:57, William Zhang wrote: >>> On 7/12/22 11:18, Krzysztof Kozlowski wrote: >>>> On 12/07/2022 19:37, William Zhang wrote: >>>>>>> + - description: BCM4908 Family based boards >>>>>>> + items: >>>>>>> + - enum: >>>>>>> + # BCM4908 SoC based boards >>>>>>> + - brcm,bcm94908 >>>>>>> + - asus,gt-ac5300 >>>>>>> + - netgear,raxe500 >>>>>>> + # BCM4906 SoC based boards >>>>>>> + - brcm,bcm94906 >>>>>>> + - netgear,r8000p >>>>>>> + - tplink,archer-c2300-v1 >>>>>>> + - enum: >>>>>>> + - brcm,bcm4908 >>>>>>> + - brcm,bcm4906 >>>>>>> + - brcm,bcm49408 >>>>>> >>>>>> This is wrong. brcm,bcm94908 followed by brcm,bcm4906 does not look >>>>>> like valid list of compatibles. >>>>>> >>>>> For 4908 board variant, it will need to be followed by 4908 chip. >>>>> Sorry >>>>> for the basic question but is there any requirement to enforce this >>>>> kind >>>>> of rule? I would assume dts writer know what he/she is doing and >>>>> select >>>>> the right combination. >>>> >>>> The entire point of DT schema is to validate DTS. Combination like >>>> above >>>> prevents that goal. >>>> >>>> Best regards, >>>> Krzysztof >>> Understand the DT schema purpose. But items property allows multiple >>> enums in the list which gives a lot of flexibility but make it hard to >>> validate. I am not familiar with DT schema, is there any directive to >>> specify one enum value depending on another so dts validation tool can >>> report error if combination is wrong? >>> >>> This is our preferred format of all bcmbca compatible string >>> especially when we could have more than 10 chip variants for the same >>> chip family and we really want to work on the chip family id. We will >>> make sure they are in the right combination in our own patch and patch >>> from other contributors. Would this work? If not, I will probably have >>> to revert the change of 4908(maybe append brcm,bcmbca as this chip >>> belongs to the same bca group) and use "enum board variant", "const >>> main chip id", "brcm,bca" for all other chips as our secondary choice. >> >> I'm not sure why I didn't even receive 1/3 and half of discussion >> e-mails. >> >> You can't just put all strings into a single bag and allow mixing them >> in any combos. Please check how it's properly handled in the current >> existing binding: >> Documentation/devicetree/bindings/arm/bcm/brcm,bcm4908.yaml >> >> Above binding enforces that non-matching compatible strings are not used >> together. > > I just noticed you're actually removing brcm,bcm4908.yaml in the 2/3 so > you must be aware of that file. > > So you see a cleanly working binding in the brcm,bcm4908.yaml but > instead copying it you decided to wrote your own one from scratch. > Incorrectly. > > This smells of NIH (not invented here). Please just use that binding I > wrote and move if it needed. Not mean to discredit any of your work and I did copy over your binding and combine them into one SoC entry to the new bcmbca.yaml and add you as one of the maintainer to this file. As this change would certainly concern you, that's why I sent RFC first. As I explained in the cover letter, the purpose of the change is to reduce the number of compatible strings and keep one entry for one chip family due to possible large number of chip variants. But since there is no way to validate the combination, I will copy the existing 4908 bindings as they are now but I would propose to append "brcm, bcmbca" as it is part of bcmbca chip. And for the other chips, we would just use enum "board variant", const "main chip id", const "brcm,bca". Does that sound good to you?
On 2022-07-13 20:37, William Zhang wrote: > Hi Rafal, > > On 7/13/22 03:58, Rafał Miłecki wrote: >> On 2022-07-13 12:50, Rafał Miłecki wrote: >>> On 2022-07-13 02:57, William Zhang wrote: >>>> On 7/12/22 11:18, Krzysztof Kozlowski wrote: >>>>> On 12/07/2022 19:37, William Zhang wrote: >>>>>>>> + - description: BCM4908 Family based boards >>>>>>>> + items: >>>>>>>> + - enum: >>>>>>>> + # BCM4908 SoC based boards >>>>>>>> + - brcm,bcm94908 >>>>>>>> + - asus,gt-ac5300 >>>>>>>> + - netgear,raxe500 >>>>>>>> + # BCM4906 SoC based boards >>>>>>>> + - brcm,bcm94906 >>>>>>>> + - netgear,r8000p >>>>>>>> + - tplink,archer-c2300-v1 >>>>>>>> + - enum: >>>>>>>> + - brcm,bcm4908 >>>>>>>> + - brcm,bcm4906 >>>>>>>> + - brcm,bcm49408 >>>>>>> >>>>>>> This is wrong. brcm,bcm94908 followed by brcm,bcm4906 does not >>>>>>> look >>>>>>> like valid list of compatibles. >>>>>>> >>>>>> For 4908 board variant, it will need to be followed by 4908 chip. >>>>>> Sorry >>>>>> for the basic question but is there any requirement to enforce >>>>>> this kind >>>>>> of rule? I would assume dts writer know what he/she is doing and >>>>>> select >>>>>> the right combination. >>>>> >>>>> The entire point of DT schema is to validate DTS. Combination like >>>>> above >>>>> prevents that goal. >>>>> >>>>> Best regards, >>>>> Krzysztof >>>> Understand the DT schema purpose. But items property allows multiple >>>> enums in the list which gives a lot of flexibility but make it hard >>>> to >>>> validate. I am not familiar with DT schema, is there any directive >>>> to >>>> specify one enum value depending on another so dts validation tool >>>> can >>>> report error if combination is wrong? >>>> >>>> This is our preferred format of all bcmbca compatible string >>>> especially when we could have more than 10 chip variants for the >>>> same >>>> chip family and we really want to work on the chip family id. We >>>> will >>>> make sure they are in the right combination in our own patch and >>>> patch >>>> from other contributors. Would this work? If not, I will probably >>>> have >>>> to revert the change of 4908(maybe append brcm,bcmbca as this chip >>>> belongs to the same bca group) and use "enum board variant", "const >>>> main chip id", "brcm,bca" for all other chips as our secondary >>>> choice. >>> >>> I'm not sure why I didn't even receive 1/3 and half of discussion >>> e-mails. >>> >>> You can't just put all strings into a single bag and allow mixing >>> them >>> in any combos. Please check how it's properly handled in the current >>> existing binding: >>> Documentation/devicetree/bindings/arm/bcm/brcm,bcm4908.yaml >>> >>> Above binding enforces that non-matching compatible strings are not >>> used >>> together. >> >> I just noticed you're actually removing brcm,bcm4908.yaml in the 2/3 >> so >> you must be aware of that file. >> >> So you see a cleanly working binding in the brcm,bcm4908.yaml but >> instead copying it you decided to wrote your own one from scratch. >> Incorrectly. >> >> This smells of NIH (not invented here). Please just use that binding I >> wrote and move if it needed. > > Not mean to discredit any of your work and I did copy over your > binding and combine them into one SoC entry to the new bcmbca.yaml and > add you as one of the maintainer to this file. As this change would > certainly concern you, that's why I sent RFC first. As I explained in > the cover letter, the purpose of the change is to reduce the number of > compatible strings and keep one entry for one chip family due to > possible large number of chip variants. But since there is no way to > validate the combination, I will copy the existing 4908 bindings as > they are now Right. I believe we need that. > but I would propose to append "brcm, bcmbca" as it is > part of bcmbca chip. And for the other chips, we would just use enum > "board variant", const "main chip id", const "brcm,bca". Does that > sound good to you? Nitpicking: you meant "brcm,bcmbca" (typo) but sounds absolutely fine!
On 7/13/22 13:23, Rafał Miłecki wrote: > On 2022-07-13 20:37, William Zhang wrote: >> Hi Rafal, >> >> On 7/13/22 03:58, Rafał Miłecki wrote: >>> On 2022-07-13 12:50, Rafał Miłecki wrote: >>>> On 2022-07-13 02:57, William Zhang wrote: >>>>> On 7/12/22 11:18, Krzysztof Kozlowski wrote: >>>>>> On 12/07/2022 19:37, William Zhang wrote: >>>>>>>>> + - description: BCM4908 Family based boards >>>>>>>>> + items: >>>>>>>>> + - enum: >>>>>>>>> + # BCM4908 SoC based boards >>>>>>>>> + - brcm,bcm94908 >>>>>>>>> + - asus,gt-ac5300 >>>>>>>>> + - netgear,raxe500 >>>>>>>>> + # BCM4906 SoC based boards >>>>>>>>> + - brcm,bcm94906 >>>>>>>>> + - netgear,r8000p >>>>>>>>> + - tplink,archer-c2300-v1 >>>>>>>>> + - enum: >>>>>>>>> + - brcm,bcm4908 >>>>>>>>> + - brcm,bcm4906 >>>>>>>>> + - brcm,bcm49408 >>>>>>>> >>>>>>>> This is wrong. brcm,bcm94908 followed by brcm,bcm4906 does not >>>>>>>> look >>>>>>>> like valid list of compatibles. >>>>>>>> >>>>>>> For 4908 board variant, it will need to be followed by 4908 chip. >>>>>>> Sorry >>>>>>> for the basic question but is there any requirement to enforce >>>>>>> this kind >>>>>>> of rule? I would assume dts writer know what he/she is doing and >>>>>>> select >>>>>>> the right combination. >>>>>> >>>>>> The entire point of DT schema is to validate DTS. Combination like >>>>>> above >>>>>> prevents that goal. >>>>>> >>>>>> Best regards, >>>>>> Krzysztof >>>>> Understand the DT schema purpose. But items property allows multiple >>>>> enums in the list which gives a lot of flexibility but make it hard to >>>>> validate. I am not familiar with DT schema, is there any directive to >>>>> specify one enum value depending on another so dts validation tool can >>>>> report error if combination is wrong? >>>>> >>>>> This is our preferred format of all bcmbca compatible string >>>>> especially when we could have more than 10 chip variants for the same >>>>> chip family and we really want to work on the chip family id. We will >>>>> make sure they are in the right combination in our own patch and patch >>>>> from other contributors. Would this work? If not, I will probably have >>>>> to revert the change of 4908(maybe append brcm,bcmbca as this chip >>>>> belongs to the same bca group) and use "enum board variant", "const >>>>> main chip id", "brcm,bca" for all other chips as our secondary choice. >>>> >>>> I'm not sure why I didn't even receive 1/3 and half of discussion >>>> e-mails. >>>> >>>> You can't just put all strings into a single bag and allow mixing them >>>> in any combos. Please check how it's properly handled in the current >>>> existing binding: >>>> Documentation/devicetree/bindings/arm/bcm/brcm,bcm4908.yaml >>>> >>>> Above binding enforces that non-matching compatible strings are not >>>> used >>>> together. >>> >>> I just noticed you're actually removing brcm,bcm4908.yaml in the 2/3 so >>> you must be aware of that file. >>> >>> So you see a cleanly working binding in the brcm,bcm4908.yaml but >>> instead copying it you decided to wrote your own one from scratch. >>> Incorrectly. >>> >>> This smells of NIH (not invented here). Please just use that binding I >>> wrote and move if it needed. >> >> Not mean to discredit any of your work and I did copy over your >> binding and combine them into one SoC entry to the new bcmbca.yaml and >> add you as one of the maintainer to this file. As this change would >> certainly concern you, that's why I sent RFC first. As I explained in >> the cover letter, the purpose of the change is to reduce the number of >> compatible strings and keep one entry for one chip family due to >> possible large number of chip variants. But since there is no way to >> validate the combination, I will copy the existing 4908 bindings as >> they are now > > Right. I believe we need that. > > >> but I would propose to append "brcm, bcmbca" as it is >> part of bcmbca chip. And for the other chips, we would just use enum >> "board variant", const "main chip id", const "brcm,bca". Does that >> sound good to you? > > Nitpicking: you meant "brcm,bcmbca" (typo) but sounds absolutely fine! Yup its a typo. Will append "brcm,bcmbca" and send out new patch.
On Wed, Jul 13, 2022 at 11:37:18AM -0700, William Zhang wrote: > Hi Rafal, > > On 7/13/22 03:58, Rafał Miłecki wrote: > > On 2022-07-13 12:50, Rafał Miłecki wrote: > > > On 2022-07-13 02:57, William Zhang wrote: > > > > On 7/12/22 11:18, Krzysztof Kozlowski wrote: > > > > > On 12/07/2022 19:37, William Zhang wrote: > > > > > > > > + - description: BCM4908 Family based boards > > > > > > > > + items: > > > > > > > > + - enum: > > > > > > > > + # BCM4908 SoC based boards > > > > > > > > + - brcm,bcm94908 > > > > > > > > + - asus,gt-ac5300 > > > > > > > > + - netgear,raxe500 > > > > > > > > + # BCM4906 SoC based boards > > > > > > > > + - brcm,bcm94906 > > > > > > > > + - netgear,r8000p > > > > > > > > + - tplink,archer-c2300-v1 > > > > > > > > + - enum: > > > > > > > > + - brcm,bcm4908 > > > > > > > > + - brcm,bcm4906 > > > > > > > > + - brcm,bcm49408 > > > > > > > > > > > > > > This is wrong. brcm,bcm94908 followed by brcm,bcm4906 does not look > > > > > > > like valid list of compatibles. > > > > > > > > > > > > > For 4908 board variant, it will need to be followed by > > > > > > 4908 chip. Sorry > > > > > > for the basic question but is there any requirement to > > > > > > enforce this kind > > > > > > of rule? I would assume dts writer know what he/she is > > > > > > doing and select > > > > > > the right combination. > > > > > > > > > > The entire point of DT schema is to validate DTS. > > > > > Combination like above > > > > > prevents that goal. > > > > > > > > > > Best regards, > > > > > Krzysztof > > > > Understand the DT schema purpose. But items property allows multiple > > > > enums in the list which gives a lot of flexibility but make it hard to > > > > validate. I am not familiar with DT schema, is there any directive to > > > > specify one enum value depending on another so dts validation tool can > > > > report error if combination is wrong? > > > > > > > > This is our preferred format of all bcmbca compatible string > > > > especially when we could have more than 10 chip variants for the same > > > > chip family and we really want to work on the chip family id. We will > > > > make sure they are in the right combination in our own patch and patch > > > > from other contributors. Would this work? If not, I will probably have > > > > to revert the change of 4908(maybe append brcm,bcmbca as this chip > > > > belongs to the same bca group) and use "enum board variant", "const > > > > main chip id", "brcm,bca" for all other chips as our secondary choice. > > > > > > I'm not sure why I didn't even receive 1/3 and half of discussion > > > e-mails. > > > > > > You can't just put all strings into a single bag and allow mixing them > > > in any combos. Please check how it's properly handled in the current > > > existing binding: > > > Documentation/devicetree/bindings/arm/bcm/brcm,bcm4908.yaml > > > > > > Above binding enforces that non-matching compatible strings are not used > > > together. > > > > I just noticed you're actually removing brcm,bcm4908.yaml in the 2/3 so > > you must be aware of that file. > > > > So you see a cleanly working binding in the brcm,bcm4908.yaml but > > instead copying it you decided to wrote your own one from scratch. > > Incorrectly. > > > > This smells of NIH (not invented here). Please just use that binding I > > wrote and move if it needed. > > Not mean to discredit any of your work and I did copy over your binding and > combine them into one SoC entry to the new bcmbca.yaml and add you as one of > the maintainer to this file. As this change would certainly concern you, > that's why I sent RFC first. As I explained in the cover letter, the > purpose of the change is to reduce the number of compatible strings and keep > one entry for one chip family due to possible large number of chip variants. > But since there is no way to validate the combination, I will copy the > existing 4908 bindings as they are now but I would propose to append "brcm, > bcmbca" as it is part of bcmbca chip. And for the other chips, we would just > use enum "board variant", const "main chip id", const "brcm,bca". Does that > sound good to you? If you want fewer combinations of compatibles, adding a genericish "brcm,bcmbca" is not going to help. Is there much value to adding it? What can you do with that information (and nothing else) is the question to ask. Rob
On 07/18/2022 01:02 PM, Rob Herring wrote: > On Wed, Jul 13, 2022 at 11:37:18AM -0700, William Zhang wrote: >> Hi Rafal, >> >> On 7/13/22 03:58, Rafał Miłecki wrote: >>> On 2022-07-13 12:50, Rafał Miłecki wrote: >>>> On 2022-07-13 02:57, William Zhang wrote: >>>>> On 7/12/22 11:18, Krzysztof Kozlowski wrote: >>>>>> On 12/07/2022 19:37, William Zhang wrote: >>>>>>>>> + - description: BCM4908 Family based boards >>>>>>>>> + items: >>>>>>>>> + - enum: >>>>>>>>> + # BCM4908 SoC based boards >>>>>>>>> + - brcm,bcm94908 >>>>>>>>> + - asus,gt-ac5300 >>>>>>>>> + - netgear,raxe500 >>>>>>>>> + # BCM4906 SoC based boards >>>>>>>>> + - brcm,bcm94906 >>>>>>>>> + - netgear,r8000p >>>>>>>>> + - tplink,archer-c2300-v1 >>>>>>>>> + - enum: >>>>>>>>> + - brcm,bcm4908 >>>>>>>>> + - brcm,bcm4906 >>>>>>>>> + - brcm,bcm49408 >>>>>>>> >>>>>>>> This is wrong. brcm,bcm94908 followed by brcm,bcm4906 does not look >>>>>>>> like valid list of compatibles. >>>>>>>> >>>>>>> For 4908 board variant, it will need to be followed by >>>>>>> 4908 chip. Sorry >>>>>>> for the basic question but is there any requirement to >>>>>>> enforce this kind >>>>>>> of rule? I would assume dts writer know what he/she is >>>>>>> doing and select >>>>>>> the right combination. >>>>>> >>>>>> The entire point of DT schema is to validate DTS. >>>>>> Combination like above >>>>>> prevents that goal. >>>>>> >>>>>> Best regards, >>>>>> Krzysztof >>>>> Understand the DT schema purpose. But items property allows multiple >>>>> enums in the list which gives a lot of flexibility but make it hard to >>>>> validate. I am not familiar with DT schema, is there any directive to >>>>> specify one enum value depending on another so dts validation tool can >>>>> report error if combination is wrong? >>>>> >>>>> This is our preferred format of all bcmbca compatible string >>>>> especially when we could have more than 10 chip variants for the same >>>>> chip family and we really want to work on the chip family id. We will >>>>> make sure they are in the right combination in our own patch and patch >>>>> from other contributors. Would this work? If not, I will probably have >>>>> to revert the change of 4908(maybe append brcm,bcmbca as this chip >>>>> belongs to the same bca group) and use "enum board variant", "const >>>>> main chip id", "brcm,bca" for all other chips as our secondary choice. >>>> >>>> I'm not sure why I didn't even receive 1/3 and half of discussion >>>> e-mails. >>>> >>>> You can't just put all strings into a single bag and allow mixing them >>>> in any combos. Please check how it's properly handled in the current >>>> existing binding: >>>> Documentation/devicetree/bindings/arm/bcm/brcm,bcm4908.yaml >>>> >>>> Above binding enforces that non-matching compatible strings are not used >>>> together. >>> >>> I just noticed you're actually removing brcm,bcm4908.yaml in the 2/3 so >>> you must be aware of that file. >>> >>> So you see a cleanly working binding in the brcm,bcm4908.yaml but >>> instead copying it you decided to wrote your own one from scratch. >>> Incorrectly. >>> >>> This smells of NIH (not invented here). Please just use that binding I >>> wrote and move if it needed. >> >> Not mean to discredit any of your work and I did copy over your binding and >> combine them into one SoC entry to the new bcmbca.yaml and add you as one of >> the maintainer to this file. As this change would certainly concern you, >> that's why I sent RFC first. As I explained in the cover letter, the >> purpose of the change is to reduce the number of compatible strings and keep >> one entry for one chip family due to possible large number of chip variants. >> But since there is no way to validate the combination, I will copy the >> existing 4908 bindings as they are now but I would propose to append "brcm, >> bcmbca" as it is part of bcmbca chip. And for the other chips, we would just >> use enum "board variant", const "main chip id", const "brcm,bca". Does that >> sound good to you? > > If you want fewer combinations of compatibles, adding a genericish > "brcm,bcmbca" is not going to help. Is there much value to adding it? > What can you do with that information (and nothing else) is the > question to ask. > Since we moved all the Broadcom Broadband BCA origin SoC to ARCH_BCMBCA arch, I think it makes sense to have that as the last part of the compatible string, not really for reducing the number. And for that matter, we have agreed to not change anything to Rafal dts definitions including the number of compat strings but just appending brcm,bca. So we are all good now. > Rob >
diff --git a/Documentation/devicetree/bindings/arm/bcm/brcm,bcmbca.yaml b/Documentation/devicetree/bindings/arm/bcm/brcm,bcmbca.yaml index d9dc4f22f4a5..906c3e1de372 100644 --- a/Documentation/devicetree/bindings/arm/bcm/brcm,bcmbca.yaml +++ b/Documentation/devicetree/bindings/arm/bcm/brcm,bcmbca.yaml @@ -28,6 +28,23 @@ properties: - const: brcm,bcm47622 - const: brcm,bcmbca + - description: BCM4908 Family based boards + items: + - enum: + # BCM4908 SoC based boards + - brcm,bcm94908 + - asus,gt-ac5300 + - netgear,raxe500 + # BCM4906 SoC based boards + - brcm,bcm94906 + - netgear,r8000p + - tplink,archer-c2300-v1 + - enum: + - brcm,bcm4908 + - brcm,bcm4906 + - brcm,bcm49408 + - const: brcm,bcmbca + - description: BCM4912 based boards items: - enum:
Merge BCM4908 SoC device tree description into BCMBCA and combined all BCM4908 chip variants into the same BCM4908 chip family item. Each compatible string represent the whole chip family. The board variants and chip varints go into the first and second enum in the compatible string item list. Signed-off-by: William Zhang <william.zhang@broadcom.com> --- .../bindings/arm/bcm/brcm,bcmbca.yaml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)