Re: [PATCH v4 1/5] dt-bindings: phy: Add STM32MP25 COMBOPHY bindings

[Christian Bruel <christian.bruel@foss.st.com>]

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.