| Message ID | 20240503212139.5811-4-ansuelsmth@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mips: bmips: improve handling of RAC and CBR addr | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On Fri, 03 May 2024 23:20:59 +0200, Christian Marangi wrote: > Document brcm,bmips-cbr-reg and brcm,bmips-broken-cbr-reg property. > > Some SoC suffer from a BUG where read_c0_brcm_cbr() might return 0 > if called from TP1. The CBR address is always the same on the SoC > hence it can be provided in DT to handle broken case where bootloader > doesn't init it or SMP where read_c0_brcm_cbr() returns 0 from TP1. > > Usage of this property is to give an address also in these broken > configuration/bootloader. > > If the SoC/Bootloader ALWAYS provide a broken CBR address the property > "brcm,bmips-broken-cbr-reg" can be used to ignore any value already set > in the registers for CBR address. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > .../devicetree/bindings/mips/brcm/soc.yaml | 32 +++++++++++++++++++ > 1 file changed, 32 insertions(+) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: ./Documentation/devicetree/bindings/mips/brcm/soc.yaml:83:37: [warning] too few spaces after comma (commas) dtschema/dtc warnings/errors: doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240503212139.5811-4-ansuelsmth@gmail.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Fri, May 03, 2024 at 11:20:59PM +0200, Christian Marangi wrote: > Document brcm,bmips-cbr-reg and brcm,bmips-broken-cbr-reg property. > > Some SoC suffer from a BUG where read_c0_brcm_cbr() might return 0 > if called from TP1. The CBR address is always the same on the SoC > hence it can be provided in DT to handle broken case where bootloader > doesn't init it or SMP where read_c0_brcm_cbr() returns 0 from TP1. > > Usage of this property is to give an address also in these broken > configuration/bootloader. > > If the SoC/Bootloader ALWAYS provide a broken CBR address the property > "brcm,bmips-broken-cbr-reg" can be used to ignore any value already set > in the registers for CBR address. Why can't these be implied from an SoC specific compatible? It's not a great design where you have to update the DT which should be provided from the bootloader in order to work-around bootloader issues... > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > .../devicetree/bindings/mips/brcm/soc.yaml | 32 +++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mips/brcm/soc.yaml b/Documentation/devicetree/bindings/mips/brcm/soc.yaml > index 975945ca2888..29af8f0db785 100644 > --- a/Documentation/devicetree/bindings/mips/brcm/soc.yaml > +++ b/Documentation/devicetree/bindings/mips/brcm/soc.yaml > @@ -55,6 +55,21 @@ properties: > under the "cpus" node. > $ref: /schemas/types.yaml#/definitions/uint32 > > + brcm,bmips-broken-cbr-reg: > + description: Declare that the Bootloader init a broken > + CBR address in the registers and the one provided from > + DT should always be used. Why wouldn't brcm,bmips-cbr-reg being present indicate to use it? > + type: boolean > + > + brcm,bmips-cbr-reg: > + description: Reference address of the CBR. > + Some SoC suffer from a BUG where read_c0_brcm_cbr() might > + return 0 if called from TP1. The CBR address is always the > + same on the SoC hence it can be provided in DT to handle > + broken case where bootloader doesn't initialise it or SMP > + where read_c0_brcm_cbr() returns 0 from TP1. > + $ref: /schemas/types.yaml#/definitions/uint32 CBR is never defined anywhere in this patch. > + > patternProperties: > "^cpu@[0-9]$": > type: object > @@ -64,6 +79,23 @@ properties: > required: > - mips-hpt-frequency > > +dependencies: > + brcm,bmips-broken-cbr-reg: [ brcm,bmips-cbr-reg ] The inline syntax (i.e. []) means you need quotes for commas. This has no effect because you are applying it to the root node. Needs to be a the same level as the properties. > + > +if: > + properties: > + compatible: > + contains: > + anyOf: > + - const: brcm,bcm6358 > + - const: brcm,bcm6368 Replace anyOf+const with enum. > + > +then: > + properties: > + cpus: > + required: > + - brcm,bmips-cbr-reg > + > additionalProperties: true > > examples: > -- > 2.43.0 >
On 5/7/24 06:07, Rob Herring wrote: > On Fri, May 03, 2024 at 11:20:59PM +0200, Christian Marangi wrote: >> Document brcm,bmips-cbr-reg and brcm,bmips-broken-cbr-reg property. >> >> Some SoC suffer from a BUG where read_c0_brcm_cbr() might return 0 >> if called from TP1. The CBR address is always the same on the SoC >> hence it can be provided in DT to handle broken case where bootloader >> doesn't init it or SMP where read_c0_brcm_cbr() returns 0 from TP1. >> >> Usage of this property is to give an address also in these broken >> configuration/bootloader. >> >> If the SoC/Bootloader ALWAYS provide a broken CBR address the property >> "brcm,bmips-broken-cbr-reg" can be used to ignore any value already set >> in the registers for CBR address. > > Why can't these be implied from an SoC specific compatible? Because some SoCs with the same compatible have it right, and some wrong, courtesy of how the various OEMs implemented it. > > It's not a great design where you have to update the DT which should be > provided from the bootloader in order to work-around bootloader > issues... The bootloader was designed without DT in mind, and while CFE had a callback mechanism to query environment variables and whatnot, those devices were stripped out of it. > >> >> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> >> --- >> .../devicetree/bindings/mips/brcm/soc.yaml | 32 +++++++++++++++++++ >> 1 file changed, 32 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/mips/brcm/soc.yaml b/Documentation/devicetree/bindings/mips/brcm/soc.yaml >> index 975945ca2888..29af8f0db785 100644 >> --- a/Documentation/devicetree/bindings/mips/brcm/soc.yaml >> +++ b/Documentation/devicetree/bindings/mips/brcm/soc.yaml >> @@ -55,6 +55,21 @@ properties: >> under the "cpus" node. >> $ref: /schemas/types.yaml#/definitions/uint32 >> >> + brcm,bmips-broken-cbr-reg: >> + description: Declare that the Bootloader init a broken >> + CBR address in the registers and the one provided from >> + DT should always be used. > > Why wouldn't brcm,bmips-cbr-reg being present indicate to use it? > >> + type: boolean >> + >> + brcm,bmips-cbr-reg: >> + description: Reference address of the CBR. >> + Some SoC suffer from a BUG where read_c0_brcm_cbr() might >> + return 0 if called from TP1. The CBR address is always the >> + same on the SoC hence it can be provided in DT to handle >> + broken case where bootloader doesn't initialise it or SMP >> + where read_c0_brcm_cbr() returns 0 from TP1. >> + $ref: /schemas/types.yaml#/definitions/uint32 > > CBR is never defined anywhere in this patch. The very presence of "brcm,bmips-cbr-reg" property should be enough to indicate to the kernel that it should the value provided, rather than the value returned from read_c0_brcm_cbr(). That is, I don't think there is a need to indicate to the kernel that the CBR value is broken, if you provide a new value that is enough of a clue to tel you that.
diff --git a/Documentation/devicetree/bindings/mips/brcm/soc.yaml b/Documentation/devicetree/bindings/mips/brcm/soc.yaml index 975945ca2888..29af8f0db785 100644 --- a/Documentation/devicetree/bindings/mips/brcm/soc.yaml +++ b/Documentation/devicetree/bindings/mips/brcm/soc.yaml @@ -55,6 +55,21 @@ properties: under the "cpus" node. $ref: /schemas/types.yaml#/definitions/uint32 + brcm,bmips-broken-cbr-reg: + description: Declare that the Bootloader init a broken + CBR address in the registers and the one provided from + DT should always be used. + type: boolean + + brcm,bmips-cbr-reg: + description: Reference address of the CBR. + Some SoC suffer from a BUG where read_c0_brcm_cbr() might + return 0 if called from TP1. The CBR address is always the + same on the SoC hence it can be provided in DT to handle + broken case where bootloader doesn't initialise it or SMP + where read_c0_brcm_cbr() returns 0 from TP1. + $ref: /schemas/types.yaml#/definitions/uint32 + patternProperties: "^cpu@[0-9]$": type: object @@ -64,6 +79,23 @@ properties: required: - mips-hpt-frequency +dependencies: + brcm,bmips-broken-cbr-reg: [ brcm,bmips-cbr-reg ] + +if: + properties: + compatible: + contains: + anyOf: + - const: brcm,bcm6358 + - const: brcm,bcm6368 + +then: + properties: + cpus: + required: + - brcm,bmips-cbr-reg + additionalProperties: true examples:
Document brcm,bmips-cbr-reg and brcm,bmips-broken-cbr-reg property. Some SoC suffer from a BUG where read_c0_brcm_cbr() might return 0 if called from TP1. The CBR address is always the same on the SoC hence it can be provided in DT to handle broken case where bootloader doesn't init it or SMP where read_c0_brcm_cbr() returns 0 from TP1. Usage of this property is to give an address also in these broken configuration/bootloader. If the SoC/Bootloader ALWAYS provide a broken CBR address the property "brcm,bmips-broken-cbr-reg" can be used to ignore any value already set in the registers for CBR address. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- .../devicetree/bindings/mips/brcm/soc.yaml | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+)