diff mbox

[3/3] ASoC: fsl_ssi: remove register defaults

Message ID 569AD8A7.7080803@maciej.szmigiero.name (mailing list archive)
State New, archived
Headers show

Commit Message

Maciej S. Szmigiero Jan. 16, 2016, 11:56 p.m. UTC 
Hi Fabio,

On 11.01.2016 15:05, Fabio Estevam wrote:
> Hi Maciej,
> 
> On Mon, Jan 11, 2016 at 11:57 AM, Maciej S. Szmigiero
> <mail@maciej.szmigiero.name> wrote:
>> Hi Fabio,
> 
>> This will disable register cache so it isn't right.
>> Could you try REGCACHE_FLAT instead, please?
> 
> Yes, with REGCACHE_FLAT I don't get the warning.
> 
> Regards,

Is it possible for you to test the following updated patch?



You'll also need to apply regmap fix from
http://www.spinics.net/lists/kernel/msg2161934.html to make it work.

Thanks in advance.

> Fabio Estevam

Best regards,
Maciej Szmigiero

Comments

Timur Tabi Jan. 17, 2016, 12:10 a.m. UTC | #1 
Maciej S. Szmigiero wrote:
> +static const struct regmap_config fsl_ssi_regconfig_imx21 = {
> +	.max_register = CCSR_SSI_SRMSK,
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.val_format_endian = REGMAP_ENDIAN_NATIVE,
> +	.num_reg_defaults_raw = CCSR_SSI_SRMSK / 4 + 1,
> +	.readable_reg = fsl_ssi_readable_reg,
> +	.volatile_reg = fsl_ssi_volatile_reg,
> +	.precious_reg = fsl_ssi_precious_reg,
> +	.writeable_reg = fsl_ssi_writeable_reg,
> +	.cache_type = REGCACHE_RBTREE,
> +};
> +
>   static const struct regmap_config fsl_ssi_regconfig = {
>   	.max_register = CCSR_SSI_SACCDIS,
>   	.reg_bits = 32,
>   	.val_bits = 32,
>   	.reg_stride = 4,
>   	.val_format_endian = REGMAP_ENDIAN_NATIVE,
> -	.reg_defaults = fsl_ssi_reg_defaults,
> -	.num_reg_defaults = ARRAY_SIZE(fsl_ssi_reg_defaults),
> +	.num_reg_defaults_raw = CCSR_SSI_SACCDIS / 4 + 1,
>   	.readable_reg = fsl_ssi_readable_reg,
>   	.volatile_reg = fsl_ssi_volatile_reg,
>   	.precious_reg = fsl_ssi_precious_reg,

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.
Maciej S. Szmigiero Jan. 17, 2016, 1:01 a.m. UTC | #2 
On 17.01.2016 01:10, Timur Tabi wrote:
> Maciej S. Szmigiero wrote:
>> +static const struct regmap_config fsl_ssi_regconfig_imx21 = {
>> +    .max_register = CCSR_SSI_SRMSK,
>> +    .reg_bits = 32,
>> +    .val_bits = 32,
>> +    .reg_stride = 4,
>> +    .val_format_endian = REGMAP_ENDIAN_NATIVE,
>> +    .num_reg_defaults_raw = CCSR_SSI_SRMSK / 4 + 1,
>> +    .readable_reg = fsl_ssi_readable_reg,
>> +    .volatile_reg = fsl_ssi_volatile_reg,
>> +    .precious_reg = fsl_ssi_precious_reg,
>> +    .writeable_reg = fsl_ssi_writeable_reg,
>> +    .cache_type = REGCACHE_RBTREE,
>> +};
>> +
>>   static const struct regmap_config fsl_ssi_regconfig = {
>>       .max_register = CCSR_SSI_SACCDIS,
>>       .reg_bits = 32,
>>       .val_bits = 32,
>>       .reg_stride = 4,
>>       .val_format_endian = REGMAP_ENDIAN_NATIVE,
>> -    .reg_defaults = fsl_ssi_reg_defaults,
>> -    .num_reg_defaults = ARRAY_SIZE(fsl_ssi_reg_defaults),
>> +    .num_reg_defaults_raw = CCSR_SSI_SACCDIS / 4 + 1,
>>       .readable_reg = fsl_ssi_readable_reg,
>>       .volatile_reg = fsl_ssi_volatile_reg,
>>       .precious_reg = fsl_ssi_precious_reg,
> 
> 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
Timur Tabi Jan. 17, 2016, 5:16 a.m. UTC | #3 
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?
Maciej S. Szmigiero Jan. 17, 2016, 2:16 p.m. UTC | #4 
On 17.01.2016 06:16, Timur Tabi wrote:
> 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?

Looks like it can be done with just one static regmap config struct
used then as template - I will post updated patch.

Maciej
diff mbox

Patch

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 40dfd8a36484..16ee5745c992 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -112,20 +112,6 @@  struct fsl_ssi_rxtx_reg_val {
 	struct fsl_ssi_reg_val tx;
 };
 
-static const struct reg_default fsl_ssi_reg_defaults[] = {
-	{CCSR_SSI_SCR,     0x00000000},
-	{CCSR_SSI_SIER,    0x00003003},
-	{CCSR_SSI_STCR,    0x00000200},
-	{CCSR_SSI_SRCR,    0x00000200},
-	{CCSR_SSI_STCCR,   0x00040000},
-	{CCSR_SSI_SRCCR,   0x00040000},
-	{CCSR_SSI_SACNT,   0x00000000},
-	{CCSR_SSI_STMSK,   0x00000000},
-	{CCSR_SSI_SRMSK,   0x00000000},
-	{CCSR_SSI_SACCEN,  0x00000000},
-	{CCSR_SSI_SACCDIS, 0x00000000},
-};
-
 static bool fsl_ssi_readable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -184,14 +170,27 @@  static bool fsl_ssi_writeable_reg(struct device *dev, unsigned int reg)
 	}
 }
 
+static const struct regmap_config fsl_ssi_regconfig_imx21 = {
+	.max_register = CCSR_SSI_SRMSK,
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.val_format_endian = REGMAP_ENDIAN_NATIVE,
+	.num_reg_defaults_raw = CCSR_SSI_SRMSK / 4 + 1,
+	.readable_reg = fsl_ssi_readable_reg,
+	.volatile_reg = fsl_ssi_volatile_reg,
+	.precious_reg = fsl_ssi_precious_reg,
+	.writeable_reg = fsl_ssi_writeable_reg,
+	.cache_type = REGCACHE_RBTREE,
+};
+
 static const struct regmap_config fsl_ssi_regconfig = {
 	.max_register = CCSR_SSI_SACCDIS,
 	.reg_bits = 32,
 	.val_bits = 32,
 	.reg_stride = 4,
 	.val_format_endian = REGMAP_ENDIAN_NATIVE,
-	.reg_defaults = fsl_ssi_reg_defaults,
-	.num_reg_defaults = ARRAY_SIZE(fsl_ssi_reg_defaults),
+	.num_reg_defaults_raw = CCSR_SSI_SACCDIS / 4 + 1,
 	.readable_reg = fsl_ssi_readable_reg,
 	.volatile_reg = fsl_ssi_volatile_reg,
 	.precious_reg = fsl_ssi_precious_reg,
@@ -201,6 +200,7 @@  static const struct regmap_config fsl_ssi_regconfig = {
 
 struct fsl_ssi_soc_data {
 	bool imx;
+	bool imx21regs;
 	bool offline_config;
 	u32 sisr_write_mask;
 };
@@ -295,6 +295,7 @@  struct fsl_ssi_private {
 
 static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = {
 	.imx = false,
+	.imx21regs = false,
 	.offline_config = true,
 	.sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC |
 			CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
@@ -303,12 +304,14 @@  static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = {
 
 static struct fsl_ssi_soc_data fsl_ssi_imx21 = {
 	.imx = true,
+	.imx21regs = true,
 	.offline_config = true,
 	.sisr_write_mask = 0,
 };
 
 static struct fsl_ssi_soc_data fsl_ssi_imx35 = {
 	.imx = true,
+	.imx21regs = false,
 	.offline_config = true,
 	.sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC |
 			CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
@@ -317,6 +320,7 @@  static struct fsl_ssi_soc_data fsl_ssi_imx35 = {
 
 static struct fsl_ssi_soc_data fsl_ssi_imx51 = {
 	.imx = true,
+	.imx21regs = false,
 	.offline_config = false,
 	.sisr_write_mask = CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
 		CCSR_SSI_SISR_TUE0 | CCSR_SSI_SISR_TUE1,
@@ -586,8 +590,11 @@  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);
+
+	if (!ssi_private->soc->imx21regs) {
+		regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
+		regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+	}
 
 	/*
 	 * Enable SSI, Transmit and Receive. AC97 has to communicate with the
@@ -1397,6 +1404,7 @@  static int fsl_ssi_probe(struct platform_device *pdev)
 	struct resource *res;
 	void __iomem *iomem;
 	char name[64];
+	const struct regmap_config *regconfig;
 
 	of_id = of_match_device(fsl_ssi_ids, &pdev->dev);
 	if (!of_id || !of_id->data)
@@ -1444,15 +1452,21 @@  static int fsl_ssi_probe(struct platform_device *pdev)
 		return PTR_ERR(iomem);
 	ssi_private->ssi_phys = res->start;
 
+	if (ssi_private->soc->imx21regs)
+		regconfig = &fsl_ssi_regconfig_imx21;
+	else
+		regconfig = &fsl_ssi_regconfig;
+
 	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);
+							  regconfig);
 	} 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,
+							      regconfig);
 	}
 	if (IS_ERR(ssi_private->regs)) {
 		dev_err(&pdev->dev, "Failed to init register map\n");