diff mbox series

[v2,12/13] media: qcom: camss: Add support for setting CSIPHY clock name csiphyX

Message ID 20230817143812.677554-13-bryan.odonoghue@linaro.org
State Superseded
Headers show
Series media: qcom: camss: Add parameter passing to remove several outstanding bugs | expand

Commit Message

Bryan O'Donoghue Aug. 17, 2023, 2:38 p.m. UTC
Several of our upstream and soon-to-be upstream SoC CAMSS dtsi declare
csiphyX as opposed to the older clock name csiX_phy.

For newer SoCs csiphyX turns out to be a clock you really need to set.

On sc8280xp for example we will encounter difficult to track down and
root-cause RX CRC errors without setting the csiX_phy clock. On sdm845 and
sm8250 we declare the csiXphy clock but seem to get away with not setting
it.

The right approach here is to set the clock when it is declared. If a SoC
doesn't require or a SoC driver implementer doesn't think we need, then the
clock ought to simply be omitted from the clock list.

Include csiphyX in the set of permissible strings which will subsequently
lead to the csiphyX clock being set during csiphy_set_clock_rates() phase.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-csiphy.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Konrad Dybcio Aug. 18, 2023, 12:32 p.m. UTC | #1
On 17.08.2023 16:38, Bryan O'Donoghue wrote:
> Several of our upstream and soon-to-be upstream SoC CAMSS dtsi declare
> csiphyX as opposed to the older clock name csiX_phy.
> 
> For newer SoCs csiphyX turns out to be a clock you really need to set.
> 
> On sc8280xp for example we will encounter difficult to track down and
> root-cause RX CRC errors without setting the csiX_phy clock. On sdm845 and
> sm8250 we declare the csiXphy clock but seem to get away with not setting
> it.
> 
> The right approach here is to set the clock when it is declared. If a SoC
> doesn't require or a SoC driver implementer doesn't think we need, then the
> clock ought to simply be omitted from the clock list.
> 
> Include csiphyX in the set of permissible strings which will subsequently
> lead to the csiphyX clock being set during csiphy_set_clock_rates() phase.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
So.. is this just a namechange? Is it really necessary?

Konrad
Bryan O'Donoghue Aug. 19, 2023, 2:21 p.m. UTC | #2
On 18/08/2023 13:32, Konrad Dybcio wrote:
> On 17.08.2023 16:38, Bryan O'Donoghue wrote:
>> Several of our upstream and soon-to-be upstream SoC CAMSS dtsi declare
>> csiphyX as opposed to the older clock name csiX_phy.
>>
>> For newer SoCs csiphyX turns out to be a clock you really need to set.
>>
>> On sc8280xp for example we will encounter difficult to track down and
>> root-cause RX CRC errors without setting the csiX_phy clock. On sdm845 and
>> sm8250 we declare the csiXphy clock but seem to get away with not setting
>> it.
>>
>> The right approach here is to set the clock when it is declared. If a SoC
>> doesn't require or a SoC driver implementer doesn't think we need, then the
>> clock ought to simply be omitted from the clock list.
>>
>> Include csiphyX in the set of permissible strings which will subsequently
>> lead to the csiphyX clock being set during csiphy_set_clock_rates() phase.
>>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
> So.. is this just a namechange? Is it really necessary?
> 
> Konrad

Not at all no.

We currently don't set csiphyX clocks. That's "not a problem" on sdm845 
and sm8250 "because" but on sc8280xp if you don't set that clock you'll 
get all sorts of RX CRC errors.

Obvs my extensive and detailed commit log transmits exactly no detail.

Let me fix that in the next version.

;)

---
bod
diff mbox series

Patch

diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
index baf78c525fbfc..d9c751f457703 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
@@ -687,6 +687,10 @@  int msm_csiphy_subdev_init(struct camss *camss,
 				if (csiphy->rate_set[i])
 					break;
 			}
+
+			csiphy->rate_set[i] = csiphy_match_clock_name(clock->name, "csiphy%d", k);
+			if (csiphy->rate_set[i])
+				break;
 		}
 	}