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

On Tue, 2022-06-21 at 09:14 +0200, Krzysztof Kozlowski wrote:

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

quoted

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 <
nfraprado@collabora.com>
---

 .../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 right for me.

quoted

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.

I discussed it with Wenbin, MMC seems a little different with USB,

Hi Wenbin,

   Please give some comments about MMC, thanks

quoted

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