| 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: > > 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 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: > > Enable the cmu-peric0 clock controller. It feeds USI and I3c. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > arch/arm64/boot/dts/exynos/google/gs101.dtsi | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi > index 9747cb3fa03a..d0b0ad70c6ba 100644 > --- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi > +++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi > @@ -339,6 +339,18 @@ ppi_cluster2: interrupt-partition-2 { > }; > }; > > + cmu_peric0: clock-controller@10800000 { > + compatible = "google,gs101-cmu-peric0"; > + reg = <0x10800000 0x4000>; > + #clock-cells = <1>; > + clocks = <&ext_24_5m>, > + <&cmu_top CLK_DOUT_CMU_PERIC0_BUS>, > + <&cmu_top CLK_DOUT_CMU_PERIC0_IP>; > + clock-names = "oscclk", > + "dout_cmu_peric0_bus", I'd pull this line to the above line. Other than that: Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > + "dout_cmu_peric0_ip"; > + }; > + > sysreg_peric0: syscon@10820000 { > compatible = "google,gs101-peric0-sysreg", "syscon"; > reg = <0x10820000 0x10000>; > -- > 2.43.0.472.g3155946c3a-goog >
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:39, Sam Protsenko wrote: > On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >> Enable the cmu-peric0 clock controller. It feeds USI and I3c. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> --- >> arch/arm64/boot/dts/exynos/google/gs101.dtsi | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi >> index 9747cb3fa03a..d0b0ad70c6ba 100644 >> --- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi >> +++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi >> @@ -339,6 +339,18 @@ ppi_cluster2: interrupt-partition-2 { >> }; >> }; >> >> + cmu_peric0: clock-controller@10800000 { >> + compatible = "google,gs101-cmu-peric0"; >> + reg = <0x10800000 0x4000>; >> + #clock-cells = <1>; >> + clocks = <&ext_24_5m>, >> + <&cmu_top CLK_DOUT_CMU_PERIC0_BUS>, >> + <&cmu_top CLK_DOUT_CMU_PERIC0_IP>; >> + clock-names = "oscclk", >> + "dout_cmu_peric0_bus", > > I'd pull this line to the above line. Other than that: > No, it's fine. If clocks span over multiple lines (one clock per line), the names should follow in general. It's easier to read and match entries. 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 Thu, 14 Dec 2023 10:52:33 +0000, Tudor Ambarus wrote: > Add google,gs101-hsi2c dedicated compatible for representing > I2C of Google GS101 SoC. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring <robh@kernel.org>
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(-)