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

  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.

The QMP USB3-DP PHY block provides a single multi-protocol PHY and even
if some resources are only used by either the USB or DP part of the
device there is no real benefit in describing these resources in child
nodes.

The original MSM8996 binding also ended up describing the individual
register blocks as belonging to either the wrapper node or the PHY child
nodes.

This is an unnecessary level of detail which has lead to problems when
later IP blocks using different register layouts have been forced to fit
the original mould rather than updating the binding. The bindings are
arguable also incomplete as they only the describe register blocks used
by the current Linux drivers (e.g. does not include the PCS LANE
registers).

This is specifically true for later USB4-USB3-DP QMP PHYs where the TX
registers are used by both the USB3 and DP parts of the PHY (and where
the USB4 part of the PHY was not covered by the binding at all). Notably
there are also no DP "RX" (sic) registers as described by the current
bindings and the DP "PCS" region is really a set of DP_PHY registers.

Add a new binding for the USB4-USB3-DP QMP PHYs found on SC8280XP which
further bindings can be based on.

Note that the binding uses a PHY type index to access either the USB3 or
DP part of the PHY and that this can later be used also for the USB4
part if needed.

Similarly, the clock inputs and outputs can later be extended to support
USB4.

Also note that the current binding is simply removed instead of being
deprecated as it was only recently merged and would not allow for
supporting DP mode.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 .../phy/qcom,sc7180-qmp-usb3-dp-phy.yaml      |  12 --
 .../phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml    | 111 ++++++++++++++++++
 2 files changed, 111 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml
  

On Fri, Nov 11, 2022 at 10:24:45AM +0100, Johan Hovold wrote:

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml
> @@ -0,0 +1,111 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm QMP USB4-USB3-DP PHY controller (SC8280XP)
> +
> +maintainers:
> +  - Vinod Koul <vkoul@kernel.org>
> +
> +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

This trips up the binding checker which I failed to rerun after adding
this reference.

Apparently I missed adding a literal block marker '|' to the
description. Will add in v2.

Johan
  

On Fri, 11 Nov 2022 10:24:45 +0100, 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.
> 
> The QMP USB3-DP PHY block provides a single multi-protocol PHY and even
> if some resources are only used by either the USB or DP part of the
> device there is no real benefit in describing these resources in child
> nodes.
> 
> The original MSM8996 binding also ended up describing the individual
> register blocks as belonging to either the wrapper node or the PHY child
> nodes.
> 
> This is an unnecessary level of detail which has lead to problems when
> later IP blocks using different register layouts have been forced to fit
> the original mould rather than updating the binding. The bindings are
> arguable also incomplete as they only the describe register blocks used
> by the current Linux drivers (e.g. does not include the PCS LANE
> registers).
> 
> This is specifically true for later USB4-USB3-DP QMP PHYs where the TX
> registers are used by both the USB3 and DP parts of the PHY (and where
> the USB4 part of the PHY was not covered by the binding at all). Notably
> there are also no DP "RX" (sic) registers as described by the current
> bindings and the DP "PCS" region is really a set of DP_PHY registers.
> 
> Add a new binding for the USB4-USB3-DP QMP PHYs found on SC8280XP which
> further bindings can be based on.
> 
> Note that the binding uses a PHY type index to access either the USB3 or
> DP part of the PHY and that this can later be used also for the USB4
> part if needed.
> 
> Similarly, the clock inputs and outputs can later be extended to support
> USB4.
> 
> Also note that the current binding is simply removed instead of being
> deprecated as it was only recently merged and would not allow for
> supporting DP mode.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  .../phy/qcom,sc7180-qmp-usb3-dp-phy.yaml      |  12 --
>  .../phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml    | 111 ++++++++++++++++++
>  2 files changed, 111 insertions(+), 12 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml:16:11: [error] syntax error: mapping values are not allowed here (syntax)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.example.dts'
Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml:16:11: mapping values are not allowed here
make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml:16:11: mapping values are not allowed here
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml: ignoring, error parsing file
make: *** [Makefile:1492: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
  

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?

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

Best regards,
Krzysztof
  

On 11/11/2022 12: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.
> 
> The QMP USB3-DP PHY block provides a single multi-protocol PHY and even
> if some resources are only used by either the USB or DP part of the
> device there is no real benefit in describing these resources in child
> nodes.
> 
> The original MSM8996 binding also ended up describing the individual
> register blocks as belonging to either the wrapper node or the PHY child
> nodes.
> 
> This is an unnecessary level of detail which has lead to problems when
> later IP blocks using different register layouts have been forced to fit
> the original mould rather than updating the binding. The bindings are
> arguable also incomplete as they only the describe register blocks used
> by the current Linux drivers (e.g. does not include the PCS LANE
> registers).
> 
> This is specifically true for later USB4-USB3-DP QMP PHYs where the TX
> registers are used by both the USB3 and DP parts of the PHY (and where
> the USB4 part of the PHY was not covered by the binding at all). Notably
> there are also no DP "RX" (sic) registers as described by the current
> bindings and the DP "PCS" region is really a set of DP_PHY registers.
> 
> Add a new binding for the USB4-USB3-DP QMP PHYs found on SC8280XP which
> further bindings can be based on.
> 
> Note that the binding uses a PHY type index to access either the USB3 or
> DP part of the PHY and that this can later be used also for the USB4
> part if needed.
> 
> Similarly, the clock inputs and outputs can later be extended to support
> USB4.
> 
> Also note that the current binding is simply removed instead of being
> deprecated as it was only recently merged and would not allow for
> supporting DP mode.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   .../phy/qcom,sc7180-qmp-usb3-dp-phy.yaml      |  12 --
>   .../phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml    | 111 ++++++++++++++++++
>   2 files changed, 111 insertions(+), 12 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc7180-qmp-usb3-dp-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc7180-qmp-usb3-dp-phy.yaml
> index 50b1fce530d5..2f4a419197a8 100644
> --- a/Documentation/devicetree/bindings/phy/qcom,sc7180-qmp-usb3-dp-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/qcom,sc7180-qmp-usb3-dp-phy.yaml
> @@ -23,7 +23,6 @@ properties:
>         - qcom,sc7180-qmp-usb3-dp-phy
>         - qcom,sc7280-qmp-usb3-dp-phy
>         - qcom,sc8180x-qmp-usb3-dp-phy
> -      - qcom,sc8280xp-qmp-usb43dp-phy
>         - qcom,sdm845-qmp-usb3-dp-phy
>         - qcom,sm8250-qmp-usb3-dp-phy
>     reg:
> @@ -169,17 +168,6 @@ required:
>   
>   additionalProperties: false
>   
> -allOf:
> -  - if:
> -      properties:
> -        compatible:
> -          contains:
> -            enum:
> -              - qcom,sc8280xp-qmp-usb43dp-phy
> -    then:
> -      required:
> -        - power-domains
> -
>   examples:
>     - |
>       #include <dt-bindings/clock/qcom,gcc-sdm845.h>
> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml
> new file mode 100644
> index 000000000000..bd04150acee4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml
> @@ -0,0 +1,111 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm QMP USB4-USB3-DP PHY controller (SC8280XP)
> +
> +maintainers:
> +  - Vinod Koul <vkoul@kernel.org>
> +
> +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
> +
> +  "#phy-cells":
> +    const: 1
> +    description: |
> +      PHY index
> +        - PHY_TYPE_USB3
> +        - PHY_TYPE_DP

I'm stepping on Rob's and Krzysztof's ground here, but it might be more 
logical and future proof to use indices instead of phy types.

Just for my understanding, would USB4 support add another qserdes+tx/rx 
construct or would it be the same USB3 register space?

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - power-domains
> +  - resets
> +  - reset-names
> +  - vdda-phy-supply
> +  - vdda-pll-supply
> +  - "#clock-cells"
> +  - clock-output-names
> +  - "#phy-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,gcc-sc8280xp.h>
> +
> +    phy@88eb000 {
> +      compatible = "qcom,sc8280xp-qmp-usb43dp-phy";
> +      reg = <0x088eb000 0x4000>;
> +
> +      clocks = <&gcc GCC_USB3_PRIM_PHY_AUX_CLK>,
> +               <&gcc GCC_USB4_EUD_CLKREF_CLK>,
> +               <&gcc GCC_USB3_PRIM_PHY_COM_AUX_CLK>,
> +               <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>;
> +      clock-names = "aux", "ref", "com_aux", "usb3_pipe";
> +
> +      power-domains = <&gcc USB30_PRIM_GDSC>;
> +
> +      resets = <&gcc GCC_USB3_PHY_PRIM_BCR>,
> +               <&gcc GCC_USB4_DP_PHY_PRIM_BCR>;
> +      reset-names = "phy", "common";
> +
> +      vdda-phy-supply = <&vreg_l9d>;
> +      vdda-pll-supply = <&vreg_l4d>;
> +
> +      #clock-cells = <1>;
> +      clock-output-names = "usb3_pipe", "dp_link", "dp_vco_div";
> +
> +      #phy-cells = <1>;
> +    };
  

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

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

Note that for SC8280XP this list may later get extended with clocks for
USB4 (once we understand how to use them), but those would not apply to
the older USB3-DP PHYs, so we'd still need to describe that in the
binding somehow.

Johan
  

On Sat, Nov 12, 2022 at 02:43:03PM +0300, Dmitry Baryshkov wrote:
> On 11/11/2022 12: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.
> > 
> > The QMP USB3-DP PHY block provides a single multi-protocol PHY and even
> > if some resources are only used by either the USB or DP part of the
> > device there is no real benefit in describing these resources in child
> > nodes.
> > 
> > The original MSM8996 binding also ended up describing the individual
> > register blocks as belonging to either the wrapper node or the PHY child
> > nodes.
> > 
> > This is an unnecessary level of detail which has lead to problems when
> > later IP blocks using different register layouts have been forced to fit
> > the original mould rather than updating the binding. The bindings are
> > arguable also incomplete as they only the describe register blocks used
> > by the current Linux drivers (e.g. does not include the PCS LANE
> > registers).
> > 
> > This is specifically true for later USB4-USB3-DP QMP PHYs where the TX
> > registers are used by both the USB3 and DP parts of the PHY (and where
> > the USB4 part of the PHY was not covered by the binding at all). Notably
> > there are also no DP "RX" (sic) registers as described by the current
> > bindings and the DP "PCS" region is really a set of DP_PHY registers.
> > 
> > Add a new binding for the USB4-USB3-DP QMP PHYs found on SC8280XP which
> > further bindings can be based on.
> > 
> > Note that the binding uses a PHY type index to access either the USB3 or
> > DP part of the PHY and that this can later be used also for the USB4
> > part if needed.
> > 
> > Similarly, the clock inputs and outputs can later be extended to support
> > USB4.
> > 
> > Also note that the current binding is simply removed instead of being
> > deprecated as it was only recently merged and would not allow for
> > supporting DP mode.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---

> > +  "#clock-cells":
> > +    const: 1
> > +
> > +  clock-output-names:
> > +    items:
> > +      - const: usb3_pipe
> > +      - const: dp_link
> > +      - const: dp_vco_div
> > +
> > +  "#phy-cells":
> > +    const: 1
> > +    description: |
> > +      PHY index
> > +        - PHY_TYPE_USB3
> > +        - PHY_TYPE_DP
> 
> I'm stepping on Rob's and Krzysztof's ground here, but it might be more 
> logical and future proof to use indices instead of phy types.

Why would that be more future-proof?

I initially added defines for these indexes to a QMP header, but noticed
that we already have PHY drivers that use the PHY types for this. So
there's already a precedent for this and I didn't see any real benefit
to adding multiple per-SoC defines for the same thing.

> Just for my understanding, would USB4 support add another qserdes+tx/rx 
> construct or would it be the same USB3 register space?

The TX/RX registers are shared by the all three parts of the PHY (USB4,
USB3, DP), while USB4 has two dedicated sets of PLL (serdes) and PCS
registers.

Johan
  

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
  

On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote:
> 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.

> >>> +  "#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.

I'm using this property as documentation of the valid indexes that can
be used when referring to clocks provided by this device.

There are currently three and the mapping is described by the
'clock-output-names' property.
 
> > 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.

Headers for the indexes.

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

As I mentioned above, this is not about the names that Linux gives to
its representation of these clocks. Its just about defining the valid
indexes in the binding.

If you think that that using 'clock-output-names' for this is a bit too
unconventional, I can add back the header with defines like the above
instead.

Note that the clock schema has:

  clock-output-names:
    description: |
      Recommended to be a list of strings of clock output signal
      names indexed by the first cell in the clock specifier.
      However, the meaning of clock-output-names is domain
      specific to the clock provider, ...

Johan
  

On 14/11/2022 17:18, Johan Hovold wrote:
> On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote:
>> 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.
> 
>>>>> +  "#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.
> 
> I'm using this property as documentation of the valid indexes that can
> be used when referring to clocks provided by this device.
> 
> There are currently three and the mapping is described by the
> 'clock-output-names' property.
>   
>>> 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.
> 
> Headers for the indexes.
> 
>>>
>>> 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

Maybe QMP_COMBO_USB3_PIPE_CLK, QMP_COMBO_DP_LINK_CLK, 
QMP_COMBO_DP_VCO_DIV_CLK?

I'll then extend this header with QMP_UFS_RX_SYMBOL_0_CLK 
QMP_UFS_RX_SYMBOL_1_CLK and QMP_UFS_TX_SYMBOL_0_CLK.

>>
>> 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.
> 
> As I mentioned above, this is not about the names that Linux gives to
> its representation of these clocks. Its just about defining the valid
> indexes in the binding.
> 
> If you think that that using 'clock-output-names' for this is a bit too
> unconventional, I can add back the header with defines like the above
> instead.
> 
> Note that the clock schema has:
> 
>    clock-output-names:
>      description: |
>        Recommended to be a list of strings of clock output signal
>        names indexed by the first cell in the clock specifier.
>        However, the meaning of clock-output-names is domain
>        specific to the clock provider, ...
> 
> Johan
  

On 14/11/2022 16:37, Johan Hovold wrote:
> On Sat, Nov 12, 2022 at 02:43:03PM +0300, Dmitry Baryshkov wrote:
>> On 11/11/2022 12: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.
>>>
>>> The QMP USB3-DP PHY block provides a single multi-protocol PHY and even
>>> if some resources are only used by either the USB or DP part of the
>>> device there is no real benefit in describing these resources in child
>>> nodes.
>>>
>>> The original MSM8996 binding also ended up describing the individual
>>> register blocks as belonging to either the wrapper node or the PHY child
>>> nodes.
>>>
>>> This is an unnecessary level of detail which has lead to problems when
>>> later IP blocks using different register layouts have been forced to fit
>>> the original mould rather than updating the binding. The bindings are
>>> arguable also incomplete as they only the describe register blocks used
>>> by the current Linux drivers (e.g. does not include the PCS LANE
>>> registers).
>>>
>>> This is specifically true for later USB4-USB3-DP QMP PHYs where the TX
>>> registers are used by both the USB3 and DP parts of the PHY (and where
>>> the USB4 part of the PHY was not covered by the binding at all). Notably
>>> there are also no DP "RX" (sic) registers as described by the current
>>> bindings and the DP "PCS" region is really a set of DP_PHY registers.
>>>
>>> Add a new binding for the USB4-USB3-DP QMP PHYs found on SC8280XP which
>>> further bindings can be based on.
>>>
>>> Note that the binding uses a PHY type index to access either the USB3 or
>>> DP part of the PHY and that this can later be used also for the USB4
>>> part if needed.
>>>
>>> Similarly, the clock inputs and outputs can later be extended to support
>>> USB4.
>>>
>>> Also note that the current binding is simply removed instead of being
>>> deprecated as it was only recently merged and would not allow for
>>> supporting DP mode.
>>>
>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>>> ---
> 
>>> +  "#clock-cells":
>>> +    const: 1
>>> +
>>> +  clock-output-names:
>>> +    items:
>>> +      - const: usb3_pipe
>>> +      - const: dp_link
>>> +      - const: dp_vco_div
>>> +
>>> +  "#phy-cells":
>>> +    const: 1
>>> +    description: |
>>> +      PHY index
>>> +        - PHY_TYPE_USB3
>>> +        - PHY_TYPE_DP
>>
>> I'm stepping on Rob's and Krzysztof's ground here, but it might be more
>> logical and future proof to use indices instead of phy types.
> 
> Why would that be more future-proof?
> 
> I initially added defines for these indexes to a QMP header, but noticed
> that we already have PHY drivers that use the PHY types for this. So
> there's already a precedent for this and I didn't see any real benefit
> to adding multiple per-SoC defines for the same thing.

As you guessed from my question, I was thinking about USB4 (for which we 
do not have a separate PHY_TYPE, but that probably shouldn't matter). 
Would it be a separate PHY here, or would it be a combo UBS3+USB4 plus 
separate DP phy?

Yes, we have other PHYs, which use types as an index, however it's 
slightly more common to have indices instead.

Anyway, this is a minor issue. It might be just that I'm more common to 
using indices everywhere (in other words, I have preference here, but 
it's not a strong requirement from my side).


>> Just for my understanding, would USB4 support add another qserdes+tx/rx
>> construct or would it be the same USB3 register space?
> 
> The TX/RX registers are shared by the all three parts of the PHY (USB4,
> USB3, DP), while USB4 has two dedicated sets of PLL (serdes) and PCS
> registers.

Ack, thanks.
  

On Mon, Nov 14, 2022 at 06:19:25PM +0300, Dmitry Baryshkov wrote:
> On 14/11/2022 17:18, Johan Hovold wrote:
> > On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote:
> >> 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:

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

> >>> 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
> 
> Maybe QMP_COMBO_USB3_PIPE_CLK, QMP_COMBO_DP_LINK_CLK, 
> QMP_COMBO_DP_VCO_DIV_CLK?

"COMBO" is just the name of the Linux driver and does not belong in the
binding.
 
> I'll then extend this header with QMP_UFS_RX_SYMBOL_0_CLK 
> QMP_UFS_RX_SYMBOL_1_CLK and QMP_UFS_TX_SYMBOL_0_CLK.

Yeah, I had those in mind when creating the header and using a generic
QMP prefix (even if I didn't end up using the header in v1).

This could just be mapping of (arbitrary) QMP indexes to clocks and we
use it for USB3, DP, UFS and later also USB4.

This will however mean that the indexes are not necessarily zero-based
and consecutive for a specific SoC and PHY. But that's perhaps a
non-issue (cf. the PHY_TYPE defines).

We'd still need to describe which clocks are available on a particular
SoC and PHY, and that's partly why I used 'clock-output-names' to fix
the mapping in the binding. Guess we can just list the valid defines in
the property description as I did for #phy-cells.

Johan
  

On 14/11/2022 15:18, Johan Hovold wrote:
> On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote:
>> 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.
> 
>>>>> +  "#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.
> 
> I'm using this property as documentation of the valid indexes that can
> be used when referring to clocks provided by this device.
> 
> There are currently three and the mapping is described by the
> 'clock-output-names' property.

That's not the purpose of this property. Drop it then. The names do not
define the ABI and do not document it, actually. You require now that
every DTB, if providing clock-output-names, will have exactly such names
instead of having fixed IDs. DTBs are not for defining the ABI.

>  
>>> 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.
> 
> Headers for the indexes.
> 
>>>
>>> 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.
> 
> As I mentioned above, this is not about the names that Linux gives to
> its representation of these clocks. Its just about defining the valid
> indexes in the binding.

With or without the header, the IDs are part of ABI and are fixed. The
headers are I think always encouraged because it makes above sentence
explicit. Without the headers developers might want to change the IDs.

> 
> If you think that that using 'clock-output-names' for this is a bit too
> unconventional, I can add back the header with defines like the above
> instead.
> 
> Note that the clock schema has:
> 
>   clock-output-names:
>     description: |
>       Recommended to be a list of strings of clock output signal
>       names indexed by the first cell in the clock specifier.

Exactly. Not to describe the ABI behind the ID.

>       However, the meaning of clock-output-names is domain
>       specific to the clock provider, ...

... because you might have more cells. Just because clock-output-names
do not fit some drivers it does not mean you can use it any way you
wish. It is still for names of provided clocks.

Best regards,
Krzysztof
  

On 14/11/2022 18:38, Johan Hovold wrote:
> On Mon, Nov 14, 2022 at 06:19:25PM +0300, Dmitry Baryshkov wrote:
>> On 14/11/2022 17:18, Johan Hovold wrote:
>>> On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote:
>>>> 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:
> 
>>>>> 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.
> 
>>>>> 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
>>
>> Maybe QMP_COMBO_USB3_PIPE_CLK, QMP_COMBO_DP_LINK_CLK,
>> QMP_COMBO_DP_VCO_DIV_CLK?
> 
> "COMBO" is just the name of the Linux driver and does not belong in the
> binding.

We do not have any standard (iow, coming from the docs) name, so we can 
invent it on our own.

>   
>> I'll then extend this header with QMP_UFS_RX_SYMBOL_0_CLK
>> QMP_UFS_RX_SYMBOL_1_CLK and QMP_UFS_TX_SYMBOL_0_CLK.
> 
> Yeah, I had those in mind when creating the header and using a generic
> QMP prefix (even if I didn't end up using the header in v1).
> 
> This could just be mapping of (arbitrary) QMP indexes to clocks and we
> use it for USB3, DP, UFS and later also USB4.
> 
> This will however mean that the indexes are not necessarily zero-based
> and consecutive for a specific SoC and PHY. But that's perhaps a
> non-issue (cf. the PHY_TYPE defines).

Ugh. Please, no. We have symbol clocks for UFS PHY, USB+DP clocks for 
USB+DP PHY, but let's not go for the unified clocks index definition.

> 
> We'd still need to describe which clocks are available on a particular
> SoC and PHY, and that's partly why I used 'clock-output-names' to fix
> the mapping in the binding. Guess we can just list the valid defines in
> the property description as I did for #phy-cells.
> 
> Johan
  

On Mon, Nov 14, 2022 at 06:31:02PM +0300, Dmitry Baryshkov wrote:
> On 14/11/2022 16:37, Johan Hovold wrote:
> > On Sat, Nov 12, 2022 at 02:43:03PM +0300, Dmitry Baryshkov wrote:
> >> On 11/11/2022 12:24, Johan Hovold wrote:

> >>> +  "#clock-cells":
> >>> +    const: 1
> >>> +
> >>> +  clock-output-names:
> >>> +    items:
> >>> +      - const: usb3_pipe
> >>> +      - const: dp_link
> >>> +      - const: dp_vco_div
> >>> +
> >>> +  "#phy-cells":
> >>> +    const: 1
> >>> +    description: |
> >>> +      PHY index
> >>> +        - PHY_TYPE_USB3
> >>> +        - PHY_TYPE_DP
> >>
> >> I'm stepping on Rob's and Krzysztof's ground here, but it might be more
> >> logical and future proof to use indices instead of phy types.
> > 
> > Why would that be more future-proof?
> > 
> > I initially added defines for these indexes to a QMP header, but noticed
> > that we already have PHY drivers that use the PHY types for this. So
> > there's already a precedent for this and I didn't see any real benefit
> > to adding multiple per-SoC defines for the same thing.
> 
> As you guessed from my question, I was thinking about USB4 (for which we 
> do not have a separate PHY_TYPE, but that probably shouldn't matter). 

Yeah, that's easy enough to add.

> Would it be a separate PHY here, or would it be a combo UBS3+USB4 plus 
> separate DP phy?

We don't know yet.

> Yes, we have other PHYs, which use types as an index, however it's 
> slightly more common to have indices instead.

If you look at (yaml) bindings using a single phy-cell, the majority
simply ignores describing what the index is used for and which values
are valid. Of the few that do describe it, the cell index is either used
for something which does not allow itself for mapping to PHY_TYPE or
PHY_TYPE is used.

> Anyway, this is a minor issue. It might be just that I'm more common to 
> using indices everywhere (in other words, I have preference here, but 
> it's not a strong requirement from my side).

I don't have a strong preference here either. Let's see what Krzysztof
and Rob says.

Johan
  

On Mon, Nov 14, 2022 at 04:49:37PM +0100, Krzysztof Kozlowski wrote:
> On 14/11/2022 15:18, Johan Hovold wrote:
> > On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote:
> >> 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.
> > 
> >>>>> +  "#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.
> > 
> > I'm using this property as documentation of the valid indexes that can
> > be used when referring to clocks provided by this device.
> > 
> > There are currently three and the mapping is described by the
> > 'clock-output-names' property.
> 
> That's not the purpose of this property. Drop it then. The names do not
> define the ABI and do not document it, actually. You require now that
> every DTB, if providing clock-output-names, will have exactly such names
> instead of having fixed IDs. DTBs are not for defining the ABI.

Fair enough, I'll drop it. But there doesn't seem to be a good way to
describe the indexes currently and most bindings simply ignore to do so.

So what is the preference then? Just leave things undocumented, listing
indexes in a free-text 'description', or adding a free-text reference to
a binding header file and using those define names in a free-text
'description'?

And if going with the last option, does this mean that every SoC and PHY
type needs its own header for those three clocks or so to avoid having
a common dumping ground header file where indexes will not necessarily
be 0-based and consecutive.

Johan
  

On 14/11/2022 17:32, Johan Hovold wrote:
> On Mon, Nov 14, 2022 at 04:49:37PM +0100, Krzysztof Kozlowski wrote:
>> On 14/11/2022 15:18, Johan Hovold wrote:
>>> On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote:
>>>> 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.
>>>
>>>>>>> +  "#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.
>>>
>>> I'm using this property as documentation of the valid indexes that can
>>> be used when referring to clocks provided by this device.
>>>
>>> There are currently three and the mapping is described by the
>>> 'clock-output-names' property.
>>
>> That's not the purpose of this property. Drop it then. The names do not
>> define the ABI and do not document it, actually. You require now that
>> every DTB, if providing clock-output-names, will have exactly such names
>> instead of having fixed IDs. DTBs are not for defining the ABI.
> 
> Fair enough, I'll drop it. But there doesn't seem to be a good way to
> describe the indexes currently and most bindings simply ignore to do so.
> 
> So what is the preference then? Just leave things undocumented, listing
> indexes in a free-text 'description', or adding a free-text reference to
> a binding header file and using those define names in a free-text
> 'description'?

Either 2 or 3. Several bindings for small number of constants choose
option 2.

> 
> And if going with the last option, does this mean that every SoC and PHY
> type needs its own header for those three clocks or so to avoid having
> a common dumping ground header file where indexes will not necessarily
> be 0-based and consecutive.

phy-qcom-qmp-combo.c has one qcom_qmp_dp_clks_hw_get(), so why would you
have many of header files?

Best regards,
Krzysztof
  

On Mon, Nov 14, 2022 at 07:14:48PM +0300, Dmitry Baryshkov wrote:
> On 14/11/2022 18:38, Johan Hovold wrote:
> > On Mon, Nov 14, 2022 at 06:19:25PM +0300, Dmitry Baryshkov wrote:
> >> On 14/11/2022 17:18, Johan Hovold wrote:
> >>> On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote:
> >>>> 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:
> > 
> >>>>> 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.
> > 
> >>>>> 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
> >>
> >> Maybe QMP_COMBO_USB3_PIPE_CLK, QMP_COMBO_DP_LINK_CLK,
> >> QMP_COMBO_DP_VCO_DIV_CLK?
> > 
> > "COMBO" is just the name of the Linux driver and does not belong in the
> > binding.
> 
> We do not have any standard (iow, coming from the docs) name, so we can 
> invent it on our own.

I still think the naming should reflect the hardware and not the Linux
implementation if this is going into the binding.

And the USB4_USB3_DP defines are going to be a superset of USB3_DP (as
far we know know).

> >> I'll then extend this header with QMP_UFS_RX_SYMBOL_0_CLK
> >> QMP_UFS_RX_SYMBOL_1_CLK and QMP_UFS_TX_SYMBOL_0_CLK.
> > 
> > Yeah, I had those in mind when creating the header and using a generic
> > QMP prefix (even if I didn't end up using the header in v1).
> > 
> > This could just be mapping of (arbitrary) QMP indexes to clocks and we
> > use it for USB3, DP, UFS and later also USB4.
> > 
> > This will however mean that the indexes are not necessarily zero-based
> > and consecutive for a specific SoC and PHY. But that's perhaps a
> > non-issue (cf. the PHY_TYPE defines).
> 
> Ugh. Please, no. We have symbol clocks for UFS PHY, USB+DP clocks for 
> USB+DP PHY, but let's not go for the unified clocks index definition.

Yeah, this is the kind of issues I wanted to avoid by not using a per
SoC header for three clocks which will almost always use the same
indexes.

Because how can you be sure that your unified per-PHY type defines will
never have to be amended? Or some index left out?

The only way then is to have per-SoC defines which is a pain to
maintain (just consider that driver mapping table when some odd SoC
shows up).

Johan
  

On Mon, Nov 14, 2022 at 05:39:26PM +0100, Krzysztof Kozlowski wrote:
> On 14/11/2022 17:32, Johan Hovold wrote:

> > Fair enough, I'll drop it. But there doesn't seem to be a good way to
> > describe the indexes currently and most bindings simply ignore to do so.
> > 
> > So what is the preference then? Just leave things undocumented, listing
> > indexes in a free-text 'description', or adding a free-text reference to
> > a binding header file and using those define names in a free-text
> > 'description'?
> 
> Either 2 or 3. Several bindings for small number of constants choose
> option 2.

Ok, we have three now, but USB4 will bump this to ten or so.
 
> > And if going with the last option, does this mean that every SoC and PHY
> > type needs its own header for those three clocks or so to avoid having
> > a common dumping ground header file where indexes will not necessarily
> > be 0-based and consecutive.
> 
> phy-qcom-qmp-combo.c has one qcom_qmp_dp_clks_hw_get(), so why would you
> have many of header files?

We don't know what kind of clock outputs later revisions of these PHYs
will have. The only way to guarantee 0-based consecutive indexes appears
to be to use per-SoC defines (e.g. as for the GCC bindings).

Johan
  

On 14/11/2022 19:42, Johan Hovold wrote:
> On Mon, Nov 14, 2022 at 07:14:48PM +0300, Dmitry Baryshkov wrote:
>> On 14/11/2022 18:38, Johan Hovold wrote:
>>> On Mon, Nov 14, 2022 at 06:19:25PM +0300, Dmitry Baryshkov wrote:
>>>> On 14/11/2022 17:18, Johan Hovold wrote:
>>>>> On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote:
>>>>>> 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:
>>>
>>>>>>> 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.
>>>
>>>>>>> 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
>>>>
>>>> Maybe QMP_COMBO_USB3_PIPE_CLK, QMP_COMBO_DP_LINK_CLK,
>>>> QMP_COMBO_DP_VCO_DIV_CLK?
>>>
>>> "COMBO" is just the name of the Linux driver and does not belong in the
>>> binding.
>>
>> We do not have any standard (iow, coming from the docs) name, so we can
>> invent it on our own.
> 
> I still think the naming should reflect the hardware and not the Linux
> implementation if this is going into the binding.

Well, I always viewed docs as

> 
> And the USB4_USB3_DP defines are going to be a superset of USB3_DP (as
> far we know know).
> 
>>>> I'll then extend this header with QMP_UFS_RX_SYMBOL_0_CLK
>>>> QMP_UFS_RX_SYMBOL_1_CLK and QMP_UFS_TX_SYMBOL_0_CLK.
>>>
>>> Yeah, I had those in mind when creating the header and using a generic
>>> QMP prefix (even if I didn't end up using the header in v1).
>>>
>>> This could just be mapping of (arbitrary) QMP indexes to clocks and we
>>> use it for USB3, DP, UFS and later also USB4.
>>>
>>> This will however mean that the indexes are not necessarily zero-based
>>> and consecutive for a specific SoC and PHY. But that's perhaps a
>>> non-issue (cf. the PHY_TYPE defines).
>>
>> Ugh. Please, no. We have symbol clocks for UFS PHY, USB+DP clocks for
>> USB+DP PHY, but let's not go for the unified clocks index definition.
> 
> Yeah, this is the kind of issues I wanted to avoid by not using a per
> SoC header for three clocks which will almost always use the same
> indexes.
> 
> Because how can you be sure that your unified per-PHY type defines will
> never have to be amended? Or some index left out?
> 
> The only way then is to have per-SoC defines which is a pain to
> maintain (just consider that driver mapping table when some odd SoC
> shows up).

My vote is definitely against a per-SoC defines.

> 
> Johan
  

On Mon, Nov 14, 2022 at 07:51:31PM +0300, Dmitry Baryshkov wrote:
> On 14/11/2022 19:42, Johan Hovold wrote:
> > On Mon, Nov 14, 2022 at 07:14:48PM +0300, Dmitry Baryshkov wrote:

> >> Ugh. Please, no. We have symbol clocks for UFS PHY, USB+DP clocks for
> >> USB+DP PHY, but let's not go for the unified clocks index definition.
> > 
> > Yeah, this is the kind of issues I wanted to avoid by not using a per
> > SoC header for three clocks which will almost always use the same
> > indexes.
> > 
> > Because how can you be sure that your unified per-PHY type defines will
> > never have to be amended? Or some index left out?
> > 
> > The only way then is to have per-SoC defines which is a pain to
> > maintain (just consider that driver mapping table when some odd SoC
> > shows up).
> 
> My vote is definitely against a per-SoC defines.

Simply stating that doesn't address the problem I was trying to
describe.

Johan
  

On 14/11/2022 17:48, Johan Hovold wrote:
> On Mon, Nov 14, 2022 at 05:39:26PM +0100, Krzysztof Kozlowski wrote:
>> On 14/11/2022 17:32, Johan Hovold wrote:
> 
>>> Fair enough, I'll drop it. But there doesn't seem to be a good way to
>>> describe the indexes currently and most bindings simply ignore to do so.
>>>
>>> So what is the preference then? Just leave things undocumented, listing
>>> indexes in a free-text 'description', or adding a free-text reference to
>>> a binding header file and using those define names in a free-text
>>> 'description'?
>>
>> Either 2 or 3. Several bindings for small number of constants choose
>> option 2.
> 
> Ok, we have three now, but USB4 will bump this to ten or so.

Then probably header file is the way to go.

>  
>>> And if going with the last option, does this mean that every SoC and PHY
>>> type needs its own header for those three clocks or so to avoid having
>>> a common dumping ground header file where indexes will not necessarily
>>> be 0-based and consecutive.
>>
>> phy-qcom-qmp-combo.c has one qcom_qmp_dp_clks_hw_get(), so why would you
>> have many of header files?
> 
> We don't know what kind of clock outputs later revisions of these PHYs
> will have. The only way to guarantee 0-based consecutive indexes appears
> to be to use per-SoC defines (e.g. as for the GCC bindings).

Which is also fine. I don't understand still why it is a problem - even
if you have multiple files, one for each SoC/phy. If USB4 brings here 10
more clocks and other SoCs/phys might bring many more options, then what
else can you do? Grow the binding file with big text-based mapping of
IDs? It's not a viable solution. Header or headers is the only
maintainable way for such cases.

Best regards,
Krzysztof
  

On Mon, Nov 14, 2022 at 05:56:21PM +0100, Krzysztof Kozlowski wrote:
> On 14/11/2022 17:48, Johan Hovold wrote:
> > On Mon, Nov 14, 2022 at 05:39:26PM +0100, Krzysztof Kozlowski wrote:
> >> On 14/11/2022 17:32, Johan Hovold wrote:
> > 
> >>> Fair enough, I'll drop it. But there doesn't seem to be a good way to
> >>> describe the indexes currently and most bindings simply ignore to do so.
> >>>
> >>> So what is the preference then? Just leave things undocumented, listing
> >>> indexes in a free-text 'description', or adding a free-text reference to
> >>> a binding header file and using those define names in a free-text
> >>> 'description'?
> >>
> >> Either 2 or 3. Several bindings for small number of constants choose
> >> option 2.
> > 
> > Ok, we have three now, but USB4 will bump this to ten or so.
> 
> Then probably header file is the way to go.
> 
> >  
> >>> And if going with the last option, does this mean that every SoC and PHY
> >>> type needs its own header for those three clocks or so to avoid having
> >>> a common dumping ground header file where indexes will not necessarily
> >>> be 0-based and consecutive.
> >>
> >> phy-qcom-qmp-combo.c has one qcom_qmp_dp_clks_hw_get(), so why would you
> >> have many of header files?
> > 
> > We don't know what kind of clock outputs later revisions of these PHYs
> > will have. The only way to guarantee 0-based consecutive indexes appears
> > to be to use per-SoC defines (e.g. as for the GCC bindings).
> 
> Which is also fine. I don't understand still why it is a problem - even
> if you have multiple files, one for each SoC/phy. If USB4 brings here 10
> more clocks and other SoCs/phys might bring many more options, then what
> else can you do? Grow the binding file with big text-based mapping of
> IDs? It's not a viable solution. Header or headers is the only
> maintainable way for such cases.

So then we must add per-SoC (and PHY type) headers even if we can
possibly reuse defines from one platform for another as long as they
appear to be similar enough? For example, using a "SC7180_USB3_DP" infix
for the current platforms and add a new series of indexes for SC8280XP:

	QMP_SC7180_USB3_DP_USB3_PIPE			0
	QMP_SC7180_USB3_DP_DP_LINK			1
	QMP_SC7180_USB3_DP_DP_VCO_DIV			2

	QMP_SC8280XP_USB4_USB3_DP_USB3_PIPE		0
	QMP_SC8280XP_USB4_USB3_DP_DP_LINK		1
	QMP_SC8280XP_USB4_USB3_DP_DP_VCO_DIV		2
	QMP_SC8280XP_USB4_USB3_DP_USB4_PCIE_PIPE	3
	...
	QMP_SC8280XP_USB4_USB3_DP_USB4_RX1		9

Johan
  

On 14/11/2022 18:08, Johan Hovold wrote:
>>
>> Which is also fine. I don't understand still why it is a problem - even
>> if you have multiple files, one for each SoC/phy. If USB4 brings here 10
>> more clocks and other SoCs/phys might bring many more options, then what
>> else can you do? Grow the binding file with big text-based mapping of
>> IDs? It's not a viable solution. Header or headers is the only
>> maintainable way for such cases.
> 
> So then we must add per-SoC (and PHY type) headers even if we can
> possibly reuse defines from one platform for another as long as they
> appear to be similar enough?

No, you don't have to. I just got impression that future devices will
bring so many changes that anyway you will end up with per-SoC defines.

> For example, using a "SC7180_USB3_DP" infix
> for the current platforms and add a new series of indexes for SC8280XP:
> 
> 	QMP_SC7180_USB3_DP_USB3_PIPE			0
> 	QMP_SC7180_USB3_DP_DP_LINK			1
> 	QMP_SC7180_USB3_DP_DP_VCO_DIV			2
> 
> 	QMP_SC8280XP_USB4_USB3_DP_USB3_PIPE		0
> 	QMP_SC8280XP_USB4_USB3_DP_DP_LINK		1
> 	QMP_SC8280XP_USB4_USB3_DP_DP_VCO_DIV		2
> 	QMP_SC8280XP_USB4_USB3_DP_USB4_PCIE_PIPE	3
> 	...
> 	QMP_SC8280XP_USB4_USB3_DP_USB4_RX1		9

The names are just a names, you can even use QMP_SC7180_* on SC8280XP.
You can skip the SoC part and have something shared. We already have
such patterns - although maybe more often for outside components (like
PMICs). The differences are:
1. For per-SoC name it's quite obvious which clock is supported on fiven
SoC,
2. With shared names, you should document somewhere mapping between
supported clocks and SoCs. Also what to do if new device comes with 10
new clocks entirely different - re-use/map existing defines or add
completely new set of 10 of them?

Best regards,
Krzysztof
  

On Tue, Nov 15, 2022 at 09:12:54AM +0100, Krzysztof Kozlowski wrote:
> On 14/11/2022 18:08, Johan Hovold wrote:
> >>
> >> Which is also fine. I don't understand still why it is a problem - even
> >> if you have multiple files, one for each SoC/phy. If USB4 brings here 10
> >> more clocks and other SoCs/phys might bring many more options, then what
> >> else can you do? Grow the binding file with big text-based mapping of
> >> IDs? It's not a viable solution. Header or headers is the only
> >> maintainable way for such cases.
> > 
> > So then we must add per-SoC (and PHY type) headers even if we can
> > possibly reuse defines from one platform for another as long as they
> > appear to be similar enough?
> 
> No, you don't have to. I just got impression that future devices will
> bring so many changes that anyway you will end up with per-SoC defines.
> 
> > For example, using a "SC7180_USB3_DP" infix
> > for the current platforms and add a new series of indexes for SC8280XP:
> > 
> > 	QMP_SC7180_USB3_DP_USB3_PIPE			0
> > 	QMP_SC7180_USB3_DP_DP_LINK			1
> > 	QMP_SC7180_USB3_DP_DP_VCO_DIV			2
> > 
> > 	QMP_SC8280XP_USB4_USB3_DP_USB3_PIPE		0
> > 	QMP_SC8280XP_USB4_USB3_DP_DP_LINK		1
> > 	QMP_SC8280XP_USB4_USB3_DP_DP_VCO_DIV		2
> > 	QMP_SC8280XP_USB4_USB3_DP_USB4_PCIE_PIPE	3
> > 	...
> > 	QMP_SC8280XP_USB4_USB3_DP_USB4_RX1		9
> 
> The names are just a names, you can even use QMP_SC7180_* on SC8280XP.
> You can skip the SoC part and have something shared. We already have
> such patterns - although maybe more often for outside components (like
> PMICs). The differences are:
> 1. For per-SoC name it's quite obvious which clock is supported on fiven
> SoC,
> 2. With shared names, you should document somewhere mapping between
> supported clocks and SoCs. Also what to do if new device comes with 10
> new clocks entirely different - re-use/map existing defines or add
> completely new set of 10 of them?

Ok, thanks. I'll go with a common prefix per PHY type for now, and we
can worry about hypothetical hardware revisions later.

I'll use a "QMP_USB43DP_" prefix for the new SC8280XP binding, which can
be reused also for the older SoCs with USB3-DP PHYs if/when we convert
them as their indexes will be a subset of the SC8280XP ones:

	/* QMP USB4-USB3-DP clocks */
	#define QMP_USB43DP_USB3_PIPE_CLK	0
	#define QMP_USB43DP_DP_LINK_CLK		1
	#define QMP_USB43DP_DP_VCO_DIV_CLK	2

Since I'm adding a new header anyway, I decided to go with dedicated
indexes also for the PHY selection (instead of using the PHY_TYPE
defines):

	/* QMP USB4-USB3-DP PHYs */
	#define QMP_USB43DP_USB3_PHY		0
	#define QMP_USB43DP_DP_PHY		1

I'll add these to a common dt-bindings/phy/phy-qcom-qmp.h header so that
it can be used also for the UFS clocks (with a "QMP_UFS_" prefix).

Johan
  

On 15/11/2022 15:22, Johan Hovold wrote:
> On Tue, Nov 15, 2022 at 09:12:54AM +0100, Krzysztof Kozlowski wrote:
>> On 14/11/2022 18:08, Johan Hovold wrote:
>>>>
>>>> Which is also fine. I don't understand still why it is a problem - even
>>>> if you have multiple files, one for each SoC/phy. If USB4 brings here 10
>>>> more clocks and other SoCs/phys might bring many more options, then what
>>>> else can you do? Grow the binding file with big text-based mapping of
>>>> IDs? It's not a viable solution. Header or headers is the only
>>>> maintainable way for such cases.
>>>
>>> So then we must add per-SoC (and PHY type) headers even if we can
>>> possibly reuse defines from one platform for another as long as they
>>> appear to be similar enough?
>>
>> No, you don't have to. I just got impression that future devices will
>> bring so many changes that anyway you will end up with per-SoC defines.
>>
>>> For example, using a "SC7180_USB3_DP" infix
>>> for the current platforms and add a new series of indexes for SC8280XP:
>>>
>>> 	QMP_SC7180_USB3_DP_USB3_PIPE			0
>>> 	QMP_SC7180_USB3_DP_DP_LINK			1
>>> 	QMP_SC7180_USB3_DP_DP_VCO_DIV			2
>>>
>>> 	QMP_SC8280XP_USB4_USB3_DP_USB3_PIPE		0
>>> 	QMP_SC8280XP_USB4_USB3_DP_DP_LINK		1
>>> 	QMP_SC8280XP_USB4_USB3_DP_DP_VCO_DIV		2
>>> 	QMP_SC8280XP_USB4_USB3_DP_USB4_PCIE_PIPE	3
>>> 	...
>>> 	QMP_SC8280XP_USB4_USB3_DP_USB4_RX1		9
>>
>> The names are just a names, you can even use QMP_SC7180_* on SC8280XP.
>> You can skip the SoC part and have something shared. We already have
>> such patterns - although maybe more often for outside components (like
>> PMICs). The differences are:
>> 1. For per-SoC name it's quite obvious which clock is supported on fiven
>> SoC,
>> 2. With shared names, you should document somewhere mapping between
>> supported clocks and SoCs. Also what to do if new device comes with 10
>> new clocks entirely different - re-use/map existing defines or add
>> completely new set of 10 of them?
> 
> Ok, thanks. I'll go with a common prefix per PHY type for now, and we
> can worry about hypothetical hardware revisions later.
> 
> I'll use a "QMP_USB43DP_" prefix for the new SC8280XP binding, which can
> be reused also for the older SoCs with USB3-DP PHYs if/when we convert
> them as their indexes will be a subset of the SC8280XP ones:
> 
> 	/* QMP USB4-USB3-DP clocks */
> 	#define QMP_USB43DP_USB3_PIPE_CLK	0
> 	#define QMP_USB43DP_DP_LINK_CLK		1
> 	#define QMP_USB43DP_DP_VCO_DIV_CLK	2
> 
> Since I'm adding a new header anyway, I decided to go with dedicated
> indexes also for the PHY selection (instead of using the PHY_TYPE
> defines):
> 
> 	/* QMP USB4-USB3-DP PHYs */
> 	#define QMP_USB43DP_USB3_PHY		0
> 	#define QMP_USB43DP_DP_PHY		1
> 
> I'll add these to a common dt-bindings/phy/phy-qcom-qmp.h header so that
> it can be used also for the UFS clocks (with a "QMP_UFS_" prefix).

Sounds good, thanks.

Best regards,
Krzysztof