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: Hongxing Zhu
> Sent: 2024年4月25日 16:35
> To: 'Rob Herring' <robh@xxxxxxxxxx>
> 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
> 
> > -----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;
> > conor+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.
> 
In another words, phy driver can't get a global view of the HSIO use case
 and doesn't know how to set PCIE_AB_SELECT and PHYX1_EPCS_SEL when first
 instance is probed, during the initialization of multi HSIO instances.

To set the global states properly, the "fsl,hsio-cfg" is new added here.

> 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