| Message ID | 20240209144221.3602382-2-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add support for H616 thermal system | expand |
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) >
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) > > > > > > >
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) > > > > > > > > > > > > > > >
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)
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. 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(+)