[PATCH v4 1/7] soc: sunxi: sram: export register 0 for THS on H616

[Jernej Škrabec]

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 at 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 at 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)
> > >   
> > 
> > 
> > 
> > 
> > 
> 
>