| Message ID | 20221123204615.25358-2-quic_sibis@quicinc.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | SCM: Add support for wait-queue aware firmware | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 23/11/2022 21:46, Sibi Sankar wrote: > From: Guru Das Srinagesh <quic_gurus@quicinc.com> > > Add an interrupt specification to the bindings to support the wait-queue > feature. Subject - this is qcom,scm, not qcom-scm. > > Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com> > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > > v5: > - Pick up R-b > > v4: > - Qualify bindings [Krzysztoff] > > Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml > index 25688571ee7c..aea6e0c86a89 100644 > --- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml > +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml > @@ -73,6 +73,12 @@ properties: > '#reset-cells': > const: 1 > > + interrupts: > + description: > + The wait-queue interrupt that firmware raises as part of handshake > + protocol to handle sleeping SCM calls. > + maxItems: 1 Which devices have interrupts? We talked about it here: https://lore.kernel.org/all/2464d90f-64e9-5e3c-404b-10394c3bc302@quicinc.com/ and here: https://lore.kernel.org/all/c20edd0d-7613-5683-60e7-54317cac6e0b@linaro.org/ But I still don't get which devices support it and which do not. BTW: https://lore.kernel.org/all/20221122092345.44369-2-krzysztof.kozlowski@linaro.org/ Best regards, Krzysztof
Hey Krzysztof, Thanks for taking time to review the series. On 11/24/22 16:43, Krzysztof Kozlowski wrote: > On 23/11/2022 21:46, Sibi Sankar wrote: >> From: Guru Das Srinagesh <quic_gurus@quicinc.com> >> >> Add an interrupt specification to the bindings to support the wait-queue >> feature. > > Subject - this is qcom,scm, not qcom-scm. ack > > >> >> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com> >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> --- >> >> v5: >> - Pick up R-b >> >> v4: >> - Qualify bindings [Krzysztoff] >> >> Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml >> index 25688571ee7c..aea6e0c86a89 100644 >> --- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml >> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml >> @@ -73,6 +73,12 @@ properties: >> '#reset-cells': >> const: 1 >> >> + interrupts: >> + description: >> + The wait-queue interrupt that firmware raises as part of handshake >> + protocol to handle sleeping SCM calls. >> + maxItems: 1 > > Which devices have interrupts? > > We talked about it here: > https://lore.kernel.org/all/2464d90f-64e9-5e3c-404b-10394c3bc302@quicinc.com/ > and here: > https://lore.kernel.org/all/c20edd0d-7613-5683-60e7-54317cac6e0b@linaro.org/ > > But I still don't get which devices support it and which do not. lol, I thought we reached a consensus earlier because of the "ok" and R-b. Like I explained earlier the bootloader would be adding interrupt on the fly, wouldn't such cases cause binding check failure if we list all the devices supporting it? Also some of the SM8450 devices in the wild would be running firmware not having the feature but I guess eventually most of the them will end up supporting the feature in the end. > > > BTW: > https://lore.kernel.org/all/20221122092345.44369-2-krzysztof.kozlowski@linaro.org/ Yup had a look at ^^, IIRC there are additional SoCs that can have the interconnects specified in their device tree. > > > Best regards, > Krzysztof >
On 28/11/2022 06:57, Sibi Sankar wrote: >> >> Which devices have interrupts? >> >> We talked about it here: >> https://lore.kernel.org/all/2464d90f-64e9-5e3c-404b-10394c3bc302@quicinc.com/ >> and here: >> https://lore.kernel.org/all/c20edd0d-7613-5683-60e7-54317cac6e0b@linaro.org/ >> >> But I still don't get which devices support it and which do not. > > lol, I thought we reached a consensus earlier because of the "ok" and > R-b. Like I explained earlier the bootloader would be adding interrupt > on the fly, wouldn't such cases cause binding check failure if we list > all the devices supporting it? What type of failure? I don't get. Is this interrupt valid for SM8250? SDM845? MSM8996? and so on? Now you make it valid. > Also some of the SM8450 devices in the > wild would be running firmware not having the feature but I guess > eventually most of the them will end up supporting the feature in the > end. That's not what I meant. Your patch describes the case for one variant but you are affecting all of them. Best regards, Krzysztof
On 11/28/22 14:00, Krzysztof Kozlowski wrote: > On 28/11/2022 06:57, Sibi Sankar wrote: > >>> >>> Which devices have interrupts? >>> >>> We talked about it here: >>> https://lore.kernel.org/all/2464d90f-64e9-5e3c-404b-10394c3bc302@quicinc.com/ >>> and here: >>> https://lore.kernel.org/all/c20edd0d-7613-5683-60e7-54317cac6e0b@linaro.org/ >>> >>> But I still don't get which devices support it and which do not. >> >> lol, I thought we reached a consensus earlier because of the "ok" and >> R-b. Like I explained earlier the bootloader would be adding interrupt >> on the fly, wouldn't such cases cause binding check failure if we list >> all the devices supporting it? > > What type of failure? I don't get. Is this interrupt valid for SM8250? > SDM845? MSM8996? and so on? Now you make it valid. ok if we mark the interrupt as required for SM8450 and not specify the interrupt in the board file (since the bootloader will be adding it on the fly), dtbs_check will throw 'interrupts' is a required property for the board file. This was the failure I was talking about. > >> Also some of the SM8450 devices in the >> wild would be running firmware not having the feature but I guess >> eventually most of the them will end up supporting the feature in the >> end. > > That's not what I meant. Your patch describes the case for one variant > but you are affecting all of them. Not really, the driver treats interrupts as optional. If the interrupt isn't present we assume that the feature isn't supported. If the bootloader adds the property during boot then we assume the fw has waitqueue support. - Sibi > > > > Best regards, > Krzysztof >
On 29/11/2022 11:05, Sibi Sankar wrote: > On 11/28/22 14:00, Krzysztof Kozlowski wrote: >> On 28/11/2022 06:57, Sibi Sankar wrote: >> >>>> >>>> Which devices have interrupts? >>>> >>>> We talked about it here: >>>> https://lore.kernel.org/all/2464d90f-64e9-5e3c-404b-10394c3bc302@quicinc.com/ >>>> and here: >>>> https://lore.kernel.org/all/c20edd0d-7613-5683-60e7-54317cac6e0b@linaro.org/ >>>> >>>> But I still don't get which devices support it and which do not. >>> >>> lol, I thought we reached a consensus earlier because of the "ok" and >>> R-b. Like I explained earlier the bootloader would be adding interrupt >>> on the fly, wouldn't such cases cause binding check failure if we list >>> all the devices supporting it? >> >> What type of failure? I don't get. Is this interrupt valid for SM8250? >> SDM845? MSM8996? and so on? Now you make it valid. > > ok if we mark the interrupt as required for SM8450 and not specify the > interrupt in the board file (since the bootloader will be adding it on > the fly), dtbs_check will throw 'interrupts' is a required property for > the board file. This was the failure I was talking about. OK, but no one said here about making it required. So how this issue can happen? Please read above chapter again. I said nothing about required, but I said "valid". > >> >>> Also some of the SM8450 devices in the >>> wild would be running firmware not having the feature but I guess >>> eventually most of the them will end up supporting the feature in the >>> end. >> >> That's not what I meant. Your patch describes the case for one variant >> but you are affecting all of them. > > Not really, the driver treats interrupts as optional. Linux implementation matters less. We talk about device/hardware (firmware in this case). > If the interrupt > isn't present we assume that the feature isn't supported. If the > bootloader adds the property during boot then we assume the fw has > waitqueue support. Sure, my question stays. Which devices do not support it at all? Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml index 25688571ee7c..aea6e0c86a89 100644 --- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml @@ -73,6 +73,12 @@ properties: '#reset-cells': const: 1 + interrupts: + description: + The wait-queue interrupt that firmware raises as part of handshake + protocol to handle sleeping SCM calls. + maxItems: 1 + qcom,dload-mode: $ref: /schemas/types.yaml#/definitions/phandle-array items: