| Message ID | 20231214105243.3707730-1-tudor.ambarus@linaro.org |
|---|---|
| Headers | show |
| Series | GS101 Oriole: CMU_PERIC0 support and USI updates | expand |
On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > The gs101 clock names are derived from the clock register names under > some certain rules. In particular, for the gate clocks the following is > documented and expected in the gs101 clock driver: > > Replace CLK_CON_GAT_CLKCMU with CLK_GOUT_CMU and gout_cmu > Replace CLK_CON_GAT_GATE_CLKCMU with CLK_GOUT_CMU and gout_cmu > > For gates remove _UID _BLK _IPCLKPORT and _RSTNSYNC > Doesn't it break existing gs101 device tree? > The CMU TOP gate clock names missed to include the required "CMU" > differentiator which will cause name collisions with the gate clock names > of other clock units. Fix the TOP gate clock names and include "CMU" in > their name. > > Fixes: 0a910f160638 ("dt-bindings: clock: Add Google gs101 clock management unit bindings") > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- (snip)
On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Add dt-schema documentation for the Connectivity Peripheral 0 (PERIC0) > clock management unit. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > .../bindings/clock/google,gs101-clock.yaml | 25 +++++- > include/dt-bindings/clock/google,gs101.h | 86 +++++++++++++++++++ > 2 files changed, 109 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml > index 3eebc03a309b..ba54c13c55bc 100644 > --- a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml > +++ b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml > @@ -30,14 +30,15 @@ properties: > - google,gs101-cmu-top > - google,gs101-cmu-apm > - google,gs101-cmu-misc > + - google,gs101-cmu-peric0 > > clocks: > minItems: 1 > - maxItems: 2 > + maxItems: 3 > > clock-names: > minItems: 1 > - maxItems: 2 > + maxItems: 3 > > "#clock-cells": > const: 1 > @@ -88,6 +89,26 @@ allOf: > - const: dout_cmu_misc_bus > - const: dout_cmu_misc_sss > > + - if: > + properties: > + compatible: > + contains: > + const: google,gs101-cmu-peric0 > + > + then: > + properties: > + clocks: > + items: > + - description: External reference clock (24.576 MHz) > + - description: Connectivity Peripheral 0 bus clock (from CMU_TOP) > + - description: Connectivity Peripheral 0 IP clock (from CMU_TOP) > + > + clock-names: > + items: > + - const: oscclk > + - const: dout_cmu_peric0_bus > + - const: dout_cmu_peric0_ip > + > additionalProperties: false > > examples: > diff --git a/include/dt-bindings/clock/google,gs101.h b/include/dt-bindings/clock/google,gs101.h > index 21adec22387c..7d7a896416a7 100644 > --- a/include/dt-bindings/clock/google,gs101.h > +++ b/include/dt-bindings/clock/google,gs101.h > @@ -389,4 +389,90 @@ > #define CLK_GOUT_MISC_WDT_CLUSTER1_PCLK 73 > #define CLK_GOUT_MISC_XIU_D_MISC_ACLK 74 > > +/* CMU_PERIC0 */ This comments looks off here. Other than than, LGTM: Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > +/* CMU_PERIC0 MUX */ > +#define CLK_MOUT_PERIC0_BUS_USER 1 > +#define CLK_MOUT_PERIC0_I3C_USER 2 > +#define CLK_MOUT_PERIC0_USI0_UART_USER 3 > +#define CLK_MOUT_PERIC0_USI14_USI_USER 4 > +#define CLK_MOUT_PERIC0_USI1_USI_USER 5 > +#define CLK_MOUT_PERIC0_USI2_USI_USER 6 > +#define CLK_MOUT_PERIC0_USI3_USI_USER 7 > +#define CLK_MOUT_PERIC0_USI4_USI_USER 8 > +#define CLK_MOUT_PERIC0_USI5_USI_USER 9 > +#define CLK_MOUT_PERIC0_USI6_USI_USER 10 > +#define CLK_MOUT_PERIC0_USI7_USI_USER 11 > +#define CLK_MOUT_PERIC0_USI8_USI_USER 12 > + > +/* CMU_PERIC0 Dividers */ > +#define CLK_DOUT_PERIC0_I3C 13 > +#define CLK_DOUT_PERIC0_USI0_UART 14 > +#define CLK_DOUT_PERIC0_USI14_USI 15 > +#define CLK_DOUT_PERIC0_USI1_USI 16 > +#define CLK_DOUT_PERIC0_USI2_USI 17 > +#define CLK_DOUT_PERIC0_USI3_USI 18 > +#define CLK_DOUT_PERIC0_USI4_USI 19 > +#define CLK_DOUT_PERIC0_USI5_USI 20 > +#define CLK_DOUT_PERIC0_USI6_USI 21 > +#define CLK_DOUT_PERIC0_USI7_USI 22 > +#define CLK_DOUT_PERIC0_USI8_USI 23 > + > +/* CMU_PERIC0 Gates */ > +#define CLK_GOUT_PERIC0_IP 24 > +#define CLK_GOUT_PERIC0_PERIC0_CMU_PERIC0_PCLK 25 > +#define CLK_GOUT_PERIC0_CLK_PERIC0_OSCCLK_CLK 26 > +#define CLK_GOUT_PERIC0_D_TZPC_PERIC0_PCLK 27 > +#define CLK_GOUT_PERIC0_GPC_PERIC0_PCLK 28 > +#define CLK_GOUT_PERIC0_GPIO_PERIC0_PCLK 29 > +#define CLK_GOUT_PERIC0_LHM_AXI_P_PERIC0_I_CLK 30 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_0 31 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_1 32 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_10 33 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_11 34 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_12 35 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_13 36 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_14 37 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_15 38 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_2 39 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_3 40 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_4 41 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_5 42 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_6 43 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_7 44 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_8 45 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_9 46 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_0 47 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_1 48 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_10 49 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_11 50 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_12 51 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_13 52 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_14 53 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_15 54 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_2 55 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_3 56 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_4 57 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_5 58 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_6 59 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_7 60 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_8 61 > +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_9 62 > +#define CLK_GOUT_PERIC0_PERIC0_TOP1_IPCLK_0 63 > +#define CLK_GOUT_PERIC0_PERIC0_TOP1_IPCLK_2 64 > +#define CLK_GOUT_PERIC0_PERIC0_TOP1_PCLK_0 65 > +#define CLK_GOUT_PERIC0_PERIC0_TOP1_PCLK_2 66 > +#define CLK_GOUT_PERIC0_CLK_PERIC0_BUSP_CLK 67 > +#define CLK_GOUT_PERIC0_CLK_PERIC0_I3C_CLK 68 > +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI0_UART_CLK 69 > +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI14_USI_CLK 70 > +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI1_USI_CLK 71 > +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI2_USI_CLK 72 > +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI3_USI_CLK 73 > +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI4_USI_CLK 74 > +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI5_USI_CLK 75 > +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI6_USI_CLK 76 > +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI7_USI_CLK 77 > +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK 78 > +#define CLK_GOUT_PERIC0_SYSREG_PERIC0_PCLK 79 > + > #endif /* _DT_BINDINGS_CLOCK_GOOGLE_GS101_H */ > -- > 2.43.0.472.g3155946c3a-goog >
On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > GS101 only allows 32-bit register accesses. When using 8-bit reg > accesses on gs101, a SError Interrupt is raised causing the system > unusable. > > Make reg-io-width a required property and expect for it a value of 4. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > Documentation/devicetree/bindings/serial/samsung_uart.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > index 133259ed3a34..cc896d7e2a3d 100644 > --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml > +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > @@ -143,6 +143,10 @@ allOf: > then: > required: > - samsung,uart-fifosize > + - reg-io-width > + properties: > + reg-io-width: > + const: 4 > > unevaluatedProperties: false > > -- > 2.43.0.472.g3155946c3a-goog >
On 12/14/23 15:07, Sam Protsenko wrote: > On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >> The gs101 clock names are derived from the clock register names under >> some certain rules. In particular, for the gate clocks the following is >> documented and expected in the gs101 clock driver: >> >> Replace CLK_CON_GAT_CLKCMU with CLK_GOUT_CMU and gout_cmu >> Replace CLK_CON_GAT_GATE_CLKCMU with CLK_GOUT_CMU and gout_cmu >> >> For gates remove _UID _BLK _IPCLKPORT and _RSTNSYNC >> > > Doesn't it break existing gs101 device tree? No, compilation went fine at this point. The TOP gates are not used in the device tree at this point. And since the bindings patch was just applied I think we should fix it, so that we avoid name clashes as described below (I found a clash with a gate from PERIC0). > >> The CMU TOP gate clock names missed to include the required "CMU" >> differentiator which will cause name collisions with the gate clock names >> of other clock units. Fix the TOP gate clock names and include "CMU" in >> their name. >> >> Fixes: 0a910f160638 ("dt-bindings: clock: Add Google gs101 clock management unit bindings") >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> --- > > (snip) Thanks for the review! ta
On Thu, Dec 14, 2023 at 9:16 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > > > On 12/14/23 15:07, Sam Protsenko wrote: > > On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >> > >> The gs101 clock names are derived from the clock register names under > >> some certain rules. In particular, for the gate clocks the following is > >> documented and expected in the gs101 clock driver: > >> > >> Replace CLK_CON_GAT_CLKCMU with CLK_GOUT_CMU and gout_cmu > >> Replace CLK_CON_GAT_GATE_CLKCMU with CLK_GOUT_CMU and gout_cmu > >> > >> For gates remove _UID _BLK _IPCLKPORT and _RSTNSYNC > >> > > > > Doesn't it break existing gs101 device tree? > > No, compilation went fine at this point. The TOP gates are not used in > the device tree at this point. And since the bindings patch was just > applied I think we should fix it, so that we avoid name clashes as > described below (I found a clash with a gate from PERIC0). > Ok, in that case feel free to add: Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > > > >> The CMU TOP gate clock names missed to include the required "CMU" > >> differentiator which will cause name collisions with the gate clock names > >> of other clock units. Fix the TOP gate clock names and include "CMU" in > >> their name. > >> > >> Fixes: 0a910f160638 ("dt-bindings: clock: Add Google gs101 clock management unit bindings") > >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > >> --- > > > > (snip) > > Thanks for the review! > ta
On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Testing USI8 I2C with an eeprom revealed that when the USI8 leaf clock > is disabled it leads to the CMU_TOP PERIC0 IP gate clock disablement, > which then makes the system hang. To prevent this, mark > CLK_GOUT_CMU_PERIC0_IP as critical. Other clocks will be marked > accordingly when tested. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > drivers/clk/samsung/clk-gs101.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c > index 3d194520b05e..08d80fca9cd6 100644 > --- a/drivers/clk/samsung/clk-gs101.c > +++ b/drivers/clk/samsung/clk-gs101.c > @@ -1402,7 +1402,7 @@ static const struct samsung_gate_clock cmu_top_gate_clks[] __initconst = { > "mout_cmu_peric0_bus", CLK_CON_GAT_GATE_CLKCMU_PERIC0_BUS, > 21, 0, 0), > GATE(CLK_GOUT_CMU_PERIC0_IP, "gout_cmu_peric0_ip", "mout_cmu_peric0_ip", > - CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, 0, 0), > + CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, CLK_IS_CRITICAL, 0), This clock doesn't seem like a leaf clock. It's also not a bus clock. Leaving it always running makes the whole PERIC0 CMU clocked, which usually should be avoided. Is it possible that the system freezes because some other clock (which depends on peric0_ip) gets disabled as a consequence of disabling peric0_ip? Maybe it's some leaf clock which is not implemented yet in the clock driver? Just looks weird to me that the system hangs because of CMU IP clock disablement. It's usually something much more specific. > GATE(CLK_GOUT_CMU_PERIC1_BUS, "gout_cmu_peric1_bus", > "mout_cmu_peric1_bus", CLK_CON_GAT_GATE_CLKCMU_PERIC1_BUS, > 21, 0, 0), > -- > 2.43.0.472.g3155946c3a-goog >
On Thu, Dec 14, 2023 at 4:53 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Enable the eeprom found on the battery connector. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > .../boot/dts/exynos/google/gs101-oriole.dts | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts b/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts > index 4a71f752200d..11b299d21c5d 100644 > --- a/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts > +++ b/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts > @@ -63,6 +63,19 @@ &ext_200m { > clock-frequency = <200000000>; > }; > > +&hsi2c_8 { > + pinctrl-names = "default"; > + pinctrl-0 = <&hsi2c8_bus>; > + #address-cells = <1>; > + #size-cells = <0>; Not sure if those 4 above properties belong in board's dts or in SoC's dtsi. Krzysztof, what do you think? Other than that: Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > + status = "okay"; > + > + eeprom: eeprom@50 { > + compatible = "atmel,24c08"; > + reg = <0x50>; > + }; > +}; > + > &pinctrl_far_alive { > key_voldown: key-voldown-pins { > samsung,pins = "gpa7-3"; > @@ -99,6 +112,11 @@ &usi_uart { > status = "okay"; > }; > > +&usi8 { > + samsung,mode = <USI_V2_I2C>; > + status = "okay"; > +}; > + > &watchdog_cl0 { > timeout-sec = <30>; > status = "okay"; > -- > 2.43.0.472.g3155946c3a-goog >
On 12/14/23 15:37, Sam Protsenko wrote: > On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >> Testing USI8 I2C with an eeprom revealed that when the USI8 leaf clock >> is disabled it leads to the CMU_TOP PERIC0 IP gate clock disablement, >> which then makes the system hang. To prevent this, mark >> CLK_GOUT_CMU_PERIC0_IP as critical. Other clocks will be marked >> accordingly when tested. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> --- >> drivers/clk/samsung/clk-gs101.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c >> index 3d194520b05e..08d80fca9cd6 100644 >> --- a/drivers/clk/samsung/clk-gs101.c >> +++ b/drivers/clk/samsung/clk-gs101.c >> @@ -1402,7 +1402,7 @@ static const struct samsung_gate_clock cmu_top_gate_clks[] __initconst = { >> "mout_cmu_peric0_bus", CLK_CON_GAT_GATE_CLKCMU_PERIC0_BUS, >> 21, 0, 0), >> GATE(CLK_GOUT_CMU_PERIC0_IP, "gout_cmu_peric0_ip", "mout_cmu_peric0_ip", >> - CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, 0, 0), >> + CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, CLK_IS_CRITICAL, 0), > > This clock doesn't seem like a leaf clock. It's also not a bus clock. > Leaving it always running makes the whole PERIC0 CMU clocked, which > usually should be avoided. Is it possible that the system freezes > because some other clock (which depends on peric0_ip) gets disabled as > a consequence of disabling peric0_ip? Maybe it's some leaf clock which > is not implemented yet in the clock driver? Just looks weird to me > that the system hangs because of CMU IP clock disablement. It's > usually something much more specific. The system hang happened when I tested USI8 in I2C configuration with an eeprom. After the eeprom is read the leaf gate clock that gets disabled is the one on PERIC0 (CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK). I assume this leads to the CMU_TOP gate (CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP) disablement which makes the system hang. Either marking the CMU_TOP gate clock as critical (as I did in this patch) or marking the leaf PERIC0 gate clock as critical, gets rid of the system hang. Did I choose wrong? Thanks, ta > >> GATE(CLK_GOUT_CMU_PERIC1_BUS, "gout_cmu_peric1_bus", >> "mout_cmu_peric1_bus", CLK_CON_GAT_GATE_CLKCMU_PERIC1_BUS, >> 21, 0, 0), >> -- >> 2.43.0.472.g3155946c3a-goog >>
Hi Tudor, On Thu, 14 Dec 2023 at 10:52, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > The gs101 clock names are derived from the clock register names under > some certain rules. In particular, for the gate clocks the following is > documented and expected in the gs101 clock driver: > > Replace CLK_CON_GAT_CLKCMU with CLK_GOUT_CMU and gout_cmu > Replace CLK_CON_GAT_GATE_CLKCMU with CLK_GOUT_CMU and gout_cmu > > For gates remove _UID _BLK _IPCLKPORT and _RSTNSYNC > > The CMU TOP gate clock names missed to include the required "CMU" > differentiator which will cause name collisions with the gate clock names > of other clock units. Fix the TOP gate clock names and include "CMU" in > their name. > > Fixes: 0a910f160638 ("dt-bindings: clock: Add Google gs101 clock management unit bindings") > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> [...] Thanks for spotting this inconsistency on the cmu_top gates and fixing it! Reviewed-by: Peter Griffin <peter.griffin@linaro.org>
On Thu, Dec 14, 2023 at 10:01 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > > > On 12/14/23 15:37, Sam Protsenko wrote: > > On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >> > >> Testing USI8 I2C with an eeprom revealed that when the USI8 leaf clock > >> is disabled it leads to the CMU_TOP PERIC0 IP gate clock disablement, > >> which then makes the system hang. To prevent this, mark > >> CLK_GOUT_CMU_PERIC0_IP as critical. Other clocks will be marked > >> accordingly when tested. > >> > >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > >> --- > >> drivers/clk/samsung/clk-gs101.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c > >> index 3d194520b05e..08d80fca9cd6 100644 > >> --- a/drivers/clk/samsung/clk-gs101.c > >> +++ b/drivers/clk/samsung/clk-gs101.c > >> @@ -1402,7 +1402,7 @@ static const struct samsung_gate_clock cmu_top_gate_clks[] __initconst = { > >> "mout_cmu_peric0_bus", CLK_CON_GAT_GATE_CLKCMU_PERIC0_BUS, > >> 21, 0, 0), > >> GATE(CLK_GOUT_CMU_PERIC0_IP, "gout_cmu_peric0_ip", "mout_cmu_peric0_ip", > >> - CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, 0, 0), > >> + CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, CLK_IS_CRITICAL, 0), > > > > This clock doesn't seem like a leaf clock. It's also not a bus clock. > > Leaving it always running makes the whole PERIC0 CMU clocked, which > > usually should be avoided. Is it possible that the system freezes > > because some other clock (which depends on peric0_ip) gets disabled as > > a consequence of disabling peric0_ip? Maybe it's some leaf clock which > > is not implemented yet in the clock driver? Just looks weird to me > > that the system hangs because of CMU IP clock disablement. It's > > usually something much more specific. > > The system hang happened when I tested USI8 in I2C configuration with an > eeprom. After the eeprom is read the leaf gate clock that gets disabled > is the one on PERIC0 (CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK). I assume > this leads to the CMU_TOP gate (CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP) > disablement which makes the system hang. Either marking the CMU_TOP gate > clock as critical (as I did in this patch) or marking the leaf PERIC0 > gate clock as critical, gets rid of the system hang. Did I choose wrong? > Did you already implement 100% of clocks in CMU_PERIC0? If no, there is a chance some other leaf clock (which is not implemented yet in your driver) gets disabled as a result of PERIC0_IP disablement, which might actually lead to that hang you observe. Usually it's some meaningful leaf clock, e.g. GIC or interconnect clocks. Please check clk-exynos850.c driver for CLK_IS_CRITICAL and CLK_IGNORE_UNUSED flags and the corresponding comments I left there, maybe it'll give you more particular idea about what to look for. Yes, making the whole CMU always running without understanding why (i.e. because of which particular leaf clock) might not be the best way of handling this issue. I might be mistaken, but at least please check if you implemented all clocks for PERIC0 first and if making some meaningful leaf clock critical makes more sense. > Thanks, > ta > > > >> GATE(CLK_GOUT_CMU_PERIC1_BUS, "gout_cmu_peric1_bus", > >> "mout_cmu_peric1_bus", CLK_CON_GAT_GATE_CLKCMU_PERIC1_BUS, > >> 21, 0, 0), > >> -- > >> 2.43.0.472.g3155946c3a-goog > >>
On 12/14/23 16:09, Sam Protsenko wrote: > On Thu, Dec 14, 2023 at 10:01 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >> >> >> On 12/14/23 15:37, Sam Protsenko wrote: >>> On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >>>> >>>> Testing USI8 I2C with an eeprom revealed that when the USI8 leaf clock >>>> is disabled it leads to the CMU_TOP PERIC0 IP gate clock disablement, >>>> which then makes the system hang. To prevent this, mark >>>> CLK_GOUT_CMU_PERIC0_IP as critical. Other clocks will be marked >>>> accordingly when tested. >>>> >>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >>>> --- >>>> drivers/clk/samsung/clk-gs101.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c >>>> index 3d194520b05e..08d80fca9cd6 100644 >>>> --- a/drivers/clk/samsung/clk-gs101.c >>>> +++ b/drivers/clk/samsung/clk-gs101.c >>>> @@ -1402,7 +1402,7 @@ static const struct samsung_gate_clock cmu_top_gate_clks[] __initconst = { >>>> "mout_cmu_peric0_bus", CLK_CON_GAT_GATE_CLKCMU_PERIC0_BUS, >>>> 21, 0, 0), >>>> GATE(CLK_GOUT_CMU_PERIC0_IP, "gout_cmu_peric0_ip", "mout_cmu_peric0_ip", >>>> - CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, 0, 0), >>>> + CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, CLK_IS_CRITICAL, 0), >>> >>> This clock doesn't seem like a leaf clock. It's also not a bus clock. >>> Leaving it always running makes the whole PERIC0 CMU clocked, which >>> usually should be avoided. Is it possible that the system freezes >>> because some other clock (which depends on peric0_ip) gets disabled as >>> a consequence of disabling peric0_ip? Maybe it's some leaf clock which >>> is not implemented yet in the clock driver? Just looks weird to me >>> that the system hangs because of CMU IP clock disablement. It's >>> usually something much more specific. >> >> The system hang happened when I tested USI8 in I2C configuration with an >> eeprom. After the eeprom is read the leaf gate clock that gets disabled >> is the one on PERIC0 (CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK). I assume >> this leads to the CMU_TOP gate (CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP) >> disablement which makes the system hang. Either marking the CMU_TOP gate >> clock as critical (as I did in this patch) or marking the leaf PERIC0 >> gate clock as critical, gets rid of the system hang. Did I choose wrong? >> > > Did you already implement 100% of clocks in CMU_PERIC0? If no, there yes. > is a chance some other leaf clock (which is not implemented yet in > your driver) gets disabled as a result of PERIC0_IP disablement, which > might actually lead to that hang you observe. Usually it's some > meaningful leaf clock, e.g. GIC or interconnect clocks. Please check > clk-exynos850.c driver for CLK_IS_CRITICAL and CLK_IGNORE_UNUSED flags > and the corresponding comments I left there, maybe it'll give you more > particular idea about what to look for. Yes, making the whole CMU > always running without understanding why (i.e. because of which > particular leaf clock) might not be the best way of handling this because of CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK > issue. I might be mistaken, but at least please check if you > implemented all clocks for PERIC0 first and if making some meaningful > leaf clock critical makes more sense. > Thanks, ta
On Thu, Dec 14, 2023 at 10:15 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > > > On 12/14/23 16:09, Sam Protsenko wrote: > > On Thu, Dec 14, 2023 at 10:01 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >> > >> > >> > >> On 12/14/23 15:37, Sam Protsenko wrote: > >>> On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >>>> > >>>> Testing USI8 I2C with an eeprom revealed that when the USI8 leaf clock > >>>> is disabled it leads to the CMU_TOP PERIC0 IP gate clock disablement, > >>>> which then makes the system hang. To prevent this, mark > >>>> CLK_GOUT_CMU_PERIC0_IP as critical. Other clocks will be marked > >>>> accordingly when tested. > >>>> > >>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > >>>> --- > >>>> drivers/clk/samsung/clk-gs101.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c > >>>> index 3d194520b05e..08d80fca9cd6 100644 > >>>> --- a/drivers/clk/samsung/clk-gs101.c > >>>> +++ b/drivers/clk/samsung/clk-gs101.c > >>>> @@ -1402,7 +1402,7 @@ static const struct samsung_gate_clock cmu_top_gate_clks[] __initconst = { > >>>> "mout_cmu_peric0_bus", CLK_CON_GAT_GATE_CLKCMU_PERIC0_BUS, > >>>> 21, 0, 0), > >>>> GATE(CLK_GOUT_CMU_PERIC0_IP, "gout_cmu_peric0_ip", "mout_cmu_peric0_ip", > >>>> - CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, 0, 0), > >>>> + CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, CLK_IS_CRITICAL, 0), > >>> > >>> This clock doesn't seem like a leaf clock. It's also not a bus clock. > >>> Leaving it always running makes the whole PERIC0 CMU clocked, which > >>> usually should be avoided. Is it possible that the system freezes > >>> because some other clock (which depends on peric0_ip) gets disabled as > >>> a consequence of disabling peric0_ip? Maybe it's some leaf clock which > >>> is not implemented yet in the clock driver? Just looks weird to me > >>> that the system hangs because of CMU IP clock disablement. It's > >>> usually something much more specific. > >> > >> The system hang happened when I tested USI8 in I2C configuration with an > >> eeprom. After the eeprom is read the leaf gate clock that gets disabled > >> is the one on PERIC0 (CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK). I assume > >> this leads to the CMU_TOP gate (CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP) > >> disablement which makes the system hang. Either marking the CMU_TOP gate > >> clock as critical (as I did in this patch) or marking the leaf PERIC0 > >> gate clock as critical, gets rid of the system hang. Did I choose wrong? > >> > > > > Did you already implement 100% of clocks in CMU_PERIC0? If no, there > > yes. Ok. Are there any other CMUs (perhaps not implemented yet) which consume clocks from CMU_PERIC0, specifically PERIC0_IP clock or some clocks derived from it? If so, is there a chance some particular leaf clock in those CMUs actually renders the system frozen when disabled as a consequence of disabling PERIC0_IP, and would explain better why the freeze happens? For now I think it's ok to have that CLK_IS_CRITICAL flag here, because as you said you implemented all clocks in this CMU and neither of those looks like a critical one. But I'd advice to add a TODO comment saying it's probably a temporary solution before actual leaf clock which leads to freeze is identified (which probably resides in some other not implemented yet CMU). > > > is a chance some other leaf clock (which is not implemented yet in > > your driver) gets disabled as a result of PERIC0_IP disablement, which > > might actually lead to that hang you observe. Usually it's some > > meaningful leaf clock, e.g. GIC or interconnect clocks. Please check > > clk-exynos850.c driver for CLK_IS_CRITICAL and CLK_IGNORE_UNUSED flags > > and the corresponding comments I left there, maybe it'll give you more > > particular idea about what to look for. Yes, making the whole CMU > > always running without understanding why (i.e. because of which > > particular leaf clock) might not be the best way of handling this > > because of CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK That's not a root cause here. And I think PERIC0_IP is neither. > > > issue. I might be mistaken, but at least please check if you > > implemented all clocks for PERIC0 first and if making some meaningful > > leaf clock critical makes more sense. > > > > Thanks, > ta
On 14/12/2023 11:52, Tudor Ambarus wrote: > GS101 only allows 32-bit register accesses. When using 8-bit reg > accesses on gs101, a SError Interrupt is raised causing the system > unusable. > > Make reg-io-width a required property and expect for it a value of 4. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > Documentation/devicetree/bindings/serial/samsung_uart.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > index 133259ed3a34..cc896d7e2a3d 100644 > --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml > +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > @@ -143,6 +143,10 @@ allOf: > then: > required: > - samsung,uart-fifosize > + - reg-io-width > + properties: > + reg-io-width: > + const: 4 If all your ports are like this, then I say this is compatible-specific. Make it here "reg-io-width: false" and set in the driver proper type in s3c24xx_serial_init_port_default() (or new function). Although maybe let's first resolve discussion of next patch. Best regards, Krzysztof
On 14/12/2023 16:55, Sam Protsenko wrote: > On Thu, Dec 14, 2023 at 4:53 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >> Enable the eeprom found on the battery connector. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> --- >> .../boot/dts/exynos/google/gs101-oriole.dts | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts b/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts >> index 4a71f752200d..11b299d21c5d 100644 >> --- a/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts >> +++ b/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts >> @@ -63,6 +63,19 @@ &ext_200m { >> clock-frequency = <200000000>; >> }; >> >> +&hsi2c_8 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&hsi2c8_bus>; >> + #address-cells = <1>; >> + #size-cells = <0>; > > Not sure if those 4 above properties belong in board's dts or in SoC's > dtsi. Krzysztof, what do you think? The cells should be in DTSI, because you cannot have an enabled i2c bus without nodes in normal cases. The not-normal case is incomplete description, which does not happen here. The pinctrls I guess as well in DTSI, because you do not customize the pins in the DTS. IOW, if the pinctrl nodes are coming from shared pinctrl.DTSI, then pinctrl-0/names stay in DTSI as well. Best regards, Krzysztof
On 14/12/2023 11:52, Tudor Ambarus wrote: > The gs101 clock names are derived from the clock register names under > some certain rules. In particular, for the gate clocks the following is > documented and expected in the gs101 clock driver: > > Replace CLK_CON_GAT_CLKCMU with CLK_GOUT_CMU and gout_cmu > Replace CLK_CON_GAT_GATE_CLKCMU with CLK_GOUT_CMU and gout_cmu > > For gates remove _UID _BLK _IPCLKPORT and _RSTNSYNC I don't understand what it has to do with the bindings. > > The CMU TOP gate clock names missed to include the required "CMU" > differentiator which will cause name collisions with the gate clock names > of other clock units. Fix the TOP gate clock names and include "CMU" in > their name. Neither here. Clock names are not related to defines. > > Fixes: 0a910f160638 ("dt-bindings: clock: Add Google gs101 clock management unit bindings") > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > drivers/clk/samsung/clk-gs101.c | 167 ++++++++++++----------- > include/dt-bindings/clock/google,gs101.h | 144 +++++++++---------- I miss the point why bindings must be changed with driver. Really, guys, we are milling the first GS101 patches for entire cycle. Almost 3 months. The moment I merge bindings you tell me they are wrong. Few days after merging them. Best regards, Krzysztof
Hi, Krzysztof, On 12/15/23 08:13, Krzysztof Kozlowski wrote: > On 14/12/2023 11:52, Tudor Ambarus wrote: >> The gs101 clock names are derived from the clock register names under >> some certain rules. In particular, for the gate clocks the following is >> documented and expected in the gs101 clock driver: >> >> Replace CLK_CON_GAT_CLKCMU with CLK_GOUT_CMU and gout_cmu >> Replace CLK_CON_GAT_GATE_CLKCMU with CLK_GOUT_CMU and gout_cmu >> >> For gates remove _UID _BLK _IPCLKPORT and _RSTNSYNC > > I don't understand what it has to do with the bindings. > >> >> The CMU TOP gate clock names missed to include the required "CMU" >> differentiator which will cause name collisions with the gate clock names >> of other clock units. Fix the TOP gate clock names and include "CMU" in >> their name. > > Neither here. Clock names are not related to defines. > When saying "clock names" I meant the clock symbolic names that are defined in the bindings, the _id passed in GATE(_id, ) if you want. >> >> Fixes: 0a910f160638 ("dt-bindings: clock: Add Google gs101 clock management unit bindings") >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> --- >> drivers/clk/samsung/clk-gs101.c | 167 ++++++++++++----------- >> include/dt-bindings/clock/google,gs101.h | 144 +++++++++---------- > > I miss the point why bindings must be changed with driver. The clock symbolic names that are defined in the bindings file are used as IDs in the clock driver. Having the changes split per file will result in compilation errors breaking bisect. > > Really, guys, we are milling the first GS101 patches for entire cycle. > Almost 3 months. The moment I merge bindings you tell me they are wrong. > Few days after merging them. I apologize. It happens when we work in parallel. The clock symbolic names were mangled just in v6. It was considered that the clock names used in the datasheet are too long and the dt becomes unreadable. I just recently updated the peric0 clock symbolic names according to the clock symbolic name mangling strategy, that's why we spot the inconsistency and the symbolic name collision so late. Cheers, ta
On 15/12/2023 11:23, Tudor Ambarus wrote: > Hi, Krzysztof, > > On 12/15/23 08:13, Krzysztof Kozlowski wrote: >> On 14/12/2023 11:52, Tudor Ambarus wrote: >>> The gs101 clock names are derived from the clock register names under >>> some certain rules. In particular, for the gate clocks the following is >>> documented and expected in the gs101 clock driver: >>> >>> Replace CLK_CON_GAT_CLKCMU with CLK_GOUT_CMU and gout_cmu >>> Replace CLK_CON_GAT_GATE_CLKCMU with CLK_GOUT_CMU and gout_cmu >>> >>> For gates remove _UID _BLK _IPCLKPORT and _RSTNSYNC >> >> I don't understand what it has to do with the bindings. >> >>> >>> The CMU TOP gate clock names missed to include the required "CMU" >>> differentiator which will cause name collisions with the gate clock names >>> of other clock units. Fix the TOP gate clock names and include "CMU" in >>> their name. >> >> Neither here. Clock names are not related to defines. >> > > When saying "clock names" I meant the clock symbolic names that are > defined in the bindings, the _id passed in GATE(_id, ) if you want. Please re-phrase the commit message to say that you need to rename the defines in the bindings headers. If you change anything else, like clock names, then it should be separate patch. Best regards, Krzysztof
On 15/12/2023 20:24, Krzysztof Kozlowski wrote: > On 15/12/2023 11:23, Tudor Ambarus wrote: >> Hi, Krzysztof, >> >> On 12/15/23 08:13, Krzysztof Kozlowski wrote: >>> On 14/12/2023 11:52, Tudor Ambarus wrote: >>>> The gs101 clock names are derived from the clock register names under >>>> some certain rules. In particular, for the gate clocks the following is >>>> documented and expected in the gs101 clock driver: >>>> >>>> Replace CLK_CON_GAT_CLKCMU with CLK_GOUT_CMU and gout_cmu >>>> Replace CLK_CON_GAT_GATE_CLKCMU with CLK_GOUT_CMU and gout_cmu >>>> >>>> For gates remove _UID _BLK _IPCLKPORT and _RSTNSYNC >>> >>> I don't understand what it has to do with the bindings. >>> >>>> >>>> The CMU TOP gate clock names missed to include the required "CMU" >>>> differentiator which will cause name collisions with the gate clock names >>>> of other clock units. Fix the TOP gate clock names and include "CMU" in >>>> their name. >>> >>> Neither here. Clock names are not related to defines. >>> >> >> When saying "clock names" I meant the clock symbolic names that are >> defined in the bindings, the _id passed in GATE(_id, ) if you want. > > Please re-phrase the commit message to say that you need to rename the > defines in the bindings headers. If you change anything else, like clock > names, then it should be separate patch. I forgot: You can also respin this patch separately, as soon as possible, because it has to go this cycle. Best regards, Krzysztof
On 12/15/23 19:25, Krzysztof Kozlowski wrote: > On 15/12/2023 20:24, Krzysztof Kozlowski wrote: >> On 15/12/2023 11:23, Tudor Ambarus wrote: >>> Hi, Krzysztof, >>> >>> On 12/15/23 08:13, Krzysztof Kozlowski wrote: >>>> On 14/12/2023 11:52, Tudor Ambarus wrote: >>>>> The gs101 clock names are derived from the clock register names under >>>>> some certain rules. In particular, for the gate clocks the following is >>>>> documented and expected in the gs101 clock driver: >>>>> >>>>> Replace CLK_CON_GAT_CLKCMU with CLK_GOUT_CMU and gout_cmu >>>>> Replace CLK_CON_GAT_GATE_CLKCMU with CLK_GOUT_CMU and gout_cmu >>>>> >>>>> For gates remove _UID _BLK _IPCLKPORT and _RSTNSYNC >>>> >>>> I don't understand what it has to do with the bindings. >>>> >>>>> >>>>> The CMU TOP gate clock names missed to include the required "CMU" >>>>> differentiator which will cause name collisions with the gate clock names >>>>> of other clock units. Fix the TOP gate clock names and include "CMU" in >>>>> their name. >>>> >>>> Neither here. Clock names are not related to defines. >>>> >>> >>> When saying "clock names" I meant the clock symbolic names that are >>> defined in the bindings, the _id passed in GATE(_id, ) if you want. >> >> Please re-phrase the commit message to say that you need to rename the >> defines in the bindings headers. If you change anything else, like clock >> names, then it should be separate patch. > > I forgot: > You can also respin this patch separately, as soon as possible, because > it has to go this cycle. > Sent here: https://lore.kernel.org/linux-arm-kernel/20231218064333.479885-1-tudor.ambarus@linaro.org/ Thanks, ta
Hi, Sam! On 12/14/23 16:43, Sam Protsenko wrote: > On Thu, Dec 14, 2023 at 10:15 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >> >> >> On 12/14/23 16:09, Sam Protsenko wrote: >>> On Thu, Dec 14, 2023 at 10:01 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >>>> >>>> >>>> >>>> On 12/14/23 15:37, Sam Protsenko wrote: >>>>> On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >>>>>> >>>>>> Testing USI8 I2C with an eeprom revealed that when the USI8 leaf clock >>>>>> is disabled it leads to the CMU_TOP PERIC0 IP gate clock disablement, >>>>>> which then makes the system hang. To prevent this, mark >>>>>> CLK_GOUT_CMU_PERIC0_IP as critical. Other clocks will be marked >>>>>> accordingly when tested. >>>>>> >>>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >>>>>> --- >>>>>> drivers/clk/samsung/clk-gs101.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c >>>>>> index 3d194520b05e..08d80fca9cd6 100644 >>>>>> --- a/drivers/clk/samsung/clk-gs101.c >>>>>> +++ b/drivers/clk/samsung/clk-gs101.c >>>>>> @@ -1402,7 +1402,7 @@ static const struct samsung_gate_clock cmu_top_gate_clks[] __initconst = { >>>>>> "mout_cmu_peric0_bus", CLK_CON_GAT_GATE_CLKCMU_PERIC0_BUS, >>>>>> 21, 0, 0), >>>>>> GATE(CLK_GOUT_CMU_PERIC0_IP, "gout_cmu_peric0_ip", "mout_cmu_peric0_ip", >>>>>> - CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, 0, 0), >>>>>> + CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, CLK_IS_CRITICAL, 0), >>>>> >>>>> This clock doesn't seem like a leaf clock. It's also not a bus clock. >>>>> Leaving it always running makes the whole PERIC0 CMU clocked, which >>>>> usually should be avoided. Is it possible that the system freezes >>>>> because some other clock (which depends on peric0_ip) gets disabled as >>>>> a consequence of disabling peric0_ip? Maybe it's some leaf clock which >>>>> is not implemented yet in the clock driver? Just looks weird to me >>>>> that the system hangs because of CMU IP clock disablement. It's >>>>> usually something much more specific. >>>> >>>> The system hang happened when I tested USI8 in I2C configuration with an >>>> eeprom. After the eeprom is read the leaf gate clock that gets disabled >>>> is the one on PERIC0 (CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK). I assume >>>> this leads to the CMU_TOP gate (CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP) >>>> disablement which makes the system hang. Either marking the CMU_TOP gate >>>> clock as critical (as I did in this patch) or marking the leaf PERIC0 >>>> gate clock as critical, gets rid of the system hang. Did I choose wrong? >>>> >>> >>> Did you already implement 100% of clocks in CMU_PERIC0? If no, there >> >> yes. I checked again all the clocks. I implemented all but one, the one defined by the CLK_CON_BUF_CLKBUF_PERIC0_IP register. Unfortunately I don't have any reference on how it should be defined so I won't touch it yet. But I have some good news too, see below. > > Ok. Are there any other CMUs (perhaps not implemented yet) which > consume clocks from CMU_PERIC0, specifically PERIC0_IP clock or some > clocks derived from it? If so, is there a chance some particular leaf > clock in those CMUs actually renders the system frozen when disabled > as a consequence of disabling PERIC0_IP, and would explain better why > the freeze happens? > > For now I think it's ok to have that CLK_IS_CRITICAL flag here, > because as you said you implemented all clocks in this CMU and neither > of those looks like a critical one. But I'd advice to add a TODO > comment saying it's probably a temporary solution before actual leaf > clock which leads to freeze is identified (which probably resides in > some other not implemented yet CMU). > >> >>> is a chance some other leaf clock (which is not implemented yet in >>> your driver) gets disabled as a result of PERIC0_IP disablement, which >>> might actually lead to that hang you observe. Usually it's some >>> meaningful leaf clock, e.g. GIC or interconnect clocks. Please check >>> clk-exynos850.c driver for CLK_IS_CRITICAL and CLK_IGNORE_UNUSED flags >>> and the corresponding comments I left there, maybe it'll give you more >>> particular idea about what to look for. Yes, making the whole CMU >>> always running without understanding why (i.e. because of which >>> particular leaf clock) might not be the best way of handling this >> >> because of CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK > > That's not a root cause here. And I think PERIC0_IP is neither. > you were right! >> >>> issue. I might be mistaken, but at least please check if you >>> implemented all clocks for PERIC0 first and if making some meaningful >>> leaf clock critical makes more sense. >>> I determined which leaf clocks shall be marked as critical. I enabled the debugfs clock write access. Then I made sure that the parents of the PERIC0 CMU have at least one user so that they don't get disabled after an enable-disable sequence on a leaf clock. The I took all the PERIC0 gate clocks and enabled and disabled them one by one. Whichever hang the system when the clock was disabled was marked as critical. The list of critical leaf clocks is as following: "gout_peric0_peric0_cmu_peric0_pclk", "gout_peric0_lhm_axi_p_peric0_i_clk", "gout_peric0_peric0_top1_ipclk_0", "gout_peric0_peric0_top1_pclk_0". I'll update v2 with this instead. Thanks for the help, Sam! Cheers, ta
On Tue, Dec 19, 2023 at 10:47 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Hi, Sam! > > On 12/14/23 16:43, Sam Protsenko wrote: > > On Thu, Dec 14, 2023 at 10:15 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >> > >> > >> > >> On 12/14/23 16:09, Sam Protsenko wrote: > >>> On Thu, Dec 14, 2023 at 10:01 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >>>> > >>>> > >>>> > >>>> On 12/14/23 15:37, Sam Protsenko wrote: > >>>>> On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >>>>>> > >>>>>> Testing USI8 I2C with an eeprom revealed that when the USI8 leaf clock > >>>>>> is disabled it leads to the CMU_TOP PERIC0 IP gate clock disablement, > >>>>>> which then makes the system hang. To prevent this, mark > >>>>>> CLK_GOUT_CMU_PERIC0_IP as critical. Other clocks will be marked > >>>>>> accordingly when tested. > >>>>>> > >>>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > >>>>>> --- > >>>>>> drivers/clk/samsung/clk-gs101.c | 2 +- > >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c > >>>>>> index 3d194520b05e..08d80fca9cd6 100644 > >>>>>> --- a/drivers/clk/samsung/clk-gs101.c > >>>>>> +++ b/drivers/clk/samsung/clk-gs101.c > >>>>>> @@ -1402,7 +1402,7 @@ static const struct samsung_gate_clock cmu_top_gate_clks[] __initconst = { > >>>>>> "mout_cmu_peric0_bus", CLK_CON_GAT_GATE_CLKCMU_PERIC0_BUS, > >>>>>> 21, 0, 0), > >>>>>> GATE(CLK_GOUT_CMU_PERIC0_IP, "gout_cmu_peric0_ip", "mout_cmu_peric0_ip", > >>>>>> - CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, 0, 0), > >>>>>> + CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, CLK_IS_CRITICAL, 0), > >>>>> > >>>>> This clock doesn't seem like a leaf clock. It's also not a bus clock. > >>>>> Leaving it always running makes the whole PERIC0 CMU clocked, which > >>>>> usually should be avoided. Is it possible that the system freezes > >>>>> because some other clock (which depends on peric0_ip) gets disabled as > >>>>> a consequence of disabling peric0_ip? Maybe it's some leaf clock which > >>>>> is not implemented yet in the clock driver? Just looks weird to me > >>>>> that the system hangs because of CMU IP clock disablement. It's > >>>>> usually something much more specific. > >>>> > >>>> The system hang happened when I tested USI8 in I2C configuration with an > >>>> eeprom. After the eeprom is read the leaf gate clock that gets disabled > >>>> is the one on PERIC0 (CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK). I assume > >>>> this leads to the CMU_TOP gate (CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP) > >>>> disablement which makes the system hang. Either marking the CMU_TOP gate > >>>> clock as critical (as I did in this patch) or marking the leaf PERIC0 > >>>> gate clock as critical, gets rid of the system hang. Did I choose wrong? > >>>> > >>> > >>> Did you already implement 100% of clocks in CMU_PERIC0? If no, there > >> > >> yes. > > I checked again all the clocks. I implemented all but one, the one > defined by the CLK_CON_BUF_CLKBUF_PERIC0_IP register. Unfortunately I > don't have any reference on how it should be defined so I won't touch it > yet. But I have some good news too, see below. > > > > > Ok. Are there any other CMUs (perhaps not implemented yet) which > > consume clocks from CMU_PERIC0, specifically PERIC0_IP clock or some > > clocks derived from it? If so, is there a chance some particular leaf > > clock in those CMUs actually renders the system frozen when disabled > > as a consequence of disabling PERIC0_IP, and would explain better why > > the freeze happens? > > > > For now I think it's ok to have that CLK_IS_CRITICAL flag here, > > because as you said you implemented all clocks in this CMU and neither > > of those looks like a critical one. But I'd advice to add a TODO > > comment saying it's probably a temporary solution before actual leaf > > clock which leads to freeze is identified (which probably resides in > > some other not implemented yet CMU). > > > >> > >>> is a chance some other leaf clock (which is not implemented yet in > >>> your driver) gets disabled as a result of PERIC0_IP disablement, which > >>> might actually lead to that hang you observe. Usually it's some > >>> meaningful leaf clock, e.g. GIC or interconnect clocks. Please check > >>> clk-exynos850.c driver for CLK_IS_CRITICAL and CLK_IGNORE_UNUSED flags > >>> and the corresponding comments I left there, maybe it'll give you more > >>> particular idea about what to look for. Yes, making the whole CMU > >>> always running without understanding why (i.e. because of which > >>> particular leaf clock) might not be the best way of handling this > >> > >> because of CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK > > > > That's not a root cause here. And I think PERIC0_IP is neither. > > > > you were right! > >> > >>> issue. I might be mistaken, but at least please check if you > >>> implemented all clocks for PERIC0 first and if making some meaningful > >>> leaf clock critical makes more sense. > >>> > > I determined which leaf clocks shall be marked as critical. I enabled > the debugfs clock write access. Then I made sure that the parents of the > PERIC0 CMU have at least one user so that they don't get disabled after > an enable-disable sequence on a leaf clock. The I took all the PERIC0 > gate clocks and enabled and disabled them one by one. Whichever hang the > system when the clock was disabled was marked as critical. The list of > critical leaf clocks is as following: > Nice! I used somehow similar procedure for clk-exynos850, doing basically the same thing, but in core clock driver code. > "gout_peric0_peric0_cmu_peric0_pclk", > "gout_peric0_lhm_axi_p_peric0_i_clk", > "gout_peric0_peric0_top1_ipclk_0", > "gout_peric0_peric0_top1_pclk_0". > > I'll update v2 with this instead. Thanks for the help, Sam! Glad you weren't discouraged by my meticulousness :) In clk-exynos850 I usually used CLK_IGNORE_UNUSED for clocks like XXX_CMU_XXX (in your case it's PERIC0_CMU_PERIC0), with a corresponding comment. Those clocks usually can be used to disable the bus clock for corresponding CMU IP-core (in your case CMU_PERIC0), which makes it impossible to access the registers from that CMU block, as its register interface is not clocked anymore. Guess I saw something similar in Exynos5433 or Exynos7 clk drivers, or maybe Sylwester or Krzysztof told me to do so -- don't really remember. For AXI clock it also seems logical to keep it running (AXI bus might be used for GIC and memory). But again, maybe CLK_IGNORE_UNUSED flag would be more appropriate that CLK_IS_CRITICAL? For the last two clocks -- it's hard to tell what exactly they do. Is TOP1 some other CMU or block name, and is there any further users for those clocks? Anyways, if you are working on v2, please consider doing next two things while at it: 1. For each critical clock: add corresponding comment explaining why it's marked so 2. Consider using CLK_IGNORE_UNUSED instead of CLK_IS_CRITICAL when appropriate; both have their use in different cases Btw, if you check other Exynos clk drivers, there is a lot of examples for flags like those. > Cheers, > ta
Hi, Sam! On 12/19/23 17:31, Sam Protsenko wrote: > On Tue, Dec 19, 2023 at 10:47 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >> Hi, Sam! >> >> On 12/14/23 16:43, Sam Protsenko wrote: >>> On Thu, Dec 14, 2023 at 10:15 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >>>> >>>> >>>> >>>> On 12/14/23 16:09, Sam Protsenko wrote: >>>>> On Thu, Dec 14, 2023 at 10:01 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 12/14/23 15:37, Sam Protsenko wrote: >>>>>>> On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >>>>>>>> >>>>>>>> Testing USI8 I2C with an eeprom revealed that when the USI8 leaf clock >>>>>>>> is disabled it leads to the CMU_TOP PERIC0 IP gate clock disablement, >>>>>>>> which then makes the system hang. To prevent this, mark >>>>>>>> CLK_GOUT_CMU_PERIC0_IP as critical. Other clocks will be marked >>>>>>>> accordingly when tested. >>>>>>>> >>>>>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >>>>>>>> --- >>>>>>>> drivers/clk/samsung/clk-gs101.c | 2 +- >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c >>>>>>>> index 3d194520b05e..08d80fca9cd6 100644 >>>>>>>> --- a/drivers/clk/samsung/clk-gs101.c >>>>>>>> +++ b/drivers/clk/samsung/clk-gs101.c >>>>>>>> @@ -1402,7 +1402,7 @@ static const struct samsung_gate_clock cmu_top_gate_clks[] __initconst = { >>>>>>>> "mout_cmu_peric0_bus", CLK_CON_GAT_GATE_CLKCMU_PERIC0_BUS, >>>>>>>> 21, 0, 0), >>>>>>>> GATE(CLK_GOUT_CMU_PERIC0_IP, "gout_cmu_peric0_ip", "mout_cmu_peric0_ip", >>>>>>>> - CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, 0, 0), >>>>>>>> + CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, CLK_IS_CRITICAL, 0), >>>>>>> >>>>>>> This clock doesn't seem like a leaf clock. It's also not a bus clock. >>>>>>> Leaving it always running makes the whole PERIC0 CMU clocked, which >>>>>>> usually should be avoided. Is it possible that the system freezes >>>>>>> because some other clock (which depends on peric0_ip) gets disabled as >>>>>>> a consequence of disabling peric0_ip? Maybe it's some leaf clock which >>>>>>> is not implemented yet in the clock driver? Just looks weird to me >>>>>>> that the system hangs because of CMU IP clock disablement. It's >>>>>>> usually something much more specific. >>>>>> >>>>>> The system hang happened when I tested USI8 in I2C configuration with an >>>>>> eeprom. After the eeprom is read the leaf gate clock that gets disabled >>>>>> is the one on PERIC0 (CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK). I assume >>>>>> this leads to the CMU_TOP gate (CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP) >>>>>> disablement which makes the system hang. Either marking the CMU_TOP gate >>>>>> clock as critical (as I did in this patch) or marking the leaf PERIC0 >>>>>> gate clock as critical, gets rid of the system hang. Did I choose wrong? >>>>>> >>>>> >>>>> Did you already implement 100% of clocks in CMU_PERIC0? If no, there >>>> >>>> yes. >> >> I checked again all the clocks. I implemented all but one, the one >> defined by the CLK_CON_BUF_CLKBUF_PERIC0_IP register. Unfortunately I >> don't have any reference on how it should be defined so I won't touch it >> yet. But I have some good news too, see below. >> >>> >>> Ok. Are there any other CMUs (perhaps not implemented yet) which >>> consume clocks from CMU_PERIC0, specifically PERIC0_IP clock or some >>> clocks derived from it? If so, is there a chance some particular leaf >>> clock in those CMUs actually renders the system frozen when disabled >>> as a consequence of disabling PERIC0_IP, and would explain better why >>> the freeze happens? >>> >>> For now I think it's ok to have that CLK_IS_CRITICAL flag here, >>> because as you said you implemented all clocks in this CMU and neither >>> of those looks like a critical one. But I'd advice to add a TODO >>> comment saying it's probably a temporary solution before actual leaf >>> clock which leads to freeze is identified (which probably resides in >>> some other not implemented yet CMU). >>> >>>> >>>>> is a chance some other leaf clock (which is not implemented yet in >>>>> your driver) gets disabled as a result of PERIC0_IP disablement, which >>>>> might actually lead to that hang you observe. Usually it's some >>>>> meaningful leaf clock, e.g. GIC or interconnect clocks. Please check >>>>> clk-exynos850.c driver for CLK_IS_CRITICAL and CLK_IGNORE_UNUSED flags >>>>> and the corresponding comments I left there, maybe it'll give you more >>>>> particular idea about what to look for. Yes, making the whole CMU >>>>> always running without understanding why (i.e. because of which >>>>> particular leaf clock) might not be the best way of handling this >>>> >>>> because of CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK >>> >>> That's not a root cause here. And I think PERIC0_IP is neither. >>> >> >> you were right! >>>> >>>>> issue. I might be mistaken, but at least please check if you >>>>> implemented all clocks for PERIC0 first and if making some meaningful >>>>> leaf clock critical makes more sense. >>>>> >> >> I determined which leaf clocks shall be marked as critical. I enabled >> the debugfs clock write access. Then I made sure that the parents of the >> PERIC0 CMU have at least one user so that they don't get disabled after >> an enable-disable sequence on a leaf clock. The I took all the PERIC0 >> gate clocks and enabled and disabled them one by one. Whichever hang the >> system when the clock was disabled was marked as critical. The list of >> critical leaf clocks is as following: >> > > Nice! I used somehow similar procedure for clk-exynos850, doing > basically the same thing, but in core clock driver code. > >> "gout_peric0_peric0_cmu_peric0_pclk", >> "gout_peric0_lhm_axi_p_peric0_i_clk", >> "gout_peric0_peric0_top1_ipclk_0", >> "gout_peric0_peric0_top1_pclk_0". >> >> I'll update v2 with this instead. Thanks for the help, Sam! > > Glad you weren't discouraged by my meticulousness :) In clk-exynos850 > I usually used CLK_IGNORE_UNUSED for clocks like XXX_CMU_XXX (in your > case it's PERIC0_CMU_PERIC0), with a corresponding comment. Those > clocks usually can be used to disable the bus clock for corresponding > CMU IP-core (in your case CMU_PERIC0), which makes it impossible to > access the registers from that CMU block, as its register interface is > not clocked anymore. Guess I saw something similar in Exynos5433 or > Exynos7 clk drivers, or maybe Sylwester or Krzysztof told me to do so > -- don't really remember. For AXI clock it also seems logical to keep > it running (AXI bus might be used for GIC and memory). But again, > maybe CLK_IGNORE_UNUSED flag would be more appropriate that > CLK_IS_CRITICAL? For the last two clocks -- it's hard to tell what > exactly they do. Is TOP1 some other CMU or block name, and is there > any further users for those clocks? > > Anyways, if you are working on v2, please consider doing next two > things while at it: > > 1. For each critical clock: add corresponding comment explaining why > it's marked so Will do. > 2. Consider using CLK_IGNORE_UNUSED instead of CLK_IS_CRITICAL when > appropriate; both have their use in different cases > > Btw, if you check other Exynos clk drivers, there is a lot of examples > for flags like those. > Thanks for the feedback, it's educative. I played a little with the clk debugfs and I think all should be marked as critical. What I did was to make sure that their parents are enabled already and then I enabled and disabled each. Each time I disabled one of them the system hung. Thus in case they will be used, if one disable them on an error path, it will hang the system. We can't disable them at suspend either. Thus I propose to keep them as critical. Thanks! ta
On Wed, Dec 20, 2023 at 8:22 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Hi, Sam! > > On 12/19/23 17:31, Sam Protsenko wrote: > > On Tue, Dec 19, 2023 at 10:47 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >> > >> Hi, Sam! > >> > >> On 12/14/23 16:43, Sam Protsenko wrote: > >>> On Thu, Dec 14, 2023 at 10:15 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >>>> > >>>> > >>>> > >>>> On 12/14/23 16:09, Sam Protsenko wrote: > >>>>> On Thu, Dec 14, 2023 at 10:01 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On 12/14/23 15:37, Sam Protsenko wrote: > >>>>>>> On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >>>>>>>> > >>>>>>>> Testing USI8 I2C with an eeprom revealed that when the USI8 leaf clock > >>>>>>>> is disabled it leads to the CMU_TOP PERIC0 IP gate clock disablement, > >>>>>>>> which then makes the system hang. To prevent this, mark > >>>>>>>> CLK_GOUT_CMU_PERIC0_IP as critical. Other clocks will be marked > >>>>>>>> accordingly when tested. > >>>>>>>> > >>>>>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > >>>>>>>> --- > >>>>>>>> drivers/clk/samsung/clk-gs101.c | 2 +- > >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>>>> > >>>>>>>> diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c > >>>>>>>> index 3d194520b05e..08d80fca9cd6 100644 > >>>>>>>> --- a/drivers/clk/samsung/clk-gs101.c > >>>>>>>> +++ b/drivers/clk/samsung/clk-gs101.c > >>>>>>>> @@ -1402,7 +1402,7 @@ static const struct samsung_gate_clock cmu_top_gate_clks[] __initconst = { > >>>>>>>> "mout_cmu_peric0_bus", CLK_CON_GAT_GATE_CLKCMU_PERIC0_BUS, > >>>>>>>> 21, 0, 0), > >>>>>>>> GATE(CLK_GOUT_CMU_PERIC0_IP, "gout_cmu_peric0_ip", "mout_cmu_peric0_ip", > >>>>>>>> - CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, 0, 0), > >>>>>>>> + CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP, 21, CLK_IS_CRITICAL, 0), > >>>>>>> > >>>>>>> This clock doesn't seem like a leaf clock. It's also not a bus clock. > >>>>>>> Leaving it always running makes the whole PERIC0 CMU clocked, which > >>>>>>> usually should be avoided. Is it possible that the system freezes > >>>>>>> because some other clock (which depends on peric0_ip) gets disabled as > >>>>>>> a consequence of disabling peric0_ip? Maybe it's some leaf clock which > >>>>>>> is not implemented yet in the clock driver? Just looks weird to me > >>>>>>> that the system hangs because of CMU IP clock disablement. It's > >>>>>>> usually something much more specific. > >>>>>> > >>>>>> The system hang happened when I tested USI8 in I2C configuration with an > >>>>>> eeprom. After the eeprom is read the leaf gate clock that gets disabled > >>>>>> is the one on PERIC0 (CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK). I assume > >>>>>> this leads to the CMU_TOP gate (CLK_CON_GAT_GATE_CLKCMU_PERIC0_IP) > >>>>>> disablement which makes the system hang. Either marking the CMU_TOP gate > >>>>>> clock as critical (as I did in this patch) or marking the leaf PERIC0 > >>>>>> gate clock as critical, gets rid of the system hang. Did I choose wrong? > >>>>>> > >>>>> > >>>>> Did you already implement 100% of clocks in CMU_PERIC0? If no, there > >>>> > >>>> yes. > >> > >> I checked again all the clocks. I implemented all but one, the one > >> defined by the CLK_CON_BUF_CLKBUF_PERIC0_IP register. Unfortunately I > >> don't have any reference on how it should be defined so I won't touch it > >> yet. But I have some good news too, see below. > >> > >>> > >>> Ok. Are there any other CMUs (perhaps not implemented yet) which > >>> consume clocks from CMU_PERIC0, specifically PERIC0_IP clock or some > >>> clocks derived from it? If so, is there a chance some particular leaf > >>> clock in those CMUs actually renders the system frozen when disabled > >>> as a consequence of disabling PERIC0_IP, and would explain better why > >>> the freeze happens? > >>> > >>> For now I think it's ok to have that CLK_IS_CRITICAL flag here, > >>> because as you said you implemented all clocks in this CMU and neither > >>> of those looks like a critical one. But I'd advice to add a TODO > >>> comment saying it's probably a temporary solution before actual leaf > >>> clock which leads to freeze is identified (which probably resides in > >>> some other not implemented yet CMU). > >>> > >>>> > >>>>> is a chance some other leaf clock (which is not implemented yet in > >>>>> your driver) gets disabled as a result of PERIC0_IP disablement, which > >>>>> might actually lead to that hang you observe. Usually it's some > >>>>> meaningful leaf clock, e.g. GIC or interconnect clocks. Please check > >>>>> clk-exynos850.c driver for CLK_IS_CRITICAL and CLK_IGNORE_UNUSED flags > >>>>> and the corresponding comments I left there, maybe it'll give you more > >>>>> particular idea about what to look for. Yes, making the whole CMU > >>>>> always running without understanding why (i.e. because of which > >>>>> particular leaf clock) might not be the best way of handling this > >>>> > >>>> because of CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK > >>> > >>> That's not a root cause here. And I think PERIC0_IP is neither. > >>> > >> > >> you were right! > >>>> > >>>>> issue. I might be mistaken, but at least please check if you > >>>>> implemented all clocks for PERIC0 first and if making some meaningful > >>>>> leaf clock critical makes more sense. > >>>>> > >> > >> I determined which leaf clocks shall be marked as critical. I enabled > >> the debugfs clock write access. Then I made sure that the parents of the > >> PERIC0 CMU have at least one user so that they don't get disabled after > >> an enable-disable sequence on a leaf clock. The I took all the PERIC0 > >> gate clocks and enabled and disabled them one by one. Whichever hang the > >> system when the clock was disabled was marked as critical. The list of > >> critical leaf clocks is as following: > >> > > > > Nice! I used somehow similar procedure for clk-exynos850, doing > > basically the same thing, but in core clock driver code. > > > >> "gout_peric0_peric0_cmu_peric0_pclk", > >> "gout_peric0_lhm_axi_p_peric0_i_clk", > >> "gout_peric0_peric0_top1_ipclk_0", > >> "gout_peric0_peric0_top1_pclk_0". > >> > >> I'll update v2 with this instead. Thanks for the help, Sam! > > > > Glad you weren't discouraged by my meticulousness :) In clk-exynos850 > > I usually used CLK_IGNORE_UNUSED for clocks like XXX_CMU_XXX (in your > > case it's PERIC0_CMU_PERIC0), with a corresponding comment. Those > > clocks usually can be used to disable the bus clock for corresponding > > CMU IP-core (in your case CMU_PERIC0), which makes it impossible to > > access the registers from that CMU block, as its register interface is > > not clocked anymore. Guess I saw something similar in Exynos5433 or > > Exynos7 clk drivers, or maybe Sylwester or Krzysztof told me to do so > > -- don't really remember. For AXI clock it also seems logical to keep > > it running (AXI bus might be used for GIC and memory). But again, > > maybe CLK_IGNORE_UNUSED flag would be more appropriate that > > CLK_IS_CRITICAL? For the last two clocks -- it's hard to tell what > > exactly they do. Is TOP1 some other CMU or block name, and is there > > any further users for those clocks? > > > > Anyways, if you are working on v2, please consider doing next two > > things while at it: > > > > 1. For each critical clock: add corresponding comment explaining why > > it's marked so > > Will do. > > > 2. Consider using CLK_IGNORE_UNUSED instead of CLK_IS_CRITICAL when > > appropriate; both have their use in different cases > > > > Btw, if you check other Exynos clk drivers, there is a lot of examples > > for flags like those. > > > Thanks for the feedback, it's educative. > > I played a little with the clk debugfs and I think all should be marked > as critical. What I did was to make sure that their parents are enabled > already and then I enabled and disabled each. Each time I disabled one > of them the system hung. Thus in case they will be used, if one disable > them on an error path, it will hang the system. We can't disable them at > suspend either. Thus I propose to keep them as critical. > Do you see those clocks potentially used by some actual consumers in future? If no, maybe CLK_IGNORE_UNUSED is enough (just to make sure the core clock framework won't disable those during the clocks initialization)? Anyway, I don't have any strong preferences in this case. If you think CLK_IS_CRITICAL is better in this case, I'd say go for it. Also, on a bit different note: please make sure there is no "clk_ignore_unused" param in your kernel cmdline (e.g. passed from the bootloader via dts). The clock driver should be functional without that param. Though it might take some additional work. > Thanks! > ta
Add support for PERIC0 clocks. Use them for USI in serial and I2C configurations. Tested the serial at different baudrates (115200, 1M, 3M) and the I2C with an at24 eeprom, all went fine. Apart of the DT and defconfig changes, the patch set spans through the tty and clk subsystems. The expectation is that Krzysztof will apply the whole series through the Samsung SoC tree. If the tty and clk subsystem maintainers can give an acked-by or reviewed-by on the relevant patches that would be most appreciated! Thanks! ta Tudor Ambarus (13): dt-bindings: clock: google,gs101: fix CMU_TOP gate clock names dt-bindings: clock: google,gs101-clock: add PERIC0 clock management unit dt-bindings: i2c: exynos5: add google,gs101-hsi2c compatible dt-bindings: serial: samsung: gs101: make reg-io-width required property tty: serial: samsung: add gs101 earlycon support clk: samsung: gs101: add support for cmu_peric0 clk: samsung: gs101: mark PERIC0 IP TOP gate clock as critical arm64: dts: exynos: gs101: enable cmu-peric0 clock controller arm64: dts: exynos: gs101: update USI UART to use peric0 clocks arm64: dts: exynos: gs101: define USI8 with I2C configuration arm64: dts: exynos: gs101: enable eeprom on gs101-oriole arm64: defconfig: sync with savedefconfig arm64: defconfig: make at24 eeprom builtin .../bindings/clock/google,gs101-clock.yaml | 25 +- .../devicetree/bindings/i2c/i2c-exynos5.yaml | 1 + .../bindings/serial/samsung_uart.yaml | 4 + .../boot/dts/exynos/google/gs101-oriole.dts | 18 + arch/arm64/boot/dts/exynos/google/gs101.dtsi | 52 +- arch/arm64/configs/defconfig | 146 ++-- drivers/clk/samsung/clk-gs101.c | 748 ++++++++++++++++-- drivers/tty/serial/samsung_tty.c | 11 + include/dt-bindings/clock/google,gs101.h | 230 ++++-- 9 files changed, 980 insertions(+), 255 deletions(-)