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