Re: [PATCH 2/3] dt-bindings: usb: mtk-xhci: Allow middle optional clocks to be missing

On 20/06/2022 17:50, Nícolas F. R. A. Prado wrote:

On Mon, Jun 20, 2022 at 10:50:57AM +0200, Krzysztof Kozlowski wrote:

quoted

On 20/06/2022 08:59, Chunfeng Yun wrote:

quoted

On Sun, 2022-06-19 at 14:05 +0200, Krzysztof Kozlowski wrote:

quoted

On 19/06/2022 09:46, Chunfeng Yun wrote:

quoted

On Fri, 2022-06-17 at 18:25 -0700, Krzysztof Kozlowski wrote:

quoted

On 17/06/2022 15:29, Nícolas F. R. A. Prado wrote:

quoted

The current clock list in the binding doesn't allow for one of
the
optional clocks to be missing and a subsequent clock to be
present.
An
example where this is an issue is in mt8192.dtsi, which has
"sys_ck",
"ref_ck", "xhci_ck" and would cause dtbs_check warnings.

Change the clock list in a way that allows the middle optional
clocks to
be missing, while still guaranteeing a fixed order. The
"ref_ck" is
kept
as a const even though it is optional for simplicity, since it
is
present in all current dts files.

Signed-off-by: Nícolas F. R. A. Prado <redacted>
---

 .../devicetree/bindings/usb/mediatek,mtk-xhci.yaml       | 9
+++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git
a/Documentation/devicetree/bindings/usb/mediatek,mtk-
xhci.yaml b/Documentation/devicetree/bindings/usb/mediatek,mtk-
xhci.yaml
index 63cbc2b62d18..99a1b233ec90 100644

--- a/Documentation/devicetree/bindings/usb/mediatek,mtk-

xhci.yaml

+++ b/Documentation/devicetree/bindings/usb/mediatek,mtk-

xhci.yaml

@@ -80,8 +80,13 @@ properties:
     items:
       - const: sys_ck  # required, the following ones are

optional
       - const: ref_ck
-      - const: mcu_ck
-      - const: dma_ck
+      - enum:
+          - mcu_ck
+          - dma_ck
+          - xhci_ck
+      - enum:
+          - dma_ck
+          - xhci_ck
       - const: xhci_ck

You allow now almost any order here, including incorrect like
sys,ref,xhci,xhci,xhci.

The order of clocks has to be fixed and we cannot allow
flexibility.
Are
you sure that these clocks are actually optional (not wired to
the
device)?

In fact, these optional clocks are fixed, due to no gates are
provided,
SW can't control them by CCF;
In this case, I usually use a fixed clock, or ignore it.

But in some versions these clocks are controllable or not?

Some SoCs are controllable, some ones are not (fixed clock).

Thanks for confirming. Then I would prefer to make these clocks required
(not optional) and always provide them - via common clock framework or
fixed-clock.

Hi Krzysztof and Chunfeng,

thank you both for the feedback.

Since the solution I proposed in this patch is not acceptable I see two options:
1. Split the clocks in several if blocks matched by compatibles
2. Make the clocks required and use fixed-clock nodes for the missing clocks in
   the DT

My understanding is that 1 is the desirable solution if the clock is really
missing in some hardware variants, while 2 is desirable if all hardware variants
really receive all the clocks, only that on some variants they're fixed and not
controlable by SW.

From what I'm reading of this discussion it seems that the latter is the case
here and thus we should go for 2. Is this correct?

This is how I understood it as well, so correct from my side.

Also Chunfeng, do you have information on whether the same is true for the MMC
HW block? I recently submitted some changes to that binding [1] but I followed
approach 1 there instead. However if all the clocks are present in the HW level
there as well it would make more sense for me to change it to follow approach 2.

Thanks,
Nícolas

[1] https://lore.kernel.org/all/20220617230114.2438875-1-nfraprado@collabora.com (local)

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel