Message ID | 569AD8A7.7080803@maciej.szmigiero.name (mailing list archive) |
---|---|
State | Not Applicable |
Delegated to: | Scott Wood |
Headers | show |
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.
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
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?
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 --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");