| Message ID | 20240127001926.495769-10-andre.draszik@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/9] clk: samsung: gs-101: drop extra empty line | expand |
On Sat, 27 Jan 2024 at 00:19, André Draszik <andre.draszik@linaro.org> wrote: > > Now that we have hooked it up in the DTS, we can drop the > CLK_IGNORE_UNUSED from here. > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > --- Reviewed-by: Peter Griffin <peter.griffin@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 7f6c3b52d9ff..d55ed64d0e29 100644 > --- a/drivers/clk/samsung/clk-gs101.c > +++ b/drivers/clk/samsung/clk-gs101.c > @@ -3393,7 +3393,7 @@ static const struct samsung_gate_clock peric1_gate_clks[] __initconst = { > GATE(CLK_GOUT_PERIC1_SYSREG_PERIC1_PCLK, > "gout_peric1_sysreg_peric1_pclk", "mout_peric1_bus_user", > CLK_CON_GAT_GOUT_BLK_PERIC1_UID_SYSREG_PERIC1_IPCLKPORT_PCLK, > - 21, CLK_IGNORE_UNUSED, 0), > + 21, 0, 0), > }; > > static const struct samsung_cmu_info peric1_cmu_info __initconst = { > -- > 2.43.0.429.g432eaa2c6b-goog >
On 27/01/2024 01:19, André Draszik wrote: > Now that we have hooked it up in the DTS, we can drop the Your driver patch cannot depend on DTS. Not for a new platform. I am repeating this all the time last days... > CLK_IGNORE_UNUSED from here. > > Signed-off-by: André Draszik <andre.draszik@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 7f6c3b52d9ff..d55ed64d0e29 100644 > --- a/drivers/clk/samsung/clk-gs101.c > +++ b/drivers/clk/samsung/clk-gs101.c > @@ -3393,7 +3393,7 @@ static const struct samsung_gate_clock peric1_gate_clks[] __initconst = { > GATE(CLK_GOUT_PERIC1_SYSREG_PERIC1_PCLK, > "gout_peric1_sysreg_peric1_pclk", "mout_peric1_bus_user", > CLK_CON_GAT_GOUT_BLK_PERIC1_UID_SYSREG_PERIC1_IPCLKPORT_PCLK, > - 21, CLK_IGNORE_UNUSED, 0), I don't understand. You just added this clock in this patchset. This means that your patch #3 is incorrect. Best regards, Krzysztof
Hi Krzysztof, On Mon, 2024-01-29 at 12:03 +0100, Krzysztof Kozlowski wrote: > On 27/01/2024 01:19, André Draszik wrote: > > Now that we have hooked it up in the DTS, we can drop the > > Your driver patch cannot depend on DTS. Not for a new platform. I am > repeating this all the time last days... > > > CLK_IGNORE_UNUSED from here. > > > > Signed-off-by: André Draszik <andre.draszik@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 7f6c3b52d9ff..d55ed64d0e29 100644 > > --- a/drivers/clk/samsung/clk-gs101.c > > +++ b/drivers/clk/samsung/clk-gs101.c > > @@ -3393,7 +3393,7 @@ static const struct samsung_gate_clock peric1_gate_clks[] __initconst = { > > GATE(CLK_GOUT_PERIC1_SYSREG_PERIC1_PCLK, > > "gout_peric1_sysreg_peric1_pclk", "mout_peric1_bus_user", > > CLK_CON_GAT_GOUT_BLK_PERIC1_UID_SYSREG_PERIC1_IPCLKPORT_PCLK, > > - 21, CLK_IGNORE_UNUSED, 0), > > I don't understand. You just added this clock in this patchset. This > means that your patch #3 is incorrect. In patch #3 I'm hooking up all the clocks to Linux. If I don't CLK_IGNORE_UNUSED for the 'sysreg' pclk in patch #3, then it'll hang on loading drivers that require sysreg access (because Linux disabled the clock). I can not change patch #8 to come between 2 and 3 either, because at that stage neither the clock nor the DT node reference &cmu_peric1 actually exist, and the clock and can't be claimed by sysreg. Since we can not mix DT and driver changes in the same commit, I can not merge patches #3 and #4 and #8 either. I had to do it this way so that the platform always boots for every commit to keep things bisectable. Alternatively, I could merge patches #4 and #8 (but that seems wrong to me), or drop patches #7, #8 and #9 from this series and apply it later in the -rc phase? Is there a better way that you have in mind that we're missing, that keeps things atomic and bootable/bisectable? Cheers, Andre'
On 29/01/2024 14:47, André Draszik wrote: > Hi Krzysztof, > > On Mon, 2024-01-29 at 12:03 +0100, Krzysztof Kozlowski wrote: >> On 27/01/2024 01:19, André Draszik wrote: >>> Now that we have hooked it up in the DTS, we can drop the >> >> Your driver patch cannot depend on DTS. Not for a new platform. I am >> repeating this all the time last days... >> >>> CLK_IGNORE_UNUSED from here. >>> >>> Signed-off-by: André Draszik <andre.draszik@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 7f6c3b52d9ff..d55ed64d0e29 100644 >>> --- a/drivers/clk/samsung/clk-gs101.c >>> +++ b/drivers/clk/samsung/clk-gs101.c >>> @@ -3393,7 +3393,7 @@ static const struct samsung_gate_clock peric1_gate_clks[] __initconst = { >>> GATE(CLK_GOUT_PERIC1_SYSREG_PERIC1_PCLK, >>> "gout_peric1_sysreg_peric1_pclk", "mout_peric1_bus_user", >>> CLK_CON_GAT_GOUT_BLK_PERIC1_UID_SYSREG_PERIC1_IPCLKPORT_PCLK, >>> - 21, CLK_IGNORE_UNUSED, 0), >> >> I don't understand. You just added this clock in this patchset. This >> means that your patch #3 is incorrect. > > In patch #3 I'm hooking up all the clocks to Linux. If I don't CLK_IGNORE_UNUSED > for the 'sysreg' pclk in patch #3, then it'll hang on loading drivers that > require sysreg access (because Linux disabled the clock). Then add clk_ignore_unused to cmdline. That's anyway recommended for development platforms without full clock and pd description (pd_ignore_unused). Not mentioning that we might default to clk_ignore_unused at some point soon. > > I can not change patch #8 to come between 2 and 3 either, because at that stage > neither the clock nor the DT node reference &cmu_peric1 actually exist, and the > clock and can't be claimed by sysreg. At the point of me applying this patch, there will be no DTS node either. This ordering fixes nothing. > > Since we can not mix DT and driver changes in the same commit, I can not merge > patches #3 and #4 and #8 either. > > I had to do it this way so that the platform always boots for every commit to keep > things bisectable. But it is not bisectable - you did not fix anything. You can try by yourself: # git checkout drivers # git am patch #1, #2, #3 and #9 # git checkout dt # git am patch #4, #5, #6, #7, #8 and now try to bisect it. You will have the same problems you try to avoid. So what is solved by this ordering? Nothing. > > Alternatively, I could merge patches #4 and #8 (but that seems wrong to me), or > drop patches #7, #8 and #9 from this series and apply it later in the -rc phase? Probably the mistake was done in the way how you upstream things: adding sysreg syscon without its clocks. Additionally: 1. Disabling unused clocks is current OS policy, so why the policy should affect DTS and driver ordering? 2. This is platform did not receive a release kernel, so glitches are okay. For this case #9 must be squashed with #3. #4 with #9. > > > Is there a better way that you have in mind that we're missing, that keeps things > atomic and bootable/bisectable? Best regards, Krzysztof
On Mon, 2024-01-29 at 15:08 +0100, Krzysztof Kozlowski wrote:
> For this case #9 must be squashed with #3. #4 with #9.
Will do as you suggest, Krzysztof (I think you meant #4 with #8).
Thanks for your patience.
Cheers,
Andre'
On 29/01/2024 16:21, André Draszik wrote: > On Mon, 2024-01-29 at 15:08 +0100, Krzysztof Kozlowski wrote: >> For this case #9 must be squashed with #3. #4 with #9. > > Will do as you suggest, Krzysztof (I think you meant #4 with #8). Yes, indeed, the DTS patches. Best regards, Krzysztof
diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c index 7f6c3b52d9ff..d55ed64d0e29 100644 --- a/drivers/clk/samsung/clk-gs101.c +++ b/drivers/clk/samsung/clk-gs101.c @@ -3393,7 +3393,7 @@ static const struct samsung_gate_clock peric1_gate_clks[] __initconst = { GATE(CLK_GOUT_PERIC1_SYSREG_PERIC1_PCLK, "gout_peric1_sysreg_peric1_pclk", "mout_peric1_bus_user", CLK_CON_GAT_GOUT_BLK_PERIC1_UID_SYSREG_PERIC1_IPCLKPORT_PCLK, - 21, CLK_IGNORE_UNUSED, 0), + 21, 0, 0), }; static const struct samsung_cmu_info peric1_cmu_info __initconst = {
Now that we have hooked it up in the DTS, we can drop the CLK_IGNORE_UNUSED from here. Signed-off-by: André Draszik <andre.draszik@linaro.org> --- drivers/clk/samsung/clk-gs101.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)