Re: [PATCH v4 0/2] Add support for TPS25750

[Abdel Alkuor]

On Wed, Aug 23, 2023 at 10:02:51AM +0300, Heikki Krogerus wrote:
> On Sun, Aug 20, 2023 at 03:32:25PM -0400, Abdel Alkuor wrote:
> > TPS25750 is USB Type-C and PD controller. The device is
> > highly configurable using App Customization online Tool 
> > developed by TI to generate loadable binary.
> > 
> > TPS25750 supports three modes; PTCH, APP, and BOOT. A configuration
> > can only be applied when the controller is on PTCH mode.
> > 
> > The controller attempts to load a configuration from EEPROM on
> > I2Cm bus. If no EEPROM is detected, then the driver tries to load
> > a configuration on I2Cs bus using a firmware file defined
> > in DT.
> > 
> > The driver implements the binary loading sequence which 
> > can be found on pg.53 in TPS25750 Host Interface Technical
> > Reference Manual (Rev. A) https://tinyurl.com/y9rkhu8a
> > 
> > The driver only supports resume pm callback as power management is
> > automatically controlled by the device. See pg.47 in TPS25750
> > datasheet https://tinyurl.com/3vfd2k43
> > 
> > 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
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> > 
> > Abdel Alkuor (2):
> >   dt-bindings: usb: Add ti,tps25750
> >   USB: typec: Add TI TPS25750 USB Type-C controller
> > 
> >  .../devicetree/bindings/usb/ti,tps25750.yaml  |   81 ++
> >  drivers/usb/typec/Kconfig                     |   13 +
> >  drivers/usb/typec/Makefile                    |    1 +
> >  drivers/usb/typec/tps25750.c                  | 1077 +++++++++++++++++
> >  drivers/usb/typec/tps25750.h                  |  162 +++
> >  5 files changed, 1334 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/ti,tps25750.yaml
> >  create mode 100644 drivers/usb/typec/tps25750.c
> >  create mode 100644 drivers/usb/typec/tps25750.h
Hi Heikki,
> 
> TPS25750 has the same host interface as TI TPS65xxx controllers, no?
> The register offsets at least are exactly the same.
Correct, the register offsets are the same. That being said, TPS25750 is a 
subset of TPS6598x where some registers are structured differently, and even some
registers are not supported. Hence, Each has its own host interface manual.
> 
> You need to first try to incorporate support for TI25750 support into
> the existing tipd driver (drivers/usb/typec/tipd/).
>
Incorporating TPS25750 into TPS6598x driver is doable which requires the
following changes:

- 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 followings need 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.

Before writing the binary configuration, PBMs command is used to set
up the patch bundle which consists of binary length, timeout, and
slave address. The slave address is not the device I2C address, the
bundle slave address can be any value except the ones that are listed
for possible device addresses which are 0x20, 0x21, 0x22, and 0x23 based
on pg.44 in https://www.ti.com/lit/ds/symlink/tps25750.pdf.
Currently, I'm using 0x0F for the bundle slave address which I think
is not the right way of assigning such address as a conflict may arise
when there is another device with 0x0F address is present on the same
I2C bus. 

Should the bundle address be set as property in the device node? Or
should we use i2c_probe_func_quick_read to scan the bus and find an
available address that is not used?

Thanks,
Abdel