| Message ID | 20240828143452.1407532-2-christian.bruel@foss.st.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add STM32MP25 USB3/PCIE COMBOPHY driver | expand |
On Wed, Aug 28, 2024 at 04:34:48PM +0200, Christian Bruel wrote: > Document the bindings for STM32 COMBOPHY interface, used to support > the PCIe and USB3 stm32mp25 drivers. > Following entries can be used to tune caracterisation parameters > - st,output-micro-ohms and st,output-vswing-microvolt bindings entries > to tune the impedance and voltage swing using discrete simulation results > - st,rx-equalizer register to set the internal rx equalizer filter value. > > Signed-off-by: Christian Bruel <christian.bruel@foss.st.com> > --- > .../bindings/phy/st,stm32mp25-combophy.yaml | 128 ++++++++++++++++++ > 1 file changed, 128 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml > > diff --git a/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml > new file mode 100644 > index 000000000000..8d4a40b94507 > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml > @@ -0,0 +1,128 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/phy/st,stm32mp25-combophy.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: STMicroelectronics STM32MP25 USB3/PCIe COMBOPHY > + > +maintainers: > + - Christian Bruel <christian.bruel@foss.st.com> > + > +description: > + Single lane PHY shared (exclusive) between the USB3 and PCIe controllers. > + Supports 5Gbit/s for USB3 and PCIe gen2 or 2.5Gbit/s for PCIe gen1. > + > +properties: > + compatible: > + const: st,stm32mp25-combophy > + > + reg: > + maxItems: 1 > + > + "#phy-cells": > + const: 1 > + > + clocks: > + minItems: 2 > + items: > + - description: apb Bus clock mandatory to access registers. > + - description: ker Internal RCC reference clock for USB3 or PCIe > + - description: pad Optional on board clock input for PCIe only. Typically an > + external 100Mhz oscillator wired on dedicated CLKIN pad. Used as reference > + clock input instead of the ker > + > + clock-names: > + minItems: 2 > + items: > + - const: apb > + - const: ker > + - const: pad > + > + resets: > + maxItems: 1 > + > + reset-names: > + const: phy > + > + power-domains: > + maxItems: 1 > + > + wakeup-source: true > + > + interrupts: > + maxItems: 1 > + description: interrupt used for wakeup > + > + access-controllers: > + minItems: 1 > + maxItems: 2 Can you please describe the items here? > + st,syscfg: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: Phandle to the SYSCON entry required for configuring PCIe > + or USB3. Why is a phandle required for this lookup, rather than doing it by compatible? > + > + st,ssc-on: > + type: boolean flag, not boolean, for presence based stuff. And in the driver, s/of_property_read_bool/of_property_present/. > + description: > + A boolean property whose presence indicates that the SSC for common clock > + needs to be set. And what, may I ask, does "SSC" mean? "Common clock" is also a bit of a "linuxism", what does this actually do in the hardware block? > + > + st,rx-equalizer: > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 0 > + maximum: 7 > + default: 2 > + description: > + A 3 bit value to tune the RX fixed equalizer setting for optimal eye compliance > + > + st,output-micro-ohms: > + minimum: 3999000 > + maximum: 6090000 > + default: 4968000 > + description: > + A value property to tune the Single Ended Output Impedance, simulations results > + at 25C for a VDDP=0.8V. The hardware accepts discrete values in this range. > + > + st,output-vswing-microvolt: > + minimum: 442000 > + maximum: 803000 > + default: 803000 > + description: > + A value property in microvolt to tune the Single Ended Output Voltage Swing to change the > + Vlo, Vhi for a VDDP = 0.8V. The hardware accepts discrete values in this range. > + > +required: > + - "#phy-cells" > + - compatible > + - clocks > + - clock-names > + - reg > + - resets > + - reset-names > + - st,syscfg The order here should reflect the ordering in a node, so compatible and reg first, rather than sorted alphanumerically. > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/st,stm32mp25-rcc.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/reset/st,stm32mp25-rcc.h> > + > + combophy: phy@480c0000 { You can drop the label here, it ain't used by anything. Cheers, Conor. > + compatible = "st,stm32mp25-combophy"; > + reg = <0x480c0000 0x1000>; > + #phy-cells = <1>; > + clocks = <&rcc CK_BUS_USB3PCIEPHY>, <&rcc CK_KER_USB3PCIEPHY>; > + clock-names = "apb", "ker"; > + resets = <&rcc USB3PCIEPHY_R>; > + reset-names = "phy"; > + st,syscfg = <&syscfg>; > + access-controllers = <&rifsc 67>; > + power-domains = <&CLUSTER_PD>; > + wakeup-source; > + interrupts-extended = <&exti1 45 IRQ_TYPE_EDGE_FALLING>; > + }; > +... > -- > 2.34.1 >
On 8/28/24 18:11, Conor Dooley wrote: > On Wed, Aug 28, 2024 at 04:34:48PM +0200, Christian Bruel wrote: >> Document the bindings for STM32 COMBOPHY interface, used to support >> the PCIe and USB3 stm32mp25 drivers. >> Following entries can be used to tune caracterisation parameters >> - st,output-micro-ohms and st,output-vswing-microvolt bindings entries >> to tune the impedance and voltage swing using discrete simulation results >> - st,rx-equalizer register to set the internal rx equalizer filter value. >> >> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com> >> --- >> .../bindings/phy/st,stm32mp25-combophy.yaml | 128 ++++++++++++++++++ >> 1 file changed, 128 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml >> >> diff --git a/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml >> new file mode 100644 >> index 000000000000..8d4a40b94507 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml >> @@ -0,0 +1,128 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/phy/st,stm32mp25-combophy.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: STMicroelectronics STM32MP25 USB3/PCIe COMBOPHY >> + >> +maintainers: >> + - Christian Bruel <christian.bruel@foss.st.com> >> + >> +description: >> + Single lane PHY shared (exclusive) between the USB3 and PCIe controllers. >> + Supports 5Gbit/s for USB3 and PCIe gen2 or 2.5Gbit/s for PCIe gen1. >> + >> +properties: >> + compatible: >> + const: st,stm32mp25-combophy >> + >> + reg: >> + maxItems: 1 >> + >> + "#phy-cells": >> + const: 1 >> + >> + clocks: >> + minItems: 2 >> + items: >> + - description: apb Bus clock mandatory to access registers. >> + - description: ker Internal RCC reference clock for USB3 or PCIe >> + - description: pad Optional on board clock input for PCIe only. Typically an >> + external 100Mhz oscillator wired on dedicated CLKIN pad. Used as reference >> + clock input instead of the ker >> + >> + clock-names: >> + minItems: 2 >> + items: >> + - const: apb >> + - const: ker >> + - const: pad >> + >> + resets: >> + maxItems: 1 >> + >> + reset-names: >> + const: phy >> + >> + power-domains: >> + maxItems: 1 >> + >> + wakeup-source: true >> + >> + interrupts: >> + maxItems: 1 >> + description: interrupt used for wakeup >> + >> + access-controllers: >> + minItems: 1 >> + maxItems: 2 > Can you please describe the items here? I can specialize the description: "Phandle to the rifsc firewall device to check access right." otherwise described in access-controllers/access-controllers.yaml, see also bindings/bus/st,stm32mp25-rifsc.yaml > >> + st,syscfg: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: Phandle to the SYSCON entry required for configuring PCIe >> + or USB3. > Why is a phandle required for this lookup, rather than doing it by > compatible? the phandle is used to select the sysconf SoC configuration register depending on the PCIe/USB3 mode (selected by xlate function), so it's not like a lookup here. This sysconf register is also used for other settings such as the PLL, Reference clock selection, ... > >> + >> + st,ssc-on: >> + type: boolean > flag, not boolean, for presence based stuff. And in the driver, > s/of_property_read_bool/of_property_present/. ok > >> + description: >> + A boolean property whose presence indicates that the SSC for common clock >> + needs to be set. > And what, may I ask, does "SSC" mean? "Common clock" is also a bit of a > "linuxism", what does this actually do in the hardware block? SSC for Spread Spectrum Clocking. It is an hardware setting for the 100Mhz PCIe reference common clock, I will rephrase the description > >> + >> + st,rx-equalizer: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + minimum: 0 >> + maximum: 7 >> + default: 2 >> + description: >> + A 3 bit value to tune the RX fixed equalizer setting for optimal eye compliance >> + >> + st,output-micro-ohms: >> + minimum: 3999000 >> + maximum: 6090000 >> + default: 4968000 >> + description: >> + A value property to tune the Single Ended Output Impedance, simulations results >> + at 25C for a VDDP=0.8V. The hardware accepts discrete values in this range. >> + >> + st,output-vswing-microvolt: >> + minimum: 442000 >> + maximum: 803000 >> + default: 803000 >> + description: >> + A value property in microvolt to tune the Single Ended Output Voltage Swing to change the >> + Vlo, Vhi for a VDDP = 0.8V. The hardware accepts discrete values in this range. >> + >> +required: >> + - "#phy-cells" >> + - compatible >> + - clocks >> + - clock-names >> + - reg >> + - resets >> + - reset-names >> + - st,syscfg > The order here should reflect the ordering in a node, so compatible and > reg first, rather than sorted alphanumerically. ok >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/st,stm32mp25-rcc.h> >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/reset/st,stm32mp25-rcc.h> >> + >> + combophy: phy@480c0000 { > You can drop the label here, it ain't used by anything. ok thanks, Christian > > Cheers, > Conor. > >> + compatible = "st,stm32mp25-combophy"; >> + reg = <0x480c0000 0x1000>; >> + #phy-cells = <1>; >> + clocks = <&rcc CK_BUS_USB3PCIEPHY>, <&rcc CK_KER_USB3PCIEPHY>; >> + clock-names = "apb", "ker"; >> + resets = <&rcc USB3PCIEPHY_R>; >> + reset-names = "phy"; >> + st,syscfg = <&syscfg>; >> + access-controllers = <&rifsc 67>; >> + power-domains = <&CLUSTER_PD>; >> + wakeup-source; >> + interrupts-extended = <&exti1 45 IRQ_TYPE_EDGE_FALLING>; >> + }; >> +... >> -- >> 2.34.1 >>
On Thu, Aug 29, 2024 at 01:06:53PM +0200, Christian Bruel wrote: > On 8/28/24 18:11, Conor Dooley wrote: > > On Wed, Aug 28, 2024 at 04:34:48PM +0200, Christian Bruel wrote: > > > Document the bindings for STM32 COMBOPHY interface, used to support > > > the PCIe and USB3 stm32mp25 drivers. > > > Following entries can be used to tune caracterisation parameters > > > - st,output-micro-ohms and st,output-vswing-microvolt bindings entries > > > to tune the impedance and voltage swing using discrete simulation results > > > - st,rx-equalizer register to set the internal rx equalizer filter value. > > > > > > Signed-off-by: Christian Bruel <christian.bruel@foss.st.com> > > > --- > > > .../bindings/phy/st,stm32mp25-combophy.yaml | 128 ++++++++++++++++++ > > > 1 file changed, 128 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml > > > new file mode 100644 > > > index 000000000000..8d4a40b94507 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml > > > @@ -0,0 +1,128 @@ > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/phy/st,stm32mp25-combophy.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: STMicroelectronics STM32MP25 USB3/PCIe COMBOPHY > > > + > > > +maintainers: > > > + - Christian Bruel <christian.bruel@foss.st.com> > > > + > > > +description: > > > + Single lane PHY shared (exclusive) between the USB3 and PCIe controllers. > > > + Supports 5Gbit/s for USB3 and PCIe gen2 or 2.5Gbit/s for PCIe gen1. > > > + > > > +properties: > > > + compatible: > > > + const: st,stm32mp25-combophy > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + "#phy-cells": > > > + const: 1 > > > + > > > + clocks: > > > + minItems: 2 > > > + items: > > > + - description: apb Bus clock mandatory to access registers. > > > + - description: ker Internal RCC reference clock for USB3 or PCIe > > > + - description: pad Optional on board clock input for PCIe only. Typically an > > > + external 100Mhz oscillator wired on dedicated CLKIN pad. Used as reference > > > + clock input instead of the ker > > > + > > > + clock-names: > > > + minItems: 2 > > > + items: > > > + - const: apb > > > + - const: ker > > > + - const: pad > > > + > > > + resets: > > > + maxItems: 1 > > > + > > > + reset-names: > > > + const: phy > > > + > > > + power-domains: > > > + maxItems: 1 > > > + > > > + wakeup-source: true > > > + > > > + interrupts: > > > + maxItems: 1 > > > + description: interrupt used for wakeup > > > + > > > + access-controllers: > > > + minItems: 1 > > > + maxItems: 2 > > Can you please describe the items here? > > I can specialize the description: "Phandle to the rifsc firewall device to check access right." Right, but there are potentially two access controllers here. You need to describe which is which, so that people can hook them up in the correct order. In what case are there two? Your dts patch only has one. > otherwise described in access-controllers/access-controllers.yaml, see also bindings/bus/st,stm32mp25-rifsc.yaml > > > > > > + st,syscfg: > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > + description: Phandle to the SYSCON entry required for configuring PCIe > > > + or USB3. > > Why is a phandle required for this lookup, rather than doing it by > > compatible? > > the phandle is used to select the sysconf SoC configuration register > depending on the PCIe/USB3 mode (selected by xlate function), so it's not > like a lookup here. If "syscon_regmap_lookup_by_phandle()" is not a lookup, then I do not know what is. An example justification for it would be that there are multiple combophys on the same soc, each using a different sysconf region. Your dts suggests that is not the case though, since you have st,syscfg = <&syscfg>; in it, rather than st,syscfg = <&syscfg0>;. > This sysconf register is also used for other settings > such as the PLL, Reference clock selection, ... > > > > > > + > > > + st,ssc-on: > > > + type: boolean > > flag, not boolean, for presence based stuff. And in the driver, > > s/of_property_read_bool/of_property_present/. > > ok > > > > > > + description: > > > + A boolean property whose presence indicates that the SSC for common clock > > > + needs to be set. > > And what, may I ask, does "SSC" mean? "Common clock" is also a bit of a > > "linuxism", what does this actually do in the hardware block? > > SSC for Spread Spectrum Clocking. It is an hardware setting for the 100Mhz PCIe reference common clock, Ah, so not really a "common clock" linuxism at all. > I will rephrase the description How is someone supposed to decide between on and off? Is it always on for PCIe, or only on in some configurations? Or maybe only (or always?) on if the pad clock is provided? Cheers, Conor.
On 8/29/24 18:44, Conor Dooley wrote: > On Thu, Aug 29, 2024 at 01:06:53PM +0200, Christian Bruel wrote: >> On 8/28/24 18:11, Conor Dooley wrote: >>> On Wed, Aug 28, 2024 at 04:34:48PM +0200, Christian Bruel wrote: >>>> Document the bindings for STM32 COMBOPHY interface, used to support >>>> the PCIe and USB3 stm32mp25 drivers. >>>> Following entries can be used to tune caracterisation parameters >>>> - st,output-micro-ohms and st,output-vswing-microvolt bindings entries >>>> to tune the impedance and voltage swing using discrete simulation results >>>> - st,rx-equalizer register to set the internal rx equalizer filter value. >>>> >>>> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com> >>>> --- >>>> .../bindings/phy/st,stm32mp25-combophy.yaml | 128 ++++++++++++++++++ >>>> 1 file changed, 128 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml >>>> >>>> diff --git a/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml >>>> new file mode 100644 >>>> index 000000000000..8d4a40b94507 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml >>>> @@ -0,0 +1,128 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/phy/st,stm32mp25-combophy.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>>> +title: STMicroelectronics STM32MP25 USB3/PCIe COMBOPHY >>>> + >>>> +maintainers: >>>> + - Christian Bruel <christian.bruel@foss.st.com> >>>> + >>>> +description: >>>> + Single lane PHY shared (exclusive) between the USB3 and PCIe controllers. >>>> + Supports 5Gbit/s for USB3 and PCIe gen2 or 2.5Gbit/s for PCIe gen1. >>>> + >>>> +properties: >>>> + compatible: >>>> + const: st,stm32mp25-combophy >>>> + >>>> + reg: >>>> + maxItems: 1 >>>> + >>>> + "#phy-cells": >>>> + const: 1 >>>> + >>>> + clocks: >>>> + minItems: 2 >>>> + items: >>>> + - description: apb Bus clock mandatory to access registers. >>>> + - description: ker Internal RCC reference clock for USB3 or PCIe >>>> + - description: pad Optional on board clock input for PCIe only. Typically an >>>> + external 100Mhz oscillator wired on dedicated CLKIN pad. Used as reference >>>> + clock input instead of the ker >>>> + >>>> + clock-names: >>>> + minItems: 2 >>>> + items: >>>> + - const: apb >>>> + - const: ker >>>> + - const: pad >>>> + >>>> + resets: >>>> + maxItems: 1 >>>> + >>>> + reset-names: >>>> + const: phy >>>> + >>>> + power-domains: >>>> + maxItems: 1 >>>> + >>>> + wakeup-source: true >>>> + >>>> + interrupts: >>>> + maxItems: 1 >>>> + description: interrupt used for wakeup >>>> + >>>> + access-controllers: >>>> + minItems: 1 >>>> + maxItems: 2 >>> Can you please describe the items here? >> I can specialize the description: "Phandle to the rifsc firewall device to check access right." > Right, but there are potentially two access controllers here. You need > to describe which is which, so that people can hook them up in the > correct order. In what case are there two? Your dts patch only has one. (sorry, resent in plain test) yes, we don't have more than 1 controller in the current setup, I'll use maxItems: 1 >> otherwise described in access-controllers/access-controllers.yaml, see also bindings/bus/st,stm32mp25-rifsc.yaml >> >>>> + st,syscfg: >>>> + $ref: /schemas/types.yaml#/definitions/phandle >>>> + description: Phandle to the SYSCON entry required for configuring PCIe >>>> + or USB3. >>> Why is a phandle required for this lookup, rather than doing it by >>> compatible? >> the phandle is used to select the sysconf SoC configuration register >> depending on the PCIe/USB3 mode (selected by xlate function), so it's not >> like a lookup here. > If "syscon_regmap_lookup_by_phandle()" is not a lookup, then I do not > know what is. An example justification for it would be that there are > multiple combophys on the same soc, each using a different sysconf > region. Your dts suggests that is not the case though, since you have > st,syscfg = <&syscfg>; in it, rather than st,syscfg = <&syscfg0>;. I didn't get your suggestion earlier to use "syscon_regmap_lookup_by_compatible()". We have several other syscon in the other. That's why we choose a direct syscfg phandle > >> This sysconf register is also used for other settings >> such as the PLL, Reference clock selection, ... >> >>>> + >>>> + st,ssc-on: >>>> + type: boolean >>> flag, not boolean, for presence based stuff. And in the driver, >>> s/of_property_read_bool/of_property_present/. >> ok >> >>>> + description: >>>> + A boolean property whose presence indicates that the SSC for common clock >>>> + needs to be set. >>> And what, may I ask, does "SSC" mean? "Common clock" is also a bit of a >>> "linuxism", what does this actually do in the hardware block? >> SSC for Spread Spectrum Clocking. It is an hardware setting for the 100Mhz PCIe reference common clock, > Ah, so not really a "common clock" linuxism at all. > >> I will rephrase the description > How is someone supposed to decide between on and off? Is it always on > for PCIe, or only on in some configurations? Or maybe only (or always?) on > if the pad clock is provided? SSC is off by default and available for the PCIe pad clock. it must be defined for USB3 thanks Christian > > Cheers, > Conor.
On Fri, Aug 30, 2024 at 02:53:15PM +0200, Christian Bruel wrote: > > On 8/29/24 18:44, Conor Dooley wrote: > > On Thu, Aug 29, 2024 at 01:06:53PM +0200, Christian Bruel wrote: > > > On 8/28/24 18:11, Conor Dooley wrote: > > > > On Wed, Aug 28, 2024 at 04:34:48PM +0200, Christian Bruel wrote: > > > > > + st,syscfg: > > > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > > > + description: Phandle to the SYSCON entry required for configuring PCIe > > > > > + or USB3. > > > > Why is a phandle required for this lookup, rather than doing it by > > > > compatible? > > > the phandle is used to select the sysconf SoC configuration register > > > depending on the PCIe/USB3 mode (selected by xlate function), so it's not > > > like a lookup here. > > If "syscon_regmap_lookup_by_phandle()" is not a lookup, then I do not > > know what is. An example justification for it would be that there are > > multiple combophys on the same soc, each using a different sysconf > > region. Your dts suggests that is not the case though, since you have > > st,syscfg = <&syscfg>; in it, rather than st,syscfg = <&syscfg0>;. > > I didn't get your suggestion earlier to use "syscon_regmap_lookup_by_compatible()". > > We have several other syscon in the other. That's why we choose a direct syscfg phandle In the other what? SoCs? Way I see it, if you're going to support different socs in the same driver, it's almost a certainty that the offsets within a syscon that particular features lie at are going to change between socs, so even if you have a phandle you're going to need to have the offsets in your match data. And if you're going to have offsets in match data, you may as well have the compatibles for the syscon in match data too. If the layout of the syscon hasn't changed between devices, then you should have a fallback compatible for the syscon too, making syscon_regmap_lookup_by_compatible() function without changes to the driver. If you do have multiple syscons, but they do different things, they should have different compatibles, so having multiple syscons doesn't justify using a property for this either in and of itself. If you have multiple syscons with the same layout (and therefore the same compatible) then a phandle makes sense, but if that's the case then you almost certainly have multiple combophys too! Otherwise, if you have one syscon, but the controls for more than one combophy are in it, then having a phandle _with an offset_ makes sense. If you know there are other SoCs with more than one combo phy, do they use different syscons, or is the same syscon used for more than one combophy?
On 8/30/24 16:55, Conor Dooley wrote: > On Fri, Aug 30, 2024 at 02:53:15PM +0200, Christian Bruel wrote: >> On 8/29/24 18:44, Conor Dooley wrote: >>> On Thu, Aug 29, 2024 at 01:06:53PM +0200, Christian Bruel wrote: >>>> On 8/28/24 18:11, Conor Dooley wrote: >>>>> On Wed, Aug 28, 2024 at 04:34:48PM +0200, Christian Bruel wrote: >>>>>> + st,syscfg: >>>>>> + $ref: /schemas/types.yaml#/definitions/phandle >>>>>> + description: Phandle to the SYSCON entry required for configuring PCIe >>>>>> + or USB3. >>>>> Why is a phandle required for this lookup, rather than doing it by >>>>> compatible? >>>> the phandle is used to select the sysconf SoC configuration register >>>> depending on the PCIe/USB3 mode (selected by xlate function), so it's not >>>> like a lookup here. >>> If "syscon_regmap_lookup_by_phandle()" is not a lookup, then I do not >>> know what is. An example justification for it would be that there are >>> multiple combophys on the same soc, each using a different sysconf >>> region. Your dts suggests that is not the case though, since you have >>> st,syscfg = <&syscfg>; in it, rather than st,syscfg = <&syscfg0>;. >> I didn't get your suggestion earlier to use "syscon_regmap_lookup_by_compatible()". >> >> We have several other syscon in the other. That's why we choose a direct syscfg phandle > In the other what? SoCs? > > Way I see it, if you're going to support different socs in the same > driver, it's almost a certainty that the offsets within a syscon that > particular features lie at are going to change between socs, so even if > you have a phandle you're going to need to have the offsets in your > match data. And if you're going to have offsets in match data, you may > as well have the compatibles for the syscon in match data too. > If the layout of the syscon hasn't changed between devices, then you > should have a fallback compatible for the syscon too, making > syscon_regmap_lookup_by_compatible() function without changes to the > driver. > > If you do have multiple syscons, but they do different things, they > should have different compatibles, so having multiple syscons doesn't > justify using a property for this either in and of itself. If you have > multiple syscons with the same layout (and therefore the same > compatible) then a phandle makes sense, but if that's the case then you > almost certainly have multiple combophys too! Otherwise, if you have one > syscon, but the controls for more than one combophy are in it, then > having a phandle _with an offset_ makes sense. > > If you know there are other SoCs with more than one combo phy, do they > use different syscons, or is the same syscon used for more than one > combophy? we have other syscon for other subsystem in the same SoC, but I not the same layout We indeed have a different compatible for the syscfg top (not the COMBOPHY registers), I can use "syscon_regmap_lookup_by_compatible(st,stm32mp25-syscfg)" effectively one justification I had in mind for using a phandle is that we can use an argument to the COMBOPHY registers offset in the syscfg. Having this DT flexibility to adjust the offset for new SoC revisions using the same driver looked like a nice to have. For the time being the lookup_by_compatible pointing the syscfg syscon is OK thanks for the clarification. Christian
diff --git a/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml new file mode 100644 index 000000000000..8d4a40b94507 --- /dev/null +++ b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml @@ -0,0 +1,128 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/phy/st,stm32mp25-combophy.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: STMicroelectronics STM32MP25 USB3/PCIe COMBOPHY + +maintainers: + - Christian Bruel <christian.bruel@foss.st.com> + +description: + Single lane PHY shared (exclusive) between the USB3 and PCIe controllers. + Supports 5Gbit/s for USB3 and PCIe gen2 or 2.5Gbit/s for PCIe gen1. + +properties: + compatible: + const: st,stm32mp25-combophy + + reg: + maxItems: 1 + + "#phy-cells": + const: 1 + + clocks: + minItems: 2 + items: + - description: apb Bus clock mandatory to access registers. + - description: ker Internal RCC reference clock for USB3 or PCIe + - description: pad Optional on board clock input for PCIe only. Typically an + external 100Mhz oscillator wired on dedicated CLKIN pad. Used as reference + clock input instead of the ker + + clock-names: + minItems: 2 + items: + - const: apb + - const: ker + - const: pad + + resets: + maxItems: 1 + + reset-names: + const: phy + + power-domains: + maxItems: 1 + + wakeup-source: true + + interrupts: + maxItems: 1 + description: interrupt used for wakeup + + access-controllers: + minItems: 1 + maxItems: 2 + + st,syscfg: + $ref: /schemas/types.yaml#/definitions/phandle + description: Phandle to the SYSCON entry required for configuring PCIe + or USB3. + + st,ssc-on: + type: boolean + description: + A boolean property whose presence indicates that the SSC for common clock + needs to be set. + + st,rx-equalizer: + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0 + maximum: 7 + default: 2 + description: + A 3 bit value to tune the RX fixed equalizer setting for optimal eye compliance + + st,output-micro-ohms: + minimum: 3999000 + maximum: 6090000 + default: 4968000 + description: + A value property to tune the Single Ended Output Impedance, simulations results + at 25C for a VDDP=0.8V. The hardware accepts discrete values in this range. + + st,output-vswing-microvolt: + minimum: 442000 + maximum: 803000 + default: 803000 + description: + A value property in microvolt to tune the Single Ended Output Voltage Swing to change the + Vlo, Vhi for a VDDP = 0.8V. The hardware accepts discrete values in this range. + +required: + - "#phy-cells" + - compatible + - clocks + - clock-names + - reg + - resets + - reset-names + - st,syscfg + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/st,stm32mp25-rcc.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/reset/st,stm32mp25-rcc.h> + + combophy: phy@480c0000 { + compatible = "st,stm32mp25-combophy"; + reg = <0x480c0000 0x1000>; + #phy-cells = <1>; + clocks = <&rcc CK_BUS_USB3PCIEPHY>, <&rcc CK_KER_USB3PCIEPHY>; + clock-names = "apb", "ker"; + resets = <&rcc USB3PCIEPHY_R>; + reset-names = "phy"; + st,syscfg = <&syscfg>; + access-controllers = <&rifsc 67>; + power-domains = <&CLUSTER_PD>; + wakeup-source; + interrupts-extended = <&exti1 45 IRQ_TYPE_EDGE_FALLING>; + }; +...
Document the bindings for STM32 COMBOPHY interface, used to support the PCIe and USB3 stm32mp25 drivers. Following entries can be used to tune caracterisation parameters - st,output-micro-ohms and st,output-vswing-microvolt bindings entries to tune the impedance and voltage swing using discrete simulation results - st,rx-equalizer register to set the internal rx equalizer filter value. Signed-off-by: Christian Bruel <christian.bruel@foss.st.com> --- .../bindings/phy/st,stm32mp25-combophy.yaml | 128 ++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml