Re: [PATCH 10/12] dt-bindings: usb: qcom,dwc3: Introduce flattened qcom,dwc3 binding

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Bjorn Andersson <quic_bjorande@quicinc.com>
Cc: Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Wesley Cheng <quic_wcheng@quicinc.com>,
	Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Felipe Balbi <balbi@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	Johan Hovold <johan@kernel.org>,
	Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
Subject: Re: [PATCH 10/12] dt-bindings: usb: qcom,dwc3: Introduce flattened qcom,dwc3 binding
Date: Wed, 18 Oct 2023 08:00:35 +0200	[thread overview]
Message-ID: <b52d0de7-e320-4c4c-876e-346281de23a1@linaro.org> (raw)
In-Reply-To: <20231017225433.GW3553829@hu-bjorande-lv.qualcomm.com>

On 18/10/2023 00:54, Bjorn Andersson wrote:
> On Tue, Oct 17, 2023 at 08:11:45AM +0200, Krzysztof Kozlowski wrote:
>> On 17/10/2023 05:11, Bjorn Andersson wrote:
>>> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> [..]
>>> +select:
>>> +  properties:
>>> +    compatible:
>>> +      items:
>>> +        - enum:
>>> +            - qcom,ipq4019-dwc3
> [..]
>>> +            - qcom,sm8550-dwc3
>>
>> This enum could be replaced with '{}'. Alternatively, drop enum entire
>> select replaced with:
>> - contains
>>   - items:
>>       - const: qcom,dwc3
>>       - const: snps,dwc3
>>
> 
> I thought this would be what I needed as well, but unfortunately this
> select matches either qcom,dwc3, snps,dwc3, or both. With the result
> that e.g. the example in the snps,dwc3 binding matches against this and
> as expected fails when validated against this binding.
> 
> Taking yet another look at this, and reading more about json validation
> I figured out that the following matches nodes with both the
> compatibles:
> 
> select:
>   properties:
>     compatible:
>       items:
>         - const: qcom,dwc3
>         - const: snps,dwc3
>   required:
>     - compatible
> 
> [..]
>>> +
>>> +# Required child node:
>>
>> Drop
>>
> 
> Of course.
> 
>>
>> ...
>>
>>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>> index d81c2e849ca9..d6914b8cef6a 100644
>>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>> @@ -44,14 +44,18 @@ properties:
>>>        It's either a single common DWC3 interrupt (dwc_usb3) or individual
>>>        interrupts for the host, gadget and DRD modes.
>>>      minItems: 1
>>> -    maxItems: 4
>>> +    maxItems: 5
>>>  
>>>    interrupt-names:
>>> -    minItems: 1
>>> -    maxItems: 4
>>>      oneOf:
>>> -      - const: dwc_usb3
>>> -      - items:
>>> +      - minItems: 1
>>> +        maxItems: 5
>>> +        items:
>>> +          - const: dwc_usb3
>>> +        additionalItems: true
>>
>> This is not correct change. Before, one dwc_usb3 interrupt was combined
>> allowed, or a set of host+peripheral+otg+wakeup. Now, you allow combined
>> dwc_usb3 with anything.
>>
> 
> My intention here is to make below list of 5 strings be valid according
> to the snps,dwc3 (i.e. dwc_usb3 being the first item), and valid
> according to the qcom,dwc3 binding with all 5 defined.
> 
>   interrupt-names = "dwc_usb3", "hs_phy_irq", "ss_phy_irq",
> 		    "dm_hs_phy_irq", "dp_hs_phy_irq";
> 
> When I express this as:
> 
>   interrupt-names:
>     minItems: 1
>     maxItems: 5
>     oneOf:
>       - const: dwc_usb3
>       - items:
>           enum: [host, peripheral, otg, wakeup]
> 
> I get:
> 
> /local/mnt/workspace/bjorande/linux/Documentation/devicetree/bindings/usb/qcom,dwc3.example.dtb: usb@a600000: interrupt-names: 'oneOf' conditional failed, one must be fixed:
>         ['dwc_usb3', 'hs_phy_irq', 'ss_phy_irq', 'dm_hs_phy_irq', 'dp_hs_phy_irq'] is too long
>         'dwc_usb3' is not one of ['host', 'peripheral', 'otg', 'wakeup']
>         'hs_phy_irq' is not one of ['host', 'peripheral', 'otg', 'wakeup']
>         'ss_phy_irq' is not one of ['host', 'peripheral', 'otg', 'wakeup']
>         'dm_hs_phy_irq' is not one of ['host', 'peripheral', 'otg', 'wakeup']
>         'dp_hs_phy_irq' is not one of ['host', 'peripheral', 'otg', 'wakeup']
>         from schema $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml#
> 
> Which to me sounds like the two oneOf branches allow me a single entry,
> or items from the set given here. In contrast, I believe that my
> proposal allow 1-5 items, where the first needs to be dwc_usb3.
> 
> But the proposal does look messy, so I'd appreciate some guidance on
> this one.

Then you need three branches I guess, with your branch listing the Qcom
specific interrupts. The point is that dwc_usb3 should not be allowed
with host/peripheral/otg/wakeup.

Best regards,
Krzysztof