Re: [PATCH 03/11] dt-bindings: usb: Add binding for Cypress cypd4226 I2C driver

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5498 bytes --]

On Fri, Oct 28, 2022 at 01:42:36PM +0100, Jon Hunter wrote:
> 
> On 28/10/2022 13:31, Thierry Reding wrote:
> > On Wed, Oct 26, 2022 at 08:13:57AM +0100, Jon Hunter wrote:
> > > 
> > > On 24/10/2022 08:41, Wayne Chang wrote:
> > > > add device-tree binding documentation for Cypress cypd4226 type-C
> > > > controller's I2C interface. It is a standard i2c slave with GPIO
> > > > input as IRQ interface.
> > > > 
> > > > Signed-off-by: Wayne Chang <waynec@nvidia.com>
> > > > ---
> > > >    .../bindings/usb/cypress,cypd4226.yaml        | 86 +++++++++++++++++++
> > > >    1 file changed, 86 insertions(+)
> > > >    create mode 100644 Documentation/devicetree/bindings/usb/cypress,cypd4226.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/usb/cypress,cypd4226.yaml b/Documentation/devicetree/bindings/usb/cypress,cypd4226.yaml
> > > > new file mode 100644
> > > > index 000000000000..5ac28ab4e7a1
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/usb/cypress,cypd4226.yaml
> > > > @@ -0,0 +1,86 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/usb/cypress,cypd4226.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Cypress cypd4226 UCSI I2C Type-C Controller
> > > > +
> > > > +maintainers:
> > > > +  - Wayne Chang <waynec@nvidia.com>
> > > > +
> > > > +description: |
> > > > +  The Cypress cypd4226 UCSI I2C type-C controller is a I2C interface type-C
> > > > +  controller.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: cypress,cypd4226
> > > > +
> > > > +  '#address-cells':
> > > > +    const: 1
> > > > +
> > > > +  '#size-cells':
> > > > +    const: 0
> > > > +
> > > > +  reg:
> > > > +    const: 0x08
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  cypress,firmware-build:
> > > > +    enum:
> > > > +      - nv
> > > > +      - gn
> > > > +    description: |
> > > > +      the name of the CCGx firmware built for product series.
> > > > +      should be set one of following:
> > > > +      - "nv" for the RTX product series
> > > 
> > > Please add 'NVIDIA' so that it is 'for the NVIDIA RTX product series'
> > > 
> > > > +      - "gn" for the Jetson product series
> > > 
> > > Same here please add 'NVIDIA' so that it is 'for the NVIDIA Jetson product
> > > series'.
> > > 
> > > Rob, any concerns about this property in general? Unfortunately, ACPI choose
> > > a 16-bit type for this and used 'nv' for the RTX product. I don't find 'gn'
> > > for Jetson very descriptive but we need a way to differentiate from RTX.
> > > 
> > > This is needed in the Cypress CCGX driver for the following ...
> > > 
> > > https://lore.kernel.org/lkml/20220928150840.3804313-1-waynec@nvidia.com/
> > > 
> > > Ideally, this should have been included in this series but was sent before.
> > > We can always re-work/update the above patch even though it has been queued
> > > up now.
> > 
> > The driver seems to use this 16-bit value only to compare with a
> > corresponding field in the firmware headers. How exactly we obtain this
> > value is therefore not important. However, since this 16-bit value is
> > embedded in firmware images, we also cannot substitute them with
> > something more sensible.
> 
> I am actually wondering if this is actually embedded in any images because I
> see it populated by the i2c-nvidia-gpu.c driver [0]. So I am wondering if we
> can use PROPERTY_ENTRY_STRING() for this driver instead and have a more
> descriptive name such as 'nvidia,rtx'?

What I mean by "embedded in firmware images" is that the value read from
the property is compared to values read from a firmware blob (either one
read back from the chip or one loaded using request_firmware()). See for
example ccg_check_vendor_version() and ccg_check_fw_version().

So the way that this 16-bit number is used is to define what type of
vendor firmware we support. So this is also used to avoid trying to load
a Tegra firmware on a GPU and vice versa.

So yes, we could potentially still make the i2c-nvidia-gpu.c driver add
a "nvidia,rtx" string to make it more descriptive like DT, but then we'd
still need to somehow resolve that to the "nv" string for the assignment
to uc->fw_build.

Not sure about how that would impact the AMD bits. Another of those CCGX
UCSI devices is registered by the i2c-designware-pcidrv.c driver, but it
doesn't pass a software node. From what I can tell that simply means all
of those checks will work with fw_build == 0x00. Primarily I think that
will cause flashing of the firmware not to be supported.

So yeah, having that string be something else (i.e. more descriptive)
and then match on that instead would definitely work. After looking at
this some more, using existing driver-matching may not work after all
because while there's ACPI matching and with this series DT matching,
the various GPU I2C instantiations are purely done in software, so they
have neither and therefore would need a secondary lookup mechanism. We
may be stuck with that ccgx,firmware-build property, but as you said it
should be possible to at least sanitize it.

Thierry

> 
> Jon
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-nvidia-gpu.c#n261
> -- 
> nvpublic

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]