| Message ID | 1693291534-32092-2-git-send-email-hongxing.zhu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add i.MX8Q PCIe PHY driver | expand |
Hi Krzsztof: Thanks for your review. > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: 2023年8月29日 15:48 > To: Hongxing Zhu <hongxing.zhu@nxp.com>; vkoul@kernel.org; > kishon@kernel.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > conor+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de; > festevam@gmail.com; l.stach@pengutronix.de; a.fatoum@pengutronix.de; > u.kleine-koenig@pengutronix.de > Cc: linux-phy@lists.infradead.org; devicetree@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH v1 1/3] dt-bindings: phy: Add i.MX8QM PCIe PHY binding > > On 29/08/2023 08:45, Richard Zhu wrote: > > Add i.MX8QM PCIe PHY binding. > > > > i.MX8QM HSIO(High Speed IO) module has three instances of single lane > > SERDES PHYs, an instance of two lanes PCIe GEN3 controller, an > > instance of single lane PCIe GEN3 controller, as well as an instance > > of SATA 3.0 controller. > > > > The HSIO module can be configured as the following different usecases. > > 1 - A two lanes PCIea and a single lane SATA. > > 2 - A single lane PCIea, a single lane PCIeb and a single lane SATA. > > 3 - A two lanes PCIea, a single lane PCIeb. > > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > --- > > .../bindings/phy/fsl,imx8-pcie-phy.yaml | 70 ++++++++++++++++++- > > 1 file changed, 67 insertions(+), 3 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml > > b/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml > > index 182a219387b0..764790f2b10b 100644 > > --- a/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml > > +++ b/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml > > @@ -17,16 +17,18 @@ properties: > > enum: > > - fsl,imx8mm-pcie-phy > > - fsl,imx8mp-pcie-phy > > + - fsl,imx8qm-pcie-phy > > > > reg: > > maxItems: 1 > > > > clocks: > > - maxItems: 1 > > + minItems: 1 > > + maxItems: 5 > > > > clock-names: > > - items: > > - - const: ref > > + minItems: 1 > > + maxItems: 5 > > > > resets: > > minItems: 1 > > @@ -70,6 +72,36 @@ properties: > > description: PCIe PHY power domain (optional). > > maxItems: 1 > > > > + hsio-cfg: > > Missing vendor prefix because it does not look like generic property. Okay, would add the fsl,hsio- prefix later. > > > + description: | > > + Specifies the different usecases supported by the HSIO(High > > + Speed IO) > > I don't know what are the usecases... Sorry, miss one space between "use" and "cases". i.MX8QM HSIO module can be controlled by DSC/software in these three different modes. So I add this property (fsl,hsio-cfg) here to specify the work mode of HSIO. > > > + module. PCIEAX2SATA means two lanes PCIea and a single lane SATA. > > + PCIEAX1PCIEBX1SATA represents a single lane PCIea, a single lane > > + PCIeb and a single lane SATA. PCIEAX2PCIEBX1 on behalf of a two > > + lanes PCIea, a single lane PCIeb. > > + Refer include/dt-bindings/phy/phy-imx8-pcie.h for the constants to > > + be used (optional). > > None of all this helped me to understand what part of hardware this is responsible > for. It seems you just want to program a register, but instead you should use one > of existing properties like phy-modes etc. It's my bad. Didn't describe the HW clearly above. The fsl,hsio-cfg is used to specify the work mode of HSIO subsystem, not only the PHY mode. Since the PHYs are contained in the HSIO subsystem, can't be used by PCIe or SATA controller freely. The usage of these PHYs are limited by the HSIO work modes. BTW, up to now, I still don't have a good idea to describe the HSIO by phy-modes property although I prefer this way in my mind. > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + enum: [ 1, 2, 3 ] > > + > > + ctrl-csr: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: > > + phandle to the ctrl-csr region containing the HSIO control and > > + status registers for PCIe or SATA controller (optional). > > Please try some internal review before posting to patches. Community is not cheap > reviewers taking this duty from NXP. I am pretty sure NXP can afford someone > looking at the code. > > This misses vendor prefix, as explained many times for every syscon phandle. Also > optional is redundant. Sorry about the missing prefix. The prefix would be added later. And the optional would be removed. Thanks. > > But anyway status of PCIe or SATA controller is not a property of the phy, so it > looks to me you stuff here some properties belonging to some other missing > devices. I see. You're right the status of PCIe or SATA controller is not a property of the PHY. Some bits contained in the ctrl-csr region, are used to kick off resets through the internal glue logic. So, this property is added for phy driver. > > > + > > + misc-csr: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: > > + phandle to the misc-csr region containing the HSIO control and > > + status registers for misc (optional). > > Same problems. > "fsl,hsio-" prefix would be added later. > > + > > + phy-csr: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: > > + phandle to the phy-csr region containing the HSIO control and > > + status registers for phy (optional). > > Same problems. "fsl,hsio-" prefix would be added later. > > > > + > > required: > > - "#phy-cells" > > - compatible > > @@ -78,6 +110,38 @@ required: > > - clock-names > > - fsl,refclk-pad-mode > > > > +allOf: > > + - if: > > + properties: > > + compatible: > > + enum: > > + - fsl,imx8qm-pcie-phy > > + then: > > + properties: > > + clocks: > > + minItems: 4 > > + maxItems: 5 > > + clock-names: > > + oneOf: > > + - items: > > + - const: pipe_pclk > > + - const: ctrl_ips_clk > > + - const: phy_ips_clk > > + - const: misc_ips_clk > > Drop clk everywhere. Sorry, would be changed and aligned later. > > + - items: > > + - const: apb_pclk > > No, optional clock goes to the end and please explain why APB is optional. I understand that phy should have the apb_pclk whatever it is used in which mode. Would keep aligned in the clock definitions later. Thanks. Best Regards Richard Zhu > > > > Best regards, > Krzysztof
On 30/08/2023 09:31, Hongxing Zhu wrote: >> >>> + description: | >>> + Specifies the different usecases supported by the HSIO(High >>> + Speed IO) >> >> I don't know what are the usecases... > Sorry, miss one space between "use" and "cases". I did not mean language typo, but in general - what are you describing here? > i.MX8QM HSIO module can be controlled by DSC/software in these three > different modes. So I add this property (fsl,hsio-cfg) here to specify the > work mode of HSIO. So modes of work? Or different device attached to the PHY? Or what? There is no use case in hardware and you should describe hardware. >> >>> + module. PCIEAX2SATA means two lanes PCIea and a single lane SATA. >>> + PCIEAX1PCIEBX1SATA represents a single lane PCIea, a single lane >>> + PCIeb and a single lane SATA. PCIEAX2PCIEBX1 on behalf of a two >>> + lanes PCIea, a single lane PCIeb. >>> + Refer include/dt-bindings/phy/phy-imx8-pcie.h for the constants to >>> + be used (optional). >> >> None of all this helped me to understand what part of hardware this is responsible >> for. It seems you just want to program a register, but instead you should use one >> of existing properties like phy-modes etc. > It's my bad. Didn't describe the HW clearly above. > The fsl,hsio-cfg is used to specify the work mode of HSIO subsystem, not only > the PHY mode. Since the PHYs are contained in the HSIO subsystem, can't be > used by PCIe or SATA controller freely. The usage of these PHYs are limited > by the HSIO work modes. BTW, up to now, I still don't have a good idea to > describe the HSIO by phy-modes property although I prefer this way in my mind. What is HSIO and why does it appear in context of this phy? Specifically, if this is separate subsystem, why do you put HSIO property in the phy? No, that does not seem right. >> >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [ 1, 2, 3 ] >>> + >>> + ctrl-csr: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: >>> + phandle to the ctrl-csr region containing the HSIO control and >>> + status registers for PCIe or SATA controller (optional). >> >> Please try some internal review before posting to patches. Community is not cheap >> reviewers taking this duty from NXP. I am pretty sure NXP can afford someone >> looking at the code. >> >> This misses vendor prefix, as explained many times for every syscon phandle. Also >> optional is redundant. > Sorry about the missing prefix. The prefix would be added later. > And the optional would be removed. Thanks. >> >> But anyway status of PCIe or SATA controller is not a property of the phy, so it >> looks to me you stuff here some properties belonging to some other missing >> devices. > I see. You're right the status of PCIe or SATA controller is not a property > of the PHY. Some bits contained in the ctrl-csr region, are used to kick > off resets through the internal glue logic. So, this property is added > for phy driver. Sorry, I am really fed up with the syscons. See here: https://lore.kernel.org/all/20230830031846.127957-2-william.qiu@starfivetech.com/ I cannot trust you on this anymore. Describe hardware properly. If you have resets, you have reset controller. If you have clocks, then clock controller. >> >>> + >>> + misc-csr: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: >>> + phandle to the misc-csr region containing the HSIO control and >>> + status registers for misc (optional). >> >> Same problems. >> > "fsl,hsio-" prefix would be added later. If you have some HSIO block, why do you reference it via phandle and why do you put its properties (mode) here? What is the relation between HSIO and this? So many questions... from your commit description all this looks entirely wrong. You messed description of HSIO and now try to bandage it with such properties. No. NAK. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: 2023年8月30日 15:37 > To: Hongxing Zhu <hongxing.zhu@nxp.com>; vkoul@kernel.org; > kishon@kernel.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > conor+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de; > festevam@gmail.com; l.stach@pengutronix.de; a.fatoum@pengutronix.de; > u.kleine-koenig@pengutronix.de > Cc: linux-phy@lists.infradead.org; devicetree@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH v1 1/3] dt-bindings: phy: Add i.MX8QM PCIe PHY binding > > On 30/08/2023 09:31, Hongxing Zhu wrote: > >> > >>> + description: | > >>> + Specifies the different usecases supported by the HSIO(High > >>> + Speed IO) > >> > >> I don't know what are the usecases... > > Sorry, miss one space between "use" and "cases". > > I did not mean language typo, but in general - what are you describing here? > > > i.MX8QM HSIO module can be controlled by DSC/software in these three > > different modes. So I add this property (fsl,hsio-cfg) here to specify > > the work mode of HSIO. > > So modes of work? Or different device attached to the PHY? Or what? > There is no use case in hardware and you should describe hardware. > Okay, I see. More exactly, HSIO subsystem defines three use cases. Anyway, it's should be another stuffs, and shouldn't be described in PHY dt-bindings. > >> > >>> + module. PCIEAX2SATA means two lanes PCIea and a single lane SATA. > >>> + PCIEAX1PCIEBX1SATA represents a single lane PCIea, a single lane > >>> + PCIeb and a single lane SATA. PCIEAX2PCIEBX1 on behalf of a two > >>> + lanes PCIea, a single lane PCIeb. > >>> + Refer include/dt-bindings/phy/phy-imx8-pcie.h for the constants to > >>> + be used (optional). > >> > >> None of all this helped me to understand what part of hardware this > >> is responsible for. It seems you just want to program a register, but > >> instead you should use one of existing properties like phy-modes etc. > > It's my bad. Didn't describe the HW clearly above. > > The fsl,hsio-cfg is used to specify the work mode of HSIO subsystem, > > not only the PHY mode. Since the PHYs are contained in the HSIO > > subsystem, can't be used by PCIe or SATA controller freely. The usage > > of these PHYs are limited by the HSIO work modes. BTW, up to now, I > > still don't have a good idea to describe the HSIO by phy-modes property > although I prefer this way in my mind. > > What is HSIO and why does it appear in context of this phy? > Specifically, if this is separate subsystem, why do you put HSIO property in the > phy? No, that does not seem right. > Understand, the descriptions of HSIO subsystem shouldn't be contained in the PHY dt-binding document. > >> > >>> + $ref: /schemas/types.yaml#/definitions/uint32 > >>> + enum: [ 1, 2, 3 ] > >>> + > >>> + ctrl-csr: > >>> + $ref: /schemas/types.yaml#/definitions/phandle > >>> + description: > >>> + phandle to the ctrl-csr region containing the HSIO control and > >>> + status registers for PCIe or SATA controller (optional). > >> > >> Please try some internal review before posting to patches. Community > >> is not cheap reviewers taking this duty from NXP. I am pretty sure > >> NXP can afford someone looking at the code. > >> > >> This misses vendor prefix, as explained many times for every syscon > >> phandle. Also optional is redundant. > > Sorry about the missing prefix. The prefix would be added later. > > And the optional would be removed. Thanks. > >> > >> But anyway status of PCIe or SATA controller is not a property of the > >> phy, so it looks to me you stuff here some properties belonging to > >> some other missing devices. > > I see. You're right the status of PCIe or SATA controller is not a > > property of the PHY. Some bits contained in the ctrl-csr region, are > > used to kick off resets through the internal glue logic. So, this > > property is added for phy driver. > > Sorry, I am really fed up with the syscons. See here: > https://lore.kern/ > el.org%2Fall%2F20230830031846.127957-2-william.qiu%40starfivetech.com% > 2F&data=05%7C01%7Chongxing.zhu%40nxp.com%7C4c2a5cffc6d248121c860 > 8dba92bf3b5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63828 > 9778466923849%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ > QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata > =2mYhvXFxcQstDD9yC969S1CRvzz19eN2lZykLhxz9A0%3D&reserved=0 > > I cannot trust you on this anymore. Describe hardware properly. If you have > resets, you have reset controller. If you have clocks, then clock controller. > > >> > >>> + > >>> + misc-csr: > >>> + $ref: /schemas/types.yaml#/definitions/phandle > >>> + description: > >>> + phandle to the misc-csr region containing the HSIO control and > >>> + status registers for misc (optional). > >> > >> Same problems. > >> > > "fsl,hsio-" prefix would be added later. > > > If you have some HSIO block, why do you reference it via phandle and why do > you put its properties (mode) here? What is the relation between HSIO and this? > So many questions... from your commit description all this looks entirely wrong. > You messed description of HSIO and now try to bandage it with such properties. > No. Thanks for your comments. Now, I have much more clearer view. PHY dt-binding should only have the HW descriptions of the PHY, the HSIO subsystem shouldn't be mentioned and be messed with PHY dt-binding here. I would remove all the HSIO description in next step, thanks for your review again. Best Regards Richard Zhu > > NAK. > > > > Best regards, > Krzysztof
diff --git a/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml index 182a219387b0..764790f2b10b 100644 --- a/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml +++ b/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml @@ -17,16 +17,18 @@ properties: enum: - fsl,imx8mm-pcie-phy - fsl,imx8mp-pcie-phy + - fsl,imx8qm-pcie-phy reg: maxItems: 1 clocks: - maxItems: 1 + minItems: 1 + maxItems: 5 clock-names: - items: - - const: ref + minItems: 1 + maxItems: 5 resets: minItems: 1 @@ -70,6 +72,36 @@ properties: description: PCIe PHY power domain (optional). maxItems: 1 + hsio-cfg: + description: | + Specifies the different usecases supported by the HSIO(High Speed IO) + module. PCIEAX2SATA means two lanes PCIea and a single lane SATA. + PCIEAX1PCIEBX1SATA represents a single lane PCIea, a single lane + PCIeb and a single lane SATA. PCIEAX2PCIEBX1 on behalf of a two + lanes PCIea, a single lane PCIeb. + Refer include/dt-bindings/phy/phy-imx8-pcie.h for the constants to + be used (optional). + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [ 1, 2, 3 ] + + ctrl-csr: + $ref: /schemas/types.yaml#/definitions/phandle + description: + phandle to the ctrl-csr region containing the HSIO control and + status registers for PCIe or SATA controller (optional). + + misc-csr: + $ref: /schemas/types.yaml#/definitions/phandle + description: + phandle to the misc-csr region containing the HSIO control and + status registers for misc (optional). + + phy-csr: + $ref: /schemas/types.yaml#/definitions/phandle + description: + phandle to the phy-csr region containing the HSIO control and + status registers for phy (optional). + required: - "#phy-cells" - compatible @@ -78,6 +110,38 @@ required: - clock-names - fsl,refclk-pad-mode +allOf: + - if: + properties: + compatible: + enum: + - fsl,imx8qm-pcie-phy + then: + properties: + clocks: + minItems: 4 + maxItems: 5 + clock-names: + oneOf: + - items: + - const: pipe_pclk + - const: ctrl_ips_clk + - const: phy_ips_clk + - const: misc_ips_clk + - items: + - const: apb_pclk + - const: pipe_pclk + - const: ctrl_ips_clk + - const: phy_ips_clk + - const: misc_ips_clk + else: + properties: + clocks: + maxItems: 1 + clock-names: + items: + - const: ref + additionalProperties: false examples:
Add i.MX8QM PCIe PHY binding. i.MX8QM HSIO(High Speed IO) module has three instances of single lane SERDES PHYs, an instance of two lanes PCIe GEN3 controller, an instance of single lane PCIe GEN3 controller, as well as an instance of SATA 3.0 controller. The HSIO module can be configured as the following different usecases. 1 - A two lanes PCIea and a single lane SATA. 2 - A single lane PCIea, a single lane PCIeb and a single lane SATA. 3 - A two lanes PCIea, a single lane PCIeb. Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> --- .../bindings/phy/fsl,imx8-pcie-phy.yaml | 70 ++++++++++++++++++- 1 file changed, 67 insertions(+), 3 deletions(-)