Message-ID: <20240829-manifesto-tray-65443d6e7e6e@spud>
Date: Thu, 29 Aug 2024 17:44:50 +0100
From: Conor Dooley <conor@...nel.org>
To: Christian Bruel <christian.bruel@...s.st.com>
Cc: vkoul@...nel.org, kishon@...nel.org, robh@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org, mcoquelin.stm32@...il.com,
	alexandre.torgue@...s.st.com, p.zabel@...gutronix.de,
	linux-phy@...ts.infradead.org, devicetree@...r.kernel.org,
	linux-stm32@...md-mailman.stormreply.com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	fabrice.gasnier@...s.st.com
Subject: Re: [PATCH v4 1/5] dt-bindings: phy: Add STM32MP25 COMBOPHY bindings

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@...s.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@...s.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.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists