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

[Wayne Chang]

On 10/28/22 22:07, Thierry Reding wrote:
> 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@xxxxxxxxxx>
>>>>> ---
>>>>>     .../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@xxxxxxxxxx>
>>>>> +
>>>>> +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@xxxxxxxxxx/
>>>>
>>>> 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.
> 

OK. Thanks for the review. I'll make the change to extend the property 
into string in the next patch series.

thanks,
Wayne.


> 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