RE: [PATCH v1 1/3] dt-bindings: phy: Add i.MX8QM PCIe PHY binding

[Hongxing Zhu]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: 2023年8月30日 15:37
> To: Hongxing Zhu <hongxing.zhu@xxxxxxx>; vkoul@xxxxxxxxxx;
> kishon@xxxxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx;
> festevam@xxxxxxxxx; l.stach@xxxxxxxxxxxxxx; a.fatoum@xxxxxxxxxxxxxx;
> u.kleine-koenig@xxxxxxxxxxxxxx
> Cc: linux-phy@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> kernel@xxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>
> 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