| Message ID | 20221024074128.1113554-1-waynec@nvidia.com |
|---|---|
| Headers | show |
| Series | Enable USB host and device functions on Jetson | expand |
On Mon, Oct 24, 2022 at 03:41:18PM +0800, Wayne Chang wrote: > Extend the Tegra XUSB controller device tree binding with Tegra234 > support. > > Signed-off-by: Wayne Chang <waynec@nvidia.com> > --- > .../bindings/usb/nvidia,tegra-xudc.yaml | 24 ++++++++++++------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.yaml b/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.yaml > index fd6e7c81426e..517fb692f199 100644 > --- a/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.yaml > +++ b/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.yaml > @@ -22,6 +22,7 @@ properties: > - nvidia,tegra210-xudc # For Tegra210 > - nvidia,tegra186-xudc # For Tegra186 > - nvidia,tegra194-xudc # For Tegra194 > + - nvidia,tegra234-xudc # For Tegra234 > > reg: > minItems: 2 > @@ -90,21 +91,27 @@ properties: > > phys: > minItems: 1 > + maxItems: 8 > description: > Must contain an entry for each entry in phy-names. > See ../phy/phy-bindings.txt for details. > > phy-names: > minItems: 1 > + maxItems: 8 > items: > - - const: usb2-0 > - - const: usb2-1 > - - const: usb2-2 > - - const: usb2-3 > - - const: usb3-0 > - - const: usb3-1 > - - const: usb3-2 > - - const: usb3-3 > + anyOf: > + - const: usb2-0 > + - const: usb2-1 > + - const: usb2-2 > + - const: usb2-3 > + - const: usb3-0 > + - const: usb3-1 > + - const: usb3-2 > + - const: usb3-3 items: pattern: '^usb[23]-[0-3]$' And an explanation why you need any random order. If it is just different for Tegra234, then you need an if/then schema for this. > + > + dma-coherent: > + type: boolean > > avddio-usb-supply: > description: PCIe/USB3 analog logic power supply. Must supply 1.05 V. > @@ -153,6 +160,7 @@ allOf: > enum: > - nvidia,tegra186-xudc > - nvidia,tegra194-xudc > + - nvidia,tegra234-xudc > then: > properties: > reg: > -- > 2.25.1 > >
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. Jon
On 24/10/2022 03:41, Wayne Chang wrote: > This commit enables XUSB host, device, and pad controller on > Jetson AGX Orin. > > Signed-off-by: Wayne Chang <waynec@nvidia.com> > --- > .../boot/dts/nvidia/tegra234-p3701-0000.dtsi | 48 +++++ > .../nvidia/tegra234-p3737-0000+p3701-0000.dts | 184 ++++++++++++++++++ > arch/arm64/boot/dts/nvidia/tegra234.dtsi | 170 ++++++++++++++++ > 3 files changed, 402 insertions(+) > > diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3701-0000.dtsi b/arch/arm64/boot/dts/nvidia/tegra234-p3701-0000.dtsi > index 9e4d72cfa69f..8acef87a5398 100644 > --- a/arch/arm64/boot/dts/nvidia/tegra234-p3701-0000.dtsi > +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3701-0000.dtsi > @@ -61,6 +61,29 @@ mmc@3460000 { > non-removable; > }; > > + padctl@3520000 { > + vclamp-usb-supply = <&vdd_ao_1v8>; > + avdd-usb-supply = <&vdd_ao_3v3>; > + > + ports { > + usb2-0 { > + vbus-supply = <&vdd_5v0_sys>; > + }; > + > + usb2-1 { > + vbus-supply = <&vdd_5v0_sys>; > + }; > + > + usb2-2 { > + vbus-supply = <&vdd_5v0_sys>; > + }; > + > + usb2-3 { > + vbus-supply = <&vdd_5v0_sys>; > + }; > + }; > + }; > + > rtc@c2a0000 { > status = "okay"; > }; > @@ -69,4 +92,29 @@ pmc@c360000 { > nvidia,invert-interrupt; > }; > }; > + > + vdd_5v0_sys: regulator@0 { > + compatible = "regulator-fixed"; > + regulator-name = "VIN_SYS_5V0"; > + regulator-min-microvolt = <5000000>; > + regulator-max-microvolt = <5000000>; > + regulator-always-on; > + regulator-boot-on; > + }; > + > + vdd_ao_1v8: regulator@1 { > + compatible = "regulator-fixed"; > + regulator-name = "vdd-AO-1v8"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; > + regulator-always-on; > + }; > + > + vdd_ao_3v3: regulator@2 { > + compatible = "regulator-fixed"; > + regulator-name = "vdd-AO-3v3"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + regulator-always-on; > + }; > }; > diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts > index 57ab75328814..b4630280bb32 100644 > --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts > +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts > @@ -2011,6 +2011,190 @@ hda@3510000 { > nvidia,model = "NVIDIA Jetson AGX Orin HDA"; > status = "okay"; > }; > + > + padctl@3520000 { > + status = "okay"; > + > + pads { > + usb2 { > + lanes { > + usb2-0 { > + status = "okay"; > + }; > + > + usb2-1 { > + status = "okay"; > + }; > + > + usb2-2 { > + status = "okay"; > + }; > + > + usb2-3 { > + status = "okay"; > + }; > + }; > + }; > + > + usb3 { > + lanes { > + usb3-0 { > + status = "okay"; > + }; > + > + usb3-1 { > + status = "okay"; > + }; > + > + usb3-2 { > + status = "okay"; > + }; > + }; > + }; > + }; > + > + ports { > + usb2-0 { > + mode = "otg"; > + usb-role-switch; > + status = "okay"; > + port { > + hs_typec_p1: endpoint { > + remote-endpoint = <&hs_ucsi_ccg_p1>; > + }; > + }; > + }; > + > + usb2-1 { > + mode = "host"; > + status = "okay"; > + port { > + hs_typec_p0: endpoint { > + remote-endpoint = <&hs_ucsi_ccg_p0>; > + }; > + }; > + }; > + > + usb2-2 { > + mode = "host"; > + status = "okay"; > + }; > + > + usb2-3 { > + mode = "host"; > + status = "okay"; > + }; > + > + usb3-0 { > + nvidia,usb2-companion = <1>; > + status = "okay"; > + port { > + ss_typec_p0: endpoint { > + remote-endpoint = <&ss_ucsi_ccg_p0>; > + }; > + }; > + }; > + > + usb3-1 { > + nvidia,usb2-companion = <0>; > + status = "okay"; > + port { > + ss_typec_p1: endpoint { > + remote-endpoint = <&ss_ucsi_ccg_p1>; > + }; > + }; > + }; > + > + usb3-2 { > + nvidia,usb2-companion = <3>; > + status = "okay"; > + }; > + }; > + }; > + > + usb@3550000 { > + status = "okay"; > + > + phys = <&{/bus@0/padctl@3520000/pads/usb2/lanes/usb2-0}>, > + <&{/bus@0/padctl@3520000/pads/usb3/lanes/usb3-1}>; > + phy-names = "usb2-0", "usb3-1"; > + }; > + > + usb@3610000 { > + status = "okay"; > + > + phys = <&{/bus@0/padctl@3520000/pads/usb2/lanes/usb2-0}>, > + <&{/bus@0/padctl@3520000/pads/usb2/lanes/usb2-1}>, > + <&{/bus@0/padctl@3520000/pads/usb2/lanes/usb2-2}>, > + <&{/bus@0/padctl@3520000/pads/usb2/lanes/usb2-3}>, > + <&{/bus@0/padctl@3520000/pads/usb3/lanes/usb3-0}>, > + <&{/bus@0/padctl@3520000/pads/usb3/lanes/usb3-1}>, > + <&{/bus@0/padctl@3520000/pads/usb3/lanes/usb3-2}>; > + phy-names = "usb2-0", "usb2-1", "usb2-2", "usb2-3", > + "usb3-0", "usb3-1", "usb3-2"; > + }; > + > + i2c@c240000 { > + status = "okay"; > + ucsi_ccg: ucsi_ccg@8 { No underscores in node names. > + compatible = "cypress,cypd4226"; > + cypress,firmware-build = "gn"; > + interrupt-parent = <&gpio>; > + interrupts = <TEGRA234_MAIN_GPIO(Y, 4) IRQ_TYPE_LEVEL_LOW>; > + reg = <0x08>; > + status = "okay"; The pattern of redefining full path in Tegra is confusing - I have no clue which of these status=okay are correct which are redundant. Do you? > + #address-cells = <1>; > + #size-cells = <0>; > + ccg_typec_con0: connector@0 { > + compatible = "usb-c-connector"; > + reg = <0>; > + label = "USB-C"; > + data-role = "host"; > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + #address-cells = <1>; > + #size-cells = <0>; Hm, why do you have here cells? Did you test the DTS with dtbs_check? Best regards, Krzysztof
On 28/10/2022 03:23, Krzysztof Kozlowski wrote: > On 24/10/2022 03:41, Wayne Chang wrote: >> This commit enables XUSB host, device, and pad controller on >> Jetson AGX Orin. >> >> Signed-off-by: Wayne Chang <waynec@nvidia.com> >> --- >> .../boot/dts/nvidia/tegra234-p3701-0000.dtsi | 48 +++++ >> .../nvidia/tegra234-p3737-0000+p3701-0000.dts | 184 ++++++++++++++++++ >> arch/arm64/boot/dts/nvidia/tegra234.dtsi | 170 ++++++++++++++++ >> 3 files changed, 402 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3701-0000.dtsi b/arch/arm64/boot/dts/nvidia/tegra234-p3701-0000.dtsi >> index 9e4d72cfa69f..8acef87a5398 100644 >> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3701-0000.dtsi >> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3701-0000.dtsi >> @@ -61,6 +61,29 @@ mmc@3460000 { >> non-removable; >> }; >> >> + padctl@3520000 { >> + vclamp-usb-supply = <&vdd_ao_1v8>; >> + avdd-usb-supply = <&vdd_ao_3v3>; >> + >> + ports { >> + usb2-0 { >> + vbus-supply = <&vdd_5v0_sys>; >> + }; >> + >> + usb2-1 { >> + vbus-supply = <&vdd_5v0_sys>; >> + }; >> + >> + usb2-2 { >> + vbus-supply = <&vdd_5v0_sys>; >> + }; >> + >> + usb2-3 { >> + vbus-supply = <&vdd_5v0_sys>; >> + }; >> + }; >> + }; >> + >> rtc@c2a0000 { >> status = "okay"; >> }; >> @@ -69,4 +92,29 @@ pmc@c360000 { >> nvidia,invert-interrupt; >> }; >> }; >> + >> + vdd_5v0_sys: regulator@0 { >> + compatible = "regulator-fixed"; >> + regulator-name = "VIN_SYS_5V0"; >> + regulator-min-microvolt = <5000000>; >> + regulator-max-microvolt = <5000000>; >> + regulator-always-on; >> + regulator-boot-on; >> + }; >> + >> + vdd_ao_1v8: regulator@1 { >> + compatible = "regulator-fixed"; >> + regulator-name = "vdd-AO-1v8"; >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <1800000>; >> + regulator-always-on; >> + }; >> + >> + vdd_ao_3v3: regulator@2 { >> + compatible = "regulator-fixed"; >> + regulator-name = "vdd-AO-3v3"; >> + regulator-min-microvolt = <3300000>; >> + regulator-max-microvolt = <3300000>; >> + regulator-always-on; >> + }; >> }; >> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts >> index 57ab75328814..b4630280bb32 100644 >> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts >> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts >> @@ -2011,6 +2011,190 @@ hda@3510000 { >> nvidia,model = "NVIDIA Jetson AGX Orin HDA"; >> status = "okay"; >> }; >> + >> + padctl@3520000 { >> + status = "okay"; >> + >> + pads { >> + usb2 { >> + lanes { >> + usb2-0 { >> + status = "okay"; >> + }; >> + >> + usb2-1 { >> + status = "okay"; >> + }; >> + >> + usb2-2 { >> + status = "okay"; >> + }; >> + >> + usb2-3 { >> + status = "okay"; >> + }; >> + }; >> + }; >> + >> + usb3 { >> + lanes { >> + usb3-0 { >> + status = "okay"; >> + }; >> + >> + usb3-1 { >> + status = "okay"; >> + }; >> + >> + usb3-2 { >> + status = "okay"; >> + }; >> + }; >> + }; >> + }; >> + >> + ports { >> + usb2-0 { >> + mode = "otg"; >> + usb-role-switch; >> + status = "okay"; >> + port { >> + hs_typec_p1: endpoint { >> + remote-endpoint = <&hs_ucsi_ccg_p1>; >> + }; >> + }; >> + }; >> + >> + usb2-1 { >> + mode = "host"; >> + status = "okay"; >> + port { >> + hs_typec_p0: endpoint { >> + remote-endpoint = <&hs_ucsi_ccg_p0>; >> + }; >> + }; >> + }; >> + >> + usb2-2 { >> + mode = "host"; >> + status = "okay"; >> + }; >> + >> + usb2-3 { >> + mode = "host"; >> + status = "okay"; >> + }; >> + >> + usb3-0 { >> + nvidia,usb2-companion = <1>; >> + status = "okay"; >> + port { >> + ss_typec_p0: endpoint { >> + remote-endpoint = <&ss_ucsi_ccg_p0>; >> + }; >> + }; >> + }; >> + >> + usb3-1 { >> + nvidia,usb2-companion = <0>; >> + status = "okay"; >> + port { >> + ss_typec_p1: endpoint { >> + remote-endpoint = <&ss_ucsi_ccg_p1>; >> + }; >> + }; >> + }; >> + >> + usb3-2 { >> + nvidia,usb2-companion = <3>; >> + status = "okay"; >> + }; >> + }; >> + }; >> + >> + usb@3550000 { >> + status = "okay"; >> + >> + phys = <&{/bus@0/padctl@3520000/pads/usb2/lanes/usb2-0}>, >> + <&{/bus@0/padctl@3520000/pads/usb3/lanes/usb3-1}>; >> + phy-names = "usb2-0", "usb3-1"; >> + }; >> + >> + usb@3610000 { >> + status = "okay"; >> + >> + phys = <&{/bus@0/padctl@3520000/pads/usb2/lanes/usb2-0}>, >> + <&{/bus@0/padctl@3520000/pads/usb2/lanes/usb2-1}>, >> + <&{/bus@0/padctl@3520000/pads/usb2/lanes/usb2-2}>, >> + <&{/bus@0/padctl@3520000/pads/usb2/lanes/usb2-3}>, >> + <&{/bus@0/padctl@3520000/pads/usb3/lanes/usb3-0}>, >> + <&{/bus@0/padctl@3520000/pads/usb3/lanes/usb3-1}>, >> + <&{/bus@0/padctl@3520000/pads/usb3/lanes/usb3-2}>; >> + phy-names = "usb2-0", "usb2-1", "usb2-2", "usb2-3", >> + "usb3-0", "usb3-1", "usb3-2"; >> + }; >> + >> + i2c@c240000 { >> + status = "okay"; >> + ucsi_ccg: ucsi_ccg@8 { > > No underscores in node names. > >> + compatible = "cypress,cypd4226"; >> + cypress,firmware-build = "gn"; >> + interrupt-parent = <&gpio>; >> + interrupts = <TEGRA234_MAIN_GPIO(Y, 4) IRQ_TYPE_LEVEL_LOW>; >> + reg = <0x08>; >> + status = "okay"; > > The pattern of redefining full path in Tegra is confusing - I have no > clue which of these status=okay are correct which are redundant. > > Do you? I understand you may not like this approach, however, this comment is not really relevant to just this patch, but a general comment. But yes we will ensure that this is correct. > > >> + #address-cells = <1>; >> + #size-cells = <0>; >> + ccg_typec_con0: connector@0 { >> + compatible = "usb-c-connector"; >> + reg = <0>; >> + label = "USB-C"; >> + data-role = "host"; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + port@0 { >> + reg = <0>; >> + #address-cells = <1>; >> + #size-cells = <0>; > > Hm, why do you have here cells? > > Did you test the DTS with dtbs_check? That does not look correct and so we will correct. Thanks! Jon
On 28/10/2022 05:33, Jon Hunter wrote: >>> + ucsi_ccg: ucsi_ccg@8 { >> >> No underscores in node names. >> >>> + compatible = "cypress,cypd4226"; >>> + cypress,firmware-build = "gn"; >>> + interrupt-parent = <&gpio>; >>> + interrupts = <TEGRA234_MAIN_GPIO(Y, 4) IRQ_TYPE_LEVEL_LOW>; >>> + reg = <0x08>; >>> + status = "okay"; >> >> The pattern of redefining full path in Tegra is confusing - I have no >> clue which of these status=okay are correct which are redundant. >> >> Do you? > > I understand you may not like this approach, however, this comment is > not really relevant to just this patch, but a general comment. But yes > we will ensure that this is correct. > Just to clarify - this status looks redundant, but I have no way to tell for sure... Best regards, Krzysztof
On 28/10/2022 12:27, Krzysztof Kozlowski wrote: > On 28/10/2022 05:33, Jon Hunter wrote: >>>> + ucsi_ccg: ucsi_ccg@8 { >>> >>> No underscores in node names. >>> >>>> + compatible = "cypress,cypd4226"; >>>> + cypress,firmware-build = "gn"; >>>> + interrupt-parent = <&gpio>; >>>> + interrupts = <TEGRA234_MAIN_GPIO(Y, 4) IRQ_TYPE_LEVEL_LOW>; >>>> + reg = <0x08>; >>>> + status = "okay"; >>> >>> The pattern of redefining full path in Tegra is confusing - I have no >>> clue which of these status=okay are correct which are redundant. >>> >>> Do you? >> >> I understand you may not like this approach, however, this comment is >> not really relevant to just this patch, but a general comment. But yes >> we will ensure that this is correct. >> > > Just to clarify - this status looks redundant, but I have no way to tell > for sure... I see. This is the only place where this device appears. I always forget if we are meant to explicitly set status to okay or just omit. Personally, I always prefer to be explicit. Jon
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. However, I'm also a little confused as to the meaning of the property. Looking at the driver, the fw_build field is used to check for "supported vendors". "nv" and "gn" are clearly the same vendor (NVIDIA), so that's at least not 100% accurate. The DT bindings say that this denotes the product series, which seems to be more in line with how the driver uses it. The driver also uses it to implement changes in behavior across different variants, which is something that we would typically describe using compatible strings. So I wonder if we should, at least for device tree, switch to using different compatible strings rather than this separate matching mechanism. We could then associate a "product series" with the compatible string rather than having this extra field. There's also an argument to be made for keeping the interface the same between ACPI and DT. Rob, Krzysztof? Thierry
On Fri, Oct 28, 2022 at 07:27:09AM -0400, Krzysztof Kozlowski wrote: > On 28/10/2022 05:33, Jon Hunter wrote: > >>> + ucsi_ccg: ucsi_ccg@8 { > >> > >> No underscores in node names. > >> > >>> + compatible = "cypress,cypd4226"; > >>> + cypress,firmware-build = "gn"; > >>> + interrupt-parent = <&gpio>; > >>> + interrupts = <TEGRA234_MAIN_GPIO(Y, 4) IRQ_TYPE_LEVEL_LOW>; > >>> + reg = <0x08>; > >>> + status = "okay"; > >> > >> The pattern of redefining full path in Tegra is confusing - I have no > >> clue which of these status=okay are correct which are redundant. > >> > >> Do you? > > > > I understand you may not like this approach, however, this comment is > > not really relevant to just this patch, but a general comment. But yes > > we will ensure that this is correct. > > > > Just to clarify - this status looks redundant, but I have no way to tell > for sure... But that's independent of whether we specify this using the full path or reference the node by label, isn't it? The only way to make sure that a status = "okay" is not redundant is by manual inspection. I don't know of an automated way to do that. Perhaps it's something that could be added as a check to DTC? In this particular case I don't think the status is needed. As Jon mentioned, this device is first defined here and status = "okay" is the default, so this is redundant. Thierry
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'? Jon [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-nvidia-gpu.c#n261
On Mon, Oct 24, 2022 at 03:41:27PM +0800, Wayne Chang wrote: > From: Sing-Han Chen <singhanc@nvidia.com> > > This change adds Tegra234 XUSB host mode controller support. > > In Tegra234, some of the registers have moved to bar2 space. > The new soc variable has_bar2 indicates the chip with bar2 > area. This patch adds new reg helper to let the driver reuse > the same code for those chips with bar2 support. > > The new soc variables has_ifr indicates the chip with IFR FW > loading support. IFR registers would be configured in > MB1, and FW loading will be triggered in MB2. > > Signed-off-by: Sing-Han Chen <singhanc@nvidia.com> > Co-developed-by: Wayne Chang <waynec@nvidia.com> > Signed-off-by: Wayne Chang <waynec@nvidia.com> > --- > drivers/usb/host/xhci-tegra.c | 277 +++++++++++++++++++++++++++++----- > 1 file changed, 237 insertions(+), 40 deletions(-) > > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c > index bdb776553826..86036eeece43 100644 > --- a/drivers/usb/host/xhci-tegra.c > +++ b/drivers/usb/host/xhci-tegra.c > @@ -44,6 +44,9 @@ > #define XUSB_CFG_4 0x010 > #define XUSB_BASE_ADDR_SHIFT 15 > #define XUSB_BASE_ADDR_MASK 0x1ffff > +#define XUSB_CFG_7 0x01c > +#define XUSB_BASE2_ADDR_SHIFT 16 > +#define XUSB_BASE2_ADDR_MASK 0xffff > #define XUSB_CFG_16 0x040 > #define XUSB_CFG_24 0x060 > #define XUSB_CFG_AXI_CFG 0x0f8 > @@ -75,6 +78,20 @@ > #define MBOX_SMI_INTR_FW_HANG BIT(1) > #define MBOX_SMI_INTR_EN BIT(3) > > +/* BAR2 registers */ > +#define XUSB_BAR2_ARU_MBOX_CMD 0x004 > +#define XUSB_BAR2_ARU_MBOX_DATA_IN 0x008 > +#define XUSB_BAR2_ARU_MBOX_DATA_OUT 0x00c > +#define XUSB_BAR2_ARU_MBOX_OWNER 0x010 > +#define XUSB_BAR2_ARU_SMI_INTR 0x014 > +#define XUSB_BAR2_ARU_SMI_ARU_FW_SCRATCH_DATA0 0x01c > +#define XUSB_BAR2_ARU_IFRDMA_CFG0 0x0e0 > +#define XUSB_BAR2_ARU_IFRDMA_CFG1 0x0e4 > +#define XUSB_BAR2_ARU_IFRDMA_STREAMID_FIELD 0x0e8 > +#define XUSB_BAR2_ARU_C11_CSBRANGE 0x9c > +#define XUSB_BAR2_ARU_FW_SCRATCH 0x1000 > +#define XUSB_BAR2_CSB_BASE_ADDR 0x2000 > + > /* IPFS registers */ > #define IPFS_XUSB_HOST_MSI_BAR_SZ_0 0x0c0 > #define IPFS_XUSB_HOST_MSI_AXI_BAR_ST_0 0x0c4 > @@ -111,6 +128,9 @@ > #define IMFILLRNG1_TAG_HI_SHIFT 16 > #define XUSB_FALC_IMFILLCTL 0x158 > > +/* CSB ARU registers */ Weird double-space between "ARU" and "registers". > +#define XUSB_CSB_ARU_SCRATCH0 0x100100 > + > /* MP CSB registers */ > #define XUSB_CSB_MP_ILOAD_ATTR 0x101a00 > #define XUSB_CSB_MP_ILOAD_BASE_LO 0x101a04 > @@ -131,6 +151,9 @@ > > #define IMEM_BLOCK_SIZE 256 > > +#define FW_IOCTL_TYPE_SHIFT (24) This should use tabs for spacing, like all the other definitions. Also, no need to wrap literal integers in parentheses. > +#define FW_IOCTL_CFGTBL_READ (17) No need for the parentheses. > + > struct tegra_xusb_fw_header { > __le32 boot_loadaddr_in_imem; > __le32 boot_codedfi_offset; > @@ -175,6 +198,7 @@ struct tegra_xusb_mbox_regs { > u16 data_in; > u16 data_out; > u16 owner; > + u16 smi_intr; > }; > > struct tegra_xusb_context_soc { > @@ -189,6 +213,7 @@ struct tegra_xusb_context_soc { > } fpci; > }; > > +struct tegra_xusb_soc_ops; Probably better to move the definition of the structure here and instead predeclare struct tegra_xusb. > struct tegra_xusb_soc { > const char *firmware; > const char * const *supply_names; > @@ -205,11 +230,15 @@ struct tegra_xusb_soc { > } ports; > > struct tegra_xusb_mbox_regs mbox; > + struct tegra_xusb_soc_ops *ops; const please. > > bool scale_ss_clock; > bool has_ipfs; > bool lpm_support; > bool otg_reset_sspi; > + > + bool has_bar2; > + bool has_ifr; > }; > > struct tegra_xusb_context { > @@ -230,6 +259,8 @@ struct tegra_xusb { > > void __iomem *ipfs_base; > void __iomem *fpci_base; > + void __iomem *bar2_base; > + resource_size_t bar2_start; Maybe just store struct resource *bar2, here. [...] > @@ -664,6 +754,7 @@ static void tegra_xusb_mbox_handle(struct tegra_xusb *tegra, > static irqreturn_t tegra_xusb_mbox_thread(int irq, void *data) > { > struct tegra_xusb *tegra = data; > + struct tegra_xusb_soc_ops *ops = tegra->soc->ops; const > @@ -709,6 +800,15 @@ static void tegra_xusb_config(struct tegra_xusb *tegra) > value |= regs & (XUSB_BASE_ADDR_MASK << XUSB_BASE_ADDR_SHIFT); > fpci_writel(tegra, value, XUSB_CFG_4); > > + /* Program BAR2 space */ > + if (tegra->soc->has_bar2) { You could make this depend on tegra->bar2 if you make the change above. > + value = fpci_readl(tegra, XUSB_CFG_7); > + value &= ~(XUSB_BASE2_ADDR_MASK << XUSB_BASE2_ADDR_SHIFT); > + value |= tegra->bar2_start & > + (XUSB_BASE2_ADDR_MASK << XUSB_BASE2_ADDR_SHIFT); > + fpci_writel(tegra, value, XUSB_CFG_7); > + } > + > usleep_range(100, 200); > > /* Enable bus master */ > @@ -881,21 +981,36 @@ static int tegra_xusb_request_firmware(struct tegra_xusb *tegra) > return 0; > } > > +static int tegra_xusb_wait_for_falcon(struct tegra_xusb *tegra) > +{ > + struct xhci_cap_regs __iomem *cap_regs; > + struct xhci_op_regs __iomem *op_regs; > + int ret; > + u32 val; Use "value" for consistency with the rest of the driver. > + > + cap_regs = tegra->regs; > + op_regs = tegra->regs + HC_LENGTH(readl(&cap_regs->hc_capbase)); > + > + ret = readl_poll_timeout(&op_regs->status, val, !(val & STS_CNR), 1000, 200000); > + > + if (ret) > + dev_err(tegra->dev, "XHCI Controller not ready. Falcon state: 0x%x\n", > + csb_readl(tegra, XUSB_FALC_CPUCTL)); > + > + return ret; > +} This refactoring could be a separate patch. It makes the rest of the changes harder to review. Not necessarily something that needs to be addressed, though. > + > static int tegra_xusb_load_firmware(struct tegra_xusb *tegra) > { > unsigned int code_tag_blocks, code_size_blocks, code_blocks; > - struct xhci_cap_regs __iomem *cap = tegra->regs; > struct tegra_xusb_fw_header *header; > struct device *dev = tegra->dev; > - struct xhci_op_regs __iomem *op; > - unsigned long timeout; > time64_t timestamp; > u64 address; > u32 value; > int err; > > header = (struct tegra_xusb_fw_header *)tegra->fw.virt; > - op = tegra->regs + HC_LENGTH(readl(&cap->hc_capbase)); > > if (csb_readl(tegra, XUSB_CSB_MP_ILOAD_BASE_LO) != 0) { > dev_info(dev, "Firmware already loaded, Falcon state %#x\n", > @@ -968,26 +1083,43 @@ static int tegra_xusb_load_firmware(struct tegra_xusb *tegra) > /* Boot Falcon CPU and wait for USBSTS_CNR to get cleared. */ > csb_writel(tegra, CPUCTL_STARTCPU, XUSB_FALC_CPUCTL); > > - timeout = jiffies + msecs_to_jiffies(200); > + if (tegra_xusb_wait_for_falcon(tegra)) > + return -EIO; > > - do { > - value = readl(&op->status); > - if ((value & STS_CNR) == 0) > - break; > + timestamp = le32_to_cpu(header->fwimg_created_time); > > - usleep_range(1000, 2000); > - } while (time_is_after_jiffies(timeout)); > + dev_info(dev, "Firmware timestamp: %ptTs UTC\n", ×tamp); > + > + return 0; > +} > > - value = readl(&op->status); > - if (value & STS_CNR) { > - value = csb_readl(tegra, XUSB_FALC_CPUCTL); > - dev_err(dev, "XHCI controller not read: %#010x\n", value); > +static u32 tegra_xusb_read_firmware_header(struct tegra_xusb *tegra, u32 offset) > +{ > + /* > + * We only accept reading the firmware config table > + * The offset should not exceed the fw header structure > + */ > + if (offset >= sizeof(struct tegra_xusb_fw_header)) > + return 0; You technically still allow reading 3 bytes past the header structure. Or does the firmware's CFGTL_READ IOCTL mask out the lower 2 bits of the offset? > + > + bar2_writel(tegra, (FW_IOCTL_CFGTBL_READ << FW_IOCTL_TYPE_SHIFT) | offset, > + XUSB_BAR2_ARU_FW_SCRATCH); > + return bar2_readl(tegra, XUSB_BAR2_ARU_SMI_ARU_FW_SCRATCH_DATA0); > +} > + > +static int tegra_xusb_init_ifr_firmware(struct tegra_xusb *tegra) > +{ > + time64_t timestamp; > + > + if (tegra_xusb_wait_for_falcon(tegra)) > return -EIO; > - } > > - timestamp = le32_to_cpu(header->fwimg_created_time); > +#define offsetof_32(X, Y) ((u8)(offsetof(X, Y) / sizeof(__le32))) > + timestamp = tegra_xusb_read_firmware_header(tegra, > + offsetof_32(struct tegra_xusb_fw_header, > + fwimg_created_time) << 2); > > - dev_info(dev, "Firmware timestamp: %ptTs UTC\n", ×tamp); > + dev_info(tegra->dev, "Firmware timestamp: %ptTs UTC\n", ×tamp); > > return 0; > } > @@ -1403,7 +1535,7 @@ static int tegra_xusb_probe(struct platform_device *pdev) > struct of_phandle_args args; > struct tegra_xusb *tegra; > struct device_node *np; > - struct resource *regs; > + struct resource *res, *regs; > struct xhci_hcd *xhci; > unsigned int i, j, k; > struct phy *phy; > @@ -1435,6 +1567,11 @@ static int tegra_xusb_probe(struct platform_device *pdev) > tegra->ipfs_base = devm_platform_ioremap_resource(pdev, 2); > if (IS_ERR(tegra->ipfs_base)) > return PTR_ERR(tegra->ipfs_base); > + } else if (tegra->soc->has_bar2) { > + tegra->bar2_base = devm_platform_get_and_ioremap_resource(pdev, 2, &res); If you store struct resource *bar2 in tegra, you can pass &tegra->bar2 here and ... > + if (IS_ERR(tegra->bar2_base)) > + return PTR_ERR(tegra->bar2_base); > + tegra->bar2_start = res->start; ... skip this. > } > > tegra->xhci_irq = platform_get_irq(pdev, 0); > @@ -1651,10 +1788,13 @@ static int tegra_xusb_probe(struct platform_device *pdev) > goto disable_phy; > } > > - err = tegra_xusb_request_firmware(tegra); > - if (err < 0) { > - dev_err(&pdev->dev, "failed to request firmware: %d\n", err); > - goto disable_phy; > + if (!tegra->soc->has_ifr) { > + err = tegra_xusb_request_firmware(tegra); > + if (err < 0) { > + dev_err(&pdev->dev, > + "failed to request firmware: %d\n", err); > + goto disable_phy; > + } > } > > err = tegra_xusb_unpowergate_partitions(tegra); > @@ -1663,7 +1803,10 @@ static int tegra_xusb_probe(struct platform_device *pdev) > > tegra_xusb_config(tegra); > > - err = tegra_xusb_load_firmware(tegra); > + if (tegra->soc->has_ifr) > + err = tegra_xusb_init_ifr_firmware(tegra); > + else > + err = tegra_xusb_load_firmware(tegra); > if (err < 0) { > dev_err(&pdev->dev, "failed to load firmware: %d\n", err); > goto powergate; > @@ -2070,7 +2213,10 @@ static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool runtime) > tegra_xusb_config(tegra); > tegra_xusb_restore_context(tegra); > > - err = tegra_xusb_load_firmware(tegra); > + if (tegra->soc->has_ifr) > + err = tegra_xusb_init_ifr_firmware(tegra); > + else > + err = tegra_xusb_load_firmware(tegra); Might be worth extracting this into a new function since you use this twice now. > if (err < 0) { > dev_err(tegra->dev, "failed to load firmware: %d\n", err); > goto disable_phy; > @@ -2271,6 +2417,13 @@ static const struct tegra_xusb_context_soc tegra124_xusb_context = { > }, > }; > > +static struct tegra_xusb_soc_ops tegra124_ops = { const > + .mbox_reg_readl = &fpci_readl, > + .mbox_reg_writel = &fpci_writel, > + .csb_reg_readl = &fpci_csb_readl, > + .csb_reg_writel = &fpci_csb_writel, > +}; > + > static const struct tegra_xusb_soc tegra124_soc = { > .firmware = "nvidia/tegra124/xusb.bin", > .supply_names = tegra124_supply_names, > @@ -2286,11 +2439,13 @@ static const struct tegra_xusb_soc tegra124_soc = { > .scale_ss_clock = true, > .has_ipfs = true, > .otg_reset_sspi = false, > + .ops = &tegra124_ops, > .mbox = { > .cmd = 0xe4, > .data_in = 0xe8, > .data_out = 0xec, > .owner = 0xf0, > + .smi_intr = XUSB_CFG_ARU_SMI_INTR, > }, > }; > MODULE_FIRMWARE("nvidia/tegra124/xusb.bin"); > @@ -2322,11 +2477,13 @@ static const struct tegra_xusb_soc tegra210_soc = { > .scale_ss_clock = false, > .has_ipfs = true, > .otg_reset_sspi = true, > + .ops = &tegra124_ops, > .mbox = { > .cmd = 0xe4, > .data_in = 0xe8, > .data_out = 0xec, > .owner = 0xf0, > + .smi_intr = XUSB_CFG_ARU_SMI_INTR, > }, > }; > MODULE_FIRMWARE("nvidia/tegra210/xusb.bin"); > @@ -2363,11 +2520,13 @@ static const struct tegra_xusb_soc tegra186_soc = { > .scale_ss_clock = false, > .has_ipfs = false, > .otg_reset_sspi = false, > + .ops = &tegra124_ops, > .mbox = { > .cmd = 0xe4, > .data_in = 0xe8, > .data_out = 0xec, > .owner = 0xf0, > + .smi_intr = XUSB_CFG_ARU_SMI_INTR, > }, > .lpm_support = true, > }; > @@ -2394,21 +2553,59 @@ static const struct tegra_xusb_soc tegra194_soc = { > .scale_ss_clock = false, > .has_ipfs = false, > .otg_reset_sspi = false, > + .ops = &tegra124_ops, > .mbox = { > .cmd = 0x68, > .data_in = 0x6c, > .data_out = 0x70, > .owner = 0x74, > + .smi_intr = XUSB_CFG_ARU_SMI_INTR, > }, > .lpm_support = true, > }; > MODULE_FIRMWARE("nvidia/tegra194/xusb.bin"); > > +static struct tegra_xusb_soc_ops tegra234_ops = { const > + .mbox_reg_readl = &bar2_readl, > + .mbox_reg_writel = &bar2_writel, > + .csb_reg_readl = &bar2_csb_readl, > + .csb_reg_writel = &bar2_csb_writel, > +}; > + > +static const struct tegra_xusb_soc tegra234_soc = { > + .firmware = "nvidia/tegra234/xusb.bin", > + .supply_names = tegra194_supply_names, > + .num_supplies = ARRAY_SIZE(tegra194_supply_names), > + .phy_types = tegra194_phy_types, > + .num_types = ARRAY_SIZE(tegra194_phy_types), > + .context = &tegra186_xusb_context, > + .ports = { > + .usb3 = { .offset = 0, .count = 4, }, > + .usb2 = { .offset = 4, .count = 4, }, > + }, > + .scale_ss_clock = false, > + .has_ipfs = false, > + .otg_reset_sspi = false, > + .ops = &tegra234_ops, > + .mbox = { > + .cmd = XUSB_BAR2_ARU_MBOX_CMD, > + .data_in = XUSB_BAR2_ARU_MBOX_DATA_IN, > + .data_out = XUSB_BAR2_ARU_MBOX_DATA_OUT, > + .owner = XUSB_BAR2_ARU_MBOX_OWNER, > + .smi_intr = XUSB_BAR2_ARU_SMI_INTR, > + }, > + .lpm_support = true, > + .has_bar2 = true, > + .has_ifr = true, > +}; > +MODULE_FIRMWARE("nvidia/tegra234/xusb.bin"); Can you prepare a patch to add this firmware to the linux-firmware repository? I don't see it there yet. Thierry > + > static const struct of_device_id tegra_xusb_of_match[] = { > { .compatible = "nvidia,tegra124-xusb", .data = &tegra124_soc }, > { .compatible = "nvidia,tegra210-xusb", .data = &tegra210_soc }, > { .compatible = "nvidia,tegra186-xusb", .data = &tegra186_soc }, > { .compatible = "nvidia,tegra194-xusb", .data = &tegra194_soc }, > + { .compatible = "nvidia,tegra234-xusb", .data = &tegra234_soc }, > { }, > }; > MODULE_DEVICE_TABLE(of, tegra_xusb_of_match); > -- > 2.25.1 >
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
On 28/10/2022 08:38, Thierry Reding wrote: >>> >>> I understand you may not like this approach, however, this comment is >>> not really relevant to just this patch, but a general comment. But yes >>> we will ensure that this is correct. >>> >> >> Just to clarify - this status looks redundant, but I have no way to tell >> for sure... > > But that's independent of whether we specify this using the full path or > reference the node by label, isn't it? The only way to make sure that a > status = "okay" is not redundant is by manual inspection. I don't know > of an automated way to do that. Perhaps it's something that could be > added as a check to DTC? With overrides/extends pattern it is easy to spot one case of mistakes - you see override, then status might be needed might not. You see new node (like here!) - then status=okay is redundant. Best regards, Krzysztof
On 28/10/2022 14:39, Thierry Reding wrote: ... >> +static const struct tegra_xusb_soc tegra234_soc = { >> + .firmware = "nvidia/tegra234/xusb.bin", >> + .supply_names = tegra194_supply_names, >> + .num_supplies = ARRAY_SIZE(tegra194_supply_names), >> + .phy_types = tegra194_phy_types, >> + .num_types = ARRAY_SIZE(tegra194_phy_types), >> + .context = &tegra186_xusb_context, >> + .ports = { >> + .usb3 = { .offset = 0, .count = 4, }, >> + .usb2 = { .offset = 4, .count = 4, }, >> + }, >> + .scale_ss_clock = false, >> + .has_ipfs = false, >> + .otg_reset_sspi = false, >> + .ops = &tegra234_ops, >> + .mbox = { >> + .cmd = XUSB_BAR2_ARU_MBOX_CMD, >> + .data_in = XUSB_BAR2_ARU_MBOX_DATA_IN, >> + .data_out = XUSB_BAR2_ARU_MBOX_DATA_OUT, >> + .owner = XUSB_BAR2_ARU_MBOX_OWNER, >> + .smi_intr = XUSB_BAR2_ARU_SMI_INTR, >> + }, >> + .lpm_support = true, >> + .has_bar2 = true, >> + .has_ifr = true, >> +}; >> +MODULE_FIRMWARE("nvidia/tegra234/xusb.bin"); > > Can you prepare a patch to add this firmware to the linux-firmware > repository? I don't see it there yet. Actually, we should remove the MODULE_FIRMWARE completely for Tegra234. Per the commit message the variable 'has_ifr' is used to indicate if the firmware is loaded by calling request_firmware() or via these IFR registers. I wonder if we need this 'has_ifr' variable if we should just avoid setting the 'firmware' variable for Tegra234 and use this instead of the 'has_ifr'? Jon
On 10/26/22 07:24, Rob Herring wrote: > External email: Use caution opening links or attachments > > > On Mon, Oct 24, 2022 at 03:41:18PM +0800, Wayne Chang wrote: >> Extend the Tegra XUSB controller device tree binding with Tegra234 >> support. >> >> Signed-off-by: Wayne Chang <waynec@nvidia.com> >> --- >> .../bindings/usb/nvidia,tegra-xudc.yaml | 24 ++++++++++++------- >> 1 file changed, 16 insertions(+), 8 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.yaml b/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.yaml >> index fd6e7c81426e..517fb692f199 100644 >> --- a/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.yaml >> +++ b/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.yaml >> @@ -22,6 +22,7 @@ properties: >> - nvidia,tegra210-xudc # For Tegra210 >> - nvidia,tegra186-xudc # For Tegra186 >> - nvidia,tegra194-xudc # For Tegra194 >> + - nvidia,tegra234-xudc # For Tegra234 >> >> reg: >> minItems: 2 >> @@ -90,21 +91,27 @@ properties: >> >> phys: >> minItems: 1 >> + maxItems: 8 >> description: >> Must contain an entry for each entry in phy-names. >> See ../phy/phy-bindings.txt for details. >> >> phy-names: >> minItems: 1 >> + maxItems: 8 >> items: >> - - const: usb2-0 >> - - const: usb2-1 >> - - const: usb2-2 >> - - const: usb2-3 >> - - const: usb3-0 >> - - const: usb3-1 >> - - const: usb3-2 >> - - const: usb3-3 >> + anyOf: >> + - const: usb2-0 >> + - const: usb2-1 >> + - const: usb2-2 >> + - const: usb2-3 >> + - const: usb3-0 >> + - const: usb3-1 >> + - const: usb3-2 >> + - const: usb3-3 > > items: > pattern: '^usb[23]-[0-3]$' > > And an explanation why you need any random order. If it is just > different for Tegra234, then you need an if/then schema for this. Thanks for the review. We need to pick up one or more for the corresponding phys of the USB ports. It should be a common settings among all the chips. Adding anyOf here for the reason above and passing the dtb check. Please let me know If I have any misunderstanding here. thanks, Wayne. > >> + >> + dma-coherent: >> + type: boolean >> >> avddio-usb-supply: >> description: PCIe/USB3 analog logic power supply. Must supply 1.05 V. >> @@ -153,6 +160,7 @@ allOf: >> enum: >> - nvidia,tegra186-xudc >> - nvidia,tegra194-xudc >> + - nvidia,tegra234-xudc >> then: >> properties: >> reg: >> -- >> 2.25.1 >> >>
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@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. > 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
On 11/1/22 22:53, Jon Hunter wrote: > On 28/10/2022 14:39, Thierry Reding wrote: > > ... > >>> +static const struct tegra_xusb_soc tegra234_soc = { >>> + .firmware = "nvidia/tegra234/xusb.bin", >>> + .supply_names = tegra194_supply_names, >>> + .num_supplies = ARRAY_SIZE(tegra194_supply_names), >>> + .phy_types = tegra194_phy_types, >>> + .num_types = ARRAY_SIZE(tegra194_phy_types), >>> + .context = &tegra186_xusb_context, >>> + .ports = { >>> + .usb3 = { .offset = 0, .count = 4, }, >>> + .usb2 = { .offset = 4, .count = 4, }, >>> + }, >>> + .scale_ss_clock = false, >>> + .has_ipfs = false, >>> + .otg_reset_sspi = false, >>> + .ops = &tegra234_ops, >>> + .mbox = { >>> + .cmd = XUSB_BAR2_ARU_MBOX_CMD, >>> + .data_in = XUSB_BAR2_ARU_MBOX_DATA_IN, >>> + .data_out = XUSB_BAR2_ARU_MBOX_DATA_OUT, >>> + .owner = XUSB_BAR2_ARU_MBOX_OWNER, >>> + .smi_intr = XUSB_BAR2_ARU_SMI_INTR, >>> + }, >>> + .lpm_support = true, >>> + .has_bar2 = true, >>> + .has_ifr = true, >>> +}; >>> +MODULE_FIRMWARE("nvidia/tegra234/xusb.bin"); >> >> Can you prepare a patch to add this firmware to the linux-firmware >> repository? I don't see it there yet. > > > Actually, we should remove the MODULE_FIRMWARE completely for Tegra234. > Per the commit message the variable 'has_ifr' is used to indicate if the > firmware is loaded by calling request_firmware() or via these IFR > registers. I wonder if we need this 'has_ifr' variable if we should just > avoid setting the 'firmware' variable for Tegra234 and use this instead > of the 'has_ifr'? > Thanks for the review. Yes, correct. The firmware loading is moved to MB2 and thus we do not need it anymore. I'll remove it together with the .firmware in the soc data. We could checking firmware in soc data instead of has_ifr as we now only get two ways to load the firmware. I'll make the change on it in the next patch series thanks, Wayne. > Jon >