RE: [PATCH v3 1/3] dt-bindings: phy: phy-imx8-pcie: Add header file for i.MX8Q HSIO SerDes PHY

[Hongxing Zhu]

> -----Original Message-----
> From: Rob Herring <robh@xxxxxxxxxx>
> Sent: 2024年4月25日 5:58
> To: Hongxing Zhu <hongxing.zhu@xxxxxxx>
> Cc: conor@xxxxxxxxxx; vkoul@xxxxxxxxxx; kishon@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; Frank Li <frank.li@xxxxxxx>;
> conor+dt@xxxxxxxxxx; linux-phy@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> kernel@xxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 1/3] dt-bindings: phy: phy-imx8-pcie: Add header file for
> i.MX8Q HSIO SerDes PHY
> 
> On Wed, Apr 24, 2024 at 02:21:21PM +0800, Richard Zhu wrote:
> > Add lane index and HSIO configuration definitions of the i.MX8Q HSIO
> > SerDes PHY into header file.
> 
> This belongs in the binding patch. It is part of the binding.
Should I merge this patch and the second into one binding patch?

> 
> > Signed-off-by: Richard Zhu <hongxing.zhu@xxxxxxx>
> > Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> >  include/dt-bindings/phy/phy-imx8-pcie.h | 62
> > +++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> >
> > diff --git a/include/dt-bindings/phy/phy-imx8-pcie.h
> > b/include/dt-bindings/phy/phy-imx8-pcie.h
> > index 8bbe2d6538d8..60447b95a952 100644
> > --- a/include/dt-bindings/phy/phy-imx8-pcie.h
> > +++ b/include/dt-bindings/phy/phy-imx8-pcie.h
> > @@ -11,4 +11,66 @@
> >  #define IMX8_PCIE_REFCLK_PAD_INPUT	1
> >  #define IMX8_PCIE_REFCLK_PAD_OUTPUT	2
> >
> > +/*
> > + * i.MX8QM HSIO subsystem has three lane PHYs and three controllers:
> > + * PCIEA(2 lanes capable PCIe controller), PCIEB (only support one
> > + * lane) and SATA.
> > + *
> > + * Meanwhile, i.MX8QXP HSIO subsystem has one lane PHY and PCIEB(only
> > + * support one lane) controller.
> > + *
> > + * In the different use cases. PCIEA can be bound to PHY lane0, lane1
> > + * or Lane0 and lane1. PCIEB can be bound to lane1 or lane2 PHY. SATA
> > + * can only be bound to last lane2 PHY.
> > + *
> > + * +-------------------------------+------------------+
> > + * | i.MX8QM                       | i.MX8QXP         |
> > + * |-------------------------------|------------------|
> > + * |       | PCIEA | PCIEB | SATA  |       | PCIEB    |
> > + * |-------------------------------|-------|----------|
> > + * | LAN 0 | X     |       |       | LAN 0 | *        |
> 
> LAN? Local Area Network? Just use 'Lane'.
> 
> Don't need this column                 ^^^^^^^
> 
> > + * |-------------------------------|-------|----------|
> > + * | LAN 1 | X     | *     |       |       |          |
> > + * |-------------------------------|-------|----------|
> > + * | LAN 2 |       | *     | *     |       |          |
> > + * +-------------------------------+------------------+
> > + * NOTE:
> > + * *: Choose one only.
> > + * X: Choose any of these.
> > + *
> > + * Define i.MX8Q HSIO PHY lane index here to specify the lane mask.
> > + */
> > +#define IMX8Q_HSIO_LANE0	0x1
> > +#define IMX8Q_HSIO_LANE1	0x2
> > +#define IMX8Q_HSIO_LANE2	0x4
> 
> Thinking about this some more, in most cases of the phy binding where individual
> lanes can be assigned, each lane is a phys entry.
> 
> PCIEA:
> phys = <&hsio_phy 0 PHY_MODE_PCIE>;
> or:
> phys = <&hsio_phy 0 PHY_MODE_PCIE>, <&hsio_phy 1 PHY_MODE_PCIE>;
> 
> PCIEB:
> phys = <&hsio_phy 1 PHY_MODE_PCIE>;
> or:
> phys = <&hsio_phy 2 PHY_MODE_PCIE>;
> 
> SATA:
> phys = <&hsio_phy 2 PHY_MODE_SATA>;
> 
> > +
> > +/*
> > + * Regarding the design of i.MX8QM HSIO subsystem, HSIO module can be
> > + * confiured as following three use cases.
> > + *
> > + * Define different configurations refer to the use cases, since it
> > +is
> > + * mandatory required in the initialization.
> > + *
> > + * On i.MX8QXP, HSIO module only has PCIEB and one lane PHY.
> > + * Define "IMX8Q_HSIO_CFG_PCIEB" for i.MX8QXP platforms.
> > + *
> > + * +----------------------------------------------------+----------+
> > + * |                               | i.MX8QM            | i.MX8QXP |
> > + * |-------------------------------|--------------------|----------|
> > + * |                               | LAN0 | LAN1 | LAN2 | LAN0     |
> 
> s/LAN/Lane/
> 
> > + * |-------------------------------|------|------|------|----------|
> > + * | IMX8Q_HSIO_CFG_PCIEAX2SATA    | PCIEA| PCIEA| SATA |          |
> > + * |-------------------------------|------|------|------|----------|
> > + * | IMX8Q_HSIO_CFG_PCIEAX2PCIEB   | PCIEA| PCIEA| PCIEB|          |
> > + * |-------------------------------|------|------|------|----------|
> > + * | IMX8Q_HSIO_CFG_PCIEAPCIEBSATA | PCIEA| PCIEB| SATA |          |
> > + * |-------------------------------|------|------|------|----------|
> > + * | IMX8Q_HSIO_CFG_PCIEB          | -    | -    | -    | PCIEB    |
> > + * +----------------------------------------------------+----------+
> > + */
> > +#define IMX8Q_HSIO_CFG_PCIEAX2SATA	0x1
> > +#define IMX8Q_HSIO_CFG_PCIEAX2PCIEB	0x2
> > +#define IMX8Q_HSIO_CFG_PCIEAPCIEBSATA
> 	(IMX8Q_HSIO_CFG_PCIEAX2SATA | IMX8Q_HSIO_CFG_PCIEAX2PCIEB)
> > +#define IMX8Q_HSIO_CFG_PCIEB		IMX8Q_HSIO_CFG_PCIEAX2PCIEB
> 
> Again, I don't see why you need all this. You now have mode and lanes, and per
> SoC data in the driver, so you should be able to figure out what you need from
> this.
Thanks for your comments.
It's my fault that I didn't describe it clear.

The HSIO configuration (fsl,hsio-cfg) is one global state too refer to the
design document.
It should be known and used to set pcie_ab_select and phy_x1_epcs_sel at the
 begin of HSIO initialization like this listed below.

+-------------------------------------------------------------+
|CRR(SYS.CSR) register|PCIAx2 and|PCIEAx1, PCIEBx1|PCIEAx2 and|
|                     |SATA      |SATA            |PCIEBx1    |
|---------------------|----------|----------------|-----------|
|PCIE_AB_SELECT       | 0        | 1              | 1         |
|---------------------|----------|----------------|-----------|
|PHY_X1_EPCS_SEL      | 1        | 1              | 0         |
+-------------------------------------------------------------+
For example, in the PCIEAx2 and SATA use case. The PHY_X1_EPCS_SEL should be
1b'1 and PCIE_AB_SELECT should be 1b'0 at the begin of the initialization of
 the PCIEA stance.
And in PCIEAx2 and PCIEBx1 use case. The PCIE_AB_SELECT should be 1b'1 and
PHYX1_EPCS_SEL should be 1b'0.

Otherwise, the PCIEA instance wouldn't be functional in the end.

Unfortunately, based on lane index and phy mode of PCIEA instance, I
 don't know how to set PCIE_AB_SELECT and PHY_X1_EPCC_SEL.

So, one property named "fsl,hsio-cfg" has to be introduced here to specify the
setting of the PCIE_AB_SELECT and PHY_X1_EPCS_SEL of HSIO.

This is the reason that these IMX8Q_HSIO_CFG_# macros are defined here.

Best Regards
Richard Zhu
> 
> Rob