mbox series

[v2,0/6] gs101 oriole: peripheral block 0 (peric0) fixes

Message ID 20240130093812.1746512-1-andre.draszik@linaro.org
Headers show
Series gs101 oriole: peripheral block 0 (peric0) fixes | expand

Message

André Draszik Jan. 30, 2024, 9:36 a.m. UTC 
Hi,

While working on peric1, I've noticed a few issues in the peric0 area
and these patches are the result. They should all be pretty
self-explanatory.

Cheers,
Andre'

 arch/arm64/boot/dts/exynos/google/gs101.dtsi | 11 ++++++-----
 drivers/clk/samsung/clk-gs101.c              |  8 +++-----
 2 files changed, 9 insertions(+), 10 deletions(-)

Changes in v2:
* new patch #6
- fix a typo in the commit messages of patches #3 and #5 (André)
- add some empty lines to commit messages (Sam)
- collect Reviewed-by: Tested-by: tags

Comments

Krzysztof Kozlowski Feb. 1, 2024, 9:58 a.m. UTC | #1 
On 30/01/2024 10:36, André Draszik wrote:
> This pclk clock is required any time we access the pinctrl registers of
> this block.
> 
> Since pinctrl-samsung doesn't support a clock at the moment, we just
> keep the kernel from disabling it at boot, until we have an update for
> pinctrl-samsung to handle this required clock, at which point we'll be
> able to drop the flag again.
> 
> Fixes: 893f133a040b ("clk: samsung: gs101: add support for cmu_peric0")

Dropped fixes tag. The driver looks correct, it's pinctrl issue.

I really dislike how these patches are inter-mixed with DTS. Makes
applying difficult and confuses about dependencies.

I assume there are no dependencies here.

I am repeating this and repeating, but in future I will just reject the
patches:

Your DTS and driver changes cannot depend on each other for new feature
submissions.

Best regards,
Krzysztof
Krzysztof Kozlowski Feb. 1, 2024, 10:16 a.m. UTC | #2 
On 30/01/2024 10:36, André Draszik wrote:
> While commit 6d44d1a1fb62 ("arm64: dts: exynos: gs101: define USI8 with
> I2C configuration") states that the USI8 CONFIG is 0 at reset, the boot
> loader has configured it by the time Linux runs and it has a different
> value at this stage.

This issue was pointed during review:
https://lore.kernel.org/all/CAPLW+4=U9DBmwgxyWz3cy=V-Ui7s2Z9um4xbEuyax1o=0zB_NA@mail.gmail.com/

Yet you posted new version of patchset not implementing this, just to do
it week later as new patch.

Sorry guys, it seems you need much more time to accept and go through
review, I will use two weeks delay time for applying GS patches.

Now, for the patch, we don't do it for any other nodes which have 0 as
reset value and we do not know what bootloader does there. Bootloader
also can change.

This is a required property, therefore please explain me how, really
how, this can happen:

" we should
set it to none here so as to ensure things don't work by accident"

NAK

Best regards,
Krzysztof
Krzysztof Kozlowski Feb. 1, 2024, 10:36 a.m. UTC | #3 
On Tue, 30 Jan 2024 09:36:42 +0000, André Draszik wrote:
> Wrong pclk clocks have been used in this usi8 instance here. For USI
> and I2C, we need the ipclk and pclk, where pclk is the bus clock.
> Without it, nothing can work.
> 
> It is unclear what exactly is using USI8_USI_CLK, but it is not
> required for the IP to be operational at this stage, while pclk is.
> This also brings the DT in line with the clock names expected by the
> usi and i2c drivers.
> 
> [...]

Applied, thanks!

[3/6] arm64: dts: exynos: gs101: use correct clocks for usi8
      https://git.kernel.org/krzk/linux/c/512b5a875cd8b8352257fefe71513097ee97be77

Best regards,
Krzysztof Kozlowski Feb. 1, 2024, 10:36 a.m. UTC | #4 
On Tue, 30 Jan 2024 09:36:40 +0000, André Draszik wrote:
> This pclk clock is required any time we access the pinctrl registers of
> this block.
> 
> Since pinctrl-samsung doesn't support a clock at the moment, we just
> keep the kernel from disabling it at boot, until we have an update for
> pinctrl-samsung to handle this required clock, at which point we'll be
> able to drop the flag again.
> 
> [...]

Applied, thanks!

[1/6] clk: samsung: gs101: gpio_peric0_pclk needs to be kept on
      https://git.kernel.org/krzk/linux/c/8a96d2701f7c794e45102a9cc7fc4a5c4951e699

Best regards,