Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1311089 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults "Maciej S. Szmigiero" <mail@maciej.szmigiero.name> - 2016-01-17 01:00 +0100
    Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults Timur Tabi <timur@tabi.org> - 2016-01-17 01:20 +0100
      Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults "Maciej S. Szmigiero" <mail@maciej.szmigiero.name> - 2016-01-17 02:10 +0100
        Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults Timur Tabi <timur@tabi.org> - 2016-01-17 06:20 +0100
          Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults "Maciej S. Szmigiero" <mail@maciej.szmigiero.name> - 2016-01-17 15:20 +0100
            Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults "Maciej S. Szmigiero" <mail@maciej.szmigiero.name> - 2016-01-17 15:40 +0100
              Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults Timur Tabi <timur@tabi.org> - 2016-01-17 19:40 +0100
                Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults "Maciej S. Szmigiero" <mail@maciej.szmigiero.name> - 2016-01-17 23:10 +0100
                  Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults Fabio Estevam <festevam@gmail.com> - 2016-01-18 14:00 +0100
                    Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults "Maciej S. Szmigiero" <mail@maciej.szmigiero.name> - 2016-01-18 20:10 +0100

#1311089 — Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

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?

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");


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

 

[toc] | [next] | [standalone]

#1311094

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.

[toc] | [prev] | [next] | [standalone]

#1311097

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

[toc] | [prev] | [next] | [standalone]

#1311107

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?

[toc] | [prev] | [next] | [standalone]

#1311149

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

[toc] | [prev] | [next] | [standalone]

#1311154

On 17.01.2016 15:16, Maciej S. Szmigiero wrote:
> 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.

Updated patch:
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 40dfd8a36484..105de76dd2fc 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) {
@@ -190,8 +176,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
 	.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 +186,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 +281,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 +290,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 +306,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 +576,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 +1390,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 	struct resource *res;
 	void __iomem *iomem;
 	char name[64];
+	struct regmap_config regconfig = 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)
 		return PTR_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);
+							  &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");


Also needs regmap fix from
http://www.spinics.net/lists/kernel/msg2161934.html

Maciej Szmigiero

[toc] | [prev] | [next] | [standalone]

#1311172

Maciej S. Szmigiero wrote:
> On 17.01.2016 15:16, Maciej S. Szmigiero wrote:
>> 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.
>
> Updated patch:
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 40dfd8a36484..105de76dd2fc 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) {
> @@ -190,8 +176,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
>   	.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,

Replace "4" with "sizeof(uint32_t).

>   	.readable_reg = fsl_ssi_readable_reg,
>   	.volatile_reg = fsl_ssi_volatile_reg,
>   	.precious_reg = fsl_ssi_precious_reg,
> @@ -201,6 +186,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
>
>   struct fsl_ssi_soc_data {
>   	bool imx;
> +	bool imx21regs;

Please add a comment explaining why this is needed.

>   	bool offline_config;
>   	u32 sisr_write_mask;
>   };
> @@ -295,6 +281,7 @@ struct fsl_ssi_private {
>
>   static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = {
>   	.imx = false,
> +	.imx21regs = false,

This is unnecessary.  The default is already 0 (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 +290,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,

Same here.

>   	.offline_config = true,
>   	.sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC |
>   			CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
> @@ -317,6 +306,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 +576,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);
> +	}

This needs a comment.

>
>   	/*
>   	 * Enable SSI, Transmit and Receive. AC97 has to communicate with the
> @@ -1397,6 +1390,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
>   	struct resource *res;
>   	void __iomem *iomem;
>   	char name[64];
> +	struct regmap_config regconfig = 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)
>   		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?

> +		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);
> +							  &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);

What's wrong with the original indentation?  It looks nicer than what 
you're doing here.

>   	}
>   	if (IS_ERR(ssi_private->regs)) {
>   		dev_err(&pdev->dev, "Failed to init register map\n");
>
>
> Also needs regmap fix from
> http://www.spinics.net/lists/kernel/msg2161934.html
>
> Maciej Szmigiero
>

[toc] | [prev] | [next] | [standalone]

#1311193

On 17.01.2016 19:38, Timur Tabi wrote:
> Maciej S. Szmigiero wrote:
>> On 17.01.2016 15:16, Maciej S. Szmigiero wrote:
>>> 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.

Patch updated according to Timur's suggestions (needs regmap fix):
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 40dfd8a36484..ed8de1035cda 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) {
@@ -190,8 +176,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
 	.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 / sizeof(uint32_t) + 1,
 	.readable_reg = fsl_ssi_readable_reg,
 	.volatile_reg = fsl_ssi_volatile_reg,
 	.precious_reg = fsl_ssi_precious_reg,
@@ -201,6 +186,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
 
 struct fsl_ssi_soc_data {
 	bool imx;
+	bool imx21regs; /* imx21-class SSI - no SACC{ST,EN,DIS} regs */
 	bool offline_config;
 	u32 sisr_write_mask;
 };
@@ -303,6 +289,7 @@ 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,
 };
@@ -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);
+	}
 
 	/*
 	 * Enable SSI, Transmit and Receive. AC97 has to communicate with the
@@ -1397,6 +1388,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 	struct resource *res;
 	void __iomem *iomem;
 	char name[64];
+	struct regmap_config regconfig = 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)
 		return PTR_ERR(iomem);
 	ssi_private->ssi_phys = res->start;
 
+	if (ssi_private->soc->imx21regs) {
+		/*
+		 * According to datasheet imx21-class SSI
+		 * don't have SACC{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);
+			&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");


> 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

[toc] | [prev] | [next] | [standalone]

#1311524

Hi Maciej,

On Sun, Jan 17, 2016 at 8:02 PM, Maciej S. Szmigiero
<mail@maciej.szmigiero.name> wrote:

> Patch updated according to Timur's suggestions (needs regmap fix):
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c

Tested your patch against linux-next and it did not give me any warnings:

Tested-by: Fabio Estevam <fabio.estevam@nxp.com>

[toc] | [prev] | [next] | [standalone]

#1311747

Hi Fabio,

On 18.01.2016 13:51, Fabio Estevam wrote:
> Hi Maciej,
> 
> On Sun, Jan 17, 2016 at 8:02 PM, Maciej S. Szmigiero
> <mail@maciej.szmigiero.name> wrote:
> 
>> Patch updated according to Timur's suggestions (needs regmap fix):
>> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> 
> Tested your patch against linux-next and it did not give me any warnings:
> 
> Tested-by: Fabio Estevam <fabio.estevam@nxp.com>

Thanks for testing, I have submitted this version now.

Maciej

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web