diff mbox series

[09/18] clk: qcom: gcc-sm8250: Set delay for Venus CLK resets

Message ID 20240105-topic-venus_reset-v1-9-981c7a624855@linaro.org
State New
Headers
Series Qualcomm GCC/VIDEOCC reset overhaul for Venus |

Commit Message

Konrad Dybcio Jan. 8, 2024, 12:32 p.m. UTC 
  Some Venus resets may require more time when toggling. Describe that.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gcc-sm8250.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Bryan O'Donoghue Jan. 9, 2024, 12:34 a.m. UTC | #1 
On 08/01/2024 12:32, Konrad Dybcio wrote:
> Some Venus resets may require more time when toggling. Describe that.

May or does ?

I'd prefer a strong declaration of where this value came from and why 
its being added.

May is ambiguous.

"Downstream has a 150 us delay for this. My own testing shows this to be 
necessary in upstream"

Later commits want to add a 1000 us delay. Have all of these delays been 
tested ?

If not please describe where the values come.

> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/clk/qcom/gcc-sm8250.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gcc-sm8250.c b/drivers/clk/qcom/gcc-sm8250.c
> index c6c5261264f1..61d01d4c379b 100644
> --- a/drivers/clk/qcom/gcc-sm8250.c
> +++ b/drivers/clk/qcom/gcc-sm8250.c
> @@ -3576,8 +3576,8 @@ static const struct qcom_reset_map gcc_sm8250_resets[] = {
>   	[GCC_USB3PHY_PHY_PRIM_BCR] = { 0x50004 },
>   	[GCC_USB3PHY_PHY_SEC_BCR] = { 0x50010 },
>   	[GCC_USB_PHY_CFG_AHB2PHY_BCR] = { 0x6a000 },
> -	[GCC_VIDEO_AXI0_CLK_ARES] = { 0xb024, 2 },
> -	[GCC_VIDEO_AXI1_CLK_ARES] = { 0xb028, 2 },
> +	[GCC_VIDEO_AXI0_CLK_ARES] = { 0xb024, .bit = 2, .udelay = 150 },
> +	[GCC_VIDEO_AXI1_CLK_ARES] = { 0xb028, .bit = 2, .udelay = 150 },
>   };
>   
>   static const struct clk_rcg_dfs_data gcc_dfs_clocks[] = {
>
Konrad Dybcio Jan. 9, 2024, 9:33 a.m. UTC | #2 
On 1/9/24 01:34, Bryan O'Donoghue wrote:
> On 08/01/2024 12:32, Konrad Dybcio wrote:
>> Some Venus resets may require more time when toggling. Describe that.
> 
> May or does ?
> 
> I'd prefer a strong declaration of where this value came from and why its being added.
> 
> May is ambiguous.
> 
> "Downstream has a 150 us delay for this. My own testing shows this to be necessary in upstream"

Alright

> 
> Later commits want to add a 1000 us delay. Have all of these delays been tested ?

No, we don't support Venus on many of the newer SoCs..


> 
> If not please describe where the values come.

They come from the downstream Venus driver as you mentioned.
I checked a couple different downstream SoC kernel trees and
tried to assign the values based on what I found in a kernel
for that platform. Some are fairly educated guesses.

Konrad
Bjorn Andersson Jan. 27, 2024, 11:05 p.m. UTC | #3 
On Tue, Jan 09, 2024 at 10:33:39AM +0100, Konrad Dybcio wrote:
> 
> 
> On 1/9/24 01:34, Bryan O'Donoghue wrote:
> > On 08/01/2024 12:32, Konrad Dybcio wrote:
> > > Some Venus resets may require more time when toggling. Describe that.
> > 
> > May or does ?
> > 
> > I'd prefer a strong declaration of where this value came from and why its being added.
> > 
> > May is ambiguous.
> > 
> > "Downstream has a 150 us delay for this. My own testing shows this to be necessary in upstream"
> 
> Alright
> 
> > 
> > Later commits want to add a 1000 us delay. Have all of these delays been tested ?
> 
> No, we don't support Venus on many of the newer SoCs..
> 
> 
> > 
> > If not please describe where the values come.
> 
> They come from the downstream Venus driver as you mentioned.
> I checked a couple different downstream SoC kernel trees and
> tried to assign the values based on what I found in a kernel
> for that platform. Some are fairly educated guesses.
> 

It would be nice to have documented for which cases you guessed (and in
which downstream kernel you found other values?), so that if anyone is
coming to the tree later with conflicting information they have a better
chance to reason about the discrepancy.

Thanks,
Bjorn
diff mbox series

Patch

diff --git a/drivers/clk/qcom/gcc-sm8250.c b/drivers/clk/qcom/gcc-sm8250.c
index c6c5261264f1..61d01d4c379b 100644
--- a/drivers/clk/qcom/gcc-sm8250.c
+++ b/drivers/clk/qcom/gcc-sm8250.c
@@ -3576,8 +3576,8 @@  static const struct qcom_reset_map gcc_sm8250_resets[] = {
 	[GCC_USB3PHY_PHY_PRIM_BCR] = { 0x50004 },
 	[GCC_USB3PHY_PHY_SEC_BCR] = { 0x50010 },
 	[GCC_USB_PHY_CFG_AHB2PHY_BCR] = { 0x6a000 },
-	[GCC_VIDEO_AXI0_CLK_ARES] = { 0xb024, 2 },
-	[GCC_VIDEO_AXI1_CLK_ARES] = { 0xb028, 2 },
+	[GCC_VIDEO_AXI0_CLK_ARES] = { 0xb024, .bit = 2, .udelay = 150 },
+	[GCC_VIDEO_AXI1_CLK_ARES] = { 0xb028, .bit = 2, .udelay = 150 },
 };
 
 static const struct clk_rcg_dfs_data gcc_dfs_clocks[] = {