| Message ID | 20220624155413.399190-2-fabrice.gasnier@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: ucsi: add support for stm32g0 | expand |
On 24/06/2022 17:54, Fabrice Gasnier wrote: > This patch adds DT schema documentation for the STM32G0 Type-C controller. No "This patch" https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > STM32G0 provides an integrated USB Type-C and power delivery interface. > It can be programmed with a firmware to handle UCSI protocol over I2C > interface. A GPIO is used as an interrupt line. > It may be used as a wakeup source, so use optional "wakeup-source" and > "power-domains" properties to support wakeup. > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> > --- > .../bindings/usb/st,typec-stm32g0.yaml | 83 +++++++++++++++++++ > 1 file changed, 83 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml > > diff --git a/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml > new file mode 100644 > index 0000000000000..b2729bd015a1a > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml > @@ -0,0 +1,83 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/usb/st,typec-stm32g0.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" No quotes. > + > +title: STMicroelectronics STM32G0 Type-C controller bindings s/bindings// > + > +description: | > + The STM32G0 MCU can be programmed to control Type-C connector(s) through I2C > + typically using the UCSI protocol over I2C, with a dedicated alert > + (interrupt) pin. > + > +maintainers: > + - Fabrice Gasnier <fabrice.gasnier@foss.st.com> > + > +properties: > + compatible: > + const: st,stm32g0-typec > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + connector: > + type: object> + allOf: > + - $ref: ../connector/usb-connector.yaml# Full path, so /schemas/connector/... unevaluatedProperties: false > + > + firmware-name: > + description: | > + Should contain the name of the default firmware image > + file located on the firmware search path > + > + wakeup-source: true > + power-domains: true maxItems > + > +required: > + - compatible > + - reg > + - interrupts > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + i2c5 { Just "i2c" > + #address-cells = <1>; > + #size-cells = <0>; > + > + stm32g0@53 { Generic node name describing class of the device. > + compatible = "st,stm32g0-typec"; > + reg = <0x53>; > + /* Alert pin on GPIO PE12 */ > + interrupts = <12 IRQ_TYPE_EDGE_FALLING>; > + interrupt-parent = <&gpioe>; > + > + /* Example with one type-C connector */ > + connector { > + compatible = "usb-c-connector"; > + label = "USB-C"; > + > + port { This does not look like proper schema of connector.yaml. > + con_usb_c_ep: endpoint { > + remote-endpoint = <&usbotg_hs_ep>; > + }; > + }; > + }; > + }; > + }; > + > + usbotg_hs { Generic node names, no underscores in node names. > + usb-role-switch; > + port { > + usbotg_hs_ep: endpoint { > + remote-endpoint = <&con_usb_c_ep>; > + }; > + }; > + }; > +... Best regards, Krzysztof
On 6/24/22 18:16, Krzysztof Kozlowski wrote: > On 24/06/2022 17:54, Fabrice Gasnier wrote: >> This patch adds DT schema documentation for the STM32G0 Type-C controller. > > No "This patch" Hi Krzysztof, ack, > > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > >> STM32G0 provides an integrated USB Type-C and power delivery interface. >> It can be programmed with a firmware to handle UCSI protocol over I2C >> interface. A GPIO is used as an interrupt line. >> It may be used as a wakeup source, so use optional "wakeup-source" and >> "power-domains" properties to support wakeup. >> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> >> --- >> .../bindings/usb/st,typec-stm32g0.yaml | 83 +++++++++++++++++++ >> 1 file changed, 83 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml >> >> diff --git a/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml >> new file mode 100644 >> index 0000000000000..b2729bd015a1a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml >> @@ -0,0 +1,83 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: "http://devicetree.org/schemas/usb/st,typec-stm32g0.yaml#" >> +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > > No quotes. ack, > >> + >> +title: STMicroelectronics STM32G0 Type-C controller bindings > > s/bindings// ack, > >> + >> +description: | >> + The STM32G0 MCU can be programmed to control Type-C connector(s) through I2C >> + typically using the UCSI protocol over I2C, with a dedicated alert >> + (interrupt) pin. >> + >> +maintainers: >> + - Fabrice Gasnier <fabrice.gasnier@foss.st.com> >> + >> +properties: >> + compatible: >> + const: st,stm32g0-typec >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts: >> + maxItems: 1 >> + >> + connector: >> + type: object> + allOf: >> + - $ref: ../connector/usb-connector.yaml# > > Full path, so /schemas/connector/... > > unevaluatedProperties: false ack, > >> + >> + firmware-name: >> + description: | >> + Should contain the name of the default firmware image >> + file located on the firmware search path >> + >> + wakeup-source: true >> + power-domains: true > > maxItems Do you mean maxItems regarding the "power-domains" property ? This will depend on the user platform, where it's used as an I2C device. So I'm not sure this can / should be specified here. Could please you clarify ? > >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/irq.h> >> + i2c5 { > > Just "i2c" ack, > >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + stm32g0@53 { > > Generic node name describing class of the device. I wasn't aware of generic node name for an I2C device (not talking of the controller). I may have missed it. Could you please clarify ? > >> + compatible = "st,stm32g0-typec"; >> + reg = <0x53>; >> + /* Alert pin on GPIO PE12 */ >> + interrupts = <12 IRQ_TYPE_EDGE_FALLING>; >> + interrupt-parent = <&gpioe>; >> + >> + /* Example with one type-C connector */ >> + connector { >> + compatible = "usb-c-connector"; >> + label = "USB-C"; >> + >> + port { > > This does not look like proper schema of connector.yaml. This refers to graph.yaml [1], where similar example is seen [2]. https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L79 https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L207 device-1 { port { device_1_output: endpoint { remote-endpoint = <&device_2_input>; }; }; }; device-2 { port { device_2_input: endpoint { remote-endpoint = <&device_1_output>; }; }; }; Could you please clarify this point too ? > >> + con_usb_c_ep: endpoint { >> + remote-endpoint = <&usbotg_hs_ep>; >> + }; >> + }; >> + }; >> + }; >> + }; >> + >> + usbotg_hs { > > Generic node names, no underscores in node names. ack, I guess you'd recommend "usb" here. I'll update it. Thanks for reviewing, Best Regards, Fabrice > >> + usb-role-switch; >> + port { >> + usbotg_hs_ep: endpoint { >> + remote-endpoint = <&con_usb_c_ep>; >> + }; >> + }; >> + }; >> +... > > > Best regards, > Krzysztof
On 27/06/2022 16:21, Fabrice Gasnier wrote: > On 6/24/22 18:16, Krzysztof Kozlowski wrote: >> On 24/06/2022 17:54, Fabrice Gasnier wrote: >>> This patch adds DT schema documentation for the STM32G0 Type-C controller. >> >> No "This patch" > > Hi Krzysztof, > > ack, > >> >> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 >> >>> STM32G0 provides an integrated USB Type-C and power delivery interface. >>> It can be programmed with a firmware to handle UCSI protocol over I2C >>> interface. A GPIO is used as an interrupt line. >>> It may be used as a wakeup source, so use optional "wakeup-source" and >>> "power-domains" properties to support wakeup. >>> >>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> >>> --- >>> .../bindings/usb/st,typec-stm32g0.yaml | 83 +++++++++++++++++++ >>> 1 file changed, 83 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml >>> new file mode 100644 >>> index 0000000000000..b2729bd015a1a >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml >>> @@ -0,0 +1,83 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: "http://devicetree.org/schemas/usb/st,typec-stm32g0.yaml#" >>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#" >> >> No quotes. > > ack, > >> >>> + >>> +title: STMicroelectronics STM32G0 Type-C controller bindings >> >> s/bindings// > > ack, > >> >>> + >>> +description: | >>> + The STM32G0 MCU can be programmed to control Type-C connector(s) through I2C >>> + typically using the UCSI protocol over I2C, with a dedicated alert >>> + (interrupt) pin. >>> + >>> +maintainers: >>> + - Fabrice Gasnier <fabrice.gasnier@foss.st.com> >>> + >>> +properties: >>> + compatible: >>> + const: st,stm32g0-typec >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + interrupts: >>> + maxItems: 1 >>> + >>> + connector: >>> + type: object> + allOf: >>> + - $ref: ../connector/usb-connector.yaml# >> >> Full path, so /schemas/connector/... >> >> unevaluatedProperties: false > > ack, > >> >>> + >>> + firmware-name: >>> + description: | >>> + Should contain the name of the default firmware image >>> + file located on the firmware search path >>> + >>> + wakeup-source: true >>> + power-domains: true >> >> maxItems > > Do you mean maxItems regarding the "power-domains" property ? Yes. > This will depend on the user platform, where it's used as an I2C device. > So I'm not sure this can / should be specified here. > Could please you clarify ? Then maybe this property is not valid here. Power domains usually are used for blocks of a SoC, having common power source and power gating. In your case it looks much more like a regulator supply. > >> >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - interrupts >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/interrupt-controller/irq.h> >>> + i2c5 { >> >> Just "i2c" > > ack, > >> >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + stm32g0@53 { >> >> Generic node name describing class of the device. > > > I wasn't aware of generic node name for an I2C device (not talking of > the controller). I may have missed it. > > Could you please clarify ? The class of a device is not a I2C device. I2C is just a bus. For example the generic name for Power Management IC connected over I2C (quite common case) is "pmic". For USB HCD controllers the generic name is "usb". For USB ports/connectors this is "connector". So what is your hardware? "interface" is a bit too unspecific to figure it out. > >> >>> + compatible = "st,stm32g0-typec"; >>> + reg = <0x53>; >>> + /* Alert pin on GPIO PE12 */ >>> + interrupts = <12 IRQ_TYPE_EDGE_FALLING>; >>> + interrupt-parent = <&gpioe>; >>> + >>> + /* Example with one type-C connector */ >>> + connector { >>> + compatible = "usb-c-connector"; >>> + label = "USB-C"; >>> + >>> + port { >> >> This does not look like proper schema of connector.yaml. > > This refers to graph.yaml [1], where similar example is seen [2]. > > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L79 > > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L207 Just look at the usb-conector schema. It's different. You miss ports. Maybe other properties as well. > > device-1 { > port { > device_1_output: endpoint { > remote-endpoint = <&device_2_input>; > }; > }; > }; > device-2 { > port { > device_2_input: endpoint { > remote-endpoint = <&device_1_output>; > }; > }; > }; > > > Could you please clarify this point too ? > >> >>> + con_usb_c_ep: endpoint { >>> + remote-endpoint = <&usbotg_hs_ep>; >>> + }; >>> + }; >>> + }; >>> + }; >>> + }; >>> + >>> + usbotg_hs { >> >> Generic node names, no underscores in node names. > > ack, I guess you'd recommend "usb" here. I'll update it. Yes, looks like usb. Best regards, Krzysztof
On 6/28/22 12:28, Krzysztof Kozlowski wrote: > On 27/06/2022 16:21, Fabrice Gasnier wrote: >> On 6/24/22 18:16, Krzysztof Kozlowski wrote: >>> On 24/06/2022 17:54, Fabrice Gasnier wrote: >>>> This patch adds DT schema documentation for the STM32G0 Type-C controller. >>> >>> No "This patch" >> >> Hi Krzysztof, >> >> ack, >> >>> >>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 >>> >>>> STM32G0 provides an integrated USB Type-C and power delivery interface. >>>> It can be programmed with a firmware to handle UCSI protocol over I2C >>>> interface. A GPIO is used as an interrupt line. >>>> It may be used as a wakeup source, so use optional "wakeup-source" and >>>> "power-domains" properties to support wakeup. >>>> >>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> >>>> --- >>>> .../bindings/usb/st,typec-stm32g0.yaml | 83 +++++++++++++++++++ >>>> 1 file changed, 83 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml >>>> >>>> diff --git a/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml >>>> new file mode 100644 >>>> index 0000000000000..b2729bd015a1a >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml >>>> @@ -0,0 +1,83 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id: "http://devicetree.org/schemas/usb/st,typec-stm32g0.yaml#" >>>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#" >>> >>> No quotes. >> >> ack, >> >>> >>>> + >>>> +title: STMicroelectronics STM32G0 Type-C controller bindings >>> >>> s/bindings// >> >> ack, >> >>> >>>> + >>>> +description: | >>>> + The STM32G0 MCU can be programmed to control Type-C connector(s) through I2C >>>> + typically using the UCSI protocol over I2C, with a dedicated alert >>>> + (interrupt) pin. >>>> + >>>> +maintainers: >>>> + - Fabrice Gasnier <fabrice.gasnier@foss.st.com> >>>> + >>>> +properties: >>>> + compatible: >>>> + const: st,stm32g0-typec >>>> + >>>> + reg: >>>> + maxItems: 1 >>>> + >>>> + interrupts: >>>> + maxItems: 1 >>>> + >>>> + connector: >>>> + type: object> + allOf: >>>> + - $ref: ../connector/usb-connector.yaml# >>> >>> Full path, so /schemas/connector/... >>> >>> unevaluatedProperties: false Hi Krzysztof, I Just figured out usb-connector schema has "additionalProperties: true". Adding "unevaluatedProperties: false" here seem to be useless. At least at my end, this make any dummy property added in the example below to be validated without error by the schema. Should this be updated in usb-connector.yaml instead ? Shall I omit it here in the end ? >> >> ack, >> >>> >>>> + >>>> + firmware-name: >>>> + description: | >>>> + Should contain the name of the default firmware image >>>> + file located on the firmware search path >>>> + >>>> + wakeup-source: true >>>> + power-domains: true >>> >>> maxItems >> >> Do you mean maxItems regarding the "power-domains" property ? > > Yes. > >> This will depend on the user platform, where it's used as an I2C device. >> So I'm not sure this can / should be specified here. >> Could please you clarify ? > > Then maybe this property is not valid here. Power domains usually are > used for blocks of a SoC, having common power source and power gating. > In your case it looks much more like a regulator supply. This property is used in our implementation to refer to SOC PM domain for GPIO that is used to wakeup the system. This isn't only a regulator, this PM domain serves various IPs such as I2C, GPIO, UART... (it manages regulator and clocks used in low power). I can limit to 1 item if this is fine for you ? e.g. maxItems: 1 > >> >>> >>>> + >>>> +required: >>>> + - compatible >>>> + - reg >>>> + - interrupts >>>> + >>>> +additionalProperties: false >>>> + >>>> +examples: >>>> + - | >>>> + #include <dt-bindings/interrupt-controller/irq.h> >>>> + i2c5 { >>> >>> Just "i2c" >> >> ack, >> >>> >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + >>>> + stm32g0@53 { >>> >>> Generic node name describing class of the device. >> >> >> I wasn't aware of generic node name for an I2C device (not talking of >> the controller). I may have missed it. >> >> Could you please clarify ? > > The class of a device is not a I2C device. I2C is just a bus. For > example the generic name for Power Management IC connected over I2C > (quite common case) is "pmic". > > For USB HCD controllers the generic name is "usb". For USB > ports/connectors this is "connector". So what is your hardware? > "interface" is a bit too unspecific to figure it out. Thanks, I better understand your point now. A common definition for the hardware here could be "USB Type-C PD controller". I'll improve this schema title by the way. I had a quick look in various .dts files. I could find mainly: - typec-portc@hh - usb-typec@hh - typec@hh Not sure if this has already been discussed in other reviews, it lacks the "controller" idea in the naming IMHO. Perhaps something like "typec-pd-controller" or "usb-typec-pd-controller" could be used here ? Otherwise, I could adopt the shortest "typec" name if it's fine for you ? > >> >>> >>>> + compatible = "st,stm32g0-typec"; >>>> + reg = <0x53>; >>>> + /* Alert pin on GPIO PE12 */ >>>> + interrupts = <12 IRQ_TYPE_EDGE_FALLING>; >>>> + interrupt-parent = <&gpioe>; >>>> + >>>> + /* Example with one type-C connector */ >>>> + connector { >>>> + compatible = "usb-c-connector"; >>>> + label = "USB-C"; >>>> + >>>> + port { >>> >>> This does not look like proper schema of connector.yaml. >> >> This refers to graph.yaml [1], where similar example is seen [2]. >> >> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L79 >> >> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L207 > > Just look at the usb-conector schema. It's different. You miss ports. > Maybe other properties as well. (I may miss something, and got confused around port/ports earlier) The graph properties seems to allow both the 'port' and 'ports' syntax thanks to the graph definition. The "port" syntax is also used in other typec controller schemas. There's only one port in this example. Of course other example could use two or more ports (like for USB HS / SS / aux) which would require using the "ports" node (with port@0/1/2 childs). I can adopt the "ports" node if you prefer. As I see it just doesn't bring much in the current example (The only drawback is this adds one indentation/node level w.r.t. the bellow example, so not a big deal). Please advise, Thanks for reviewing, Best Regards, Fabrice > >> >> device-1 { >> port { >> device_1_output: endpoint { >> remote-endpoint = <&device_2_input>; >> }; >> }; >> }; >> device-2 { >> port { >> device_2_input: endpoint { >> remote-endpoint = <&device_1_output>; >> }; >> }; >> }; >> >> >> Could you please clarify this point too ? >> >>> >>>> + con_usb_c_ep: endpoint { >>>> + remote-endpoint = <&usbotg_hs_ep>; >>>> + }; >>>> + }; >>>> + }; >>>> + }; >>>> + }; >>>> + >>>> + usbotg_hs { >>> >>> Generic node names, no underscores in node names. >> >> ack, I guess you'd recommend "usb" here. I'll update it. > > Yes, looks like usb. > > > Best regards, > Krzysztof
On 28/06/2022 19:01, Fabrice Gasnier wrote: >>>>> + connector: >>>>> + type: object> + allOf: >>>>> + - $ref: ../connector/usb-connector.yaml# >>>> >>>> Full path, so /schemas/connector/... >>>> >>>> unevaluatedProperties: false > > Hi Krzysztof, > > I Just figured out usb-connector schema has "additionalProperties: > true". Adding "unevaluatedProperties: false" here seem to be useless. > At least at my end, this make any dummy property added in the example > below to be validated without error by the schema. No, it's expected. The common schema allows additional properties. You specific device schema (including common) should not allow anything more and this is expressed like you mentioned. However depending on the version of dtschema, the unevaluatedProperties:false might still be not implemented. AFAIK, Rob added it quite recently. > > Should this be updated in usb-connector.yaml instead ? No > > Shall I omit it here in the end ? You need to add here unevaluatedProperties: false (on the level of this $ref) > >>> >>> ack, >>> >>>> >>>>> + >>>>> + firmware-name: >>>>> + description: | >>>>> + Should contain the name of the default firmware image >>>>> + file located on the firmware search path >>>>> + >>>>> + wakeup-source: true >>>>> + power-domains: true >>>> >>>> maxItems >>> >>> Do you mean maxItems regarding the "power-domains" property ? >> >> Yes. >> >>> This will depend on the user platform, where it's used as an I2C device. >>> So I'm not sure this can / should be specified here. >>> Could please you clarify ? >> >> Then maybe this property is not valid here. Power domains usually are >> used for blocks of a SoC, having common power source and power gating. >> In your case it looks much more like a regulator supply. > > This property is used in our implementation to refer to SOC PM domain > for GPIO that is used to wakeup the system. This isn't only a regulator, > this PM domain serves various IPs such as I2C, GPIO, UART... (it manages > regulator and clocks used in low power). > > I can limit to 1 item if this is fine for you ? > > e.g. maxItems: 1 Yes, it's good (assuming it is true :) ). > >> >>> >>>> >>>>> + >>>>> +required: >>>>> + - compatible >>>>> + - reg >>>>> + - interrupts >>>>> + >>>>> +additionalProperties: false >>>>> + >>>>> +examples: >>>>> + - | >>>>> + #include <dt-bindings/interrupt-controller/irq.h> >>>>> + i2c5 { >>>> >>>> Just "i2c" >>> >>> ack, >>> >>>> >>>>> + #address-cells = <1>; >>>>> + #size-cells = <0>; >>>>> + >>>>> + stm32g0@53 { >>>> >>>> Generic node name describing class of the device. >>> >>> >>> I wasn't aware of generic node name for an I2C device (not talking of >>> the controller). I may have missed it. >>> >>> Could you please clarify ? >> >> The class of a device is not a I2C device. I2C is just a bus. For >> example the generic name for Power Management IC connected over I2C >> (quite common case) is "pmic". >> >> For USB HCD controllers the generic name is "usb". For USB >> ports/connectors this is "connector". So what is your hardware? >> "interface" is a bit too unspecific to figure it out. > > Thanks, I better understand your point now. > > A common definition for the hardware here could be "USB Type-C PD > controller". I'll improve this schema title by the way. > > I had a quick look in various .dts files. I could find mainly: > - typec-portc@hh > - usb-typec@hh > - typec@hh > > Not sure if this has already been discussed in other reviews, it lacks > the "controller" idea in the naming IMHO. > Perhaps something like "typec-pd-controller" or > "usb-typec-pd-controller" could be used here ? > > Otherwise, I could adopt the shortest "typec" name if it's fine for you ? typec sounds good. > >> >>> >>>> >>>>> + compatible = "st,stm32g0-typec"; >>>>> + reg = <0x53>; >>>>> + /* Alert pin on GPIO PE12 */ >>>>> + interrupts = <12 IRQ_TYPE_EDGE_FALLING>; >>>>> + interrupt-parent = <&gpioe>; >>>>> + >>>>> + /* Example with one type-C connector */ >>>>> + connector { >>>>> + compatible = "usb-c-connector"; >>>>> + label = "USB-C"; >>>>> + >>>>> + port { >>>> >>>> This does not look like proper schema of connector.yaml. >>> >>> This refers to graph.yaml [1], where similar example is seen [2]. >>> >>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L79 >>> >>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L207 >> >> Just look at the usb-conector schema. It's different. You miss ports. >> Maybe other properties as well. > > > (I may miss something, and got confused around port/ports earlier) > The graph properties seems to allow both the 'port' and 'ports' syntax > thanks to the graph definition. > The "port" syntax is also used in other typec controller schemas. > > There's only one port in this example. Of course other example could use > two or more ports (like for USB HS / SS / aux) which would require using > the "ports" node (with port@0/1/2 childs). > > I can adopt the "ports" node if you prefer. As I see it just doesn't > bring much in the current example (The only drawback is this adds one > indentation/node level w.r.t. the bellow example, so not a big deal). The graph schema allows, but you include here usb-connector schema which requires to put it under "ports". You should not use it differently, so I expect here "ports" property, even with one port. Best regards, Krzysztof
On 6/29/22 07:54, Krzysztof Kozlowski wrote: > On 28/06/2022 19:01, Fabrice Gasnier wrote: > >>>>>> + connector: >>>>>> + type: object> + allOf: >>>>>> + - $ref: ../connector/usb-connector.yaml# >>>>> >>>>> Full path, so /schemas/connector/... >>>>> >>>>> unevaluatedProperties: false >> >> Hi Krzysztof, >> >> I Just figured out usb-connector schema has "additionalProperties: >> true". Adding "unevaluatedProperties: false" here seem to be useless. >> At least at my end, this make any dummy property added in the example >> below to be validated without error by the schema. > > No, it's expected. The common schema allows additional properties. You > specific device schema (including common) should not allow anything more > and this is expressed like you mentioned. > > However depending on the version of dtschema, the > unevaluatedProperties:false might still be not implemented. AFAIK, Rob > added it quite recently. > >> >> Should this be updated in usb-connector.yaml instead ? > > No > >> >> Shall I omit it here in the end ? > > You need to add here unevaluatedProperties: false (on the level of this > $ref) > >> >>>> >>>> ack, >>>> >>>>> >>>>>> + >>>>>> + firmware-name: >>>>>> + description: | >>>>>> + Should contain the name of the default firmware image >>>>>> + file located on the firmware search path >>>>>> + >>>>>> + wakeup-source: true >>>>>> + power-domains: true >>>>> >>>>> maxItems >>>> >>>> Do you mean maxItems regarding the "power-domains" property ? >>> >>> Yes. >>> >>>> This will depend on the user platform, where it's used as an I2C device. >>>> So I'm not sure this can / should be specified here. >>>> Could please you clarify ? >>> >>> Then maybe this property is not valid here. Power domains usually are >>> used for blocks of a SoC, having common power source and power gating. >>> In your case it looks much more like a regulator supply. >> >> This property is used in our implementation to refer to SOC PM domain >> for GPIO that is used to wakeup the system. This isn't only a regulator, >> this PM domain serves various IPs such as I2C, GPIO, UART... (it manages >> regulator and clocks used in low power). >> >> I can limit to 1 item if this is fine for you ? >> >> e.g. maxItems: 1 > > Yes, it's good (assuming it is true :) ). > >> >>> >>>> >>>>> >>>>>> + >>>>>> +required: >>>>>> + - compatible >>>>>> + - reg >>>>>> + - interrupts >>>>>> + >>>>>> +additionalProperties: false >>>>>> + >>>>>> +examples: >>>>>> + - | >>>>>> + #include <dt-bindings/interrupt-controller/irq.h> >>>>>> + i2c5 { >>>>> >>>>> Just "i2c" >>>> >>>> ack, >>>> >>>>> >>>>>> + #address-cells = <1>; >>>>>> + #size-cells = <0>; >>>>>> + >>>>>> + stm32g0@53 { >>>>> >>>>> Generic node name describing class of the device. >>>> >>>> >>>> I wasn't aware of generic node name for an I2C device (not talking of >>>> the controller). I may have missed it. >>>> >>>> Could you please clarify ? >>> >>> The class of a device is not a I2C device. I2C is just a bus. For >>> example the generic name for Power Management IC connected over I2C >>> (quite common case) is "pmic". >>> >>> For USB HCD controllers the generic name is "usb". For USB >>> ports/connectors this is "connector". So what is your hardware? >>> "interface" is a bit too unspecific to figure it out. >> >> Thanks, I better understand your point now. >> >> A common definition for the hardware here could be "USB Type-C PD >> controller". I'll improve this schema title by the way. >> >> I had a quick look in various .dts files. I could find mainly: >> - typec-portc@hh >> - usb-typec@hh >> - typec@hh >> >> Not sure if this has already been discussed in other reviews, it lacks >> the "controller" idea in the naming IMHO. >> Perhaps something like "typec-pd-controller" or >> "usb-typec-pd-controller" could be used here ? >> >> Otherwise, I could adopt the shortest "typec" name if it's fine for you ? > > typec sounds good. > >> >>> >>>> >>>>> >>>>>> + compatible = "st,stm32g0-typec"; >>>>>> + reg = <0x53>; >>>>>> + /* Alert pin on GPIO PE12 */ >>>>>> + interrupts = <12 IRQ_TYPE_EDGE_FALLING>; >>>>>> + interrupt-parent = <&gpioe>; >>>>>> + >>>>>> + /* Example with one type-C connector */ >>>>>> + connector { >>>>>> + compatible = "usb-c-connector"; >>>>>> + label = "USB-C"; >>>>>> + >>>>>> + port { >>>>> >>>>> This does not look like proper schema of connector.yaml. >>>> >>>> This refers to graph.yaml [1], where similar example is seen [2]. >>>> >>>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L79 >>>> >>>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L207 >>> >>> Just look at the usb-conector schema. It's different. You miss ports. >>> Maybe other properties as well. >> >> >> (I may miss something, and got confused around port/ports earlier) >> The graph properties seems to allow both the 'port' and 'ports' syntax >> thanks to the graph definition. >> The "port" syntax is also used in other typec controller schemas. >> >> There's only one port in this example. Of course other example could use >> two or more ports (like for USB HS / SS / aux) which would require using >> the "ports" node (with port@0/1/2 childs). >> >> I can adopt the "ports" node if you prefer. As I see it just doesn't >> bring much in the current example (The only drawback is this adds one >> indentation/node level w.r.t. the bellow example, so not a big deal). > > The graph schema allows, but you include here usb-connector schema which > requires to put it under "ports". You should not use it differently, so > I expect here "ports" property, even with one port. Hi Krzysztof, This makes senses. I've updated this locally and also put this in .dts file (not sent yet with this series as I lack some dependencies not yet upstream). I'm able to validate the schema, with your statement, by using dt_binding_check. /* Example with one type-C connector */ connector { compatible = "usb-c-connector"; label = "USB-C"; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; con_usb_c_ep: endpoint { remote-endpoint = <&usb_ep>; }; }; }; }; Still when build the .dts (in my downstream, with W=1) I observe various case, for a single port usage (e.g. USB HS only). With above example I get: --- Warning (graph_child_address): /soc/..../connector/ports: graph node has single child node 'port@0', #address-cells/#size-cells are not necessary Remove them as not necessary (suggested by this warning): --- /* Example with one type-C connector */ connector { compatible = "usb-c-connector"; label = "USB-C"; ports { port { con_usb_c_ep: endpoint { remote-endpoint = <&usb_ep>; }; }; }; }; Then I no longer get this warning upon build. But the dtbs_check complains: --- connector: ports: 'port@0' is a required property From schema: .. Documentation/devicetree/bindings/connector/usb-connector.yaml So It looks like to me there's something missing to handle the single port case in usb-connector.yaml, when using the "ports". Maybe usb-connector could be updated to handle "port" (w/o unit-addr) ? I'm talking about: required: - port@0 So, I came up with: --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml @@ -176,6 +176,9 @@ properties: port number as described below. properties: + port: + $ref: /schemas/graph.yaml#/properties/port + port@0: $ref: /schemas/graph.yaml#/properties/port description: High Speed (HS), present in all connectors. @@ -189,8 +192,11 @@ properties: description: Sideband Use (SBU), present in USB-C. This describes the alternate mode connection of which SBU is a part. - required: - - port@0 + oneOf: + - required: + - port + - required: + - port@0 Do you agree on this approach ? (I can add a pre-cursor patch to this series, to handle the single port case) Please advise, Best Regards, Fabrice > > Best regards, > Krzysztof
On 01/07/2022 12:04, Fabrice Gasnier wrote: > > Then I no longer get this warning upon build. But the dtbs_check complains: > --- > connector: ports: 'port@0' is a required property > From schema: .. > Documentation/devicetree/bindings/connector/usb-connector.yaml > > So It looks like to me there's something missing to handle the single > port case in usb-connector.yaml, when using the "ports". > > Maybe usb-connector could be updated to handle "port" (w/o unit-addr) ? Not really, the dtc warning looks false-positive. Especially that you need port@1 for USB 3.0 (super speed), unless you do not support it? > I'm talking about: > required: > - port@0 > > So, I came up with: > > --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml > +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml > @@ -176,6 +176,9 @@ properties: > port number as described below. > > properties: > + port: > + $ref: /schemas/graph.yaml#/properties/port > + > port@0: > $ref: /schemas/graph.yaml#/properties/port > description: High Speed (HS), present in all connectors. > @@ -189,8 +192,11 @@ properties: > description: Sideband Use (SBU), present in USB-C. This > describes the > alternate mode connection of which SBU is a part. > > - required: > - - port@0 > + oneOf: > + - required: > + - port > + - required: > + - port@0 > > > Do you agree on this approach ? (I can add a pre-cursor patch to this > series, to handle the single port case) Best regards, Krzysztof
On 7/4/22 09:55, Krzysztof Kozlowski wrote: > On 01/07/2022 12:04, Fabrice Gasnier wrote: >> >> Then I no longer get this warning upon build. But the dtbs_check complains: >> --- >> connector: ports: 'port@0' is a required property >> From schema: .. >> Documentation/devicetree/bindings/connector/usb-connector.yaml >> >> So It looks like to me there's something missing to handle the single >> port case in usb-connector.yaml, when using the "ports". >> >> Maybe usb-connector could be updated to handle "port" (w/o unit-addr) ? > > Not really, the dtc warning looks false-positive. Especially that you > need port@1 for USB 3.0 (super speed), unless you do not support it? Hi Krzysztof, Having USB2.0 High speed port only is perfectly valid. port@1 is optional to support USB3.0 as you mention. I've no opinion regarding a possible false positive warning. I'd like to sort this out, perhaps Rob has some recommendation regarding this ? Please advise, Best regards, Fabrice > >> I'm talking about: >> required: >> - port@0 >> >> So, I came up with: >> >> --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml >> +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml >> @@ -176,6 +176,9 @@ properties: >> port number as described below. >> >> properties: >> + port: >> + $ref: /schemas/graph.yaml#/properties/port >> + >> port@0: >> $ref: /schemas/graph.yaml#/properties/port >> description: High Speed (HS), present in all connectors. >> @@ -189,8 +192,11 @@ properties: >> description: Sideband Use (SBU), present in USB-C. This >> describes the >> alternate mode connection of which SBU is a part. >> >> - required: >> - - port@0 >> + oneOf: >> + - required: >> + - port >> + - required: >> + - port@0 >> >> >> Do you agree on this approach ? (I can add a pre-cursor patch to this >> series, to handle the single port case) > > > > Best regards, > Krzysztof
On 04/07/2022 11:08, Fabrice Gasnier wrote: > On 7/4/22 09:55, Krzysztof Kozlowski wrote: >> On 01/07/2022 12:04, Fabrice Gasnier wrote: >>> >>> Then I no longer get this warning upon build. But the dtbs_check complains: >>> --- >>> connector: ports: 'port@0' is a required property >>> From schema: .. >>> Documentation/devicetree/bindings/connector/usb-connector.yaml >>> >>> So It looks like to me there's something missing to handle the single >>> port case in usb-connector.yaml, when using the "ports". >>> >>> Maybe usb-connector could be updated to handle "port" (w/o unit-addr) ? >> >> Not really, the dtc warning looks false-positive. Especially that you >> need port@1 for USB 3.0 (super speed), unless you do not support it? > > Hi Krzysztof, > > Having USB2.0 High speed port only is perfectly valid. port@1 is > optional to support USB3.0 as you mention. > > I've no opinion regarding a possible false positive warning. I'd like to > sort this out, perhaps Rob has some recommendation regarding this ? I would propose to skip the DTC warning and stick to the schema with only one port@0. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml new file mode 100644 index 0000000000000..b2729bd015a1a --- /dev/null +++ b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml @@ -0,0 +1,83 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/usb/st,typec-stm32g0.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: STMicroelectronics STM32G0 Type-C controller bindings + +description: | + The STM32G0 MCU can be programmed to control Type-C connector(s) through I2C + typically using the UCSI protocol over I2C, with a dedicated alert + (interrupt) pin. + +maintainers: + - Fabrice Gasnier <fabrice.gasnier@foss.st.com> + +properties: + compatible: + const: st,stm32g0-typec + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + connector: + type: object + allOf: + - $ref: ../connector/usb-connector.yaml# + + firmware-name: + description: | + Should contain the name of the default firmware image + file located on the firmware search path + + wakeup-source: true + power-domains: true + +required: + - compatible + - reg + - interrupts + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + i2c5 { + #address-cells = <1>; + #size-cells = <0>; + + stm32g0@53 { + compatible = "st,stm32g0-typec"; + reg = <0x53>; + /* Alert pin on GPIO PE12 */ + interrupts = <12 IRQ_TYPE_EDGE_FALLING>; + interrupt-parent = <&gpioe>; + + /* Example with one type-C connector */ + connector { + compatible = "usb-c-connector"; + label = "USB-C"; + + port { + con_usb_c_ep: endpoint { + remote-endpoint = <&usbotg_hs_ep>; + }; + }; + }; + }; + }; + + usbotg_hs { + usb-role-switch; + port { + usbotg_hs_ep: endpoint { + remote-endpoint = <&con_usb_c_ep>; + }; + }; + }; +...
This patch adds DT schema documentation for the STM32G0 Type-C controller. STM32G0 provides an integrated USB Type-C and power delivery interface. It can be programmed with a firmware to handle UCSI protocol over I2C interface. A GPIO is used as an interrupt line. It may be used as a wakeup source, so use optional "wakeup-source" and "power-domains" properties to support wakeup. Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> --- .../bindings/usb/st,typec-stm32g0.yaml | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml