Re: [PATCH 2/3] thermal: qcom: tsens: Add support for 'needs_reinit_wa' for sm8150

[Bhupesh Sharma <bhupesh.sharma@xxxxxxxxxx>]

Hi Bjorn,

Thanks for your review.

On 7/19/22 8:25 AM, Bjorn Andersson wrote:
> On Fri 01 Jul 09:58 CDT 2022, Bhupesh Sharma wrote:
>
> > QCoM sm8150 tsens controller might require re-initialization
>
> Please spell out Qualcomm.

Ok.

> > via trustzone [via scm call(s)] when it enters a 'bad state'
> > causing sensor temperatures/interrupts status to be in an
> > 'invalid' state.
> >
> > Add hooks for the same in the qcom tsens driver which
> > can be used by followup patch(es).
>
> This patch enables needs_reinit_wa, which is actually implemented in
> patch 3, wouldn't it make more sense to flip them around; to first
> implement the feature and then enable it in this patch?

Yes, this was reported by the kernel test bot as well.
I will address this in v2.

> > Cc: Amit Kucheria <amitk@xxxxxxxxxx>
> > Cc: Thara Gopinath <thara.gopinath@xxxxxxxxx>
> > Cc: linux-pm@xxxxxxxxxxxxxxx
> > Cc: linux-arm-msm@xxxxxxxxxxxxxxx
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@xxxxxxxxxx>
> > ---
> >   drivers/thermal/qcom/tsens-v2.c | 11 +++++++++++
> >   drivers/thermal/qcom/tsens.c    |  4 ++++
> >   drivers/thermal/qcom/tsens.h    |  6 +++++-
> >   3 files changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
> > index b293ed32174b..61d38a56d29a 100644
> > --- a/drivers/thermal/qcom/tsens-v2.c
> > +++ b/drivers/thermal/qcom/tsens-v2.c
> > @@ -101,6 +101,17 @@ struct tsens_plat_data data_tsens_v2 = {
> >   	.fields	= tsens_v2_regfields,
> >   };
> >   
> > +/* For sm8150 tsens, its suggested to monitor the controller health
>
> /*
>   * Outside the network stack, the first line should be left empty in
>   * multiline comments.
>   */

Ok.

> > + * periodically and in case an issue is detected to reinit tsens
> > + * controller via trustzone.
> > + */
> > +struct tsens_plat_data data_tsens_sm8150 = {
>
> I doubt this is sm8150-specific, so the first question is if this should
> be attempted on all data_tsens_v2 platforms. Otherwise, how about naming
> this data_tsens_v2_reinit?

I agree. Konrad reported one more use case of the same.
So, I will take care of the same in v2.

'data_tsens_v2_reinit' makes sense to me.

Regards,
Bhupesh

> > +	.ops		= &ops_generic_v2,
> > +	.feat		= &tsens_v2_feat,
> > +	.needs_reinit_wa = true,
> > +	.fields	= tsens_v2_regfields,
> > +};
> > +
> >   /* Kept around for backward compatibility with old msm8996.dtsi */
> >   struct tsens_plat_data data_8996 = {
> >   	.num_sensors	= 13,
> > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> > index 7963ee33bf75..97f4d4454f20 100644
> > --- a/drivers/thermal/qcom/tsens.c
> > +++ b/drivers/thermal/qcom/tsens.c
> > @@ -991,6 +991,9 @@ static const struct of_device_id tsens_table[] = {
> >   	}, {
> >   		.compatible = "qcom,msm8996-tsens",
> >   		.data = &data_8996,
> > +	}, {
> > +		.compatible = "qcom,sm8150-tsens",
> > +		.data = &data_tsens_sm8150,
> >   	}, {
> >   		.compatible = "qcom,tsens-v1",
> >   		.data = &data_tsens_v1,
> > @@ -1135,6 +1138,7 @@ static int tsens_probe(struct platform_device *pdev)
> >   
> >   	priv->dev = dev;
> >   	priv->num_sensors = num_sensors;
> > +	priv->needs_reinit_wa = data->needs_reinit_wa;
> >   	priv->ops = data->ops;
> >   	for (i = 0;  i < priv->num_sensors; i++) {
> >   		if (data->hw_ids)
> > diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> > index 1471a2c00f15..48a7bda902c1 100644
> > --- a/drivers/thermal/qcom/tsens.h
> > +++ b/drivers/thermal/qcom/tsens.h
> > @@ -515,6 +515,7 @@ struct tsens_features {
> >    * @num_sensors: Number of sensors supported by platform
> >    * @ops: operations the tsens instance supports
> >    * @hw_ids: Subset of sensors ids supported by platform, if not the first n
> > + * @needs_reinit_wa: tsens controller might need reinit via trustzone
> >    * @feat: features of the IP
> >    * @fields: bitfield locations
> >    */
> > @@ -522,6 +523,7 @@ struct tsens_plat_data {
> >   	const u32		num_sensors;
> >   	const struct tsens_ops	*ops;
> >   	unsigned int		*hw_ids;
> > +	bool			needs_reinit_wa;
> >   	struct tsens_features	*feat;
> >   	const struct reg_field		*fields;
> >   };
> > @@ -544,6 +546,7 @@ struct tsens_context {
> >    * @srot_map: pointer to SROT register address space
> >    * @tm_offset: deal with old device trees that don't address TM and SROT
> >    *             address space separately
> > + * @needs_reinit_wa: tsens controller might need reinit via trustzone
> >    * @ul_lock: lock while processing upper/lower threshold interrupts
> >    * @crit_lock: lock while processing critical threshold interrupts
> >    * @rf: array of regmap_fields used to store value of the field
> > @@ -561,6 +564,7 @@ struct tsens_priv {
> >   	struct regmap			*tm_map;
> >   	struct regmap			*srot_map;
> >   	u32				tm_offset;
> > +	bool				needs_reinit_wa;
> >   
> >   	/* lock for upper/lower threshold interrupts */
> >   	spinlock_t			ul_lock;
> > @@ -593,6 +597,6 @@ extern struct tsens_plat_data data_8916, data_8939, data_8974, data_9607;
> >   extern struct tsens_plat_data data_tsens_v1, data_8976;
> >   
> >   /* TSENS v2 targets */
> > -extern struct tsens_plat_data data_8996, data_tsens_v2;
> > +extern struct tsens_plat_data data_8996, data_tsens_sm8150, data_tsens_v2;
> >   
> >   #endif /* __QCOM_TSENS_H__ */
> > --
> > 2.35.3