| Message ID | 20240701071048.751863-5-tmaimon77@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add NPCM8XX clock driver | expand |
On Mon, 2024-07-01 at 10:10 +0300, Tomer Maimon wrote: > Modify clock property handler in UART, CPU, PECI modules to reset > controller. Sooo... I'm not sure how much of a hack this is, as it's not clear to me that the devicetree spec allows multiple labels on a node, but `dtc` seems to accept it. You can reduce this patch to a short diff with: - rstc: reset-controller@f0801000 { + clk: rstc: reset-controller@f0801000 { and leave the rest of the phandles in-tact. Again, something to investigate for yourself and maybe see how it flies upstream... Andrew
On Wed, 2024-07-03 at 15:55 +0930, Andrew Jeffery wrote: > On Mon, 2024-07-01 at 10:10 +0300, Tomer Maimon wrote: > > Modify clock property handler in UART, CPU, PECI modules to reset > > controller. > > Sooo... I'm not sure how much of a hack this is, as it's not clear to > me that the devicetree spec allows multiple labels on a node, but `dtc` > seems to accept it. > > You can reduce this patch to a short diff with: > > - rstc: reset-controller@f0801000 { > + clk: rstc: reset-controller@f0801000 { > > and leave the rest of the phandles in-tact. > Well, there is some precedent: ``` $ git grep -E '.+: .+: .+ \{' -- arch/arm/boot/dts/ arch/arm/boot/dts/arm/arm-realview-eb.dtsi: charlcd: fpga_charlcd: charlcd@10008000 { arch/arm/boot/dts/arm/vexpress-v2p-ca9.dts: smbclk: oscclk2: tcrefclk { arch/arm/boot/dts/rockchip/rk3188-bqedison2qc.dts: vcc_io: vcc_hdmi: REG4 { arch/arm/boot/dts/rockchip/rk3288-firefly-reload-core.dtsi: vccio_wl: vcc_18: REG11 { arch/arm/boot/dts/rockchip/rk3288-firefly-reload.dts: vcc_5v: vcc_sys: vsys-regulator { arch/arm/boot/dts/rockchip/rk3288-firefly.dtsi: vbat_wl: vcc_sys: vsys-regulator { arch/arm/boot/dts/rockchip/rk3288-firefly.dtsi: vccio_wl: vcc_18: REG11 { arch/arm/boot/dts/rockchip/rk3288-rock2-som.dtsi: vcc_io: vccio_codec: REG2 { arch/arm/boot/dts/rockchip/rk3288-veyron.dtsi: vcc18_wl: vcc18_flashio: vcc_18: DCDC_REG4 { arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts: vqmmc_sdcard: ldo4_reg: LDO4 { arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts: vqmmc_sdcard: ldo4_reg: LDO4 { ``` Andrew
Hi Andrew, Great suggestion :-) i will try it! Thanks, Tomer On Wed, 3 Jul 2024 at 09:30, Andrew Jeffery <andrew@codeconstruct.com.au> wrote: > > On Wed, 2024-07-03 at 15:55 +0930, Andrew Jeffery wrote: > > On Mon, 2024-07-01 at 10:10 +0300, Tomer Maimon wrote: > > > Modify clock property handler in UART, CPU, PECI modules to reset > > > controller. > > > > Sooo... I'm not sure how much of a hack this is, as it's not clear to > > me that the devicetree spec allows multiple labels on a node, but `dtc` > > seems to accept it. > > > > You can reduce this patch to a short diff with: > > > > - rstc: reset-controller@f0801000 { > > + clk: rstc: reset-controller@f0801000 { > > > > and leave the rest of the phandles in-tact. > > > > Well, there is some precedent: > > ``` > $ git grep -E '.+: .+: .+ \{' -- arch/arm/boot/dts/ > arch/arm/boot/dts/arm/arm-realview-eb.dtsi: charlcd: fpga_charlcd: charlcd@10008000 { > arch/arm/boot/dts/arm/vexpress-v2p-ca9.dts: smbclk: oscclk2: tcrefclk { > arch/arm/boot/dts/rockchip/rk3188-bqedison2qc.dts: vcc_io: vcc_hdmi: REG4 { > arch/arm/boot/dts/rockchip/rk3288-firefly-reload-core.dtsi: vccio_wl: vcc_18: REG11 { > arch/arm/boot/dts/rockchip/rk3288-firefly-reload.dts: vcc_5v: vcc_sys: vsys-regulator { > arch/arm/boot/dts/rockchip/rk3288-firefly.dtsi: vbat_wl: vcc_sys: vsys-regulator { > arch/arm/boot/dts/rockchip/rk3288-firefly.dtsi: vccio_wl: vcc_18: REG11 { > arch/arm/boot/dts/rockchip/rk3288-rock2-som.dtsi: vcc_io: vccio_codec: REG2 { > arch/arm/boot/dts/rockchip/rk3288-veyron.dtsi: vcc18_wl: vcc18_flashio: vcc_18: DCDC_REG4 { > arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts: vqmmc_sdcard: ldo4_reg: LDO4 { > arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts: vqmmc_sdcard: ldo4_reg: LDO4 { > ``` > > Andrew
Hi Andrew, One more question, for this change the clock node should be removed. I am afraid that removing the clock node could cause an ABI break, not? Thanks, Tomer On Wed, 3 Jul 2024 at 11:13, Tomer Maimon <tmaimon77@gmail.com> wrote: > > Hi Andrew, > > Great suggestion :-) i will try it! > > Thanks, > > Tomer > > On Wed, 3 Jul 2024 at 09:30, Andrew Jeffery <andrew@codeconstruct.com.au> wrote: > > > > On Wed, 2024-07-03 at 15:55 +0930, Andrew Jeffery wrote: > > > On Mon, 2024-07-01 at 10:10 +0300, Tomer Maimon wrote: > > > > Modify clock property handler in UART, CPU, PECI modules to reset > > > > controller. > > > > > > Sooo... I'm not sure how much of a hack this is, as it's not clear to > > > me that the devicetree spec allows multiple labels on a node, but `dtc` > > > seems to accept it. > > > > > > You can reduce this patch to a short diff with: > > > > > > - rstc: reset-controller@f0801000 { > > > + clk: rstc: reset-controller@f0801000 { > > > > > > and leave the rest of the phandles in-tact. > > > > > > > Well, there is some precedent: > > > > ``` > > $ git grep -E '.+: .+: .+ \{' -- arch/arm/boot/dts/ > > arch/arm/boot/dts/arm/arm-realview-eb.dtsi: charlcd: fpga_charlcd: charlcd@10008000 { > > arch/arm/boot/dts/arm/vexpress-v2p-ca9.dts: smbclk: oscclk2: tcrefclk { > > arch/arm/boot/dts/rockchip/rk3188-bqedison2qc.dts: vcc_io: vcc_hdmi: REG4 { > > arch/arm/boot/dts/rockchip/rk3288-firefly-reload-core.dtsi: vccio_wl: vcc_18: REG11 { > > arch/arm/boot/dts/rockchip/rk3288-firefly-reload.dts: vcc_5v: vcc_sys: vsys-regulator { > > arch/arm/boot/dts/rockchip/rk3288-firefly.dtsi: vbat_wl: vcc_sys: vsys-regulator { > > arch/arm/boot/dts/rockchip/rk3288-firefly.dtsi: vccio_wl: vcc_18: REG11 { > > arch/arm/boot/dts/rockchip/rk3288-rock2-som.dtsi: vcc_io: vccio_codec: REG2 { > > arch/arm/boot/dts/rockchip/rk3288-veyron.dtsi: vcc18_wl: vcc18_flashio: vcc_18: DCDC_REG4 { > > arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts: vqmmc_sdcard: ldo4_reg: LDO4 { > > arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts: vqmmc_sdcard: ldo4_reg: LDO4 { > > ``` > > > > Andrew
On Wed, 2024-07-03 at 12:09 +0300, Tomer Maimon wrote: > Hi Andrew, > > One more question, for this change the clock node should be removed. > > I am afraid that removing the clock node could cause an ABI break, not? Possibly it could break other devicetrees, depending on how they're constructed. I'm hesitant to merge these patches to the OpenBMC tree without them being accepted upstream because of the potential for further breaks down the track. The situation is already too murky for my taste, and I'd prefer we keep things to a single break with whatever's accepted upstream. I recognise that's probably frustrating for you, but I think it focuses the effort in the right area (getting the changes merged upstream). I'm not sure how you best get Stephen's attention though. Andrew
Hi Andrew, On Thu, 4 Jul 2024 at 04:26, Andrew Jeffery <andrew@codeconstruct.com.au> wrote: > > On Wed, 2024-07-03 at 12:09 +0300, Tomer Maimon wrote: > > Hi Andrew, > > > > One more question, for this change the clock node should be removed. > > > > I am afraid that removing the clock node could cause an ABI break, not? > > Possibly it could break other devicetrees, depending on how they're > constructed. > > I'm hesitant to merge these patches to the OpenBMC tree without them > being accepted upstream because of the potential for further breaks > down the track. The situation is already too murky for my taste, and > I'd prefer we keep things to a single break with whatever's accepted > upstream. Yes, I understand you, I believe this saga will end soon we are doing our best for a happy ending :-) > > I recognise that's probably frustrating for you, but I think it focuses > the effort in the right area (getting the changes merged upstream). > > I'm not sure how you best get Stephen's attention though. > > Andrew Best regards, Tomer
diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi index 84a2a5172597..ed9f0edf1888 100644 --- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi @@ -74,7 +74,7 @@ peci: peci-controller@100000 { compatible = "nuvoton,npcm845-peci"; reg = <0x100000 0x1000>; interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>; - clocks = <&clk NPCM8XX_CLK_APB3>; + clocks = <&rstc NPCM8XX_CLK_APB3>; cmd-timeout-ms = <1000>; status = "disabled"; }; @@ -90,7 +90,7 @@ timer0: timer@8000 { serial0: serial@0 { compatible = "nuvoton,npcm845-uart", "nuvoton,npcm750-uart"; reg = <0x0 0x1000>; - clocks = <&clk NPCM8XX_CLK_UART>; + clocks = <&rstc NPCM8XX_CLK_UART>; interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>; reg-shift = <2>; status = "disabled"; @@ -99,7 +99,7 @@ serial0: serial@0 { serial1: serial@1000 { compatible = "nuvoton,npcm845-uart", "nuvoton,npcm750-uart"; reg = <0x1000 0x1000>; - clocks = <&clk NPCM8XX_CLK_UART>; + clocks = <&rstc NPCM8XX_CLK_UART>; interrupts = <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>; reg-shift = <2>; status = "disabled"; @@ -108,7 +108,7 @@ serial1: serial@1000 { serial2: serial@2000 { compatible = "nuvoton,npcm845-uart", "nuvoton,npcm750-uart"; reg = <0x2000 0x1000>; - clocks = <&clk NPCM8XX_CLK_UART>; + clocks = <&rstc NPCM8XX_CLK_UART>; interrupts = <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>; reg-shift = <2>; status = "disabled"; @@ -117,7 +117,7 @@ serial2: serial@2000 { serial3: serial@3000 { compatible = "nuvoton,npcm845-uart", "nuvoton,npcm750-uart"; reg = <0x3000 0x1000>; - clocks = <&clk NPCM8XX_CLK_UART>; + clocks = <&rstc NPCM8XX_CLK_UART>; interrupts = <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>; reg-shift = <2>; status = "disabled"; @@ -126,7 +126,7 @@ serial3: serial@3000 { serial4: serial@4000 { compatible = "nuvoton,npcm845-uart", "nuvoton,npcm750-uart"; reg = <0x4000 0x1000>; - clocks = <&clk NPCM8XX_CLK_UART>; + clocks = <&rstc NPCM8XX_CLK_UART>; interrupts = <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>; reg-shift = <2>; status = "disabled"; @@ -135,7 +135,7 @@ serial4: serial@4000 { serial5: serial@5000 { compatible = "nuvoton,npcm845-uart", "nuvoton,npcm750-uart"; reg = <0x5000 0x1000>; - clocks = <&clk NPCM8XX_CLK_UART>; + clocks = <&rstc NPCM8XX_CLK_UART>; interrupts = <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>; reg-shift = <2>; status = "disabled"; @@ -144,7 +144,7 @@ serial5: serial@5000 { serial6: serial@6000 { compatible = "nuvoton,npcm845-uart", "nuvoton,npcm750-uart"; reg = <0x6000 0x1000>; - clocks = <&clk NPCM8XX_CLK_UART>; + clocks = <&rstc NPCM8XX_CLK_UART2>; interrupts = <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>; reg-shift = <2>; status = "disabled"; diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.dtsi index 383938dcd3ce..3cbcea65eba2 100644 --- a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.dtsi +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.dtsi @@ -14,7 +14,7 @@ cpus { cpu0: cpu@0 { device_type = "cpu"; compatible = "arm,cortex-a35"; - clocks = <&clk NPCM8XX_CLK_CPU>; + clocks = <&rstc NPCM8XX_CLK_CPU>; reg = <0x0 0x0>; next-level-cache = <&l2>; enable-method = "psci"; @@ -23,7 +23,7 @@ cpu0: cpu@0 { cpu1: cpu@1 { device_type = "cpu"; compatible = "arm,cortex-a35"; - clocks = <&clk NPCM8XX_CLK_CPU>; + clocks = <&rstc NPCM8XX_CLK_CPU>; reg = <0x0 0x1>; next-level-cache = <&l2>; enable-method = "psci"; @@ -32,7 +32,7 @@ cpu1: cpu@1 { cpu2: cpu@2 { device_type = "cpu"; compatible = "arm,cortex-a35"; - clocks = <&clk NPCM8XX_CLK_CPU>; + clocks = <&rstc NPCM8XX_CLK_CPU>; reg = <0x0 0x2>; next-level-cache = <&l2>; enable-method = "psci"; @@ -41,7 +41,7 @@ cpu2: cpu@2 { cpu3: cpu@3 { device_type = "cpu"; compatible = "arm,cortex-a35"; - clocks = <&clk NPCM8XX_CLK_CPU>; + clocks = <&rstc NPCM8XX_CLK_CPU>; reg = <0x0 0x3>; next-level-cache = <&l2>; enable-method = "psci";
Modify clock property handler in UART, CPU, PECI modules to reset controller. Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> --- .../boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 16 ++++++++-------- arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.dtsi | 8 ++++---- 2 files changed, 12 insertions(+), 12 deletions(-)