[PATCH v3 1/3] dt-bindings: phy: phy-imx8-pcie: Add header file for i.MX8Q HSIO SerDes PHY
Hongxing Zhu
hongxing.zhu at nxp.com
Fri Apr 26 01:21:05 PDT 2024
> -----Original Message-----
> From: Hongxing Zhu
> Sent: 2024年4月25日 16:35
> To: 'Rob Herring' <robh at kernel.org>
> Cc: conor at kernel.org; vkoul at kernel.org; kishon at kernel.org;
> krzysztof.kozlowski+dt at linaro.org; Frank Li <frank.li at nxp.com>;
> conor+dt at kernel.org; linux-phy at lists.infradead.org; devicetree at vger.kernel.org;
> linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> kernel at pengutronix.de; imx at lists.linux.dev
> 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 at kernel.org>
> > Sent: 2024年4月25日 5:58
> > To: Hongxing Zhu <hongxing.zhu at nxp.com>
> > Cc: conor at kernel.org; vkoul at kernel.org; kishon at kernel.org;
> > krzysztof.kozlowski+dt at linaro.org; Frank Li <frank.li at nxp.com>;
> > conor+dt at kernel.org; linux-phy at lists.infradead.org;
> > conor+devicetree at vger.kernel.org;
> > linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> > kernel at pengutronix.de; imx at lists.linux.dev
> > 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 at nxp.com>
> > > Reviewed-by: Frank Li <Frank.Li at nxp.com>
> > > ---
> > > 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
More information about the linux-phy
mailing list