[PATCH v1] ASoC: tas2781: fixed compiling issue in m68k

[PATCH v1] ASoC: tas2781: fixed compiling issue in m68k

[Shenghao Ding]

fixed m68k compiling issue: mapping table can save code field; storing the
dev_idx as a member of block can reduce unnecessary  time and system
resource comsumption of dev_idx mapping every time the block data writing
to the dsp.

Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>

---
Changes in v1:
 - | Reported-by: kernel test robot <lkp@intel.com>
   | Closes:
   | https://lore.kernel.org/oe-kbuild-all/202309222048.RnSqEIK5-lkp@intel.
   | com/
---
 include/sound/tas2781-dsp.h       |   5 +
 sound/soc/codecs/tas2781-fmwlib.c | 234 +++++++++++++-----------------
 2 files changed, 107 insertions(+), 132 deletions(-)

diff --git a/include/sound/tas2781-dsp.h b/include/sound/tas2781-dsp.h
index bd1b72bf47a5..ea9af2726a53 100644
--- a/include/sound/tas2781-dsp.h
+++ b/include/sound/tas2781-dsp.h
@@ -77,6 +77,11 @@ struct tasdev_blk {
 	unsigned int nr_cmds;
 	unsigned int blk_size;
 	unsigned int nr_subblocks;
+	/* fixed m68k compiling issue, storing the dev_idx as a member of block
+	 * can reduce unnecessary timeand system resource comsumption of
+	 * dev_idx mapping every time the block data writing to the dsp.
+	 */
+	unsigned char dev_idx;
 	unsigned char *data;
 };
 
diff --git a/sound/soc/codecs/tas2781-fmwlib.c b/sound/soc/codecs/tas2781-fmwlib.c
index eb55abae0d7b..e27775d834e9 100644
--- a/sound/soc/codecs/tas2781-fmwlib.c
+++ b/sound/soc/codecs/tas2781-fmwlib.c
@@ -80,10 +80,72 @@ struct tas_crc {
 	unsigned char len;
 };
 
+struct blktyp_devidx_map {
+	unsigned char blktyp;
+	unsigned char dev_idx;
+};
+
 static const char deviceNumber[TASDEVICE_DSP_TAS_MAX_DEVICE] = {
 	1, 2, 1, 2, 1, 1, 0, 2, 4, 3, 1, 2, 3, 4
 };
 
+/* fixed m68k compiling issue: mapping table can save code field */
+static const struct blktyp_devidx_map ppc3_tas2781_mapping_table[] = {
+	{ MAIN_ALL_DEVICES_1X, 0x80 },
+	{ MAIN_DEVICE_A_1X, 0x81 },
+	{ COEFF_DEVICE_A_1X, 0xC1 },
+	{ PRE_DEVICE_A_1X, 0xC1 },
+	{ PRE_SOFTWARE_RESET_DEVICE_A, 0xC1 },
+	{ POST_SOFTWARE_RESET_DEVICE_A, 0xC1 },
+	{ MAIN_DEVICE_B_1X, 0x82 },
+	{ COEFF_DEVICE_B_1X, 0xC2 },
+	{ PRE_DEVICE_B_1X, 0xC2 },
+	{ PRE_SOFTWARE_RESET_DEVICE_B, 0xC2 },
+	{ POST_SOFTWARE_RESET_DEVICE_B, 0xC2 },
+	{ MAIN_DEVICE_C_1X, 0x83 },
+	{ COEFF_DEVICE_C_1X, 0xC3 },
+	{ PRE_DEVICE_C_1X, 0xC3 },
+	{ PRE_SOFTWARE_RESET_DEVICE_C, 0xC3 },
+	{ POST_SOFTWARE_RESET_DEVICE_C, 0xC3 },
+	{ MAIN_DEVICE_D_1X, 0x84 },
+	{ COEFF_DEVICE_D_1X, 0xC4 },
+	{ PRE_DEVICE_D_1X, 0xC4 },
+	{ PRE_SOFTWARE_RESET_DEVICE_D, 0xC4 },
+	{ POST_SOFTWARE_RESET_DEVICE_D, 0xC4 },
+};
+
+static const struct blktyp_devidx_map ppc3_mapping_table[] = {
+	{ MAIN_ALL_DEVICES_1X, 0x80 },
+	{ MAIN_DEVICE_A_1X, 0x81 },
+	{ COEFF_DEVICE_A_1X, 0xC1 },
+	{ PRE_DEVICE_A_1X, 0xC1 },
+	{ MAIN_DEVICE_B_1X, 0x82 },
+	{ COEFF_DEVICE_B_1X, 0xC2 },
+	{ PRE_DEVICE_B_1X, 0xC2 },
+	{ MAIN_DEVICE_C_1X, 0x83 },
+	{ COEFF_DEVICE_C_1X, 0xC3 },
+	{ PRE_DEVICE_C_1X, 0xC3 },
+	{ MAIN_DEVICE_D_1X, 0x84 },
+	{ COEFF_DEVICE_D_1X, 0xC4 },
+	{ PRE_DEVICE_D_1X, 0xC4 },
+};
+
+static const struct blktyp_devidx_map non_ppc3_mapping_table[] = {
+	{ MAIN_ALL_DEVICES, 0x80 },
+	{ MAIN_DEVICE_A, 0x81 },
+	{ COEFF_DEVICE_A, 0xC1 },
+	{ PRE_DEVICE_A, 0xC1 },
+	{ MAIN_DEVICE_B, 0x82 },
+	{ COEFF_DEVICE_B, 0xC2 },
+	{ PRE_DEVICE_B, 0xC2 },
+	{ MAIN_DEVICE_C, 0x83 },
+	{ COEFF_DEVICE_C, 0xC3 },
+	{ PRE_DEVICE_C, 0xC3 },
+	{ MAIN_DEVICE_D, 0x84 },
+	{ COEFF_DEVICE_D, 0xC4 },
+	{ PRE_DEVICE_D, 0xC4 },
+};
+
 static struct tasdevice_config_info *tasdevice_add_config(
 	struct tasdevice_priv *tas_priv, unsigned char *config_data,
 	unsigned int config_size, int *status)
@@ -316,6 +378,37 @@ int tasdevice_rca_parser(void *context, const struct firmware *fmw)
 }
 EXPORT_SYMBOL_NS_GPL(tasdevice_rca_parser, SND_SOC_TAS2781_FMWLIB);
 
+/* fixed m68k compiling issue: mapping table can save code field */
+static unsigned char map_dev_idx(struct tasdevice_fw *tas_fmw,
+	struct tasdev_blk *block)
+{
+
+	struct blktyp_devidx_map *p =
+		(struct blktyp_devidx_map *)non_ppc3_mapping_table;
+	struct tasdevice_dspfw_hdr *fw_hdr = &(tas_fmw->fw_hdr);
+	struct tasdevice_fw_fixed_hdr *fw_fixed_hdr = &(fw_hdr->fixed_hdr);
+
+	int i, n = ARRAY_SIZE(non_ppc3_mapping_table);
+	unsigned char dev_idx = 0;
+
+	if (fw_fixed_hdr->ppcver >= PPC3_VERSION_TAS2781) {
+		p = (struct blktyp_devidx_map *)ppc3_tas2781_mapping_table;
+		n = ARRAY_SIZE(ppc3_tas2781_mapping_table);
+	} else if (fw_fixed_hdr->ppcver >= PPC3_VERSION) {
+		p = (struct blktyp_devidx_map *)ppc3_mapping_table;
+		n = ARRAY_SIZE(ppc3_mapping_table);
+	}
+
+	for (i = 0; i < n; i++) {
+		if (block->type == p[i].blktyp) {
+			dev_idx = p[i].dev_idx;
+			break;
+		}
+	}
+
+	return dev_idx;
+}
+
 static int fw_parse_block_data_kernel(struct tasdevice_fw *tas_fmw,
 	struct tasdev_blk *block, const struct firmware *fmw, int offset)
 {
@@ -351,6 +444,14 @@ static int fw_parse_block_data_kernel(struct tasdevice_fw *tas_fmw,
 	block->nr_subblocks = be32_to_cpup((__be32 *)&data[offset]);
 	offset += 4;
 
+	/* fixed m68k compiling issue:
+	 * 1. mapping table can save code field.
+	 * 2. storing the dev_idx as a member of block can reduce unnecessary
+	 *    time and system resource comsumption of dev_idx mapping every
+	 *    time the block data writing to the dsp.
+	 */
+	block->dev_idx = map_dev_idx(tas_fmw, block);
+
 	if (offset + block->blk_size > fmw->size) {
 		dev_err(tas_fmw->dev, "%s: nSublocks error\n", __func__);
 		offset = -EINVAL;
@@ -768,144 +869,13 @@ EXPORT_SYMBOL_NS_GPL(tasdevice_select_cfg_blk, SND_SOC_TAS2781_FMWLIB);
 static int tasdevice_load_block_kernel(
 	struct tasdevice_priv *tasdevice, struct tasdev_blk *block)
 {
-	struct tasdevice_dspfw_hdr *fw_hdr = &(tasdevice->fmw->fw_hdr);
-	struct tasdevice_fw_fixed_hdr *fw_fixed_hdr = &(fw_hdr->fixed_hdr);
 	const unsigned int blk_size = block->blk_size;
 	unsigned int i, length;
 	unsigned char *data = block->data;
-	unsigned char dev_idx = 0;
-
-	if (fw_fixed_hdr->ppcver >= PPC3_VERSION_TAS2781) {
-		switch (block->type) {
-		case MAIN_ALL_DEVICES_1X:
-			dev_idx = 0x80;
-			break;
-		case MAIN_DEVICE_A_1X:
-			dev_idx = 0x81;
-			break;
-		case COEFF_DEVICE_A_1X:
-		case PRE_DEVICE_A_1X:
-		case PRE_SOFTWARE_RESET_DEVICE_A:
-		case POST_SOFTWARE_RESET_DEVICE_A:
-			dev_idx = 0xC1;
-			break;
-		case MAIN_DEVICE_B_1X:
-			dev_idx = 0x82;
-			break;
-		case COEFF_DEVICE_B_1X:
-		case PRE_DEVICE_B_1X:
-		case PRE_SOFTWARE_RESET_DEVICE_B:
-		case POST_SOFTWARE_RESET_DEVICE_B:
-			dev_idx = 0xC2;
-			break;
-		case MAIN_DEVICE_C_1X:
-			dev_idx = 0x83;
-			break;
-		case COEFF_DEVICE_C_1X:
-		case PRE_DEVICE_C_1X:
-		case PRE_SOFTWARE_RESET_DEVICE_C:
-		case POST_SOFTWARE_RESET_DEVICE_C:
-			dev_idx = 0xC3;
-			break;
-		case MAIN_DEVICE_D_1X:
-			dev_idx = 0x84;
-			break;
-		case COEFF_DEVICE_D_1X:
-		case PRE_DEVICE_D_1X:
-		case PRE_SOFTWARE_RESET_DEVICE_D:
-		case POST_SOFTWARE_RESET_DEVICE_D:
-			dev_idx = 0xC4;
-			break;
-		default:
-			dev_info(tasdevice->dev,
-				"%s: load block: Other Type = 0x%02x\n",
-				__func__, block->type);
-			break;
-		}
-	} else if (fw_fixed_hdr->ppcver >=
-	PPC3_VERSION) {
-		switch (block->type) {
-		case MAIN_ALL_DEVICES_1X:
-			dev_idx = 0x80;
-			break;
-		case MAIN_DEVICE_A_1X:
-			dev_idx = 0x81;
-			break;
-		case COEFF_DEVICE_A_1X:
-		case PRE_DEVICE_A_1X:
-			dev_idx = 0xC1;
-			break;
-		case MAIN_DEVICE_B_1X:
-			dev_idx = 0x82;
-			break;
-		case COEFF_DEVICE_B_1X:
-		case PRE_DEVICE_B_1X:
-			dev_idx = 0xC2;
-			break;
-		case MAIN_DEVICE_C_1X:
-			dev_idx = 0x83;
-			break;
-		case COEFF_DEVICE_C_1X:
-		case PRE_DEVICE_C_1X:
-			dev_idx = 0xC3;
-			break;
-		case MAIN_DEVICE_D_1X:
-			dev_idx = 0x84;
-			break;
-		case COEFF_DEVICE_D_1X:
-		case PRE_DEVICE_D_1X:
-			dev_idx = 0xC4;
-			break;
-		default:
-			dev_info(tasdevice->dev,
-				"%s: load block: Other Type = 0x%02x\n",
-				__func__, block->type);
-			break;
-		}
-	} else {
-		switch (block->type) {
-		case MAIN_ALL_DEVICES:
-			dev_idx = 0|0x80;
-			break;
-		case MAIN_DEVICE_A:
-			dev_idx = 0x81;
-			break;
-		case COEFF_DEVICE_A:
-		case PRE_DEVICE_A:
-			dev_idx = 0xC1;
-			break;
-		case MAIN_DEVICE_B:
-			dev_idx = 0x82;
-			break;
-		case COEFF_DEVICE_B:
-		case PRE_DEVICE_B:
-			dev_idx = 0xC2;
-			break;
-		case MAIN_DEVICE_C:
-			dev_idx = 0x83;
-			break;
-		case COEFF_DEVICE_C:
-		case PRE_DEVICE_C:
-			dev_idx = 0xC3;
-			break;
-		case MAIN_DEVICE_D:
-			dev_idx = 0x84;
-			break;
-		case COEFF_DEVICE_D:
-		case PRE_DEVICE_D:
-			dev_idx = 0xC4;
-			break;
-		default:
-			dev_info(tasdevice->dev,
-				"%s: load block: Other Type = 0x%02x\n",
-				__func__, block->type);
-			break;
-		}
-	}
 
 	for (i = 0, length = 0; i < block->nr_subblocks; i++) {
 		int rc = tasdevice_process_block(tasdevice, data + length,
-			dev_idx, blk_size - length);
+			block->dev_idx, blk_size - length);
 		if (rc < 0) {
 			dev_err(tasdevice->dev,
 				"%s: %u %u sublock write error\n",
-- 
2.34.1

Re: [PATCH v1] ASoC: tas2781: fixed compiling issue in m68k

[Mark Brown]

On Mon, 02 Oct 2023 17:04:33 +0800, Shenghao Ding wrote:
> fixed m68k compiling issue: mapping table can save code field; storing the
> dev_idx as a member of block can reduce unnecessary  time and system
> resource comsumption of dev_idx mapping every time the block data writing
> to the dsp.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: tas2781: fixed compiling issue in m68k
      commit: 4c556d1ea5a771a91f946964d931b4974a6b917e

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v1] ASoC: tas2781: fixed compiling issue in m68k

[Geert Uytterhoeven]

Hi Shenghao,

Thanks for your patch!

On Mon, Oct 2, 2023 at 4:38 PM Shenghao Ding <shenghao-ding@ti.com> wrote:
> fixed m68k compiling issue: mapping table can save code field; storing the

Please replicate the actual error message from the compiler, so it
is recorded in the commit description, and easy to find when someone
searches for the actual error message

>From the Reported-by (which is not included in the actual description,
as it is below the "---"):

       {standard input}: Assembler messages:
    >> {standard input}:992: Error: value -148 out of range
       {standard input}:992: Error: value of ffffff6c too large for
field of 1 byte at 0000045f

> dev_idx as a member of block can reduce unnecessary  time and system
> resource comsumption of dev_idx mapping every time the block data writing
> to the dsp.

I am sorry, but I don't understand what this means.
Can you please elaborate?

Also, can you please explain what the real issue is?
(My first guess when seeing the error message before was that a  signed
 integer is truncated to an unsigned char or so, but I couldn't see
 immediately where that is happening)

> Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>
>
> ---
> Changes in v1:
>  - | Reported-by: kernel test robot <lkp@intel.com>
>    | Closes:
>    | https://lore.kernel.org/oe-kbuild-all/202309222048.RnSqEIK5-lkp@intel.
>    | com/

> --- a/include/sound/tas2781-dsp.h
> +++ b/include/sound/tas2781-dsp.h
> @@ -77,6 +77,11 @@ struct tasdev_blk {
>         unsigned int nr_cmds;
>         unsigned int blk_size;
>         unsigned int nr_subblocks;
> +       /* fixed m68k compiling issue, storing the dev_idx as a member of block
> +        * can reduce unnecessary timeand system resource comsumption of
> +        * dev_idx mapping every time the block data writing to the dsp.
> +        */

What is so special about "m68k" that it (1) fails there (and only there?
and only for some config/compiler combos, as it builds fine for me?),
and (2) is needed to mention this in comments all over the place?

> +       unsigned char dev_idx;
>         unsigned char *data;
>  };
>
> diff --git a/sound/soc/codecs/tas2781-fmwlib.c b/sound/soc/codecs/tas2781-fmwlib.c
> index eb55abae0d7b..e27775d834e9 100644
> --- a/sound/soc/codecs/tas2781-fmwlib.c
> +++ b/sound/soc/codecs/tas2781-fmwlib.c
> @@ -80,10 +80,72 @@ struct tas_crc {
>         unsigned char len;
>  };
>
> +struct blktyp_devidx_map {
> +       unsigned char blktyp;
> +       unsigned char dev_idx;
> +};
> +
>  static const char deviceNumber[TASDEVICE_DSP_TAS_MAX_DEVICE] = {
>         1, 2, 1, 2, 1, 1, 0, 2, 4, 3, 1, 2, 3, 4
>  };
>
> +/* fixed m68k compiling issue: mapping table can save code field */
> +static const struct blktyp_devidx_map ppc3_tas2781_mapping_table[] = {
> +       { MAIN_ALL_DEVICES_1X, 0x80 },
> +       { MAIN_DEVICE_A_1X, 0x81 },
> +       { COEFF_DEVICE_A_1X, 0xC1 },
> +       { PRE_DEVICE_A_1X, 0xC1 },
> +       { PRE_SOFTWARE_RESET_DEVICE_A, 0xC1 },
> +       { POST_SOFTWARE_RESET_DEVICE_A, 0xC1 },
> +       { MAIN_DEVICE_B_1X, 0x82 },
> +       { COEFF_DEVICE_B_1X, 0xC2 },
> +       { PRE_DEVICE_B_1X, 0xC2 },
> +       { PRE_SOFTWARE_RESET_DEVICE_B, 0xC2 },
> +       { POST_SOFTWARE_RESET_DEVICE_B, 0xC2 },
> +       { MAIN_DEVICE_C_1X, 0x83 },
> +       { COEFF_DEVICE_C_1X, 0xC3 },
> +       { PRE_DEVICE_C_1X, 0xC3 },
> +       { PRE_SOFTWARE_RESET_DEVICE_C, 0xC3 },
> +       { POST_SOFTWARE_RESET_DEVICE_C, 0xC3 },
> +       { MAIN_DEVICE_D_1X, 0x84 },
> +       { COEFF_DEVICE_D_1X, 0xC4 },
> +       { PRE_DEVICE_D_1X, 0xC4 },
> +       { PRE_SOFTWARE_RESET_DEVICE_D, 0xC4 },
> +       { POST_SOFTWARE_RESET_DEVICE_D, 0xC4 },
> +};
> +
> +static const struct blktyp_devidx_map ppc3_mapping_table[] = {
> +       { MAIN_ALL_DEVICES_1X, 0x80 },
> +       { MAIN_DEVICE_A_1X, 0x81 },
> +       { COEFF_DEVICE_A_1X, 0xC1 },
> +       { PRE_DEVICE_A_1X, 0xC1 },
> +       { MAIN_DEVICE_B_1X, 0x82 },
> +       { COEFF_DEVICE_B_1X, 0xC2 },
> +       { PRE_DEVICE_B_1X, 0xC2 },
> +       { MAIN_DEVICE_C_1X, 0x83 },
> +       { COEFF_DEVICE_C_1X, 0xC3 },
> +       { PRE_DEVICE_C_1X, 0xC3 },
> +       { MAIN_DEVICE_D_1X, 0x84 },
> +       { COEFF_DEVICE_D_1X, 0xC4 },
> +       { PRE_DEVICE_D_1X, 0xC4 },
> +};
> +
> +static const struct blktyp_devidx_map non_ppc3_mapping_table[] = {
> +       { MAIN_ALL_DEVICES, 0x80 },
> +       { MAIN_DEVICE_A, 0x81 },
> +       { COEFF_DEVICE_A, 0xC1 },
> +       { PRE_DEVICE_A, 0xC1 },
> +       { MAIN_DEVICE_B, 0x82 },
> +       { COEFF_DEVICE_B, 0xC2 },
> +       { PRE_DEVICE_B, 0xC2 },
> +       { MAIN_DEVICE_C, 0x83 },
> +       { COEFF_DEVICE_C, 0xC3 },
> +       { PRE_DEVICE_C, 0xC3 },
> +       { MAIN_DEVICE_D, 0x84 },
> +       { COEFF_DEVICE_D, 0xC4 },
> +       { PRE_DEVICE_D, 0xC4 },
> +};
> +
>  static struct tasdevice_config_info *tasdevice_add_config(
>         struct tasdevice_priv *tas_priv, unsigned char *config_data,
>         unsigned int config_size, int *status)
> @@ -316,6 +378,37 @@ int tasdevice_rca_parser(void *context, const struct firmware *fmw)
>  }
>  EXPORT_SYMBOL_NS_GPL(tasdevice_rca_parser, SND_SOC_TAS2781_FMWLIB);
>
> +/* fixed m68k compiling issue: mapping table can save code field */
> +static unsigned char map_dev_idx(struct tasdevice_fw *tas_fmw,
> +       struct tasdev_blk *block)
> +{
> +
> +       struct blktyp_devidx_map *p =
> +               (struct blktyp_devidx_map *)non_ppc3_mapping_table;

Please do not cast away constness when not needed (also below).

> +       struct tasdevice_dspfw_hdr *fw_hdr = &(tas_fmw->fw_hdr);
> +       struct tasdevice_fw_fixed_hdr *fw_fixed_hdr = &(fw_hdr->fixed_hdr);
> +
> +       int i, n = ARRAY_SIZE(non_ppc3_mapping_table);

size_t

> +       unsigned char dev_idx = 0;
> +
> +       if (fw_fixed_hdr->ppcver >= PPC3_VERSION_TAS2781) {
> +               p = (struct blktyp_devidx_map *)ppc3_tas2781_mapping_table;
> +               n = ARRAY_SIZE(ppc3_tas2781_mapping_table);
> +       } else if (fw_fixed_hdr->ppcver >= PPC3_VERSION) {
> +               p = (struct blktyp_devidx_map *)ppc3_mapping_table;
> +               n = ARRAY_SIZE(ppc3_mapping_table);
> +       }
> +
> +       for (i = 0; i < n; i++) {
> +               if (block->type == p[i].blktyp) {
> +                       dev_idx = p[i].dev_idx;
> +                       break;
> +               }
> +       }
> +
> +       return dev_idx;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1] ASoC: tas2781: fixed compiling issue in m68k

[Andy Shevchenko]

On Mon, Oct 02, 2023 at 09:24:21PM +0200, Geert Uytterhoeven wrote:
> On Mon, Oct 2, 2023 at 4:38 PM Shenghao Ding <shenghao-ding@ti.com> wrote:

...

> Gr{oetje,eeting}s,
> 
>                         Geert

I believe patch is already in the Mark's tree, so we now ought to live with it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] ASoC: tas2781: fixed compiling issue in m68k

[Geert Uytterhoeven]

Hi Andy,

On Tue, Oct 3, 2023 at 10:52 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Oct 02, 2023 at 09:24:21PM +0200, Geert Uytterhoeven wrote:
> > On Mon, Oct 2, 2023 at 4:38 PM Shenghao Ding <shenghao-ding@ti.com> wrote:
>
> ...

> I believe patch is already in the Mark's tree, so we now ought to live with it.

I know. I would still like to know what is the real cause.
It might resurrect again, here or in other code.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [EXTERNAL] Re: [PATCH v1] ASoC: tas2781: fixed compiling issue in m68k

[Ding, Shenghao]

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Tuesday, October 3, 2023 3:24 AM
> To: Ding, Shenghao <shenghao-ding@ti.com>
> Cc: broonie@kernel.org; robh+dt@kernel.org;
> andriy.shevchenko@linux.intel.com; lgirdwood@gmail.com; perex@perex.cz;
> pierre-louis.bossart@linux.intel.com; Lu, Kevin <kevin-lu@ti.com>;
> 13916275206@139.com; alsa-devel@alsa-project.org; linux-
> kernel@vger.kernel.org; liam.r.girdwood@intel.com; mengdong.lin@intel.com;
> Xu, Baojun <baojun.xu@ti.com>; thomas.gfeller@q-drop.com; Gupta, Peeyush
> <peeyush@ti.com>; Navada Kanyana, Mukund <navada@ti.com>;
> tiwai@suse.de
> Subject: [EXTERNAL] Re: [PATCH v1] ASoC: tas2781: fixed compiling issue in
> m68k
> 
> Hi Shenghao,
> 
> Thanks for your patch!
> 
> On Mon, Oct 2, 2023 at 4:38 PM Shenghao Ding <shenghao-ding@ti.com>
> wrote:
> > fixed m68k compiling issue: mapping table can save code field; storing
> > the
> 
> Please replicate the actual error message from the compiler, so it is recorded in
> the commit description, and easy to find when someone searches for the actual
> error message
> 
> From the Reported-by (which is not included in the actual description, as it is
> below the "---"):
> 
>        {standard input}: Assembler messages:
>     >> {standard input}:992: Error: value -148 out of range
>        {standard input}:992: Error: value of ffffff6c too large for field of 1 byte at
> 0000045f
> 
> > dev_idx as a member of block can reduce unnecessary  time and system
> > resource comsumption of dev_idx mapping every time the block data
> > writing to the dsp.
> 
> I am sorry, but I don't understand what this means.
> Can you please elaborate?
> 
> Also, can you please explain what the real issue is?
> (My first guess when seeing the error message before was that a  signed
> integer is truncated to an unsigned char or so, but I couldn't see  immediately
> where that is happening)
Sorry to late feedback. I had been troubled by this issue for several weeks. At first, I also think it would one of variables overflow, according to the compiling error message. However, after investigation, no result came out. In fact, compiler will report the line number where the variable overflow is, if there was risk on the variable overflow. Yet, this compiling error did not report any line number. Finally, I didn’t realize that it would be the code section overflow until that compiling error message magically disappeared, one day I removed some functions in the tas2781-fwlib.c. So, I began to simplify some functions in the code.
> 
> > Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>
> >
> > ---
> > Changes in v1:
> >  - | Reported-by: kernel test robot <lkp@intel.com>
> >    | Closes:
> >    | https://lore.kernel.org/oe-kbuild-all/202309222048.RnSqEIK5-lkp@intel.
> >    | com/
> 
> > --- a/include/sound/tas2781-dsp.h
> > +++ b/include/sound/tas2781-dsp.h
> > @@ -77,6 +77,11 @@ struct tasdev_blk {
> >         unsigned int nr_cmds;
> >         unsigned int blk_size;
> >         unsigned int nr_subblocks;
> > +       /* fixed m68k compiling issue, storing the dev_idx as a member of block
> > +        * can reduce unnecessary timeand system resource comsumption of
> > +        * dev_idx mapping every time the block data writing to the dsp.
> > +        */
> 
> What is so special about "m68k" that it (1) fails there (and only there?
> and only for some config/compiler combos, as it builds fine for me?), and (2) is
> needed to mention this in comments all over the place?
> 
> > +       unsigned char dev_idx;
> >         unsigned char *data;
> >  };
> >
> > diff --git a/sound/soc/codecs/tas2781-fmwlib.c
> > b/sound/soc/codecs/tas2781-fmwlib.c
> > index eb55abae0d7b..e27775d834e9 100644
> > --- a/sound/soc/codecs/tas2781-fmwlib.c
> > +++ b/sound/soc/codecs/tas2781-fmwlib.c
> > @@ -80,10 +80,72 @@ struct tas_crc {
> >         unsigned char len;
> >  };
> >
> > +struct blktyp_devidx_map {
> > +       unsigned char blktyp;
> > +       unsigned char dev_idx;
> > +};
> > +
> >  static const char deviceNumber[TASDEVICE_DSP_TAS_MAX_DEVICE] = {
> >         1, 2, 1, 2, 1, 1, 0, 2, 4, 3, 1, 2, 3, 4  };
> >
> > +/* fixed m68k compiling issue: mapping table can save code field */
> > +static const struct blktyp_devidx_map ppc3_tas2781_mapping_table[] = {
> > +       { MAIN_ALL_DEVICES_1X, 0x80 },
> > +       { MAIN_DEVICE_A_1X, 0x81 },
> > +       { COEFF_DEVICE_A_1X, 0xC1 },
> > +       { PRE_DEVICE_A_1X, 0xC1 },
> > +       { PRE_SOFTWARE_RESET_DEVICE_A, 0xC1 },
> > +       { POST_SOFTWARE_RESET_DEVICE_A, 0xC1 },
> > +       { MAIN_DEVICE_B_1X, 0x82 },
> > +       { COEFF_DEVICE_B_1X, 0xC2 },
> > +       { PRE_DEVICE_B_1X, 0xC2 },
> > +       { PRE_SOFTWARE_RESET_DEVICE_B, 0xC2 },
> > +       { POST_SOFTWARE_RESET_DEVICE_B, 0xC2 },
> > +       { MAIN_DEVICE_C_1X, 0x83 },
> > +       { COEFF_DEVICE_C_1X, 0xC3 },
> > +       { PRE_DEVICE_C_1X, 0xC3 },
> > +       { PRE_SOFTWARE_RESET_DEVICE_C, 0xC3 },
> > +       { POST_SOFTWARE_RESET_DEVICE_C, 0xC3 },
> > +       { MAIN_DEVICE_D_1X, 0x84 },
> > +       { COEFF_DEVICE_D_1X, 0xC4 },
> > +       { PRE_DEVICE_D_1X, 0xC4 },
> > +       { PRE_SOFTWARE_RESET_DEVICE_D, 0xC4 },
> > +       { POST_SOFTWARE_RESET_DEVICE_D, 0xC4 }, };
> > +
> > +static const struct blktyp_devidx_map ppc3_mapping_table[] = {
> > +       { MAIN_ALL_DEVICES_1X, 0x80 },
> > +       { MAIN_DEVICE_A_1X, 0x81 },
> > +       { COEFF_DEVICE_A_1X, 0xC1 },
> > +       { PRE_DEVICE_A_1X, 0xC1 },
> > +       { MAIN_DEVICE_B_1X, 0x82 },
> > +       { COEFF_DEVICE_B_1X, 0xC2 },
> > +       { PRE_DEVICE_B_1X, 0xC2 },
> > +       { MAIN_DEVICE_C_1X, 0x83 },
> > +       { COEFF_DEVICE_C_1X, 0xC3 },
> > +       { PRE_DEVICE_C_1X, 0xC3 },
> > +       { MAIN_DEVICE_D_1X, 0x84 },
> > +       { COEFF_DEVICE_D_1X, 0xC4 },
> > +       { PRE_DEVICE_D_1X, 0xC4 },
> > +};
> > +
> > +static const struct blktyp_devidx_map non_ppc3_mapping_table[] = {
> > +       { MAIN_ALL_DEVICES, 0x80 },
> > +       { MAIN_DEVICE_A, 0x81 },
> > +       { COEFF_DEVICE_A, 0xC1 },
> > +       { PRE_DEVICE_A, 0xC1 },
> > +       { MAIN_DEVICE_B, 0x82 },
> > +       { COEFF_DEVICE_B, 0xC2 },
> > +       { PRE_DEVICE_B, 0xC2 },
> > +       { MAIN_DEVICE_C, 0x83 },
> > +       { COEFF_DEVICE_C, 0xC3 },
> > +       { PRE_DEVICE_C, 0xC3 },
> > +       { MAIN_DEVICE_D, 0x84 },
> > +       { COEFF_DEVICE_D, 0xC4 },
> > +       { PRE_DEVICE_D, 0xC4 },
> > +};
> > +
> >  static struct tasdevice_config_info *tasdevice_add_config(
> >         struct tasdevice_priv *tas_priv, unsigned char *config_data,
> >         unsigned int config_size, int *status) @@ -316,6 +378,37 @@
> > int tasdevice_rca_parser(void *context, const struct firmware *fmw)  }
> > EXPORT_SYMBOL_NS_GPL(tasdevice_rca_parser,
> SND_SOC_TAS2781_FMWLIB);
> >
> > +/* fixed m68k compiling issue: mapping table can save code field */
> > +static unsigned char map_dev_idx(struct tasdevice_fw *tas_fmw,
> > +       struct tasdev_blk *block)
> > +{
> > +
> > +       struct blktyp_devidx_map *p =
> > +               (struct blktyp_devidx_map *)non_ppc3_mapping_table;
> 
> Please do not cast away constness when not needed (also below).
> 
> > +       struct tasdevice_dspfw_hdr *fw_hdr = &(tas_fmw->fw_hdr);
> > +       struct tasdevice_fw_fixed_hdr *fw_fixed_hdr =
> > + &(fw_hdr->fixed_hdr);
> > +
> > +       int i, n = ARRAY_SIZE(non_ppc3_mapping_table);
> 
> size_t
> 
> > +       unsigned char dev_idx = 0;
> > +
> > +       if (fw_fixed_hdr->ppcver >= PPC3_VERSION_TAS2781) {
> > +               p = (struct blktyp_devidx_map *)ppc3_tas2781_mapping_table;
> > +               n = ARRAY_SIZE(ppc3_tas2781_mapping_table);
> > +       } else if (fw_fixed_hdr->ppcver >= PPC3_VERSION) {
> > +               p = (struct blktyp_devidx_map *)ppc3_mapping_table;
> > +               n = ARRAY_SIZE(ppc3_mapping_table);
> > +       }
> > +
> > +       for (i = 0; i < n; i++) {
> > +               if (block->type == p[i].blktyp) {
> > +                       dev_idx = p[i].dev_idx;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       return dev_idx;
> > +}
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [EXTERNAL] Re: [PATCH v1] ASoC: tas2781: fixed compiling issue in m68k

[Geert Uytterhoeven]

Hi Shenghao,

On Tue, Oct 3, 2023 at 12:47 PM Ding, Shenghao <shenghao-ding@ti.com> wrote:
> > -----Original Message-----
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Sent: Tuesday, October 3, 2023 3:24 AM
> > To: Ding, Shenghao <shenghao-ding@ti.com>
> > Cc: broonie@kernel.org; robh+dt@kernel.org;
> > andriy.shevchenko@linux.intel.com; lgirdwood@gmail.com; perex@perex.cz;
> > pierre-louis.bossart@linux.intel.com; Lu, Kevin <kevin-lu@ti.com>;
> > 13916275206@139.com; alsa-devel@alsa-project.org; linux-
> > kernel@vger.kernel.org; liam.r.girdwood@intel.com; mengdong.lin@intel.com;
> > Xu, Baojun <baojun.xu@ti.com>; thomas.gfeller@q-drop.com; Gupta, Peeyush
> > <peeyush@ti.com>; Navada Kanyana, Mukund <navada@ti.com>;
> > tiwai@suse.de
> > Subject: [EXTERNAL] Re: [PATCH v1] ASoC: tas2781: fixed compiling issue in
> > m68k
> >
> > Hi Shenghao,
> >
> > Thanks for your patch!
> >
> > On Mon, Oct 2, 2023 at 4:38 PM Shenghao Ding <shenghao-ding@ti.com>
> > wrote:
> > > fixed m68k compiling issue: mapping table can save code field; storing
> > > the
> >
> > Please replicate the actual error message from the compiler, so it is recorded in
> > the commit description, and easy to find when someone searches for the actual
> > error message
> >
> > From the Reported-by (which is not included in the actual description, as it is
> > below the "---"):
> >
> >        {standard input}: Assembler messages:
> >     >> {standard input}:992: Error: value -148 out of range
> >        {standard input}:992: Error: value of ffffff6c too large for field of 1 byte at
> > 0000045f
> >
> > > dev_idx as a member of block can reduce unnecessary  time and system
> > > resource comsumption of dev_idx mapping every time the block data
> > > writing to the dsp.
> >
> > I am sorry, but I don't understand what this means.
> > Can you please elaborate?
> >
> > Also, can you please explain what the real issue is?
> > (My first guess when seeing the error message before was that a  signed
> > integer is truncated to an unsigned char or so, but I couldn't see  immediately
> > where that is happening)
> Sorry to late feedback. I had been troubled by this issue for several weeks. At first, I also think it would one of variables overflow, according to the compiling error message. However, after investigation, no result came out. In fact, compiler will report the line number where the variable overflow is, if there was risk on the variable overflow. Yet, this compiling error did not report any line number. Finally, I didn’t realize that it would be the code section overflow until that compiling error message magically disappeared, one day I removed some functions in the tas2781-fwlib.c. So, I began to simplify some functions in the code.

I managed to reproduce the issue with the m68k cross-compiler from
https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/13.2.0/

    | sound/soc/codecs/tas2781-fmwlib.c:779:            switch (block->type) {
            subq.l #1,%a0       |, tmp56
            moveq #36,%d0       |,
            cmp.l %a0,%d0       | tmp56,
            jcs .L122           |
            move.l %a0,%d0      | tmp56, tmp59
            add.l %d0,%d0       | tmp59
            move.w .L95(%pc,%d0.l),%d0  |, tmp60
    .L125:
    | sound/soc/codecs/tas2781-fmwlib.c:827:            switch (block->type) {
            jmp %pc@(2,%d0:w)   | tmp66
            .balignw 2,0x284c
    .L95:
            .word .L109-.L95
            .word .L113-.L95

            [...]

    | sound/soc/codecs/tas2781-fmwlib.c:827:            switch (block->type) {
            moveq #36,%d0       |,
            cmp.l %a0,%d0       | tmp77,
            jcs .L122           |
            move.l %a0,%d0      | tmp77,
            add.l %d0,%d0       |
            move.l %d0,%a0      |, tmp65
--->        move.w .L95(%pc,%a0.l),%d0  |, tmp66
            jra .L125           |

Looks like the compiler is sharing the jump table at L95 for two
different switch() statements, but the PC-relative offset to refer to
the table is too large when using -m68000.  It works fine with -m68020.

Probably a compiler bug?

> > > Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>
> > >
> > > ---
> > > Changes in v1:
> > >  - | Reported-by: kernel test robot <lkp@intel.com>
> > >    | Closes:
> > >    | https://lore.kernel.org/oe-kbuild-all/202309222048.RnSqEIK5-lkp@intel.com/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [EXTERNAL] Re: [PATCH v1] ASoC: tas2781: fixed compiling issue in m68k

[Andreas Schwab]

On Okt 03 2023, Geert Uytterhoeven wrote:

> Looks like the compiler is sharing the jump table at L95 for two
> different switch() statements, but the PC-relative offset to refer to
> the table is too large when using -m68000.  It works fine with -m68020.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104028

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."