From: Maciej S. Szmigiero <hidden> Date: 2015-12-20 20:33:13
There is no guarantee that on fsl_ssi module load
SSI registers will have their power-on-reset values.
In fact, if the driver is reloaded the values in
registers will be whatever they were set to previously.
This fixes hard lockup on fsl_ssi module reload,
at least in AC'97 mode.
Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support MEGA Fast")
Signed-off-by: Maciej S. Szmigiero <redacted>
---
sound/soc/fsl/fsl_ssi.c | 16 ----------------
1 file changed, 16 deletions(-)
On Sun, Dec 20, 2015 at 6:33 PM, Maciej S. Szmigiero
[off-list ref] wrote:
There is no guarantee that on fsl_ssi module load
SSI registers will have their power-on-reset values.
In fact, if the driver is reloaded the values in
registers will be whatever they were set to previously.
This fixes hard lockup on fsl_ssi module reload,
at least in AC'97 mode.
Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support MEGA Fast")
Signed-off-by: Maciej S. Szmigiero <redacted>
From: Timur Tabi <hidden> Date: 2016-01-10 21:36:35
Maciej S. Szmigiero wrote:
There is no guarantee that on fsl_ssi module load
SSI registers will have their power-on-reset values.
In fact, if the driver is reloaded the values in
registers will be whatever they were set to previously.
This fixes hard lockup on fsl_ssi module reload,
at least in AC'97 mode.
Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support MEGA Fast")
Signed-off-by: Maciej S. Szmigiero<redacted>
Acked-by: Timur Tabi <redacted>
I'm surprised that we're actually encouraging drivers to contain
hard-coded register values.
Hi Maciej,
On Sun, Dec 20, 2015 at 6:33 PM, Maciej S. Szmigiero
[off-list ref] wrote:
There is no guarantee that on fsl_ssi module load
SSI registers will have their power-on-reset values.
In fact, if the driver is reloaded the values in
registers will be whatever they were set to previously.
This fixes hard lockup on fsl_ssi module reload,
at least in AC'97 mode.
Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support MEGA Fast")
Signed-off-by: Maciej S. Szmigiero <redacted>
This will disable register cache so it isn't right.
Could you try REGCACHE_FLAT instead, please?
Looks like the problem here is rbtree cache does some non-atomic allocations in
read / write path when not supplied with default register values.
Best regards,
Maciej Szmigiero
I suspect not, it looks like the driver is using the cache for
suspend/resume handling. I've dropped the patch for now. Either the
driver should explicitly write to the relevant registers outside of
interrupt context to ensure the cache entry exists or it should keep the
defaults and explicitly write them to hardware at startup to ensure
sync (the former is more likely to be safe).
I suspect not, it looks like the driver is using the cache for
suspend/resume handling. I've dropped the patch for now. Either the
driver should explicitly write to the relevant registers outside of
interrupt context to ensure the cache entry exists or it should keep the
defaults and explicitly write them to hardware at startup to ensure
sync (the former is more likely to be safe).
Is it acceptable to switch it to flat cache instead to not keep the register
defaults in driver?
Maciej
From: Mark Brown <broonie@kernel.org> Date: 2016-01-11 14:54:47
On Mon, Jan 11, 2016 at 03:10:20PM +0100, Maciej S. Szmigiero wrote:
On 11.01.2016 15:00, Mark Brown wrote:
quoted
I suspect not, it looks like the driver is using the cache for
suspend/resume handling. I've dropped the patch for now. Either the
driver should explicitly write to the relevant registers outside of
interrupt context to ensure the cache entry exists or it should keep the
defaults and explicitly write them to hardware at startup to ensure
sync (the former is more likely to be safe).
Is it acceptable to switch it to flat cache instead to not keep the register
defaults in driver?
That's possibly problematic because the flat cache will of necessity end
up with defaults (of 0 from the kzalloc()) for all the registers.
You'll still have default values in the cache, though some of the
behaviour around optimising syncs does change without them explicitly
given. It does deal with the allocation issue but given that the issue
was incorrect defaults I'd be a bit concerned.
From: Timur Tabi <hidden> Date: 2016-01-11 15:45:40
Mark Brown wrote:
That's possibly problematic because the flat cache will of necessity end
up with defaults (of 0 from the kzalloc()) for all the registers.
You'll still have default values in the cache, though some of the
behaviour around optimising syncs does change without them explicitly
given. It does deal with the allocation issue but given that the issue
was incorrect defaults I'd be a bit concerned.
Ok, I'm confused. Granted, all of this regcache stuff was added after I
stopped working on this driver, so I'm out of the loop. But it appears
that the regcache cannot properly handle an uninitialized cache. I
would expect it to know to perform hard reads of any registers that are
uninitialized.
If the regcache wants to have an initialized cache, then it should
automatically perform reads an all non-volatile, non-precious registers
at initialization.
From: Mark Brown <broonie@kernel.org> Date: 2016-01-11 16:12:40
On Mon, Jan 11, 2016 at 09:45:37AM -0600, Timur Tabi wrote:
Ok, I'm confused. Granted, all of this regcache stuff was added after I
stopped working on this driver, so I'm out of the loop. But it appears that
the regcache cannot properly handle an uninitialized cache. I would expect
it to know to perform hard reads of any registers that are uninitialized.
regcache handles this fine, it's perfectly happy to just go and allocate
the cache as registers get used (this is why the code that's doing the
allocation exists...). What is causing problems here is that the first
access to the register is happening in interrupt context so we can't do
a GFP_KERNEL allocation for it. Most users don't do anything at all in
interrupt context so it's not an issue for them, drivers that want to
use regmap in interrupt context need to handle this.
We can't rely on knowing which registers are valid and which registers
can be read without side effects, it's optional for drivers to provide
that information. Even with that information it's not always clear that
we want to stop and read every single value when we are initialising the
device, that might be excessively slow (remember a lot of regmap devices
are I2C or SPI connected, some with large register maps). We should
have a helper to do that though for drivers where it does make sense.
From: Timur Tabi <hidden> Date: 2016-01-12 01:23:57
Mark Brown wrote:
regcache handles this fine, it's perfectly happy to just go and allocate
the cache as registers get used (this is why the code that's doing the
allocation exists...). What is causing problems here is that the first
access to the register is happening in interrupt context so we can't do
a GFP_KERNEL allocation for it.
Considering how small and not-sparse the SSI register space is, would
using REGCACHE_FLAT be appropriate?
From: Mark Brown <broonie@kernel.org> Date: 2016-01-12 01:34:37
On Mon, Jan 11, 2016 at 07:23:54PM -0600, Timur Tabi wrote:
Mark Brown wrote:
quoted
regcache handles this fine, it's perfectly happy to just go and allocate
the cache as registers get used (this is why the code that's doing the
allocation exists...). What is causing problems here is that the first
access to the register is happening in interrupt context so we can't do
a GFP_KERNEL allocation for it.
Considering how small and not-sparse the SSI register space is, would using
REGCACHE_FLAT be appropriate?
Quite possibly (it'll be more efficient and it's intended for such use
cases) but as I said in my other reply that then has the issue that it
implicitly gives default values to all the registers so I'd expect we
still need to handle the cache initialisation explicitly (or
alternatively the hardware sync with the cache on startup).
From: Timur Tabi <hidden> Date: 2016-01-12 01:53:49
Mark Brown wrote:
Quite possibly (it'll be more efficient and it's intended for such use
cases) but as I said in my other reply that then has the issue that it
implicitly gives default values to all the registers so I'd expect we
still need to handle the cache initialisation explicitly (or
alternatively the hardware sync with the cache on startup).
Why does REGCACHE_FLAT assume that all registers have a default value of
0? Shouldn't it have the same behavior w.r.t. cache values as
REGCACHE_RBTREE?
Is this really necessary? Why do we need separate register configs for
one specific SOC? There are already too many "if
(some_stupid_imx_variant)" blocks in this driver.
Is this really necessary? Why do we need separate register configs for one specific SOC?
There are already too many "if (some_stupid_imx_variant)" blocks in this driver.
This is because (at least according to the datasheet) imx21-class SSI
registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so
reading them for cache initialization may not be safe.
Also, a "MXC 91221 only" comment before these regs in FSL tree
(drivers/mxc/ssi/registers.h) seems to confirm that these registers
aren't present at least on some SSI (or SoC) models.
Best regards,
Maciej Szmigiero
From: Timur Tabi <hidden> Date: 2016-01-17 05:16:09
Maciej S. Szmigiero wrote:
This is because (at least according to the datasheet) imx21-class SSI
registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so
reading them for cache initialization may not be safe.
Also, a "MXC 91221 only" comment before these regs in FSL tree
(drivers/mxc/ssi/registers.h) seems to confirm that these registers
aren't present at least on some SSI (or SoC) models.
Can't we just mark them as precious or something, so that we don't have
to have two structures?
From: Maciej S. Szmigiero <hidden> Date: 2016-01-17 14:16:23
On 17.01.2016 06:16, Timur Tabi wrote:
Maciej S. Szmigiero wrote:
quoted
This is because (at least according to the datasheet) imx21-class SSI
registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so
reading them for cache initialization may not be safe.
Also, a "MXC 91221 only" comment before these regs in FSL tree
(drivers/mxc/ssi/registers.h) seems to confirm that these registers
aren't present at least on some SSI (or SoC) models.
Can't we just mark them as precious or something, so that we don't have to have two structures?
Looks like it can be done with just one static regmap config struct
used then as template - I will post updated patch.
Maciej
From: Maciej S. Szmigiero <hidden> Date: 2016-01-17 14:39:17
On 17.01.2016 15:16, Maciej S. Szmigiero wrote:
On 17.01.2016 06:16, Timur Tabi wrote:
quoted
Maciej S. Szmigiero wrote:
quoted
This is because (at least according to the datasheet) imx21-class SSI
registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so
reading them for cache initialization may not be safe.
Also, a "MXC 91221 only" comment before these regs in FSL tree
(drivers/mxc/ssi/registers.h) seems to confirm that these registers
aren't present at least on some SSI (or SoC) models.
Can't we just mark them as precious or something, so that we don't have to have two structures?
Looks like it can be done with just one static regmap config struct
used then as template - I will post updated patch.
@@ -1397,6 +1390,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)structresource*res;void__iomem*iomem;charname[64];+structregmap_configregconfig=fsl_ssi_regconfig;of_id=of_match_device(fsl_ssi_ids,&pdev->dev);if(!of_id||!of_id->data)
@@ -1444,15 +1438,22 @@ static int fsl_ssi_probe(struct platform_device *pdev)returnPTR_ERR(iomem);ssi_private->ssi_phys=res->start;+if(ssi_private->soc->imx21regs){+/* According to datasheet imx21-class SSI have less regs */+regconfig.max_register=CCSR_SSI_SRMSK;+regconfig.num_reg_defaults_raw=CCSR_SSI_SRMSK/4+1;+}+ret=of_property_match_string(np,"clock-names","ipg");if(ret<0){ssi_private->has_ipg_clk_name=false;ssi_private->regs=devm_regmap_init_mmio(&pdev->dev,iomem,-&fsl_ssi_regconfig);+®config);}else{ssi_private->has_ipg_clk_name=true;ssi_private->regs=devm_regmap_init_mmio_clk(&pdev->dev,-"ipg",iomem,&fsl_ssi_regconfig);+"ipg",iomem,+®config);}if(IS_ERR(ssi_private->regs)){dev_err(&pdev->dev,"Failed to init register map\n");
From: Timur Tabi <hidden> Date: 2016-01-17 18:38:25
Maciej S. Szmigiero wrote:
quoted hunk
On 17.01.2016 15:16, Maciej S. Szmigiero wrote:
quoted
On 17.01.2016 06:16, Timur Tabi wrote:
quoted
Maciej S. Szmigiero wrote:
quoted
This is because (at least according to the datasheet) imx21-class SSI
registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so
reading them for cache initialization may not be safe.
Also, a "MXC 91221 only" comment before these regs in FSL tree
(drivers/mxc/ssi/registers.h) seems to confirm that these registers
aren't present at least on some SSI (or SoC) models.
Can't we just mark them as precious or something, so that we don't have to have two structures?
Looks like it can be done with just one static regmap config struct
used then as template - I will post updated patch.
@@ -1444,15 +1438,22 @@ static int fsl_ssi_probe(struct platform_device *pdev) return PTR_ERR(iomem); ssi_private->ssi_phys = res->start;+ if (ssi_private->soc->imx21regs) {+ /* According to datasheet imx21-class SSI have less regs */
First of all, it would be "fewer regs", but even better would be to say
that certain regs don't exist.
However, I wonder if this patch is necessary at all. If the regs don't
exist on an i.MX 21, does it really matter if we write to them?
From: Maciej S. Szmigiero <hidden> Date: 2016-01-17 22:03:14
On 17.01.2016 19:38, Timur Tabi wrote:
Maciej S. Szmigiero wrote:
quoted
On 17.01.2016 15:16, Maciej S. Szmigiero wrote:
quoted
On 17.01.2016 06:16, Timur Tabi wrote:
quoted
Maciej S. Szmigiero wrote:
quoted
This is because (at least according to the datasheet) imx21-class SSI
registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so
reading them for cache initialization may not be safe.
Also, a "MXC 91221 only" comment before these regs in FSL tree
(drivers/mxc/ssi/registers.h) seems to confirm that these registers
aren't present at least on some SSI (or SoC) models.
Can't we just mark them as precious or something, so that we don't have to have two structures?
Looks like it can be done with just one static regmap config struct
used then as template - I will post updated patch.
Patch updated according to Timur's suggestions (needs regmap fix):
@@ -586,8 +573,12 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private *ssi_private)*/regmap_write(regs,CCSR_SSI_SACNT,CCSR_SSI_SACNT_AC97EN|CCSR_SSI_SACNT_FV);-regmap_write(regs,CCSR_SSI_SACCDIS,0xff);-regmap_write(regs,CCSR_SSI_SACCEN,0x300);++/* no SACC{ST,EN,DIS} regs on imx21-class SSI */+if(!ssi_private->soc->imx21regs){+regmap_write(regs,CCSR_SSI_SACCDIS,0xff);+regmap_write(regs,CCSR_SSI_SACCEN,0x300);+}/**EnableSSI,TransmitandReceive.AC97hastocommunicatewiththe
@@ -1397,6 +1388,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)structresource*res;void__iomem*iomem;charname[64];+structregmap_configregconfig=fsl_ssi_regconfig;of_id=of_match_device(fsl_ssi_ids,&pdev->dev);if(!of_id||!of_id->data)
@@ -1444,15 +1436,25 @@ static int fsl_ssi_probe(struct platform_device *pdev)returnPTR_ERR(iomem);ssi_private->ssi_phys=res->start;+if(ssi_private->soc->imx21regs){+/*+*Accordingtodatasheetimx21-classSSI+*don'thaveSACC{ST,EN,DIS}regs.+*/+regconfig.max_register=CCSR_SSI_SRMSK;+regconfig.num_reg_defaults_raw=+CCSR_SSI_SRMSK/sizeof(uint32_t)+1;+}+ret=of_property_match_string(np,"clock-names","ipg");if(ret<0){ssi_private->has_ipg_clk_name=false;ssi_private->regs=devm_regmap_init_mmio(&pdev->dev,iomem,-&fsl_ssi_regconfig);+®config);}else{ssi_private->has_ipg_clk_name=true;ssi_private->regs=devm_regmap_init_mmio_clk(&pdev->dev,-"ipg",iomem,&fsl_ssi_regconfig);+"ipg",iomem,®config);}if(IS_ERR(ssi_private->regs)){dev_err(&pdev->dev,"Failed to init register map\n");
However, I wonder if this patch is necessary at all.
If the regs don't exist on an i.MX 21, does it really matter if we write to them?
At least i.MX6 datasheet SSI description has the following information:
"Transfer bus errors are generated upon response to the following:
* Write transfer to a read-only register.
* Read or write access to a register space beyond the last populated register of the SSI
in its memory map (up until the end of the allocated memory address range of the
SSI)."
Maciej Szmigiero