> -----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
