Hi Krzysztof,

Sorry for the late reply.

On Mon, Dec 05, 2022 at 09:55:40AM +0100, Krzysztof Kozlowski wrote:

On 05/12/2022 08:54, Chester Lin wrote:

quoted

quoted

quoted

quoted

quoted

+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    #define S32GEN1_SCMI_CLK_GMAC0_AXI
+    #define S32GEN1_SCMI_CLK_GMAC0_TX_PCS
+    #define S32GEN1_SCMI_CLK_GMAC0_TX_RGMII
+    #define S32GEN1_SCMI_CLK_GMAC0_TX_RMII
+    #define S32GEN1_SCMI_CLK_GMAC0_TX_MII
+    #define S32GEN1_SCMI_CLK_GMAC0_RX_PCS
+    #define S32GEN1_SCMI_CLK_GMAC0_RX_RGMII
+    #define S32GEN1_SCMI_CLK_GMAC0_RX_RMII
+    #define S32GEN1_SCMI_CLK_GMAC0_RX_MII
+    #define S32GEN1_SCMI_CLK_GMAC0_TS

Why defines? Your clock controller is not ready? If so, just use raw
numbers.

Please compare v1: There is no Linux-driven clock controller here but 
rather a fluid SCMI firmware interface. Work towards getting clocks into 
a kernel-hosted .dtsi was halted in favor of (downstream) TF-A, which 
also explains the ugly examples here and for pinctrl.

This does not explain to me why you added defines in the example. Are
you saying these can change any moment?

Actually these GMAC-related SCMI clock IDs changed once in NXP's downstream TF-A,
some redundant TS clock IDs were removed and the rest of clock IDs were all moved
forward. 

This is not accepted. Your downstream TF-A cannot change bindings. As an
upstream contributor you should push this back and definitely not try to
upstream such approach.

quoted

Apart from GMAC-related IDs, some other clock IDs were also appended
in both base-clock IDs and platform-specific clock IDs [The first plat ID =
The last base ID + 1]. Due to the current design of the clk-scmi driver and the
SCMI clock protocol, IIUC, it's better to keep all clock IDs in sequence without
a blank in order to avoid query miss, which could affect the probe speed.

You miss here broken ABI! Any change in IDs causes all DTBs to be
broken. Downstream, upstream, other projects, everywhere.

Therefore thank you for clarifying that we need to be more careful about
stuff coming from (or for) NXP. Here you need to drop all defines and
all your patches must assume the ID is fixed. Once there, it's
unchangeable without breaking the ABI.

Please accept my apologies for submitting these bad patches. We were just
confused since we thought that this approach might be acceptable because
there were some similar examples got merged in the kernel tree:

https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/net/intel,dwmac-plat.yaml#L57
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/usb/intel,keembay-dwc3.yaml#L55
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/sound/intel,keembay-i2s.yaml#L73
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml#L39
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/watchdog/intel,keembay-wdt.yaml#L46
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml#L75
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml#L282

The defines in these yaml files are not actually referred by kernel DTs or
drivers so I assume that they should be provided by firmware as pure integer
numbers and the clk-scmi driver should just take them to ask firmware for doing
clk stuff.

Regards,
Chester

Best regards,
Krzysztof