On 14/11/2022 14:27, Johan Hovold wrote: > On Fri, Nov 11, 2022 at 04:17:29PM +0100, Krzysztof Kozlowski wrote: >> On 11/11/2022 10:24, Johan Hovold wrote: >>> The current QMP USB3-DP PHY bindings are based on the original MSM8996 >>> binding which provided multiple PHYs per IP block and these in turn were >>> described by child nodes. >>> >> >> Thank you for your patch. There is something to discuss/improve. >> >> >>> + >>> +maintainers: >>> + - Vinod Koul <vkoul@xxxxxxxxxx> >> >> Maybe you want to add also yourself? > > Due to the lack of public documentation for these platforms and the > amount of work involved in effectively reverse-engineering the > corresponding details from random vendor-kernel trees, it's probably > best to leave maintainence with Vinod who at least has access to some > documentation. > >>> + >>> +description: >>> + The QMP PHY controller supports physical layer functionality for a number of >>> + controllers on Qualcomm chipsets, such as, PCIe, UFS and USB. >>> + >>> + See also: >>> + - include/dt-bindings/dt-bindings/phy/phy.h >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - qcom,sc8280xp-qmp-usb43dp-phy >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + clocks: >>> + maxItems: 4 >>> + >>> + clock-names: >>> + items: >>> + - const: aux >>> + - const: ref >>> + - const: com_aux >>> + - const: usb3_pipe >>> + >>> + power-domains: >>> + maxItems: 1 >>> + >>> + resets: >>> + maxItems: 2 >>> + >>> + reset-names: >>> + items: >>> + - const: phy >>> + - const: common >>> + >>> + vdda-phy-supply: true >>> + >>> + vdda-pll-supply: true >>> + >>> + "#clock-cells": >>> + const: 1 >>> + >>> + clock-output-names: >>> + items: >>> + - const: usb3_pipe >>> + - const: dp_link >>> + - const: dp_vco_div >> >> Why defining here fixed names? The purpose of this field is to actually >> allow customizing these - at least in most cases. If these have to be >> fixed, then driver should just instantiate these clocks with such names, >> right? > > I'm only using these names as documentation of the indexes. The driver What do you mean by documentation of indexes? You require these specific entries and do not allow anything else. > doesn't use these names, but that's a Linux-specific implementation > detail. > > I noticed that several bindings leave the clock indexes unspecified, or > have header files defining some or all of them. I first added a QMP > header but that seemed like overkill, especially if we'd end up with > one header per SoC (cf. the GCC headers) due to (known and potential) > platform differences. Headers for the names? I do not recall such but that does not seem right. > > On the other hand reproducing this list in each node is admittedly a bit > redundant. > > Shall I add back a shared header for all PHYs handled by this driver > (another implementation detail) even if this could eventually lead to > describing clocks not supported by a particular SoC (so such constraints > would still need to be described by the binding somehow): > > /* QMP clocks */ > #define QMP_USB3_PIPE_CLK 0 > #define QMP_DP_LINK_CLK 1 > #define QMP_DP_VCO_DIV_CLK 2 What are these about? To remind - we talk about names of clocks this device creates. The output names. Whatever IDs you have are not related to the names. Best regards, Krzysztof
