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
