| Message ID | 20240112111747.1250915-4-xu.yang_2@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/6] dt-bindings: usb: usbmisc-imx: add fsl,imx8ulp-usbmisc compatible | expand |
Hi, Am Freitag, 12. Januar 2024, 12:17:45 CET schrieb Xu Yang: > The i.MX93 needs a wakup clock to work properly. This will add compatible > and restriction for i.MX93 platform. > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> Looks good to me: Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > --- > Changes in v2: > - no changes > Changes in v3: > - add clocks restriction > --- > .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml index > b7e664f7395b..6e75099b6394 100644 > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > @@ -57,6 +57,7 @@ properties: > - enum: > - fsl,imx8mm-usb > - fsl,imx8mn-usb > + - fsl,imx93-usb > - const: fsl,imx7d-usb > - const: fsl,imx27-usb > - items: > @@ -412,6 +413,21 @@ allOf: > samsung,picophy-pre-emp-curr-control: false > samsung,picophy-dc-vol-level-adjust: false > > + - if: > + properties: > + compatible: > + contains: > + const: fsl,imx93-usb > + then: > + properties: > + clock-names: > + items: > + - const: usb_ctrl_root_clk > + - const: usb_wakeup_clk > + clocks: > + minItems: 2 > + maxItems: 2 > + > unevaluatedProperties: false > > examples:
On 12/01/2024 12:17, Xu Yang wrote: > The i.MX93 needs a wakup clock to work properly. This will add compatible > and restriction for i.MX93 platform. > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > > --- > Changes in v2: > - no changes > Changes in v3: > - add clocks restriction > --- > .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > index b7e664f7395b..6e75099b6394 100644 > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > @@ -57,6 +57,7 @@ properties: > - enum: > - fsl,imx8mm-usb > - fsl,imx8mn-usb > + - fsl,imx93-usb > - const: fsl,imx7d-usb > - const: fsl,imx27-usb > - items: > @@ -412,6 +413,21 @@ allOf: > samsung,picophy-pre-emp-curr-control: false > samsung,picophy-dc-vol-level-adjust: false > > + - if: > + properties: > + compatible: > + contains: > + const: fsl,imx93-usb > + then: > + properties: > + clock-names: > + items: > + - const: usb_ctrl_root_clk > + - const: usb_wakeup_clk > + clocks: > + minItems: 2 > + maxItems: 2 Nothing improved regarding my comments. Why do you allow any/unspecific/unconstrain interrupts and reg? You said: "However, reset, reg and interrupts property is not special for imx93." but what does it even mean? How they can be special or not special? My comments from previous version apply. If you do not want to work on existing technical debt, BTW added by another NXP employee, then I will NAK any new submissions. Best regards, Krzysztof
Hi Krzysztof, > > On 12/01/2024 12:17, Xu Yang wrote: > > The i.MX93 needs a wakup clock to work properly. This will add compatible > > and restriction for i.MX93 platform. > > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > > > > --- > > Changes in v2: > > - no changes > > Changes in v3: > > - add clocks restriction > > --- > > .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci- > hdrc-usb2.yaml > > index b7e664f7395b..6e75099b6394 100644 > > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > > @@ -57,6 +57,7 @@ properties: > > - enum: > > - fsl,imx8mm-usb > > - fsl,imx8mn-usb > > + - fsl,imx93-usb > > - const: fsl,imx7d-usb > > - const: fsl,imx27-usb > > - items: > > @@ -412,6 +413,21 @@ allOf: > > samsung,picophy-pre-emp-curr-control: false > > samsung,picophy-dc-vol-level-adjust: false > > > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: fsl,imx93-usb > > + then: > > + properties: > > + clock-names: > > + items: > > + - const: usb_ctrl_root_clk > > + - const: usb_wakeup_clk > > + clocks: > > + minItems: 2 > > + maxItems: 2 > > Nothing improved regarding my comments. Why do you allow > any/unspecific/unconstrain interrupts and reg? > > You said: > "However, reset, reg and interrupts property is not special for imx93." > but what does it even mean? How they can be special or not special? > > My comments from previous version apply. If you do not want to work on > existing technical debt, BTW added by another NXP employee, then I will > NAK any new submissions. You want me to adjust below properties to be more common properties and add device specific limitations, right? --- reg: minItems: 1 maxItems: 2 interrupts: minItems: 1 maxItems: 2 clocks: minItems: 1 maxItems: 3 clock-names: minItems: 1 maxItems: 3 --- For most of the devices, property reg, interrupts, clocks and clock-names has 1 item. So these properties can set maxItems to 1. For special devices, I should list them standalone, is this your expectation? Thanks, Xu Yang > > Best regards, > Krzysztof
On 16/01/2024 08:49, Xu Yang wrote: > Hi Krzysztof, > >> >> On 12/01/2024 12:17, Xu Yang wrote: >>> The i.MX93 needs a wakup clock to work properly. This will add compatible >>> and restriction for i.MX93 platform. >>> >>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com> >>> >>> --- >>> Changes in v2: >>> - no changes >>> Changes in v3: >>> - add clocks restriction >>> --- >>> .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 16 ++++++++++++++++ >>> 1 file changed, 16 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci- >> hdrc-usb2.yaml >>> index b7e664f7395b..6e75099b6394 100644 >>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml >>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml >>> @@ -57,6 +57,7 @@ properties: >>> - enum: >>> - fsl,imx8mm-usb >>> - fsl,imx8mn-usb >>> + - fsl,imx93-usb >>> - const: fsl,imx7d-usb >>> - const: fsl,imx27-usb >>> - items: >>> @@ -412,6 +413,21 @@ allOf: >>> samsung,picophy-pre-emp-curr-control: false >>> samsung,picophy-dc-vol-level-adjust: false >>> >>> + - if: >>> + properties: >>> + compatible: >>> + contains: >>> + const: fsl,imx93-usb >>> + then: >>> + properties: >>> + clock-names: >>> + items: >>> + - const: usb_ctrl_root_clk >>> + - const: usb_wakeup_clk >>> + clocks: >>> + minItems: 2 >>> + maxItems: 2 >> >> Nothing improved regarding my comments. Why do you allow >> any/unspecific/unconstrain interrupts and reg? >> >> You said: >> "However, reset, reg and interrupts property is not special for imx93." >> but what does it even mean? How they can be special or not special? >> >> My comments from previous version apply. If you do not want to work on >> existing technical debt, BTW added by another NXP employee, then I will >> NAK any new submissions. > > You want me to adjust below properties to be more common properties > and add device specific limitations, right? Yes > > --- > reg: > minItems: 1 > maxItems: 2 > > interrupts: > minItems: 1 > maxItems: 2 > > clocks: > minItems: 1 > maxItems: 3 > > clock-names: > minItems: 1 > maxItems: 3 > --- > > For most of the devices, property reg, interrupts, clocks and clock-names > has 1 item. So these properties can set maxItems to 1. For special devices, > I should list them standalone, is this your expectation? Just like you constrain clocks for new variant, your variant should have constrained reg, interrupts and whatever else is there variable. I don't require fixing all the devices in this binding, but at least your new one and preferably other NXP as well. Best regards, Krzysztof
Hi Krzysztof, > > On 16/01/2024 08:49, Xu Yang wrote: > > Hi Krzysztof, > > > >> > >> On 12/01/2024 12:17, Xu Yang wrote: > >>> The i.MX93 needs a wakup clock to work properly. This will add compatible > >>> and restriction for i.MX93 platform. > >>> > >>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > >>> > >>> --- > >>> Changes in v2: > >>> - no changes > >>> Changes in v3: > >>> - add clocks restriction > >>> --- > >>> .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 16 ++++++++++++++++ > >>> 1 file changed, 16 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci- > >> hdrc-usb2.yaml > >>> index b7e664f7395b..6e75099b6394 100644 > >>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > >>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > >>> @@ -57,6 +57,7 @@ properties: > >>> - enum: > >>> - fsl,imx8mm-usb > >>> - fsl,imx8mn-usb > >>> + - fsl,imx93-usb > >>> - const: fsl,imx7d-usb > >>> - const: fsl,imx27-usb > >>> - items: > >>> @@ -412,6 +413,21 @@ allOf: > >>> samsung,picophy-pre-emp-curr-control: false > >>> samsung,picophy-dc-vol-level-adjust: false > >>> > >>> + - if: > >>> + properties: > >>> + compatible: > >>> + contains: > >>> + const: fsl,imx93-usb > >>> + then: > >>> + properties: > >>> + clock-names: > >>> + items: > >>> + - const: usb_ctrl_root_clk > >>> + - const: usb_wakeup_clk > >>> + clocks: > >>> + minItems: 2 > >>> + maxItems: 2 > >> > >> Nothing improved regarding my comments. Why do you allow > >> any/unspecific/unconstrain interrupts and reg? > >> > >> You said: > >> "However, reset, reg and interrupts property is not special for imx93." > >> but what does it even mean? How they can be special or not special? > >> > >> My comments from previous version apply. If you do not want to work on > >> existing technical debt, BTW added by another NXP employee, then I will > >> NAK any new submissions. > > > > You want me to adjust below properties to be more common properties > > and add device specific limitations, right? > > Yes > > > > > --- > > reg: > > minItems: 1 > > maxItems: 2 > > > > interrupts: > > minItems: 1 > > maxItems: 2 > > > > clocks: > > minItems: 1 > > maxItems: 3 > > > > clock-names: > > minItems: 1 > > maxItems: 3 > > --- > > > > For most of the devices, property reg, interrupts, clocks and clock-names > > has 1 item. So these properties can set maxItems to 1. For special devices, > > I should list them standalone, is this your expectation? > > Just like you constrain clocks for new variant, your variant should have > constrained reg, interrupts and whatever else is there variable. I don't > require fixing all the devices in this binding, but at least your new > one and preferably other NXP as well. Okay. I will do this and test it. Thanks, Xu Yang
Hi Krzysztof, > > On 16/01/2024 08:49, Xu Yang wrote: > > Hi Krzysztof, > > > >> > >> On 12/01/2024 12:17, Xu Yang wrote: > >>> The i.MX93 needs a wakup clock to work properly. This will add compatible > >>> and restriction for i.MX93 platform. > >>> > >>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > >>> > >>> --- > >>> Changes in v2: > >>> - no changes > >>> Changes in v3: > >>> - add clocks restriction > >>> --- > >>> .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 16 ++++++++++++++++ > >>> 1 file changed, 16 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci- > >> hdrc-usb2.yaml > >>> index b7e664f7395b..6e75099b6394 100644 > >>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > >>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > >>> @@ -57,6 +57,7 @@ properties: > >>> - enum: > >>> - fsl,imx8mm-usb > >>> - fsl,imx8mn-usb > >>> + - fsl,imx93-usb > >>> - const: fsl,imx7d-usb > >>> - const: fsl,imx27-usb > >>> - items: > >>> @@ -412,6 +413,21 @@ allOf: > >>> samsung,picophy-pre-emp-curr-control: false > >>> samsung,picophy-dc-vol-level-adjust: false > >>> > >>> + - if: > >>> + properties: > >>> + compatible: > >>> + contains: > >>> + const: fsl,imx93-usb > >>> + then: > >>> + properties: > >>> + clock-names: > >>> + items: > >>> + - const: usb_ctrl_root_clk > >>> + - const: usb_wakeup_clk > >>> + clocks: > >>> + minItems: 2 > >>> + maxItems: 2 > >> > >> Nothing improved regarding my comments. Why do you allow > >> any/unspecific/unconstrain interrupts and reg? > >> > >> You said: > >> "However, reset, reg and interrupts property is not special for imx93." > >> but what does it even mean? How they can be special or not special? > >> > >> My comments from previous version apply. If you do not want to work on > >> existing technical debt, BTW added by another NXP employee, then I will > >> NAK any new submissions. > > > > You want me to adjust below properties to be more common properties > > and add device specific limitations, right? > > Yes > > > > > --- > > reg: > > minItems: 1 > > maxItems: 2 > > > > interrupts: > > minItems: 1 > > maxItems: 2 > > > > clocks: > > minItems: 1 > > maxItems: 3 > > > > clock-names: > > minItems: 1 > > maxItems: 3 > > --- > > > > For most of the devices, property reg, interrupts, clocks and clock-names > > has 1 item. So these properties can set maxItems to 1. For special devices, > > I should list them standalone, is this your expectation? > > Just like you constrain clocks for new variant, your variant should have > constrained reg, interrupts and whatever else is there variable. I don't > require fixing all the devices in this binding, but at least your new > one and preferably other NXP as well. > I'm tring to set the maxItems of property reg, interrupts, clocks and clock-names to 1, then constrain the minItems and maxItems to 3 for one soc later like below, in such way I needn't to constrain reg and interrupts for most of the socs. But dt-validate failed at the first place when validate clocks property. Is there a way to achieve this? --- reg: maxItems: 1 interrupts: maxItems: 1 clocks: maxItems: 1 clock-names: maxItems: 1 allOf: - if: properties: compatible: oneOf: - items: - const: fsl,imx27-usb then: properties: clocks: minItems: 3 maxItems: 3 clock-names: minItems: 3 maxItems: 3 --- Thanks, Xu Yang
diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml index b7e664f7395b..6e75099b6394 100644 --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml @@ -57,6 +57,7 @@ properties: - enum: - fsl,imx8mm-usb - fsl,imx8mn-usb + - fsl,imx93-usb - const: fsl,imx7d-usb - const: fsl,imx27-usb - items: @@ -412,6 +413,21 @@ allOf: samsung,picophy-pre-emp-curr-control: false samsung,picophy-dc-vol-level-adjust: false + - if: + properties: + compatible: + contains: + const: fsl,imx93-usb + then: + properties: + clock-names: + items: + - const: usb_ctrl_root_clk + - const: usb_wakeup_clk + clocks: + minItems: 2 + maxItems: 2 + unevaluatedProperties: false examples:
The i.MX93 needs a wakup clock to work properly. This will add compatible and restriction for i.MX93 platform. Signed-off-by: Xu Yang <xu.yang_2@nxp.com> --- Changes in v2: - no changes Changes in v3: - add clocks restriction --- .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)