[v4,3/7] i2c: qcom-cci: Stop complaining about DT set clock rate

Commit Message

Richard Acayan Sept. 4, 2024, 2:04 a.m. UTC

From: Bryan O'Donoghue <[e-mail]>

It is common practice in the downstream and upstream CCI dt to set CCI
clock rates to 19.2 MHz. It appears to be fairly common for initial code to
set the CCI clock rate to 37.5 MHz.

Applying the widely used CCI clock rates from downstream ought not to cause
warning messages in the upstream kernel where our general policy is to
usually copy downstream hardware clock rates across the range of Qualcomm
drivers.

Drop the warning it is pervasive across CAMSS users but doesn't add any
information or warrant any changes to the DT to align the DT clock rate to
the bootloader clock rate.

Signed-off-by: Bryan O'Donoghue <[e-mail]>
Reviewed-by: Vladimir Zapolskiy <[e-mail]>
Link: https://lore.kernel.org/linux-arm-msm/[e-mail]
Signed-off-by: Richard Acayan <[e-mail]>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 8 --------
 1 file changed, 8 deletions(-)

Comments

Konrad Dybcio Sept. 5, 2024, 1:57 p.m. UTC | #1

On 4.09.2024 4:04 AM, Richard Acayan wrote:
> From: Bryan O'Donoghue <[e-mail]>
> 
> It is common practice in the downstream and upstream CCI dt to set CCI
> clock rates to 19.2 MHz. It appears to be fairly common for initial code to
> set the CCI clock rate to 37.5 MHz.
> 
> Applying the widely used CCI clock rates from downstream ought not to cause
> warning messages in the upstream kernel where our general policy is to
> usually copy downstream hardware clock rates across the range of Qualcomm
> drivers.
> 
> Drop the warning it is pervasive across CAMSS users but doesn't add any
> information or warrant any changes to the DT to align the DT clock rate to
> the bootloader clock rate.
> 
> Signed-off-by: Bryan O'Donoghue <[e-mail]>
> Reviewed-by: Vladimir Zapolskiy <[e-mail]>
> Link: https://lore.kernel.org/linux-arm-msm/[e-mail]
> Signed-off-by: Richard Acayan <[e-mail]>
> ---

I.. am not sure this is really a problem? On some platforms the core
clock is only 19.2 Mhz, but e.g. on sdm845 we have:

static const struct freq_tbl ftbl_cam_cc_cci_clk_src[] = {
        F(19200000, P_BI_TCXO, 1, 0, 0),
        F(37500000, P_CAM_CC_PLL0_OUT_EVEN, 16, 0, 0),
        F(50000000, P_CAM_CC_PLL0_OUT_EVEN, 12, 0, 0),
        F(100000000, P_CAM_CC_PLL0_OUT_EVEN, 6, 0, 0),
        { }
};

Shouldn't this be somehow dynamically calculated?

Konrad

Vladimir Zapolskiy Sept. 5, 2024, 2:18 p.m. UTC | #2

Hi Konrad,

On 9/5/24 16:57, Konrad Dybcio wrote:
> On 4.09.2024 4:04 AM, Richard Acayan wrote:
>> From: Bryan O'Donoghue <[e-mail]>
>>
>> It is common practice in the downstream and upstream CCI dt to set CCI
>> clock rates to 19.2 MHz. It appears to be fairly common for initial code to
>> set the CCI clock rate to 37.5 MHz.
>>
>> Applying the widely used CCI clock rates from downstream ought not to cause
>> warning messages in the upstream kernel where our general policy is to
>> usually copy downstream hardware clock rates across the range of Qualcomm
>> drivers.
>>
>> Drop the warning it is pervasive across CAMSS users but doesn't add any
>> information or warrant any changes to the DT to align the DT clock rate to
>> the bootloader clock rate.
>>
>> Signed-off-by: Bryan O'Donoghue <[e-mail]>
>> Reviewed-by: Vladimir Zapolskiy <[e-mail]>
>> Link: https://lore.kernel.org/linux-arm-msm/[e-mail]
>> Signed-off-by: Richard Acayan <[e-mail]>
>> ---
> 
> I.. am not sure this is really a problem? On some platforms the core
> clock is only 19.2 Mhz, but e.g. on sdm845 we have:
> 
> static const struct freq_tbl ftbl_cam_cc_cci_clk_src[] = {
>          F(19200000, P_BI_TCXO, 1, 0, 0),
>          F(37500000, P_CAM_CC_PLL0_OUT_EVEN, 16, 0, 0),
>          F(50000000, P_CAM_CC_PLL0_OUT_EVEN, 12, 0, 0),
>          F(100000000, P_CAM_CC_PLL0_OUT_EVEN, 6, 0, 0),
>          { }
> };
> 
> Shouldn't this be somehow dynamically calculated?
> 

I believe the problem fixed by the change is an unnecessary dev_warn(), in
addition it's unclear why the CCI clock rate shall be strictly 37500000 for
all "CCI v2" platforms.

If the latter is a necessity, then it would be better to set the rate
explicitly, however since it's not done for any such platforms, I would say
that it is not needed.

And if it is not needed, or a default and universal 19.2MHz rate is good
enough, then I would suggest to remove everything 'cci_clk_rate' related
from the driver, and this makes the change incomplete...

--
Best wishes,
Vladimir

Konrad Dybcio Sept. 5, 2024, 3:18 p.m. UTC | #3

On 5.09.2024 4:18 PM, Vladimir Zapolskiy wrote:
> Hi Konrad,
> 
> On 9/5/24 16:57, Konrad Dybcio wrote:
>> On 4.09.2024 4:04 AM, Richard Acayan wrote:
>>> From: Bryan O'Donoghue <[e-mail]>
>>>
>>> It is common practice in the downstream and upstream CCI dt to set CCI
>>> clock rates to 19.2 MHz. It appears to be fairly common for initial code to
>>> set the CCI clock rate to 37.5 MHz.
>>>
>>> Applying the widely used CCI clock rates from downstream ought not to cause
>>> warning messages in the upstream kernel where our general policy is to
>>> usually copy downstream hardware clock rates across the range of Qualcomm
>>> drivers.
>>>
>>> Drop the warning it is pervasive across CAMSS users but doesn't add any
>>> information or warrant any changes to the DT to align the DT clock rate to
>>> the bootloader clock rate.
>>>
>>> Signed-off-by: Bryan O'Donoghue <[e-mail]>
>>> Reviewed-by: Vladimir Zapolskiy <[e-mail]>
>>> Link: https://lore.kernel.org/linux-arm-msm/[e-mail]
>>> Signed-off-by: Richard Acayan <[e-mail]>
>>> ---
>>
>> I.. am not sure this is really a problem? On some platforms the core
>> clock is only 19.2 Mhz, but e.g. on sdm845 we have:
>>
>> static const struct freq_tbl ftbl_cam_cc_cci_clk_src[] = {
>>          F(19200000, P_BI_TCXO, 1, 0, 0),
>>          F(37500000, P_CAM_CC_PLL0_OUT_EVEN, 16, 0, 0),
>>          F(50000000, P_CAM_CC_PLL0_OUT_EVEN, 12, 0, 0),
>>          F(100000000, P_CAM_CC_PLL0_OUT_EVEN, 6, 0, 0),
>>          { }
>> };
>>
>> Shouldn't this be somehow dynamically calculated?
>>
> 
> I believe the problem fixed by the change is an unnecessary dev_warn(), in
> addition it's unclear why the CCI clock rate shall be strictly 37500000 for
> all "CCI v2" platforms.
> 
> If the latter is a necessity, then it would be better to set the rate
> explicitly, however since it's not done for any such platforms, I would say
> that it is not needed.
> 
> And if it is not needed, or a default and universal 19.2MHz rate is good
> enough, then I would suggest to remove everything 'cci_clk_rate' related
> from the driver, and this makes the change incomplete...

Downstream seems to set 37500000 for all SoCs with Titan.. but I can't
find a good reason for this.. I'll try to get some answers.

Konrad

Bryan O'Donoghue Sept. 5, 2024, 4:05 p.m. UTC | #4

On 05/09/2024 14:57, Konrad Dybcio wrote:
> On 4.09.2024 4:04 AM, Richard Acayan wrote:
>> From: Bryan O'Donoghue <[e-mail]>
>>
>> It is common practice in the downstream and upstream CCI dt to set CCI
>> clock rates to 19.2 MHz. It appears to be fairly common for initial code to
>> set the CCI clock rate to 37.5 MHz.
>>
>> Applying the widely used CCI clock rates from downstream ought not to cause
>> warning messages in the upstream kernel where our general policy is to
>> usually copy downstream hardware clock rates across the range of Qualcomm
>> drivers.
>>
>> Drop the warning it is pervasive across CAMSS users but doesn't add any
>> information or warrant any changes to the DT to align the DT clock rate to
>> the bootloader clock rate.
>>
>> Signed-off-by: Bryan O'Donoghue <[e-mail]>
>> Reviewed-by: Vladimir Zapolskiy <[e-mail]>
>> Link: https://lore.kernel.org/linux-arm-msm/[e-mail]
>> Signed-off-by: Richard Acayan <[e-mail]>
>> ---
> 
> I.. am not sure this is really a problem? On some platforms the core
> clock is only 19.2 Mhz, but e.g. on sdm845 we have:
> 
> static const struct freq_tbl ftbl_cam_cc_cci_clk_src[] = {
>          F(19200000, P_BI_TCXO, 1, 0, 0),
>          F(37500000, P_CAM_CC_PLL0_OUT_EVEN, 16, 0, 0),
>          F(50000000, P_CAM_CC_PLL0_OUT_EVEN, 12, 0, 0),
>          F(100000000, P_CAM_CC_PLL0_OUT_EVEN, 6, 0, 0),
>          { }
> };

CCI latches the code from DT and I assume that people submitting dts 
have actually tested their sensors when they do so.

The complaint about not being 19.2 MHz is surely not valid since, it can 
be any number of frequencies.

Its a redundant and useless warning.

We can do extra work to align to a set of frequencies sure but, the 
warning is not a warning about a real thing.

---
bod

Patch

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index 414882c57d7f..99e4305a3373 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -602,14 +602,6 @@  static int cci_probe(struct platform_device *pdev)
 		}
 	}
 
-	if (cci_clk_rate != cci->data->cci_clk_rate) {
-		/* cci clock set by the bootloader or via assigned clock rate
-		 * in DT.
-		 */
-		dev_warn(dev, "Found %lu cci clk rate while %lu was expected\n",
-			 cci_clk_rate, cci->data->cci_clk_rate);
-	}
-
 	ret = cci_enable_clocks(cci);
 	if (ret < 0)
 		return ret;