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