Received-SPF: pass (google.com: domain of linux-kernel+bounces-157756-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 24 Apr 2024 16:57:42 -0500
From: Rob Herring <robh@kernel.org>
To: Richard Zhu <hongxing.zhu@nxp.com>
Cc: conor@kernel.org, vkoul@kernel.org, kishon@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, frank.li@nxp.com,
	conor+dt@kernel.org, linux-phy@lists.infradead.org,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, kernel@pengutronix.de,
	imx@lists.linux.dev
Subject: Re: [PATCH v3 1/3] dt-bindings: phy: phy-imx8-pcie: Add header file
 for i.MX8Q HSIO SerDes PHY
Message-ID: <20240424215742.GB460126-robh@kernel.org>
References: <1713939683-15328-1-git-send-email-hongxing.zhu@nxp.com>
 <1713939683-15328-2-git-send-email-hongxing.zhu@nxp.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1713939683-15328-2-git-send-email-hongxing.zhu@nxp.com>

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.

> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Reviewed-by: Frank Li <Frank.Li@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.

Rob