mbox series

[v4,0/7] add support for H616 thermal system

Message ID 20240209144221.3602382-1-andre.przywara@arm.com
Headers show
Series add support for H616 thermal system | expand

Message

Andre Przywara Feb. 9, 2024, 2:42 p.m. UTC 
Hi,

this is v4 of the series originally by Martin, complemented by patches
to fix the problem with way-too-high temperatures reported on
some boards, and some refactoring and simplifications, compared to the
original drop.

For the SYS_CFG bit poke code I now avoided the term "syscon" at all,
instead using "sram" instead, which seems to be less controversial. Apart
from that the register poke part hasn't changed much, but I hope it's more
acceptable now that it doesn't claim to be using a syscon device.

Many thanks to Maksim's investigation into the code, which revealed that
the calibrate functions between the H6 and H616 are actually the same,
just with support for more sensors. So his new patch 4/7 refactors the
existing function, to make it compatible to the H616, which makes the
actual support patch 6/7 very simple.

See the Changelog below for more details.

==================

This series introduces support for the thermal sensors in the Allwinner
H616 SoCs, which includes its siblings H618 and T507. The actual
temperature reading turns out to be very similar to the H6 SoC, just
with support for two more sensors. One nasty complication is caused
by reports about temperatures above 200C, which are related to the
firmware being run (because the vendor U-Boot contains a hack avoiding
this problem). Some investigation and digging in BSP code later
we identified that bit 16 in register 0x3000000 (SYS_CFG) needs to be
cleared for the raw temperature register values to contain reasonable
values.
To achieve this, patch 1/7 exports this very register from the already
existing SRAM/syscon device. Patch 5/7 then adds code to the thermal
driver to find that device via a new DT property, and query its regmap
to clear bit 16 in there.

I am open to suggestions on how to solve this in a better way, but the
current solution works, and uses an existing driver and an existing
scheme (in the EMAC driver).

The rest of the patches are fairly straight-forward and build on
Martin's original work, with some simplifications, resulting in more
code sharing.

Please have a look!

Cheers,
Andre

Changelog v3 .. v4:
- rebase on top of v6.8-rc2
- rework SYS_CFG bit poking patches to avoid syscon
- use sram lock for the SRAM driver regmap as well
- correctly advertise new allwinner,sram property in binding
- fix conditional definition of sram property
- new patch 4/7 to make the H6 and H616 calibrate functions compatible
- drop now obsolete definition of sun50i_h616_ths_calibrate()

Changelog v2 .. v3:
- rebase on top of v6.7-rc3
- add patches to clear bit 16 in SYS_CFG register 0x3000000
- add syscon to the binding documentation
- add patch explaining the unknown control register value

Changelog v1 .. v2:
- Fix typos
- Remove h616 calc and init functions
- Use TEMP_CALIB_MASK insteaf of 0xffff
- Adjust calibration function to new offset and scale
- Add proper comment to bindings patch
- Split delta calculations to 2 lines
- Add parentheses around caldata[2|3] >> 12
- Negate bindings if for clocks


Andre Przywara (3):
  soc: sunxi: sram: export register 0 for THS on H616
  thermal: sun8i: explain unknown H6 register value
  thermal: sun8i: add SRAM register access code

Maksim Kiselev (1):
  thermal: sun8i: extend H6 calibration to support 4 sensors

Martin Botka (3):
  dt-bindings: thermal: sun8i: Add H616 THS controller
  thermal: sun8i: add support for H616 THS controller
  arm64: dts: allwinner: h616: Add thermal sensor and zones

 .../thermal/allwinner,sun8i-a83t-ths.yaml     |  34 +++--
 .../arm64/boot/dts/allwinner/sun50i-h616.dtsi |  88 +++++++++++++
 drivers/soc/sunxi/sunxi_sram.c                |  23 ++++
 drivers/thermal/sun8i_thermal.c               | 122 +++++++++++++++---
 4 files changed, 235 insertions(+), 32 deletions(-)

Comments

Jernej Škrabec Feb. 14, 2024, 8:29 p.m. UTC | #1 
Dne petek, 09. februar 2024 ob 15:42:15 CET je Andre Przywara napisal(a):
> The Allwinner H616 SoC contains a mysterious bit at register offset 0x0
> in the SRAM control block. If bit 16 is set (the reset value), the
> temperature readings of the THS are way off, leading to reports about
> 200C, at normal ambient temperatures. Clearing this bits brings the
> reported values down to reasonable ranges.
> The BSP code clears this bit in firmware (U-Boot), and has an explicit
> comment about this, but offers no real explanation.
> 
> Since we should not rely on firmware settings, allow other code (the THS
> driver) to access this register, by exporting it through the already
> existing regmap. This mimics what we already do for the LDO control and
> the EMAC register.

Are you sure that this bit doesn't control actual SRAM region?

Best regards,
Jernej

> 
> Since this bit is in the very same register as the actual SRAM switch,
> we need to change the regmap lock to the SRAM lock. Fortunately regmap
> has provisions for that, so we just need to hook in there.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/soc/sunxi/sunxi_sram.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/soc/sunxi/sunxi_sram.c b/drivers/soc/sunxi/sunxi_sram.c
> index 4458b2e0562b0..71cdd1b257eeb 100644
> --- a/drivers/soc/sunxi/sunxi_sram.c
> +++ b/drivers/soc/sunxi/sunxi_sram.c
> @@ -287,6 +287,7 @@ EXPORT_SYMBOL(sunxi_sram_release);
>  struct sunxi_sramc_variant {
>  	int num_emac_clocks;
>  	bool has_ldo_ctrl;
> +	bool has_ths_offset;
>  };
>  
>  static const struct sunxi_sramc_variant sun4i_a10_sramc_variant = {
> @@ -308,8 +309,10 @@ static const struct sunxi_sramc_variant sun50i_a64_sramc_variant = {
>  
>  static const struct sunxi_sramc_variant sun50i_h616_sramc_variant = {
>  	.num_emac_clocks = 2,
> +	.has_ths_offset = true,
>  };
>  
> +#define SUNXI_SRAM_THS_OFFSET_REG	0x0
>  #define SUNXI_SRAM_EMAC_CLOCK_REG	0x30
>  #define SUNXI_SYS_LDO_CTRL_REG		0x150
>  
> @@ -318,6 +321,8 @@ static bool sunxi_sram_regmap_accessible_reg(struct device *dev,
>  {
>  	const struct sunxi_sramc_variant *variant = dev_get_drvdata(dev);
>  
> +	if (reg == SUNXI_SRAM_THS_OFFSET_REG && variant->has_ths_offset)
> +		return true;
>  	if (reg >= SUNXI_SRAM_EMAC_CLOCK_REG &&
>  	    reg <  SUNXI_SRAM_EMAC_CLOCK_REG + variant->num_emac_clocks * 4)
>  		return true;
> @@ -327,6 +332,21 @@ static bool sunxi_sram_regmap_accessible_reg(struct device *dev,
>  	return false;
>  }
>  
> +
> +static void sunxi_sram_lock(void *_lock)
> +{
> +	spinlock_t *lock = _lock;
> +
> +	spin_lock(lock);
> +}
> +
> +static void sunxi_sram_unlock(void *_lock)
> +{
> +	spinlock_t *lock = _lock;
> +
> +	spin_unlock(lock);
> +}
> +
>  static struct regmap_config sunxi_sram_regmap_config = {
>  	.reg_bits       = 32,
>  	.val_bits       = 32,
> @@ -336,6 +356,9 @@ static struct regmap_config sunxi_sram_regmap_config = {
>  	/* other devices have no business accessing other registers */
>  	.readable_reg	= sunxi_sram_regmap_accessible_reg,
>  	.writeable_reg	= sunxi_sram_regmap_accessible_reg,
> +	.lock		= sunxi_sram_lock,
> +	.unlock		= sunxi_sram_unlock,
> +	.lock_arg	= &sram_lock,
>  };
>  
>  static int __init sunxi_sram_probe(struct platform_device *pdev)
>
Jernej Škrabec Feb. 14, 2024, 8:31 p.m. UTC | #2 
Dne petek, 09. februar 2024 ob 15:42:17 CET je Andre Przywara napisal(a):
> So far we were ORing in some "unknown" value into the THS control
> register on the Allwinner H6. This part of the register is not explained
> in the H6 manual, but the H616 manual details those bits, and on closer
> inspection the THS IP blocks in both SoCs seem very close:
> - The BSP code for both SoCs writes the same values into THS_CTRL.
> - The reset values of at least the first three registers are the same.
> 
> Replace the "unknown" value with its proper meaning: "acquire time",
> most probably the sample part of the sample & hold circuit of the ADC,
> according to its explanation in the H616 manual.
> 
> No functional change, just a macro rename and adjustment.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Nice!

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> ---
>  drivers/thermal/sun8i_thermal.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> index 6a8e386dbc8dc..42bee03c4e507 100644
> --- a/drivers/thermal/sun8i_thermal.c
> +++ b/drivers/thermal/sun8i_thermal.c
> @@ -50,7 +50,8 @@
>  #define SUN8I_THS_CTRL2_T_ACQ1(x)		((GENMASK(15, 0) & (x)) << 16)
>  #define SUN8I_THS_DATA_IRQ_STS(x)		BIT(x + 8)
>  
> -#define SUN50I_THS_CTRL0_T_ACQ(x)		((GENMASK(15, 0) & (x)) << 16)
> +#define SUN50I_THS_CTRL0_T_ACQ(x)		(GENMASK(15, 0) & ((x) - 1))
> +#define SUN50I_THS_CTRL0_T_SAMPLE_PER(x)	((GENMASK(15, 0) & ((x) - 1)) << 16)
>  #define SUN50I_THS_FILTER_EN			BIT(2)
>  #define SUN50I_THS_FILTER_TYPE(x)		(GENMASK(1, 0) & (x))
>  #define SUN50I_H6_THS_PC_TEMP_PERIOD(x)		((GENMASK(19, 0) & (x)) << 12)
> @@ -410,25 +411,27 @@ static int sun8i_h3_thermal_init(struct ths_device *tmdev)
>  	return 0;
>  }
>  
> -/*
> - * Without this undocumented value, the returned temperatures would
> - * be higher than real ones by about 20C.
> - */
> -#define SUN50I_H6_CTRL0_UNK 0x0000002f
> -
>  static int sun50i_h6_thermal_init(struct ths_device *tmdev)
>  {
>  	int val;
>  
>  	/*
> -	 * T_acq = 20us
> -	 * clkin = 24MHz
> -	 *
> -	 * x = T_acq * clkin - 1
> -	 *   = 479
> +	 * The manual recommends an overall sample frequency of 50 KHz (20us,
> +	 * 480 cycles at 24 MHz), which provides plenty of time for both the
> +	 * acquisition time (>24 cycles) and the actual conversion time
> +	 * (>14 cycles).
> +	 * The lower half of the CTRL register holds the "acquire time", in
> +	 * clock cycles, which the manual recommends to be 2us:
> +	 * 24MHz * 2us = 48 cycles.
> +	 * The high half of THS_CTRL encodes the sample frequency, in clock
> +	 * cycles: 24MHz * 20us = 480 cycles.
> +	 * This is explained in the H616 manual, but apparently wrongly
> +	 * described in the H6 manual, although the BSP code does the same
> +	 * for both SoCs.
>  	 */
>  	regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
> -		     SUN50I_H6_CTRL0_UNK | SUN50I_THS_CTRL0_T_ACQ(479));
> +		     SUN50I_THS_CTRL0_T_ACQ(48) |
> +		     SUN50I_THS_CTRL0_T_SAMPLE_PER(480));
>  	/* average over 4 samples */
>  	regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
>  		     SUN50I_THS_FILTER_EN |
>
Jernej Škrabec Feb. 14, 2024, 8:35 p.m. UTC | #3 
Dne petek, 09. februar 2024 ob 15:42:18 CET je Andre Przywara napisal(a):
> From: Maksim Kiselev <bigunclemax@gmail.com>
> 
> The H616 SoC resembles the H6 thermal sensor controller, with a few
> changes like four sensors.
> 
> Extend sun50i_h6_ths_calibrate() function to support calibration of
> these sensors.
> 
> Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
> Co-developed-by: Martin Botka <martin.botka@somainline.org>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej
Andre Przywara Feb. 15, 2024, 1:28 a.m. UTC | #4 
On Wed, 14 Feb 2024 21:29:30 +0100
Jernej Škrabec <jernej.skrabec@gmail.com> wrote:

Hi Jernej,

thanks for having a look and the tags on the other patches!

> Dne petek, 09. februar 2024 ob 15:42:15 CET je Andre Przywara napisal(a):
> > The Allwinner H616 SoC contains a mysterious bit at register offset 0x0
> > in the SRAM control block. If bit 16 is set (the reset value), the
> > temperature readings of the THS are way off, leading to reports about
> > 200C, at normal ambient temperatures. Clearing this bits brings the
> > reported values down to reasonable ranges.
> > The BSP code clears this bit in firmware (U-Boot), and has an explicit
> > comment about this, but offers no real explanation.
> > 
> > Since we should not rely on firmware settings, allow other code (the THS
> > driver) to access this register, by exporting it through the already
> > existing regmap. This mimics what we already do for the LDO control and
> > the EMAC register.  
> 
> Are you sure that this bit doesn't control actual SRAM region?

Pretty much so, yes: I did some experiments from U-Boot:
I filled SRAM C with some pattern, then read this back. Then flipped bit
16, read again: same result. Then wrote something again and read it
back: no change. In fact no bits at 0x3000000 had any effect on SRAM
accessibility, only clearing bit 24 in 0x3000004 made the whole SRAM C
(0x28000-0x47fff) go read-as-zero/write-ignore, from the CPU side.

I then triggered the THS device, to do temperature readings, but
this didn't change a single byte in the SRAM regions, with or without
bit 16 set. It only changed the returned values, at 0x50704c0.

So yes, I am pretty certain there is no SRAM region that gets switched.
Even if we would want to claim there is: I wouldn't know which
address values to put into the SRAM DT node.

So I guess it's another example of: oh, we have this spare bit here. Or
it's some kind of chicken bit? I don't know, and I think the BSP code
we have seen didn't offer an explanation as well.

Cheers,
Andre
> 
> Best regards,
> Jernej
> 
> > 
> > Since this bit is in the very same register as the actual SRAM switch,
> > we need to change the regmap lock to the SRAM lock. Fortunately regmap
> > has provisions for that, so we just need to hook in there.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  drivers/soc/sunxi/sunxi_sram.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/drivers/soc/sunxi/sunxi_sram.c b/drivers/soc/sunxi/sunxi_sram.c
> > index 4458b2e0562b0..71cdd1b257eeb 100644
> > --- a/drivers/soc/sunxi/sunxi_sram.c
> > +++ b/drivers/soc/sunxi/sunxi_sram.c
> > @@ -287,6 +287,7 @@ EXPORT_SYMBOL(sunxi_sram_release);
> >  struct sunxi_sramc_variant {
> >  	int num_emac_clocks;
> >  	bool has_ldo_ctrl;
> > +	bool has_ths_offset;
> >  };
> >  
> >  static const struct sunxi_sramc_variant sun4i_a10_sramc_variant = {
> > @@ -308,8 +309,10 @@ static const struct sunxi_sramc_variant sun50i_a64_sramc_variant = {
> >  
> >  static const struct sunxi_sramc_variant sun50i_h616_sramc_variant = {
> >  	.num_emac_clocks = 2,
> > +	.has_ths_offset = true,
> >  };
> >  
> > +#define SUNXI_SRAM_THS_OFFSET_REG	0x0
> >  #define SUNXI_SRAM_EMAC_CLOCK_REG	0x30
> >  #define SUNXI_SYS_LDO_CTRL_REG		0x150
> >  
> > @@ -318,6 +321,8 @@ static bool sunxi_sram_regmap_accessible_reg(struct device *dev,
> >  {
> >  	const struct sunxi_sramc_variant *variant = dev_get_drvdata(dev);
> >  
> > +	if (reg == SUNXI_SRAM_THS_OFFSET_REG && variant->has_ths_offset)
> > +		return true;
> >  	if (reg >= SUNXI_SRAM_EMAC_CLOCK_REG &&
> >  	    reg <  SUNXI_SRAM_EMAC_CLOCK_REG + variant->num_emac_clocks * 4)
> >  		return true;
> > @@ -327,6 +332,21 @@ static bool sunxi_sram_regmap_accessible_reg(struct device *dev,
> >  	return false;
> >  }
> >  
> > +
> > +static void sunxi_sram_lock(void *_lock)
> > +{
> > +	spinlock_t *lock = _lock;
> > +
> > +	spin_lock(lock);
> > +}
> > +
> > +static void sunxi_sram_unlock(void *_lock)
> > +{
> > +	spinlock_t *lock = _lock;
> > +
> > +	spin_unlock(lock);
> > +}
> > +
> >  static struct regmap_config sunxi_sram_regmap_config = {
> >  	.reg_bits       = 32,
> >  	.val_bits       = 32,
> > @@ -336,6 +356,9 @@ static struct regmap_config sunxi_sram_regmap_config = {
> >  	/* other devices have no business accessing other registers */
> >  	.readable_reg	= sunxi_sram_regmap_accessible_reg,
> >  	.writeable_reg	= sunxi_sram_regmap_accessible_reg,
> > +	.lock		= sunxi_sram_lock,
> > +	.unlock		= sunxi_sram_unlock,
> > +	.lock_arg	= &sram_lock,
> >  };
> >  
> >  static int __init sunxi_sram_probe(struct platform_device *pdev)
> >   
> 
> 
> 
> 
>
Jernej Škrabec Feb. 15, 2024, 9:18 p.m. UTC | #5 
Dne četrtek, 15. februar 2024 ob 02:28:47 CET je Andre Przywara napisal(a):
> On Wed, 14 Feb 2024 21:29:30 +0100
> Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
> 
> Hi Jernej,
> 
> thanks for having a look and the tags on the other patches!
> 
> > Dne petek, 09. februar 2024 ob 15:42:15 CET je Andre Przywara napisal(a):
> > > The Allwinner H616 SoC contains a mysterious bit at register offset 0x0
> > > in the SRAM control block. If bit 16 is set (the reset value), the
> > > temperature readings of the THS are way off, leading to reports about
> > > 200C, at normal ambient temperatures. Clearing this bits brings the
> > > reported values down to reasonable ranges.
> > > The BSP code clears this bit in firmware (U-Boot), and has an explicit
> > > comment about this, but offers no real explanation.
> > > 
> > > Since we should not rely on firmware settings, allow other code (the THS
> > > driver) to access this register, by exporting it through the already
> > > existing regmap. This mimics what we already do for the LDO control and
> > > the EMAC register.  
> > 
> > Are you sure that this bit doesn't control actual SRAM region?
> 
> Pretty much so, yes: I did some experiments from U-Boot:
> I filled SRAM C with some pattern, then read this back. Then flipped bit
> 16, read again: same result. Then wrote something again and read it
> back: no change. In fact no bits at 0x3000000 had any effect on SRAM
> accessibility, only clearing bit 24 in 0x3000004 made the whole SRAM C
> (0x28000-0x47fff) go read-as-zero/write-ignore, from the CPU side.
> 
> I then triggered the THS device, to do temperature readings, but
> this didn't change a single byte in the SRAM regions, with or without
> bit 16 set. It only changed the returned values, at 0x50704c0.
> 
> So yes, I am pretty certain there is no SRAM region that gets switched.
> Even if we would want to claim there is: I wouldn't know which
> address values to put into the SRAM DT node.
> 
> So I guess it's another example of: oh, we have this spare bit here. Or
> it's some kind of chicken bit? I don't know, and I think the BSP code
> we have seen didn't offer an explanation as well.
> 

It would be nice to mention this in commit message.

> Cheers,
> Andre
> > 
> > Best regards,
> > Jernej
> > 
> > > 
> > > Since this bit is in the very same register as the actual SRAM switch,
> > > we need to change the regmap lock to the SRAM lock. Fortunately regmap
> > > has provisions for that, so we just need to hook in there.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > >  drivers/soc/sunxi/sunxi_sram.c | 23 +++++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > > 
> > > diff --git a/drivers/soc/sunxi/sunxi_sram.c b/drivers/soc/sunxi/sunxi_sram.c
> > > index 4458b2e0562b0..71cdd1b257eeb 100644
> > > --- a/drivers/soc/sunxi/sunxi_sram.c
> > > +++ b/drivers/soc/sunxi/sunxi_sram.c
> > > @@ -287,6 +287,7 @@ EXPORT_SYMBOL(sunxi_sram_release);
> > >  struct sunxi_sramc_variant {
> > >  	int num_emac_clocks;
> > >  	bool has_ldo_ctrl;
> > > +	bool has_ths_offset;
> > >  };
> > >  
> > >  static const struct sunxi_sramc_variant sun4i_a10_sramc_variant = {
> > > @@ -308,8 +309,10 @@ static const struct sunxi_sramc_variant sun50i_a64_sramc_variant = {
> > >  
> > >  static const struct sunxi_sramc_variant sun50i_h616_sramc_variant = {
> > >  	.num_emac_clocks = 2,
> > > +	.has_ths_offset = true,
> > >  };
> > >  
> > > +#define SUNXI_SRAM_THS_OFFSET_REG	0x0
> > >  #define SUNXI_SRAM_EMAC_CLOCK_REG	0x30
> > >  #define SUNXI_SYS_LDO_CTRL_REG		0x150
> > >  
> > > @@ -318,6 +321,8 @@ static bool sunxi_sram_regmap_accessible_reg(struct device *dev,
> > >  {
> > >  	const struct sunxi_sramc_variant *variant = dev_get_drvdata(dev);
> > >  
> > > +	if (reg == SUNXI_SRAM_THS_OFFSET_REG && variant->has_ths_offset)
> > > +		return true;
> > >  	if (reg >= SUNXI_SRAM_EMAC_CLOCK_REG &&
> > >  	    reg <  SUNXI_SRAM_EMAC_CLOCK_REG + variant->num_emac_clocks * 4)
> > >  		return true;
> > > @@ -327,6 +332,21 @@ static bool sunxi_sram_regmap_accessible_reg(struct device *dev,
> > >  	return false;
> > >  }
> > >  
> > > +

Nit: superfluous empty line.

Best regards,
Jernej

> > > +static void sunxi_sram_lock(void *_lock)
> > > +{
> > > +	spinlock_t *lock = _lock;
> > > +
> > > +	spin_lock(lock);
> > > +}
> > > +
> > > +static void sunxi_sram_unlock(void *_lock)
> > > +{
> > > +	spinlock_t *lock = _lock;
> > > +
> > > +	spin_unlock(lock);
> > > +}
> > > +
> > >  static struct regmap_config sunxi_sram_regmap_config = {
> > >  	.reg_bits       = 32,
> > >  	.val_bits       = 32,
> > > @@ -336,6 +356,9 @@ static struct regmap_config sunxi_sram_regmap_config = {
> > >  	/* other devices have no business accessing other registers */
> > >  	.readable_reg	= sunxi_sram_regmap_accessible_reg,
> > >  	.writeable_reg	= sunxi_sram_regmap_accessible_reg,
> > > +	.lock		= sunxi_sram_lock,
> > > +	.unlock		= sunxi_sram_unlock,
> > > +	.lock_arg	= &sram_lock,
> > >  };
> > >  
> > >  static int __init sunxi_sram_probe(struct platform_device *pdev)
> > >   
> > 
> > 
> > 
> > 
> > 
> 
>
Jernej Škrabec Feb. 15, 2024, 9:24 p.m. UTC | #6 
Dne petek, 09. februar 2024 ob 15:42:19 CET je Andre Przywara napisal(a):
> The Allwinner H616 SoC needs to clear a bit in one register in the SRAM
> controller, to report reasonable temperature values. On reset, bit 16 in
> register 0x3000000 is set, which leads to the driver reporting
> temperatures around 200C. Clearing this bit brings the values down to the
> expected range. The BSP code does a one-time write in U-Boot, with a
> comment just mentioning the effect on the THS, but offering no further
> explanation.
> 
> To not rely on firmware to set things up for us, add code that queries
> the SRAM controller device via a DT phandle link, then clear just this
> single bit.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/thermal/sun8i_thermal.c | 50 +++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> index c919b0fd5e169..8a9d2bdc71ece 100644
> --- a/drivers/thermal/sun8i_thermal.c
> +++ b/drivers/thermal/sun8i_thermal.c
> @@ -15,6 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/nvmem-consumer.h>
>  #include <linux/of.h>
> +#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
> @@ -66,6 +67,7 @@ struct tsensor {
>  struct ths_thermal_chip {
>  	bool            has_mod_clk;
>  	bool            has_bus_clk_reset;
> +	bool		needs_sram;
>  	int		sensor_num;
>  	int		offset;
>  	int		scale;
> @@ -83,6 +85,7 @@ struct ths_device {
>  	const struct ths_thermal_chip		*chip;
>  	struct device				*dev;
>  	struct regmap				*regmap;
> +	struct regmap_field			*sram_regmap_field;
>  	struct reset_control			*reset;
>  	struct clk				*bus_clk;
>  	struct clk                              *mod_clk;
> @@ -337,6 +340,34 @@ static void sun8i_ths_reset_control_assert(void *data)
>  	reset_control_assert(data);
>  }
>  
> +static struct regmap *sun8i_ths_get_sram_regmap(struct device_node *node)
> +{
> +	struct device_node *sram_node;
> +	struct platform_device *sram_pdev;
> +	struct regmap *regmap = NULL;
> +
> +	sram_node = of_parse_phandle(node, "allwinner,sram", 0);
> +	if (!sram_node)
> +		return ERR_PTR(-ENODEV);
> +
> +	sram_pdev = of_find_device_by_node(sram_node);
> +	if (!sram_pdev) {
> +		/* platform device might not be probed yet */
> +		regmap = ERR_PTR(-EPROBE_DEFER);
> +		goto out_put_node;
> +	}
> +
> +	/* If no regmap is found then the other device driver is at fault */
> +	regmap = dev_get_regmap(&sram_pdev->dev, NULL);
> +	if (!regmap)
> +		regmap = ERR_PTR(-EINVAL);
> +
> +	platform_device_put(sram_pdev);
> +out_put_node:
> +	of_node_put(sram_node);
> +	return regmap;
> +}
> +
>  static int sun8i_ths_resource_init(struct ths_device *tmdev)
>  {
>  	struct device *dev = tmdev->dev;
> @@ -381,6 +412,21 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
>  	if (ret)
>  		return ret;
>  
> +	if (tmdev->chip->needs_sram) {
> +		const struct reg_field sun8i_sram_reg_field =
> +			REG_FIELD(0x0, 16, 16);

What about global static constant for a field? Many drivers do so and code 
would be simpler.

Best regards,
Jernej

> +		struct regmap *regmap;
> +
> +		regmap = sun8i_ths_get_sram_regmap(dev->of_node);
> +		if (IS_ERR(regmap))
> +			return PTR_ERR(regmap);
> +		tmdev->sram_regmap_field = devm_regmap_field_alloc(dev,
> +						      regmap,
> +						      sun8i_sram_reg_field);
> +		if (IS_ERR(tmdev->sram_regmap_field))
> +			return PTR_ERR(tmdev->sram_regmap_field);
> +	}
> +
>  	ret = sun8i_ths_calibrate(tmdev);
>  	if (ret)
>  		return ret;
> @@ -427,6 +473,10 @@ static int sun50i_h6_thermal_init(struct ths_device *tmdev)
>  {
>  	int val;
>  
> +	/* The H616 needs to have a bit in the SRAM control register cleared. */
> +	if (tmdev->sram_regmap_field)
> +		regmap_field_write(tmdev->sram_regmap_field, 0);
> +
>  	/*
>  	 * The manual recommends an overall sample frequency of 50 KHz (20us,
>  	 * 480 cycles at 24 MHz), which provides plenty of time for both the
>