| Message ID | 20230313124040.9463-3-quic_kbajaj@quicinc.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | soc: qcom: llcc: Add support for QDU1000/QRU1000 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On Mon, 13 Mar 2023 18:10:37 +0530, Komal Bajaj wrote: > Add description for additional nodes needed to support > mulitple channel DDR configurations in LLCC. > > Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com> > --- > Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml | 9 +++++++++ > 1 file changed, 9 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/arm/msm/qcom,llcc.example.dtb: system-cache-controller@1100000: reg: [[17825792, 2097152], [19922944, 327680]] is too short From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/arm/msm/qcom,llcc.example.dtb: system-cache-controller@1100000: reg-names: ['llcc_base', 'llcc_broadcast_base'] is too short From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230313124040.9463-3-quic_kbajaj@quicinc.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 13/03/2023 13:40, Komal Bajaj wrote: > Add description for additional nodes needed to support > mulitple channel DDR configurations in LLCC. > > Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com> +Cc Mani, This will conflict with: https://lore.kernel.org/all/20230314080443.64635-3-manivannan.sadhasivam@linaro.org/ Please rebase on top of Mani's patches (assuming they are not conflicting in principle) > --- > Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml > index 38efcad56dbd..9a4a76caf490 100644 > --- a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml > +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml > @@ -37,15 +37,24 @@ properties: > items: minItems: 2 > - description: LLCC base register region > - description: LLCC broadcast base register region > + - description: Feature register to decide which LLCC configuration > + to use, this is optional > > reg-names: minItems: 2 > items: > - const: llcc_base > - const: llcc_broadcast_base > + - const: multi_channel_register > > interrupts: > maxItems: 1 > > + multi-ch-bit-off: > + items: > + - description: Specifies the offset in bits into the multi_channel_register > + and the number of bits used to decide which LLCC configuration > + to use There are here few issues. First, I don't fully understand the property. What is an LLCC configuration? Like some fused values? Second, don't make it a register specific, it will not scale easily to any new version of this interface. Although how this should look like depends on what is it. Third, you need vendor prefix and type (unless this is a generic property, but does not look like). Then "items" is probably wrong. Line break after "description: " Best regards, Krzysztof
On Wed, Mar 15, 2023 at 08:41:21AM +0100, Krzysztof Kozlowski wrote: > On 13/03/2023 13:40, Komal Bajaj wrote: > > Add description for additional nodes needed to support > > mulitple channel DDR configurations in LLCC. > > > > Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com> > > +Cc Mani, > Thanks, Krzysztof! > This will conflict with: > https://lore.kernel.org/all/20230314080443.64635-3-manivannan.sadhasivam@linaro.org/ > > Please rebase on top of Mani's patches (assuming they are not > conflicting in principle) > > > --- > > Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml > > index 38efcad56dbd..9a4a76caf490 100644 > > --- a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml > > +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml > > @@ -37,15 +37,24 @@ properties: > > items: > > minItems: 2 > > > - description: LLCC base register region > > - description: LLCC broadcast base register region > > + - description: Feature register to decide which LLCC configuration > > + to use, this is optional > > > > reg-names: > > minItems: 2 > > > items: > > - const: llcc_base > > - const: llcc_broadcast_base > > + - const: multi_channel_register Is this the actual register region or a specific register offset? We generally try to pass the base address of the region along with the size and use the offset inside the driver to access any specific registers. Thanks, Mani > > > > interrupts: > > maxItems: 1 > > > > + multi-ch-bit-off: > > + items: > > + - description: Specifies the offset in bits into the multi_channel_register > > + and the number of bits used to decide which LLCC configuration > > + to use > > There are here few issues. > First, I don't fully understand the property. What is an LLCC > configuration? Like some fused values? > > Second, don't make it a register specific, it will not scale easily to > any new version of this interface. Although how this should look like > depends on what is it. > > Third, you need vendor prefix and type (unless this is a generic > property, but does not look like). Then "items" is probably wrong. Line > break after "description: " > > Best regards, > Krzysztof >
On 06/04/2023 11:19, Komal Bajaj wrote: > >>>> >>>> interrupts: >>>> maxItems: 1 >>>> >>>> + multi-ch-bit-off: >>>> + items: >>>> + - description: Specifies the offset in bits into the multi_channel_register >>>> + and the number of bits used to decide which LLCC configuration >>>> + to use >>> There are here few issues. >>> First, I don't fully understand the property. What is an LLCC >>> configuration? Like some fused values? > > There are different configuration for LLCC based on the number of > DDR channel it uses. Here, we are basically trying to get information > about the same. > >>> >>> Second, don't make it a register specific, it will not scale easily to >>> any new version of this interface. Although how this should look like >>> depends on what is it. > > LLCC driver can only get DDR channel information from the register. And why would that exactly matter to DT? How does it solve my concern that your approach does not scale? Best regards, Krzysztof
On 4/6/2023 2:49 PM, Komal Bajaj wrote: > Didn't see my reply on the list, so sending it again. > And also I see that the dt patch is already applied. The reason why you are not seeing your replies at https://lore.kernel.org/lkml/20230313124040.9463-1-quic_kbajaj@quicinc.com/ is because your reply cc-list contain some invalid domain (codeaurora.org) email id's and any list/email mentioned after that would not be getting your emails. -- Mukesh > > Thanks Krzysztof and Manivannan for reviewing the patch. > > > On 3/15/2023 7:18 PM, Manivannan Sadhasivam wrote: >> On Wed, Mar 15, 2023 at 08:41:21AM +0100, Krzysztof Kozlowski wrote: >>> On 13/03/2023 13:40, Komal Bajaj wrote: >>>> Add description for additional nodes needed to support >>>> mulitple channel DDR configurations in LLCC. >>>> >>>> Signed-off-by: Komal Bajaj<quic_kbajaj@quicinc.com> >>> +Cc Mani, >>> >> Thanks, Krzysztof! >> >>> This will conflict with: >>> https://lore.kernel.org/all/20230314080443.64635-3-manivannan.sadhasivam@linaro.org/ >>> >>> Please rebase on top of Mani's patches (assuming they are not >>> conflicting in principle) >>> >>>> --- >>>> Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml >>>> index 38efcad56dbd..9a4a76caf490 100644 >>>> --- a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml >>>> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml >>>> @@ -37,15 +37,24 @@ properties: >>>> items: >>> minItems: 2 >>> >>>> - description: LLCC base register region >>>> - description: LLCC broadcast base register region >>>> + - description: Feature register to decide which LLCC configuration >>>> + to use, this is optional >>>> >>>> reg-names: >>> minItems: 2 >>> >>>> items: >>>> - const: llcc_base >>>> - const: llcc_broadcast_base >>>> + - const: multi_channel_register >> Is this the actual register region or a specific register offset? We generally >> try to pass the base address of the region along with the size and use the >> offset inside the driver to access any specific registers. >> >> Thanks, >> Mani > > This is a specific register offset outside the LLCC register region which has the > information of number of DDR channel. > >>>> >>>> interrupts: >>>> maxItems: 1 >>>> >>>> + multi-ch-bit-off: >>>> + items: >>>> + - description: Specifies the offset in bits into the multi_channel_register >>>> + and the number of bits used to decide which LLCC configuration >>>> + to use >>> There are here few issues. >>> First, I don't fully understand the property. What is an LLCC >>> configuration? Like some fused values? > > There are different configuration for LLCC based on the number of > DDR channel it uses. Here, we are basically trying to get information > about the same. > >>> Second, don't make it a register specific, it will not scale easily to >>> any new version of this interface. Although how this should look like >>> depends on what is it. > > LLCC driver can only get DDR channel information from the register. > >>> Third, you need vendor prefix and type (unless this is a generic >>> property, but does not look like). Then "items" is probably wrong. Line >>> break after "description: " > > Noted, will take care of this in the next patchset. > > Thanks > Komal > >>> Best regards, >>> Krzysztof >>> >
On 4/6/2023 3:04 PM, Krzysztof Kozlowski wrote: > On 06/04/2023 11:19, Komal Bajaj wrote: >> >>>>> >>>>> interrupts: >>>>> maxItems: 1 >>>>> >>>>> + multi-ch-bit-off: >>>>> + items: >>>>> + - description: Specifies the offset in bits into the multi_channel_register >>>>> + and the number of bits used to decide which LLCC configuration >>>>> + to use >>>> There are here few issues. >>>> First, I don't fully understand the property. What is an LLCC >>>> configuration? Like some fused values? >> >> There are different configuration for LLCC based on the number of >> DDR channel it uses. Here, we are basically trying to get information >> about the same. >> >>>> >>>> Second, don't make it a register specific, it will not scale easily to >>>> any new version of this interface. Although how this should look like >>>> depends on what is it. >> >> LLCC driver can only get DDR channel information from the register. Actually, any one can read this property there is no boundation to that. However, some of the bits information of this register only matters to LLCC to make decision. > And why would that exactly matter to DT? How does it solve my concern > that your approach does not scale? I understand your concern about not making it register specific, however this register is a secure fuse register where the interested bits are known to us and should only be used by llcc to select right slice configuration table to apply. How about something like this, Add another property like qcom,tcsr_feature_config under qcom,scm and read this property under qcom_scm driver and expose read exported symbol for LLCC driver. LLCC driver can use API and apply bit offset/bit size to get right value inside the target driver data(e.g .data = &XXXX_cfg }) or maintain it inside same tcsr_feature_config DT (this can be discussed) Also, we can have a yaml file for tcsr_feature_config. e.g.. scm: scm { compatible = "qcom,scm-sm8450", "qcom,scm"; qcom,dload-mode = <&tcsr 0x13000>; ... qcom,tcsr_feature_config = <&tcsr_feature_config> }; tcsr_feature_config: syscon@fd484000 { compatible = "qcom, XXXX", qcom,tcsr-featureconfig"; reg = <0xXXXXXXXX 0xYYYY>; }; -- Mukesh > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml index 38efcad56dbd..9a4a76caf490 100644 --- a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml @@ -37,15 +37,24 @@ properties: items: - description: LLCC base register region - description: LLCC broadcast base register region + - description: Feature register to decide which LLCC configuration + to use, this is optional reg-names: items: - const: llcc_base - const: llcc_broadcast_base + - const: multi_channel_register interrupts: maxItems: 1 + multi-ch-bit-off: + items: + - description: Specifies the offset in bits into the multi_channel_register + and the number of bits used to decide which LLCC configuration + to use + required: - compatible - reg
Add description for additional nodes needed to support mulitple channel DDR configurations in LLCC. Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com> --- Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml | 9 +++++++++ 1 file changed, 9 insertions(+)