Re: [PATCH 3/8] clk: qcom: clk-alpha-pll: Add HUAYRA_2290 support

[Konrad Dybcio]

On 19.02.2024 15:54, Andrew Halaney wrote:
> On Mon, Feb 19, 2024 at 02:35:48PM +0100, Konrad Dybcio wrote:
>> Commit 134b55b7e19f ("clk: qcom: support Huayra type Alpha PLL")
>> introduced an entry to the alpha offsets array, but diving into QCM2290
>> downstream and some documentation, it turned out that the name Huayra
>> apparently has been used quite liberally across many chips, even with
>> noticeably different hardware.
>>
>> Introduce another set of offsets and a new configure function for the
>> Huayra PLL found on QCM2290. This is required e.g. for the consumers
>> of GPUCC_PLL0 to properly start.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
>> ---
>>  drivers/clk/qcom/clk-alpha-pll.c | 45 ++++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/qcom/clk-alpha-pll.h |  3 +++
>>  2 files changed, 48 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
>> index 8a412ef47e16..61b5abd13782 100644
>> --- a/drivers/clk/qcom/clk-alpha-pll.c
>> +++ b/drivers/clk/qcom/clk-alpha-pll.c
>> @@ -244,6 +244,19 @@ const u8 clk_alpha_pll_regs[][PLL_OFF_MAX_REGS] = {
>>  		[PLL_OFF_OPMODE] = 0x30,
>>  		[PLL_OFF_STATUS] = 0x3c,
>>  	},
>> +	[CLK_ALPHA_PLL_TYPE_HUAYRA_2290] =  {
>> +		[PLL_OFF_L_VAL] = 0x04,
>> +		[PLL_OFF_ALPHA_VAL] = 0x08,
>> +		[PLL_OFF_USER_CTL] = 0x0c,
>> +		[PLL_OFF_CONFIG_CTL] = 0x10,
>> +		[PLL_OFF_CONFIG_CTL_U] = 0x14,
>> +		[PLL_OFF_CONFIG_CTL_U1] = 0x18,
>> +		[PLL_OFF_TEST_CTL] = 0x1c,
>> +		[PLL_OFF_TEST_CTL_U] = 0x20,
>> +		[PLL_OFF_TEST_CTL_U1] = 0x24,
>> +		[PLL_OFF_OPMODE] = 0x28,
>> +		[PLL_OFF_STATUS] = 0x38,
>> +	},
>>  };
>>  EXPORT_SYMBOL_GPL(clk_alpha_pll_regs);
>>  
>> @@ -779,6 +792,38 @@ static long clk_alpha_pll_round_rate(struct clk_hw *hw, unsigned long rate,
>>  	return clamp(rate, min_freq, max_freq);
>>  }
>>  
>> +void clk_huayra_2290_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
>> +				   const struct alpha_pll_config *config)
>> +{
>> +	clk_alpha_pll_write_config(regmap, PLL_CONFIG_CTL(pll), config->config_ctl_val);
>> +	clk_alpha_pll_write_config(regmap, PLL_CONFIG_CTL_U(pll), config->config_ctl_hi_val);
>> +	clk_alpha_pll_write_config(regmap, PLL_CONFIG_CTL_U1(pll), config->config_ctl_hi1_val);
>> +	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL(pll), config->test_ctl_val);
>> +	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U(pll), config->test_ctl_hi_val);
>> +	clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U1(pll), config->test_ctl_hi1_val);
>> +	clk_alpha_pll_write_config(regmap, PLL_L_VAL(pll), config->l);
>> +	clk_alpha_pll_write_config(regmap, PLL_ALPHA_VAL(pll), config->alpha);
>> +	clk_alpha_pll_write_config(regmap, PLL_USER_CTL(pll), config->user_ctl_val);
>> +
>> +	/* Set PLL_BYPASSNL */
>> +	regmap_update_bits(regmap, PLL_MODE(pll), PLL_BYPASSNL, PLL_BYPASSNL);
>> +
>> +	/* Wait 5 us between setting BYPASS and deasserting reset */
>> +	mb();
>> +	udelay(5);
>> +
>> +	/* Take PLL out from reset state */
>> +	regmap_update_bits(regmap, PLL_MODE(pll), PLL_RESET_N, PLL_RESET_N);
>> +
>> +	/* Wait 50us for PLL_LOCK_DET bit to go high */
>> +	mb();
>> +	usleep_range(50, 55);
> 
> I *think* you'd want to use a read to ensure your write goes through
> prior to your sleep... from memory-barriers.txt:
> 
> 	5. A readX() by a CPU thread from the peripheral will complete before
> 	   any subsequent delay() loop can begin execution on the same thread.
> 	   This ensures that two MMIO register writes by the CPU to a peripheral
> 	   will arrive at least 1us apart if the first write is immediately read
> 	   back with readX() and udelay(1) is called prior to the second
> 	   writeX():
> 
> 		writel(42, DEVICE_REGISTER_0); // Arrives at the device...
> 		readl(DEVICE_REGISTER_0);
> 		udelay(1);
> 		writel(42, DEVICE_REGISTER_1); // ...at least 1us before this.
> 
> also https://youtu.be/i6DayghhA8Q?si=7lp0be35q1HRmlnV&t=1677
> for more references on this topic.

I mentioned this feels very iffy in the cover letter, but it's a combination
of two things:

1. i followed what qualcomm downstream code did

2. qualcomm downstream code is not known for being always correct



I suppose a readback would be the correct solution, but then it should be
done for all similar calls in this driver.

Although this code has shipped in literally hundreds of millions of devices
and it didn't explode badly :P (i'm not defending it, though)

Konrad