STM32G0 provides an integrated USB Type-C and power delivery interface [1].
It can be programmed with a firmware [2] to act as a PPM. Currently it
implements UCSI protocol over I2C interface. A GPIO is used as an interrupt
line.
This series adds a driver to support it, including:
- dt-bindings documentation
- optional STM32G0 firmware control and update, over a secondary I2C address
- power management
[1] https://wiki.st.com/stm32mcu/wiki/Introduction_to_USB_Power_Delivery_with_STM32
[2] https://github.com/STMicroelectronics/x-cube-ucsi
Changes in v2:
- Krzysztof's review comments on dt-bindings: update commit message, use ports,
use unevaluatedProperties: false for usb-connector schema, define maxItems
for power-domains, adopt generic node names, remove quotes.
- Christophe's comments on driver:
use kmalloc instead of kzalloc
Use-after-free of buf: directly print the offset
Fabrice Gasnier (4):
dt-bindings: usb: typec: add bindings for stm32g0 controller
usb: typec: ucsi: stm32g0: add support for stm32g0 i2c controller
usb: typec: ucsi: stm32g0: add bootloader support
usb: typec: ucsi: stm32g0: add support for power management
.../bindings/usb/st,typec-stm32g0.yaml | 90 ++
drivers/usb/typec/ucsi/Kconfig | 10 +
drivers/usb/typec/ucsi/Makefile | 1 +
drivers/usb/typec/ucsi/ucsi_stm32g0.c | 777 ++++++++++++++++++
4 files changed, 878 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
create mode 100644 drivers/usb/typec/ucsi/ucsi_stm32g0.c
--
2.25.1
Add DT schema documentation for the STM32G0 Type-C PD (Power Delivery)
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 <[email protected]>
---
Changes in v2:
- Krzysztof's review comments: update commit message, use ports, use
unevaluatedProperties: false for usb-connector schema, define maxItems
for power-domains, adopt generic node names, remove quotes
---
.../bindings/usb/st,typec-stm32g0.yaml | 90 +++++++++++++++++++
1 file changed, 90 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..7b3a2c2124e38
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
@@ -0,0 +1,90 @@
+# 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 USB Type-C PD controller
+
+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 <[email protected]>
+
+properties:
+ compatible:
+ const: st,stm32g0-typec
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ connector:
+ type: object
+ $ref: /schemas/connector/usb-connector.yaml#
+ 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:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ typec@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";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ port@0 {
+ reg = <0>;
+ con_usb_c_ep: endpoint {
+ remote-endpoint = <&usb_ep>;
+ };
+ };
+ };
+ };
+ };
+ };
+
+ usb {
+ usb-role-switch;
+ port {
+ usb_ep: endpoint {
+ remote-endpoint = <&con_usb_c_ep>;
+ };
+ };
+ };
+...
--
2.25.1
Type-C connector can be used as a wakeup source (typically to detect
changes on the port, attach or detach...).
Add suspend / resume routines to enable wake irqs, and signal a wakeup
event in case the IRQ has fired while in suspend.
The i2c core is doing the necessary initialization when the "wakeup-source"
flag is provided.
Note: the interrupt handler shouldn't be called before the i2c bus resumes.
So, the interrupts are disabled during suspend period, and re-enabled
upon resume, to avoid i2c transfer while suspended, from the irq handler.
Signed-off-by: Fabrice Gasnier <[email protected]>
---
drivers/usb/typec/ucsi/ucsi_stm32g0.c | 52 +++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
index b1d891c9a92c0..061551d464f12 100644
--- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
+++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
@@ -66,6 +66,8 @@ struct ucsi_stm32g0 {
unsigned long flags;
const char *fw_name;
struct ucsi *ucsi;
+ bool suspended;
+ bool wakeup_event;
};
/*
@@ -416,6 +418,9 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
u32 cci;
int ret;
+ if (g0->suspended)
+ g0->wakeup_event = true;
+
ret = ucsi_stm32g0_read(g0->ucsi, UCSI_CCI, &cci, sizeof(cci));
if (ret)
return IRQ_NONE;
@@ -696,6 +701,52 @@ static int ucsi_stm32g0_remove(struct i2c_client *client)
return 0;
}
+static int ucsi_stm32g0_suspend(struct device *dev)
+{
+ struct ucsi_stm32g0 *g0 = dev_get_drvdata(dev);
+ struct i2c_client *client = g0->client;
+
+ if (g0->in_bootloader)
+ return 0;
+
+ /* Keep the interrupt disabled until the i2c bus has been resumed */
+ disable_irq(client->irq);
+
+ g0->suspended = true;
+ g0->wakeup_event = false;
+
+ if (device_may_wakeup(dev) || device_wakeup_path(dev))
+ enable_irq_wake(client->irq);
+
+ return 0;
+}
+
+static int ucsi_stm32g0_resume(struct device *dev)
+{
+ struct ucsi_stm32g0 *g0 = dev_get_drvdata(dev);
+ struct i2c_client *client = g0->client;
+
+ if (g0->in_bootloader)
+ return 0;
+
+ if (device_may_wakeup(dev) || device_wakeup_path(dev))
+ disable_irq_wake(client->irq);
+
+ enable_irq(client->irq);
+
+ /* Enforce any pending handler gets called to signal a wakeup_event */
+ synchronize_irq(client->irq);
+
+ if (g0->wakeup_event)
+ pm_wakeup_event(g0->dev, 0);
+
+ g0->suspended = false;
+
+ return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(ucsi_stm32g0_pm_ops, ucsi_stm32g0_suspend, ucsi_stm32g0_resume);
+
static const struct of_device_id __maybe_unused ucsi_stm32g0_typec_of_match[] = {
{ .compatible = "st,stm32g0-typec" },
{},
@@ -712,6 +763,7 @@ static struct i2c_driver ucsi_stm32g0_i2c_driver = {
.driver = {
.name = "ucsi-stm32g0-i2c",
.of_match_table = of_match_ptr(ucsi_stm32g0_typec_of_match),
+ .pm = pm_sleep_ptr(&ucsi_stm32g0_pm_ops),
},
.probe = ucsi_stm32g0_probe,
.remove = ucsi_stm32g0_remove,
--
2.25.1
On 11/07/2022 14:01, Fabrice Gasnier wrote:
> Add DT schema documentation for the STM32G0 Type-C PD (Power Delivery)
> 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 <[email protected]>
Thank you for your patch. There is something to discuss/improve.
> ---
> Changes in v2:
> - Krzysztof's review comments: update commit message, use ports, use
> unevaluatedProperties: false for usb-connector schema, define maxItems
> for power-domains, adopt generic node names, remove quotes
> ---
> .../bindings/usb/st,typec-stm32g0.yaml | 90 +++++++++++++++++++
> 1 file changed, 90 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..7b3a2c2124e38
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
> @@ -0,0 +1,90 @@
> +# 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 USB Type-C PD controller
> +
> +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 <[email protected]>
> +
> +properties:
> + compatible:
> + const: st,stm32g0-typec
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + connector:
> + type: object
> + $ref: /schemas/connector/usb-connector.yaml#
> + 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:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
Isn't connector a required property? I would assume the device does not
make much sense to operate without it.
What about firmware-name? Do you expect hardware to work fine without it
(default firmware?)?
Best regards,
Krzysztof
Hi,
Mon, Jul 11, 2022 at 02:01:22PM +0200, Fabrice Gasnier kirjoitti:
> Type-C connector can be used as a wakeup source (typically to detect
> changes on the port, attach or detach...).
> Add suspend / resume routines to enable wake irqs, and signal a wakeup
> event in case the IRQ has fired while in suspend.
> The i2c core is doing the necessary initialization when the "wakeup-source"
> flag is provided.
> Note: the interrupt handler shouldn't be called before the i2c bus resumes.
> So, the interrupts are disabled during suspend period, and re-enabled
> upon resume, to avoid i2c transfer while suspended, from the irq handler.
>
> Signed-off-by: Fabrice Gasnier <[email protected]>
Does this really need a separate patch? Does the support depend on the
second patch somehow?
If not, then just merge this into the first patch. That
g0->in_bootloader check you can add in the second patch.
Acked-by: Heikki Krogerus <[email protected]>
> ---
> drivers/usb/typec/ucsi/ucsi_stm32g0.c | 52 +++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> index b1d891c9a92c0..061551d464f12 100644
> --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> @@ -66,6 +66,8 @@ struct ucsi_stm32g0 {
> unsigned long flags;
> const char *fw_name;
> struct ucsi *ucsi;
> + bool suspended;
> + bool wakeup_event;
> };
>
> /*
> @@ -416,6 +418,9 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
> u32 cci;
> int ret;
>
> + if (g0->suspended)
> + g0->wakeup_event = true;
> +
> ret = ucsi_stm32g0_read(g0->ucsi, UCSI_CCI, &cci, sizeof(cci));
> if (ret)
> return IRQ_NONE;
> @@ -696,6 +701,52 @@ static int ucsi_stm32g0_remove(struct i2c_client *client)
> return 0;
> }
>
> +static int ucsi_stm32g0_suspend(struct device *dev)
> +{
> + struct ucsi_stm32g0 *g0 = dev_get_drvdata(dev);
> + struct i2c_client *client = g0->client;
> +
> + if (g0->in_bootloader)
> + return 0;
> +
> + /* Keep the interrupt disabled until the i2c bus has been resumed */
> + disable_irq(client->irq);
> +
> + g0->suspended = true;
> + g0->wakeup_event = false;
> +
> + if (device_may_wakeup(dev) || device_wakeup_path(dev))
> + enable_irq_wake(client->irq);
> +
> + return 0;
> +}
> +
> +static int ucsi_stm32g0_resume(struct device *dev)
> +{
> + struct ucsi_stm32g0 *g0 = dev_get_drvdata(dev);
> + struct i2c_client *client = g0->client;
> +
> + if (g0->in_bootloader)
> + return 0;
> +
> + if (device_may_wakeup(dev) || device_wakeup_path(dev))
> + disable_irq_wake(client->irq);
> +
> + enable_irq(client->irq);
> +
> + /* Enforce any pending handler gets called to signal a wakeup_event */
> + synchronize_irq(client->irq);
> +
> + if (g0->wakeup_event)
> + pm_wakeup_event(g0->dev, 0);
> +
> + g0->suspended = false;
> +
> + return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(ucsi_stm32g0_pm_ops, ucsi_stm32g0_suspend, ucsi_stm32g0_resume);
> +
> static const struct of_device_id __maybe_unused ucsi_stm32g0_typec_of_match[] = {
> { .compatible = "st,stm32g0-typec" },
> {},
> @@ -712,6 +763,7 @@ static struct i2c_driver ucsi_stm32g0_i2c_driver = {
> .driver = {
> .name = "ucsi-stm32g0-i2c",
> .of_match_table = of_match_ptr(ucsi_stm32g0_typec_of_match),
> + .pm = pm_sleep_ptr(&ucsi_stm32g0_pm_ops),
> },
> .probe = ucsi_stm32g0_probe,
> .remove = ucsi_stm32g0_remove,
> --
> 2.25.1
--
heikki
On 7/12/22 10:56, Heikki Krogerus wrote:
> Hi,
>
> Mon, Jul 11, 2022 at 02:01:22PM +0200, Fabrice Gasnier kirjoitti:
>> Type-C connector can be used as a wakeup source (typically to detect
>> changes on the port, attach or detach...).
>> Add suspend / resume routines to enable wake irqs, and signal a wakeup
>> event in case the IRQ has fired while in suspend.
>> The i2c core is doing the necessary initialization when the "wakeup-source"
>> flag is provided.
>> Note: the interrupt handler shouldn't be called before the i2c bus resumes.
>> So, the interrupts are disabled during suspend period, and re-enabled
>> upon resume, to avoid i2c transfer while suspended, from the irq handler.
>>
>> Signed-off-by: Fabrice Gasnier <[email protected]>
>
> Does this really need a separate patch? Does the support depend on the
> second patch somehow?
Hi Heikki,
There's no dependency. I did a separate patch mainly to ease the review.
>
> If not, then just merge this into the first patch. That
> g0->in_bootloader check you can add in the second patch.
Ok, I'll merge this into the first patches as you suggest, and add your
Acked-by.
Thanks for reviewing,
Best Regards,
Fabrice
>
> Acked-by: Heikki Krogerus <[email protected]>
>
>> ---
>> drivers/usb/typec/ucsi/ucsi_stm32g0.c | 52 +++++++++++++++++++++++++++
>> 1 file changed, 52 insertions(+)
>>
>> diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
>> index b1d891c9a92c0..061551d464f12 100644
>> --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
>> +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
>> @@ -66,6 +66,8 @@ struct ucsi_stm32g0 {
>> unsigned long flags;
>> const char *fw_name;
>> struct ucsi *ucsi;
>> + bool suspended;
>> + bool wakeup_event;
>> };
>>
>> /*
>> @@ -416,6 +418,9 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
>> u32 cci;
>> int ret;
>>
>> + if (g0->suspended)
>> + g0->wakeup_event = true;
>> +
>> ret = ucsi_stm32g0_read(g0->ucsi, UCSI_CCI, &cci, sizeof(cci));
>> if (ret)
>> return IRQ_NONE;
>> @@ -696,6 +701,52 @@ static int ucsi_stm32g0_remove(struct i2c_client *client)
>> return 0;
>> }
>>
>> +static int ucsi_stm32g0_suspend(struct device *dev)
>> +{
>> + struct ucsi_stm32g0 *g0 = dev_get_drvdata(dev);
>> + struct i2c_client *client = g0->client;
>> +
>> + if (g0->in_bootloader)
>> + return 0;
>> +
>> + /* Keep the interrupt disabled until the i2c bus has been resumed */
>> + disable_irq(client->irq);
>> +
>> + g0->suspended = true;
>> + g0->wakeup_event = false;
>> +
>> + if (device_may_wakeup(dev) || device_wakeup_path(dev))
>> + enable_irq_wake(client->irq);
>> +
>> + return 0;
>> +}
>> +
>> +static int ucsi_stm32g0_resume(struct device *dev)
>> +{
>> + struct ucsi_stm32g0 *g0 = dev_get_drvdata(dev);
>> + struct i2c_client *client = g0->client;
>> +
>> + if (g0->in_bootloader)
>> + return 0;
>> +
>> + if (device_may_wakeup(dev) || device_wakeup_path(dev))
>> + disable_irq_wake(client->irq);
>> +
>> + enable_irq(client->irq);
>> +
>> + /* Enforce any pending handler gets called to signal a wakeup_event */
>> + synchronize_irq(client->irq);
>> +
>> + if (g0->wakeup_event)
>> + pm_wakeup_event(g0->dev, 0);
>> +
>> + g0->suspended = false;
>> +
>> + return 0;
>> +}
>> +
>> +static DEFINE_SIMPLE_DEV_PM_OPS(ucsi_stm32g0_pm_ops, ucsi_stm32g0_suspend, ucsi_stm32g0_resume);
>> +
>> static const struct of_device_id __maybe_unused ucsi_stm32g0_typec_of_match[] = {
>> { .compatible = "st,stm32g0-typec" },
>> {},
>> @@ -712,6 +763,7 @@ static struct i2c_driver ucsi_stm32g0_i2c_driver = {
>> .driver = {
>> .name = "ucsi-stm32g0-i2c",
>> .of_match_table = of_match_ptr(ucsi_stm32g0_typec_of_match),
>> + .pm = pm_sleep_ptr(&ucsi_stm32g0_pm_ops),
>> },
>> .probe = ucsi_stm32g0_probe,
>> .remove = ucsi_stm32g0_remove,
>> --
>> 2.25.1
>
On 7/12/22 10:42, Krzysztof Kozlowski wrote:
> On 11/07/2022 14:01, Fabrice Gasnier wrote:
>> Add DT schema documentation for the STM32G0 Type-C PD (Power Delivery)
>> 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 <[email protected]>
>
> Thank you for your patch. There is something to discuss/improve.
>
>> ---
>> Changes in v2:
>> - Krzysztof's review comments: update commit message, use ports, use
>> unevaluatedProperties: false for usb-connector schema, define maxItems
>> for power-domains, adopt generic node names, remove quotes
>> ---
>> .../bindings/usb/st,typec-stm32g0.yaml | 90 +++++++++++++++++++
>> 1 file changed, 90 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..7b3a2c2124e38
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml
>> @@ -0,0 +1,90 @@
>> +# 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 USB Type-C PD controller
>> +
>> +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 <[email protected]>
>> +
>> +properties:
>> + compatible:
>> + const: st,stm32g0-typec
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> + connector:
>> + type: object
>> + $ref: /schemas/connector/usb-connector.yaml#
>> + 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:
>> + maxItems: 1
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - interrupts
>
> Isn't connector a required property? I would assume the device does not
> make much sense to operate without it.
Hi Krzysztof,
Indeed, that's sensible. I'll add connector to the required properties.
>
> What about firmware-name? Do you expect hardware to work fine without it
> (default firmware?)?
Basically, the default firmware could be loaded in production. The
firmware content itself may be customized to restrict the firmware
update feature. That's a pure application decision of the firmware.
There could be other means to update it too (like via CC lines, with
external tools), instead of I2C lines.
Hence, the firmware-name is made optional here.
I can update the commit message with this explanation if this clarifies it.
Thanks for reviewing,
Best Regards,
Fabrice
>
>
> Best regards,
> Krzysztof