Re: [PATCH 02/14] dt-bindings: phy: qcom,qmp-usb3-dp: fix sc8280xp bindings

Krzysztof Kozlowski
2022-11-14T14:08:03Z

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@kernel.org>
>>
>> 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