[v5,00/15] Add TPS25750 USB type-C PD controller support

  From: Abdel Alkuor <abdelalkuor@geotab.com>

TPS25750 USB type-C PD controller has the same register offsets as
tps6598x. The following is a summary of incorporating TPS25750 into
TPS6598x driver:

- Only Check VID register (0x00) for TPS6598x and cd321x, as TPS25750 doesn't
  have VID register.

- TypeC port registration will be registered differently for each PD
  controller. TPS6598x uses system configuration register (0x28) to get
  pr/dr capabilities. On the other hand, TPS25750 will use data role property
  and PD status register (0x40) to get pr/dr capabilities as TPS25750 doesn't
  have register 0x28 supported.

- TPS25750 requires writing a binary configuration to switch PD
  controller from PTCH mode to APP mode which needs the following changes:
  - Add PTCH mode to the modes list.
  - Add an argument to tps6598x_check_mode to return the current mode.
  - Currently, tps6598x_exec_cmd has cmd timeout hardcoded to 1 second,
    and doesn't wait before checking DATA_OUT response. In TPS25750, patch 4CCs
    take longer than 1 second to execute and some requires a delay before
    checking DATA_OUT. To accommodate that, cmd_timeout and response_delay will
    be added as arguments to tps6598x_exec_cmd.
  - Implement applying patch sequence for TPS25750.

- In pm suspend callback, patch mode needs to be checked and the binary
  configuration should be applied if needed.

- For interrupt, TPS25750 has only one event register (0x14) and one mask
  register (0x16) of 11 bytes each, where TPS6598x has two event
  and two mask registers of 8 bytes each. Both TPS25750 and TPS65986x
  shares the same bit field offsets for events/masks/clear but many of
  there fields are reserved in TPS25750, the following needs to be done in
  tps6598x_interrupt:
  - Read EVENT1 register as a block of 11 bytes when tps25750 is present
  - Write CLEAR1 register as a block of 11 bytes when tps25750 is present
  - Add trace_tps25750_irq
  - During testing, I noticed that when a cable is plugged into the PD
    controller and before PD controller switches to APP mode, there is a
    lag between dr/pr updates and PlugInsertOrRemoval Event, so a check
    for dr/pr change needs to be added along TPS_REG_INT_PLUG_EVENT check

- Add TPS25750 traces for status and power status registers. Trace for
  data register won't be added as it doesn't exist in the device.

- Configure sleep mode for TPS25750.

v5:
 - PATCH 1: Add tps25750 bindings to tps6598x
 - PATCH 2: Remove tps25750 driver and incorperate tps25750
 	    into tps6598x driver
v4:
 - PATCH 1: No change
 - PATCH 2: Fix comments style and drop of_match_ptr
v3:
 - PATCH 1: Fix node name
 - PATCH 2: Upload tps25750 driver patch
v2:
 - PATCH 1: General properties clean up

Abdel Alkuor (15):
  dt-bindings: usb: tps6598x: Add tps25750
  USB: typec: Add cmd timeout and response delay
  USB: typec: Add patch mode to tps6598x
  USB: typec: Load TPS25750 patch bundle
  USB: typec: Check for EEPROM present
  USB: typec: Clear dead battery flag
  USB: typec: Apply patch again after power resume
  USB: typec: Add interrupt support for TPS25750
  USB: typec: Refactor tps6598x port registration
  USB: typec: Add port registration for tps25750
  USB: typec: Enable sleep mode for tps25750
  USB: typec: Add trace for tps25750 irq
  USB: typec: Add power status trace for tps25750
  USB: typec: Add status trace for tps25750
  USB: typec: Do not check VID for tps25750

 .../devicetree/bindings/usb/ti,tps6598x.yaml  |  70 +++
 drivers/usb/typec/tipd/core.c                 | 570 +++++++++++++++---
 drivers/usb/typec/tipd/tps6598x.h             |  36 ++
 drivers/usb/typec/tipd/trace.h                |  84 +++
 4 files changed, 683 insertions(+), 77 deletions(-)
  

On 17/09/2023 17:26, Abdel Alkuor wrote:
> From: Abdel Alkuor <abdelalkuor@geotab.com>
> 
> TPS25750 is USB TypeC PD controller which is a subset of TPS6598x.
> 
> Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> ---
>  .../devicetree/bindings/usb/ti,tps6598x.yaml  | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> index 5497a60cddbc..e49bd92b5276 100644
> --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> @@ -20,6 +20,8 @@ properties:
>      enum:
>        - ti,tps6598x
>        - apple,cd321x
> +      - ti,tps25750
> +
>    reg:
>      maxItems: 1
>  
> @@ -32,10 +34,45 @@ properties:
>      items:
>        - const: irq
>  
> +  firmware-name:
> +    description: |
> +      Should contain the name of the default patch binary
> +      file located on the firmware search path which is
> +      used to switch the controller into APP mode.
> +      This is used when tps25750 doesn't have an EEPROM
> +      connected to it.
> +    maxItems: 1
> +
> +  ti,patch-address:
> +    description: |
> +      One of PBMs command data field is I2C slave address
> +      which is used when writing the patch for TPS25750.
> +      The slave address can be any value except 0x00, 0x20,
> +      0x21, 0x22, and 0x23

Why this cannot be another entry in the reg?

> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    minimum: 1
> +    maximum: 0x7e
> +
>  required:
>    - compatible
>    - reg
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: ti,tps25750
> +    then:
> +      required:
> +        - ti,patch-address
> +        - connector


Why? Connector should be required or not required for both devices. What
is different between them?


Best regards,
Krzysztof
  

On Sun, Sep 17, 2023 at 07:30:52PM +0200, Krzysztof Kozlowski wrote:
> On 17/09/2023 17:26, Abdel Alkuor wrote:
> > From: Abdel Alkuor <abdelalkuor@geotab.com>
> > 
> > TPS25750 is USB TypeC PD controller which is a subset of TPS6598x.
> > 
> > Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> > ---
> >  .../devicetree/bindings/usb/ti,tps6598x.yaml  | 70 +++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> > index 5497a60cddbc..e49bd92b5276 100644
> > --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> > +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> > @@ -20,6 +20,8 @@ properties:
> >      enum:
> >        - ti,tps6598x
> >        - apple,cd321x
> > +      - ti,tps25750
> > +
> >    reg:
> >      maxItems: 1
> >  
> > @@ -32,10 +34,45 @@ properties:
> >      items:
> >        - const: irq
> >  
> > +  firmware-name:
> > +    description: |
> > +      Should contain the name of the default patch binary
> > +      file located on the firmware search path which is
> > +      used to switch the controller into APP mode.
> > +      This is used when tps25750 doesn't have an EEPROM
> > +      connected to it.
> > +    maxItems: 1
> > +
> > +  ti,patch-address:
> > +    description: |
> > +      One of PBMs command data field is I2C slave address
> > +      which is used when writing the patch for TPS25750.
> > +      The slave address can be any value except 0x00, 0x20,
> > +      0x21, 0x22, and 0x23
> 
> Why this cannot be another entry in the reg?
> 
This address is different than the physical address of PD controller.
The patch address will be used instead of PD controller address when
writing the patch. I thought reg proprity is only for a device physical
address, should I add another entry in the reg property in this case?
> > +    $ref: /schemas/types.yaml#/definitions/uint8
> > +    minimum: 1
> > +    maximum: 0x7e
> > +
> >  required:
> >    - compatible
> >    - reg
> >  
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: ti,tps25750
> > +    then:
> > +      required:
> > +        - ti,patch-address
> > +        - connector
> 
> 
> Why? Connector should be required or not required for both devices. What
> is different between them?
> 
The data-role for tps6598x can be extracted from system config register
which it doesn't exist in tps25750, so the only way to extract
this information is by using data-role property from the Connector for
tps25750, hence Connector and data-role are set as required for
tps25750.
> 
> Best regards,
> Krzysztof
>
Thanks,
Abdel
  

Hi,

On Sun, Sep 17, 2023 at 11:26:39AM -0400, Abdel Alkuor wrote:
> From: Abdel Alkuor <abdelalkuor@geotab.com>
> 
> tps25750 doesn't have VID register, check VID for PD controllers
> other than tps25750
> 
> Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> ---
>  drivers/usb/typec/tipd/core.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 326c23bfa8e6..c1399e12a170 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -1142,10 +1142,6 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (IS_ERR(tps->regmap))
>  		return PTR_ERR(tps->regmap);
>  
> -	ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
> -	if (ret < 0 || !vid)
> -		return -ENODEV;
> -
>  	/*
>  	 * Checking can the adapter handle SMBus protocol. If it can not, the
>  	 * driver needs to take care of block reads separately.
> @@ -1176,6 +1172,12 @@ static int tps6598x_probe(struct i2c_client *client)
>  
>  	tps->irq_handler = irq_handler;
>  
> +	if (!tps->is_tps25750) {
> +		ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
> +		if (ret < 0 || !vid)
> +			return -ENODEV;
> +	}

You need to do this at the same time you enable tps25750, so I'm
guessing in patch 4.

You are also changing the execution order just because of that
is_tps25750. Instead you need to make sure you have that flag set as
soon as possible in the first place, so right after "tps" is
allocated:

        mutex_init(&tps->lock);
        tps->dev = &client->dev;
+       tps->is_tps25750 = of_device_is_compatible(np, "ti,tps25750");
 
        tps->regmap = devm_regmap_init_i2c(client, &tps6598x_regmap_config);
        if (IS_ERR(tps->regmap))


thanks,
  

On Sun, Sep 17, 2023 at 11:26:24AM -0400, Abdel Alkuor wrote:
> From: Abdel Alkuor <abdelalkuor@geotab.com>
> 
> TPS25750 USB type-C PD controller has the same register offsets as
> tps6598x. The following is a summary of incorporating TPS25750 into
> TPS6598x driver:
> 
> - Only Check VID register (0x00) for TPS6598x and cd321x, as TPS25750 doesn't
>   have VID register.
> 
> - TypeC port registration will be registered differently for each PD
>   controller. TPS6598x uses system configuration register (0x28) to get
>   pr/dr capabilities. On the other hand, TPS25750 will use data role property
>   and PD status register (0x40) to get pr/dr capabilities as TPS25750 doesn't
>   have register 0x28 supported.
> 
> - TPS25750 requires writing a binary configuration to switch PD
>   controller from PTCH mode to APP mode which needs the following changes:
>   - Add PTCH mode to the modes list.
>   - Add an argument to tps6598x_check_mode to return the current mode.
>   - Currently, tps6598x_exec_cmd has cmd timeout hardcoded to 1 second,
>     and doesn't wait before checking DATA_OUT response. In TPS25750, patch 4CCs
>     take longer than 1 second to execute and some requires a delay before
>     checking DATA_OUT. To accommodate that, cmd_timeout and response_delay will
>     be added as arguments to tps6598x_exec_cmd.
>   - Implement applying patch sequence for TPS25750.
> 
> - In pm suspend callback, patch mode needs to be checked and the binary
>   configuration should be applied if needed.
> 
> - For interrupt, TPS25750 has only one event register (0x14) and one mask
>   register (0x16) of 11 bytes each, where TPS6598x has two event
>   and two mask registers of 8 bytes each. Both TPS25750 and TPS65986x
>   shares the same bit field offsets for events/masks/clear but many of
>   there fields are reserved in TPS25750, the following needs to be done in
>   tps6598x_interrupt:
>   - Read EVENT1 register as a block of 11 bytes when tps25750 is present
>   - Write CLEAR1 register as a block of 11 bytes when tps25750 is present
>   - Add trace_tps25750_irq
>   - During testing, I noticed that when a cable is plugged into the PD
>     controller and before PD controller switches to APP mode, there is a
>     lag between dr/pr updates and PlugInsertOrRemoval Event, so a check
>     for dr/pr change needs to be added along TPS_REG_INT_PLUG_EVENT check
> 
> - Add TPS25750 traces for status and power status registers. Trace for
>   data register won't be added as it doesn't exist in the device.
> 
> - Configure sleep mode for TPS25750.

This looks mostly okay, but I'm a bit uncomfortable with flags like
is_tps25750.

I think a better way would be to supply driver data. In it you would
have a callback for everything that needs to be customised.

struct tipd_data {
        int (*interrupt)(int irq, void *data);
        ...
};
...
static const struct tipd_data tps25750_data = {
        .interrupt = tps25750_interrupt,
...

Something like that. You can on top of that still check
device_is_compatible(dev, "...") in some places.


thanks,
  

On Mon, Sep 18, 2023 at 04:29:43PM +0300, Heikki Krogerus wrote:
> Hi,
> 
> On Sun, Sep 17, 2023 at 11:26:39AM -0400, Abdel Alkuor wrote:
> > From: Abdel Alkuor <abdelalkuor@geotab.com>
> > 
> > tps25750 doesn't have VID register, check VID for PD controllers
> > other than tps25750
> > 
> > Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> > ---
> >  drivers/usb/typec/tipd/core.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > index 326c23bfa8e6..c1399e12a170 100644
> > --- a/drivers/usb/typec/tipd/core.c
> > +++ b/drivers/usb/typec/tipd/core.c
> > @@ -1142,10 +1142,6 @@ static int tps6598x_probe(struct i2c_client *client)
> >  	if (IS_ERR(tps->regmap))
> >  		return PTR_ERR(tps->regmap);
> >  
> > -	ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
> > -	if (ret < 0 || !vid)
> > -		return -ENODEV;
> > -
> >  	/*
> >  	 * Checking can the adapter handle SMBus protocol. If it can not, the
> >  	 * driver needs to take care of block reads separately.
> > @@ -1176,6 +1172,12 @@ static int tps6598x_probe(struct i2c_client *client)
> >  
> >  	tps->irq_handler = irq_handler;
> >  
> > +	if (!tps->is_tps25750) {
> > +		ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
> > +		if (ret < 0 || !vid)
> > +			return -ENODEV;
> > +	}
> 
> You need to do this at the same time you enable tps25750, so I'm
> guessing in patch 4.
> 
> You are also changing the execution order just because of that
> is_tps25750. Instead you need to make sure you have that flag set as
> soon as possible in the first place, so right after "tps" is
> allocated:
>
Good point. I will move the check in patch 4 and check it after allocating
tps.
>         mutex_init(&tps->lock);
>         tps->dev = &client->dev;
> +       tps->is_tps25750 = of_device_is_compatible(np, "ti,tps25750");
>  
>         tps->regmap = devm_regmap_init_i2c(client, &tps6598x_regmap_config);
>         if (IS_ERR(tps->regmap))
> 
> 
> thanks,
> 
> -- 
> heikki
Thanks,
Abdel
  

On Mon, Sep 18, 2023 at 04:57:49PM +0300, Heikki Krogerus wrote:
> On Sun, Sep 17, 2023 at 11:26:24AM -0400, Abdel Alkuor wrote:
> > From: Abdel Alkuor <abdelalkuor@geotab.com>
> > 
> > TPS25750 USB type-C PD controller has the same register offsets as
> > tps6598x. The following is a summary of incorporating TPS25750 into
> > TPS6598x driver:
> > 
> > - Only Check VID register (0x00) for TPS6598x and cd321x, as TPS25750 doesn't
> >   have VID register.
> > 
> > - TypeC port registration will be registered differently for each PD
> >   controller. TPS6598x uses system configuration register (0x28) to get
> >   pr/dr capabilities. On the other hand, TPS25750 will use data role property
> >   and PD status register (0x40) to get pr/dr capabilities as TPS25750 doesn't
> >   have register 0x28 supported.
> > 
> > - TPS25750 requires writing a binary configuration to switch PD
> >   controller from PTCH mode to APP mode which needs the following changes:
> >   - Add PTCH mode to the modes list.
> >   - Add an argument to tps6598x_check_mode to return the current mode.
> >   - Currently, tps6598x_exec_cmd has cmd timeout hardcoded to 1 second,
> >     and doesn't wait before checking DATA_OUT response. In TPS25750, patch 4CCs
> >     take longer than 1 second to execute and some requires a delay before
> >     checking DATA_OUT. To accommodate that, cmd_timeout and response_delay will
> >     be added as arguments to tps6598x_exec_cmd.
> >   - Implement applying patch sequence for TPS25750.
> > 
> > - In pm suspend callback, patch mode needs to be checked and the binary
> >   configuration should be applied if needed.
> > 
> > - For interrupt, TPS25750 has only one event register (0x14) and one mask
> >   register (0x16) of 11 bytes each, where TPS6598x has two event
> >   and two mask registers of 8 bytes each. Both TPS25750 and TPS65986x
> >   shares the same bit field offsets for events/masks/clear but many of
> >   there fields are reserved in TPS25750, the following needs to be done in
> >   tps6598x_interrupt:
> >   - Read EVENT1 register as a block of 11 bytes when tps25750 is present
> >   - Write CLEAR1 register as a block of 11 bytes when tps25750 is present
> >   - Add trace_tps25750_irq
> >   - During testing, I noticed that when a cable is plugged into the PD
> >     controller and before PD controller switches to APP mode, there is a
> >     lag between dr/pr updates and PlugInsertOrRemoval Event, so a check
> >     for dr/pr change needs to be added along TPS_REG_INT_PLUG_EVENT check
> > 
> > - Add TPS25750 traces for status and power status registers. Trace for
> >   data register won't be added as it doesn't exist in the device.
> > 
> > - Configure sleep mode for TPS25750.
> 
> This looks mostly okay, but I'm a bit uncomfortable with flags like
> is_tps25750.
> 
> I think a better way would be to supply driver data. In it you would
> have a callback for everything that needs to be customised.
> 
> struct tipd_data {
>         int (*interrupt)(int irq, void *data);
>         ...
> };
> ...
> static const struct tipd_data tps25750_data = {
>         .interrupt = tps25750_interrupt,
> ...
> 
> Something like that. You can on top of that still check
> device_is_compatible(dev, "...") in some places.
>
Sounds good. I will create callbacks factory struct as you suggested
and remove the flag.
> 
> thanks,
> 
> -- 
> heikki

Thanks,
Abdel