| Message ID | 20240119111132.1290455-1-tudor.ambarus@linaro.org |
|---|---|
| Headers | show |
| Series | GS101 Oriole: CMU_PERIC0 support and USI updates | expand |
On 19/01/2024 12:11, Tudor Ambarus wrote: > CMU_PERIC0 is the clock management unit used for the peric0 block which > is used for USI and I3C. Add support for all cmu_peric0 clocks but > CLK_GOUT_PERIC0_IP (not enough info in the datasheet). > > Few clocks are marked as critical because when either of them is > disabled, the system hangs even if their clock parents are enabled. > > Reviewed-by: Peter Griffin <peter.griffin@linaro.org> > Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > drivers/clk/samsung/clk-gs101.c | 583 ++++++++++++++++++++++++++++++++ > 1 file changed, 583 insertions(+) > This does not apply. Please rebase on my samsung for-next or next linux-next and resend. Best regards, Krzysztof
On Fri, 19 Jan 2024 11:11:28 +0000, Tudor Ambarus wrote: > Remove the reg-io-width property in order to comply with the bindings. > > The entire bus (PERIC) on which the GS101 serial resides only allows > 32-bit register accesses. The reg-io-width dt property is disallowed > for the "google,gs101-uart" compatible and instead the iotype is > inferred from the compatible. > > [...] Applied, thanks! [4/8] arm64: dts: exynos: gs101: remove reg-io-width from serial https://git.kernel.org/krzk/linux/c/daff9d192892ea583284eb116a07e8e0086f0e76 Best regards,
On Fri, 19 Jan 2024 11:11:29 +0000, Tudor Ambarus wrote: > Enable the cmu-peric0 clock controller. It feeds USI and I3c. > > Applied, thanks! [5/8] arm64: dts: exynos: gs101: enable cmu-peric0 clock controller https://git.kernel.org/krzk/linux/c/8a670bb84cdcf1397fc4a3bc295c0008f49bed91 Best regards,
On Fri, 19 Jan 2024 11:11:30 +0000, Tudor Ambarus wrote: > Get rid of the dummy clock and start using the cmu_peric0 clocks > for the usi_uart and serial_0 nodes. > > Tested the serial at 115200, 1000000 and 3000000 baudrates, > everthing went fine. > > > [...] Applied, thanks! [6/8] arm64: dts: exynos: gs101: update USI UART to use peric0 clocks https://git.kernel.org/krzk/linux/c/e58513b2ff78c70805d3bd7d96bfd76576d4c9e3 Best regards,
On Fri, 19 Jan 2024 11:11:31 +0000, Tudor Ambarus wrote: > USI8 I2C is used to communicate with an eeprom found on the battery > connector. Define USI8 in I2C configuration. > > USI8 CONFIG register comes with a 0x0 reset value, meaning that USI8 > doesn't have a default protocol (I2C, SPI, UART) at reset. Thus the > selection of the protocol is intentionally left for the board dts file. > > [...] Applied, thanks! [7/8] arm64: dts: exynos: gs101: define USI8 with I2C configuration https://git.kernel.org/krzk/linux/c/9ca7055a35a7b2b373ead6f3a67ee8b5e0e6e468 Best regards,
On Fri, 19 Jan 2024 11:11:32 +0000, Tudor Ambarus wrote: > Enable the eeprom found on the battery connector. > > The selection of the USI protocol is done in the board dts file because > the USI CONFIG register comes with a 0x0 reset value, meaning that USI8 > does not have a default protocol (I2C, SPI, UART) at reset. > > > [...] Applied, thanks! [8/8] arm64: dts: exynos: gs101: enable eeprom on gs101-oriole https://git.kernel.org/krzk/linux/c/6a06935ec8263be083ac897a842f06d66b138ffb Best regards,
On 1/22/24 11:13, Krzysztof Kozlowski wrote: > On 19/01/2024 12:11, Tudor Ambarus wrote: >> CMU_PERIC0 is the clock management unit used for the peric0 block which >> is used for USI and I3C. Add support for all cmu_peric0 clocks but >> CLK_GOUT_PERIC0_IP (not enough info in the datasheet). >> >> Few clocks are marked as critical because when either of them is >> disabled, the system hangs even if their clock parents are enabled. >> >> Reviewed-by: Peter Griffin <peter.griffin@linaro.org> >> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> --- >> drivers/clk/samsung/clk-gs101.c | 583 ++++++++++++++++++++++++++++++++ >> 1 file changed, 583 insertions(+) >> > > This does not apply. Please rebase on my samsung for-next or next > linux-next and resend. > Oh, yes. I rebased, did a boot test and then sent v5 at: https://lore.kernel.org/linux-arm-kernel/20240122114113.2582612-1-tudor.ambarus@linaro.org/T/#u Thanks! ta
On 19/01/2024 12:11, Tudor Ambarus wrote: > USI8 I2C is used to communicate with an eeprom found on the battery > connector. Define USI8 in I2C configuration. > > USI8 CONFIG register comes with a 0x0 reset value, meaning that USI8 > doesn't have a default protocol (I2C, SPI, UART) at reset. Thus the > selection of the protocol is intentionally left for the board dts file. ... and dropped, because this patch does not build: https://krzk.eu/#/builders/29/builds/3869 and I missed weird dependency mentioned in cover letter: "This patch set shall be queued after the cmu_misc clock name fixes from:" Sorry, this cannot work like that. DTS for new features cannot build depend on driver changes. Best regards, Krzysztof
On 19/01/2024 12:11, Tudor Ambarus wrote: > Get rid of the dummy clock and start using the cmu_peric0 clocks > for the usi_uart and serial_0 nodes. > > Tested the serial at 115200, 1000000 and 3000000 baudrates, > everthing went fine. > > Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> This also fails to build (and also cannot depend on driver changes because it is a new DTS). Dropped. Best regards, Krzysztof
On 1/23/24 07:52, Krzysztof Kozlowski wrote: > On 19/01/2024 12:11, Tudor Ambarus wrote: >> USI8 I2C is used to communicate with an eeprom found on the battery >> connector. Define USI8 in I2C configuration. >> >> USI8 CONFIG register comes with a 0x0 reset value, meaning that USI8 >> doesn't have a default protocol (I2C, SPI, UART) at reset. Thus the >> selection of the protocol is intentionally left for the board dts file. > > ... and dropped, because this patch does not build: > https://krzk.eu/#/builders/29/builds/3869 > and I missed weird dependency mentioned in cover letter: > > "This patch set shall be queued after the cmu_misc clock name fixes from:" > > Sorry, this cannot work like that. DTS for new features cannot build > depend on driver changes. No worries. What shall I do so that you re-consider the dropped patches? I'm not yet familiar with your release management, but I guess that if you submit your "fixes-clk" branch for integration into v6.8-rc2, and then merge v6.8-rc2 into your "next/dt64", you'll then be able to queue the dropped patches as well. Thanks, ta
On 23/01/2024 09:34, Tudor Ambarus wrote: > > > On 1/23/24 07:52, Krzysztof Kozlowski wrote: >> On 19/01/2024 12:11, Tudor Ambarus wrote: >>> USI8 I2C is used to communicate with an eeprom found on the battery >>> connector. Define USI8 in I2C configuration. >>> >>> USI8 CONFIG register comes with a 0x0 reset value, meaning that USI8 >>> doesn't have a default protocol (I2C, SPI, UART) at reset. Thus the >>> selection of the protocol is intentionally left for the board dts file. >> >> ... and dropped, because this patch does not build: >> https://krzk.eu/#/builders/29/builds/3869 >> and I missed weird dependency mentioned in cover letter: >> >> "This patch set shall be queued after the cmu_misc clock name fixes from:" >> >> Sorry, this cannot work like that. DTS for new features cannot build >> depend on driver changes. > > No worries. What shall I do so that you re-consider the dropped patches? > I'm not yet familiar with your release management, but I guess that if > you submit your "fixes-clk" branch for integration into v6.8-rc2, and > then merge v6.8-rc2 into your "next/dt64", you'll then be able to queue > the dropped patches as well. It is nothing specific to my release management but years old rule: DTS branch cannot contain driver commits. It is nothing new, discussed on mailing lists for various SoC architectures many times. However I don't fully understand why that dependency - except patch hunk context - exists. You shouldn't have such dependency. Best regards, Krzysztof
On 1/23/24 08:39, Krzysztof Kozlowski wrote: > On 23/01/2024 09:34, Tudor Ambarus wrote: >> >> >> On 1/23/24 07:52, Krzysztof Kozlowski wrote: >>> On 19/01/2024 12:11, Tudor Ambarus wrote: >>>> USI8 I2C is used to communicate with an eeprom found on the battery >>>> connector. Define USI8 in I2C configuration. >>>> >>>> USI8 CONFIG register comes with a 0x0 reset value, meaning that USI8 >>>> doesn't have a default protocol (I2C, SPI, UART) at reset. Thus the >>>> selection of the protocol is intentionally left for the board dts file. >>> >>> ... and dropped, because this patch does not build: >>> https://krzk.eu/#/builders/29/builds/3869 >>> and I missed weird dependency mentioned in cover letter: >>> >>> "This patch set shall be queued after the cmu_misc clock name fixes from:" >>> >>> Sorry, this cannot work like that. DTS for new features cannot build >>> depend on driver changes. >> >> No worries. What shall I do so that you re-consider the dropped patches? >> I'm not yet familiar with your release management, but I guess that if >> you submit your "fixes-clk" branch for integration into v6.8-rc2, and >> then merge v6.8-rc2 into your "next/dt64", you'll then be able to queue >> the dropped patches as well. > > It is nothing specific to my release management but years old rule: DTS > branch cannot contain driver commits. It is nothing new, discussed on > mailing lists for various SoC architectures many times. Okay, thanks for the explanation. > > However I don't fully understand why that dependency - except patch hunk > context - exists. You shouldn't have such dependency. > Let me try offline, I'll get back to you.
On 1/23/24 08:44, Tudor Ambarus wrote: >> However I don't fully understand why that dependency - except patch hunk >> context - exists. You shouldn't have such dependency. >> > Let me try offline, I'll get back to you. The dropped patches depend on the dt-bindings patch that you queued through the "next/drivers" branch: b393a6c5e656 dt-bindings: clock: google,gs101-clock: add PERIC0 clock management unit We need the peric0 bindings that are referenced in device tree, that's why the next/dt64 branch failed to build. Please let me know if there's something on my side that I have to do (now or in the future). Thanks, ta
On 23/01/2024 09:57, Tudor Ambarus wrote: > > > On 1/23/24 08:44, Tudor Ambarus wrote: >>> However I don't fully understand why that dependency - except patch hunk >>> context - exists. You shouldn't have such dependency. >>> >> Let me try offline, I'll get back to you. > > The dropped patches depend on the dt-bindings patch that you queued > through the "next/drivers" branch: > > b393a6c5e656 dt-bindings: clock: google,gs101-clock: add PERIC0 clock > management unit > > We need the peric0 bindings that are referenced in device tree, that's > why the next/dt64 branch failed to build. > > Please let me know if there's something on my side that I have to do > (now or in the future). It is useful to mention this in cover letter, so I will know how to apply the patches. I understand therefore the dependency mention in the cover letter is not accurate, so I can ignore that aspect. Best regards, Krzysztof
On 1/23/24 08:59, Krzysztof Kozlowski wrote: > On 23/01/2024 09:57, Tudor Ambarus wrote: >> >> >> On 1/23/24 08:44, Tudor Ambarus wrote: >>>> However I don't fully understand why that dependency - except patch hunk >>>> context - exists. You shouldn't have such dependency. >>>> >>> Let me try offline, I'll get back to you. >> >> The dropped patches depend on the dt-bindings patch that you queued >> through the "next/drivers" branch: >> >> b393a6c5e656 dt-bindings: clock: google,gs101-clock: add PERIC0 clock >> management unit >> >> We need the peric0 bindings that are referenced in device tree, that's >> why the next/dt64 branch failed to build. >> >> Please let me know if there's something on my side that I have to do >> (now or in the future). > > It is useful to mention this in cover letter, so I will know how to > apply the patches. I understand therefore the dependency mention in the Thanks for the patience, I learn along the way. > cover letter is not accurate, so I can ignore that aspect. > Yes, that's right. The dependency on name fixes is just as a patch hunk context, no functional or build dependencies. I now know the process, and I'll be more verbose next time. Cheers, ta
On Fri, 19 Jan 2024 11:11:30 +0000, Tudor Ambarus wrote: > Get rid of the dummy clock and start using the cmu_peric0 clocks > for the usi_uart and serial_0 nodes. > > Tested the serial at 115200, 1000000 and 3000000 baudrates, > everthing went fine. > > > [...] Applied, thanks! [6/8] arm64: dts: exynos: gs101: update USI UART to use peric0 clocks https://git.kernel.org/krzk/linux/c/3dfbd155b2e397f677f18fd3eccb8691443fb280 Best regards,
On Fri, 19 Jan 2024 11:11:31 +0000, Tudor Ambarus wrote: > USI8 I2C is used to communicate with an eeprom found on the battery > connector. Define USI8 in I2C configuration. > > USI8 CONFIG register comes with a 0x0 reset value, meaning that USI8 > doesn't have a default protocol (I2C, SPI, UART) at reset. Thus the > selection of the protocol is intentionally left for the board dts file. > > [...] Applied, thanks! [7/8] arm64: dts: exynos: gs101: define USI8 with I2C configuration https://git.kernel.org/krzk/linux/c/f3635d5ff6105e6e0450b2e7f7bb0055f0fea305 Best regards,
On Fri, 19 Jan 2024 11:11:32 +0000, Tudor Ambarus wrote: > Enable the eeprom found on the battery connector. > > The selection of the USI protocol is done in the board dts file because > the USI CONFIG register comes with a 0x0 reset value, meaning that USI8 > does not have a default protocol (I2C, SPI, UART) at reset. > > > [...] Applied, thanks! [8/8] arm64: dts: exynos: gs101: enable eeprom on gs101-oriole https://git.kernel.org/krzk/linux/c/c68efa676ca8febfd36aab50fe747c072c224d52 Best regards,
v4 drops the serial patches as they should be queued through the tty tree. It also fixes the CMU PERIC0 DIV widths -> from 3 to 4. Saw the bug while enabling SPI. This patch set shall be queued after the cmu_misc clock name fixes from: https://lore.kernel.org/linux-arm-kernel/20240109114908.3623645-1-tudor.ambarus@linaro.org/ Users that want to test the series must merge the serial patches to infer the 32-bit register accesses from the compatible, otherwise they'll see a Serror Interrupt as in this patch set we remove the reg-io-width = <4> property. The serial set can be found at: https://lore.kernel.org/linux-arm-kernel/20240119104526.1221243-1-tudor.ambarus@linaro.org/T/#t 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. Cheers, ta Changes in v4: - drop all serial patches as they should be taken via Greg's tty tree - fix CMU PERIC0 DIV widths -> from 3 to 4. Discovered the bug while testing SPI - collect Sam's R-b tags Changes in v3: - rename cmu_peric0 clocks to just "bus" and "ip" and then comply with the change in device tree and clock driver - reposition ``iotype`` of ``struct s3c24xx_uart_info`` to reduce the memory footprint of the struct. A patch set reworking the members of the struct will follow. - fix the usi8 clocks order in the device tree - collect Peter's R-b tags - changes log in each patch set as well, in the comments section under ```---``` Changes in v2: - gs101 serial - infer the reg-io-width from the compatible as the entire PERIC block allows just 32-bit register accesses. - identify the critical clocks faaaaaaarom PERIC0 and mark them accordingly (if disabled theslocks hang the system even if their parents are still enabled). - update dtsi and use USI's gate clocks instead of the dividers clocks - move hsi2c_8 cells and pinctrls into dtsi - address Sam's cosmetic changes in the device tree files - drop defconfig patches (savedefconfig output & at24 eeprom enablement) - collect Acked-by and Reviewed-by tags - changes log in each patch as well, in the comments section under ```---``` Tudor Ambarus (8): dt-bindings: clock: google,gs101-clock: add PERIC0 clock management unit dt-bindings: i2c: exynos5: add google,gs101-hsi2c compatible clk: samsung: gs101: add support for cmu_peric0 arm64: dts: exynos: gs101: remove reg-io-width from serial 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 .../bindings/clock/google,gs101-clock.yaml | 25 +- .../devicetree/bindings/i2c/i2c-exynos5.yaml | 1 + .../boot/dts/exynos/google/gs101-oriole.dts | 14 + arch/arm64/boot/dts/exynos/google/gs101.dtsi | 54 +- drivers/clk/samsung/clk-gs101.c | 583 ++++++++++++++++++ include/dt-bindings/clock/google,gs101.h | 81 +++ 6 files changed, 745 insertions(+), 13 deletions(-)