| Message ID | 20190725234032.21152-10-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: updates for 5.4 | expand |
On Thu, Jul 25, 2019 at 06:40:01PM -0500, Pierre-Louis Bossart wrote: > Per the hardware documentation, all changes to MCP_CONFIG, > MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL need to be validated with a > self-clearing write to MCP_CONFIG_UPDATE. > > For some reason, the existing code only does this write to > CONFIG_UPDATE when enabling interrupts. Add a helper and do the update > when the CONFIG is changed. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/cadence_master.c | 29 +++++++++++++++++++++-------- > 1 file changed, 21 insertions(+), 8 deletions(-) > > diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c > index 9f611a1fff0a..eb46cf651d62 100644 > --- a/drivers/soundwire/cadence_master.c > +++ b/drivers/soundwire/cadence_master.c > @@ -224,6 +224,22 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value) > return -EAGAIN; > } > > +/* > + * all changes to the MCP_CONFIG, MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL > + * need to be confirmed with a write to MCP_CONFIG_UPDATE > + */ > +static int cdns_update_config(struct sdw_cdns *cdns) > +{ > + int ret; > + > + ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE, > + CDNS_MCP_CONFIG_UPDATE_BIT); > + if (ret < 0) > + dev_err(cdns->dev, "Config update timedout\n"); > + > + return ret; > +} > + > /* > * debugfs > */ > @@ -758,15 +774,9 @@ static int _cdns_enable_interrupt(struct sdw_cdns *cdns) > */ > int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns) > { > - int ret; > - > _cdns_enable_interrupt(cdns); > - ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE, > - CDNS_MCP_CONFIG_UPDATE_BIT); > - if (ret < 0) > - dev_err(cdns->dev, "Config update timedout\n"); > > - return ret; > + return 0; > } > EXPORT_SYMBOL(sdw_cdns_enable_interrupt); > > @@ -943,7 +953,10 @@ int sdw_cdns_init(struct sdw_cdns *cdns) > > cdns_writel(cdns, CDNS_MCP_CONFIG, val); > > - return 0; > + /* commit changes */ > + ret = cdns_update_config(cdns); > + > + return ret; + return cdns_update_config(cdns); Thanks Guennadi > } > EXPORT_SYMBOL(sdw_cdns_init); > > -- > 2.20.1 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 7/26/2019 7:40 AM, Pierre-Louis Bossart wrote: > Per the hardware documentation, all changes to MCP_CONFIG, > MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL need to be validated with a > self-clearing write to MCP_CONFIG_UPDATE. > > For some reason, the existing code only does this write to > CONFIG_UPDATE when enabling interrupts. Add a helper and do the update > when the CONFIG is changed. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/cadence_master.c | 29 +++++++++++++++++++++-------- > 1 file changed, 21 insertions(+), 8 deletions(-) > > diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c > index 9f611a1fff0a..eb46cf651d62 100644 > --- a/drivers/soundwire/cadence_master.c > +++ b/drivers/soundwire/cadence_master.c > @@ -224,6 +224,22 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value) > return -EAGAIN; > } > > +/* > + * all changes to the MCP_CONFIG, MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL > + * need to be confirmed with a write to MCP_CONFIG_UPDATE > + */ > +static int cdns_update_config(struct sdw_cdns *cdns) > +{ > + int ret; > + > + ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE, > + CDNS_MCP_CONFIG_UPDATE_BIT); > + if (ret < 0) > + dev_err(cdns->dev, "Config update timedout\n"); > + > + return ret; > +} > + > /* > * debugfs > */ > @@ -758,15 +774,9 @@ static int _cdns_enable_interrupt(struct sdw_cdns *cdns) > */ > int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns) > { > - int ret; > - > _cdns_enable_interrupt(cdns); > - ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE, > - CDNS_MCP_CONFIG_UPDATE_BIT); > - if (ret < 0) > - dev_err(cdns->dev, "Config update timedout\n"); > > - return ret; Should we add cdns_update_config() here? > + return 0; > } > EXPORT_SYMBOL(sdw_cdns_enable_interrupt); > > @@ -943,7 +953,10 @@ int sdw_cdns_init(struct sdw_cdns *cdns) > > cdns_writel(cdns, CDNS_MCP_CONFIG, val); > > - return 0; > + /* commit changes */ > + ret = cdns_update_config(cdns); > + > + return ret; > } > EXPORT_SYMBOL(sdw_cdns_init); >
On 2019-07-26 01:40, Pierre-Louis Bossart wrote: > /* > * debugfs > */ > @@ -758,15 +774,9 @@ static int _cdns_enable_interrupt(struct sdw_cdns *cdns) > */ > int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns) > { > - int ret; > - > _cdns_enable_interrupt(cdns); > - ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE, > - CDNS_MCP_CONFIG_UPDATE_BIT); > - if (ret < 0) > - dev_err(cdns->dev, "Config update timedout\n"); > > - return ret; > + return 0; > } > EXPORT_SYMBOL(sdw_cdns_enable_interrupt); Rather than ignoring _cdns_enable_interrupt - despite said func always returning 0 - simply do: return _cnds_enable_interrupt(cdns) and flag caller with inline. Afterwards, one can think if such encapsulation is even required - remove existing sdw_cdns_enable_interrupt and rename _cnds_enable_interrupt?
On 2019-07-26 11:53, Cezary Rojewski wrote: > On 2019-07-26 01:40, Pierre-Louis Bossart wrote: >> /* >> * debugfs >> */ >> @@ -758,15 +774,9 @@ static int _cdns_enable_interrupt(struct sdw_cdns >> *cdns) >> */ >> int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns) >> { >> - int ret; >> - >> _cdns_enable_interrupt(cdns); >> - ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE, >> - CDNS_MCP_CONFIG_UPDATE_BIT); >> - if (ret < 0) >> - dev_err(cdns->dev, "Config update timedout\n"); >> - return ret; >> + return 0; >> } >> EXPORT_SYMBOL(sdw_cdns_enable_interrupt); > > Rather than ignoring _cdns_enable_interrupt - despite said func always > returning 0 - simply do: return _cnds_enable_interrupt(cdns) and flag > caller with inline. > > Afterwards, one can think if such encapsulation is even required - > remove existing sdw_cdns_enable_interrupt and rename > _cnds_enable_interrupt? Nevermind, I see you simplified it in the next patch..
>> @@ -758,15 +774,9 @@ static int _cdns_enable_interrupt(struct sdw_cdns >> *cdns) >> */ >> int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns) >> { >> - int ret; >> - >> _cdns_enable_interrupt(cdns); >> - ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE, >> - CDNS_MCP_CONFIG_UPDATE_BIT); >> - if (ret < 0) >> - dev_err(cdns->dev, "Config update timedout\n"); >> - return ret; > Should we add cdns_update_config() here? indeed, this would be a good improvement. The code works because we added the exit_reset() sequence which does call cdns_update_config(), but better make this function self-contained. When we enable the clock-stop mode we will not do this reset sequence.
>> @@ -943,7 +953,10 @@ int sdw_cdns_init(struct sdw_cdns *cdns) >> >> cdns_writel(cdns, CDNS_MCP_CONFIG, val); >> >> - return 0; >> + /* commit changes */ >> + ret = cdns_update_config(cdns); >> + >> + return ret; > > + return cdns_update_config(cdns); yes, will fix. thanks!
On 25-07-19, 18:40, Pierre-Louis Bossart wrote: > int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns) > { > - int ret; > - > _cdns_enable_interrupt(cdns); > - ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE, > - CDNS_MCP_CONFIG_UPDATE_BIT); > - if (ret < 0) > - dev_err(cdns->dev, "Config update timedout\n"); I was expecting cdns_update_config() to be invoked here?? > > - return ret; > + return 0; It would be better if we return a value here a not success always > @@ -943,7 +953,10 @@ int sdw_cdns_init(struct sdw_cdns *cdns) > > cdns_writel(cdns, CDNS_MCP_CONFIG, val); > > - return 0; > + /* commit changes */ > + ret = cdns_update_config(cdns); > + > + return ret; return cdns_update_config()
On 8/2/19 7:03 AM, Vinod Koul wrote: > On 25-07-19, 18:40, Pierre-Louis Bossart wrote: > >> int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns) >> { >> - int ret; >> - >> _cdns_enable_interrupt(cdns); >> - ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE, >> - CDNS_MCP_CONFIG_UPDATE_BIT); >> - if (ret < 0) >> - dev_err(cdns->dev, "Config update timedout\n"); > > I was expecting cdns_update_config() to be invoked here?? > >> >> - return ret; >> + return 0; > > It would be better if we return a value here a not success always > >> @@ -943,7 +953,10 @@ int sdw_cdns_init(struct sdw_cdns *cdns) >> >> cdns_writel(cdns, CDNS_MCP_CONFIG, val); >> >> - return 0; >> + /* commit changes */ >> + ret = cdns_update_config(cdns); >> + >> + return ret; > > return cdns_update_config() yes, all of this is fixed already.
On Thu, Jul 25, 2019 at 06:40:01PM -0500, Pierre-Louis Bossart wrote: > Per the hardware documentation, all changes to MCP_CONFIG, > MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL need to be validated with a > self-clearing write to MCP_CONFIG_UPDATE. > > For some reason, the existing code only does this write to > CONFIG_UPDATE when enabling interrupts. Add a helper and do the update > when the CONFIG is changed. > the sequence of intel_probe is as follows: 1. intel_link_power_up 2. intel_shim_init 3. sdw_cdns_init 4. sdw_cdns_enable_interrupt Since we do self-clearing write to MCP_CONFIG_UPDATE in sdw_cdns_enable_interrupt once for all the config changes, we dont perform it as part of sdw_cdns_init. It does make sense to seperate it out from sdw_cdns_enable_interrupt so that we can use when clockstop is enabled where we dont need to enable interrupts. > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/cadence_master.c | 29 +++++++++++++++++++++-------- > 1 file changed, 21 insertions(+), 8 deletions(-) > > diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c > index 9f611a1fff0a..eb46cf651d62 100644 > --- a/drivers/soundwire/cadence_master.c > +++ b/drivers/soundwire/cadence_master.c > @@ -224,6 +224,22 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value) > return -EAGAIN; > } > > +/* > + * all changes to the MCP_CONFIG, MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL > + * need to be confirmed with a write to MCP_CONFIG_UPDATE > + */ > +static int cdns_update_config(struct sdw_cdns *cdns) > +{ > + int ret; > + > + ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE, > + CDNS_MCP_CONFIG_UPDATE_BIT); > + if (ret < 0) > + dev_err(cdns->dev, "Config update timedout\n"); > + > + return ret; > +} > + > /* > * debugfs > */ > @@ -758,15 +774,9 @@ static int _cdns_enable_interrupt(struct sdw_cdns *cdns) > */ > int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns) > { > - int ret; > - > _cdns_enable_interrupt(cdns); > - ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE, > - CDNS_MCP_CONFIG_UPDATE_BIT); > - if (ret < 0) > - dev_err(cdns->dev, "Config update timedout\n"); > > - return ret; > + return 0; > } > EXPORT_SYMBOL(sdw_cdns_enable_interrupt); > > @@ -943,7 +953,10 @@ int sdw_cdns_init(struct sdw_cdns *cdns) > > cdns_writel(cdns, CDNS_MCP_CONFIG, val); > > - return 0; > + /* commit changes */ > + ret = cdns_update_config(cdns); > + > + return ret; > } > EXPORT_SYMBOL(sdw_cdns_init); > > -- > 2.20.1 >
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 9f611a1fff0a..eb46cf651d62 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -224,6 +224,22 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value) return -EAGAIN; } +/* + * all changes to the MCP_CONFIG, MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL + * need to be confirmed with a write to MCP_CONFIG_UPDATE + */ +static int cdns_update_config(struct sdw_cdns *cdns) +{ + int ret; + + ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE, + CDNS_MCP_CONFIG_UPDATE_BIT); + if (ret < 0) + dev_err(cdns->dev, "Config update timedout\n"); + + return ret; +} + /* * debugfs */ @@ -758,15 +774,9 @@ static int _cdns_enable_interrupt(struct sdw_cdns *cdns) */ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns) { - int ret; - _cdns_enable_interrupt(cdns); - ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE, - CDNS_MCP_CONFIG_UPDATE_BIT); - if (ret < 0) - dev_err(cdns->dev, "Config update timedout\n"); - return ret; + return 0; } EXPORT_SYMBOL(sdw_cdns_enable_interrupt); @@ -943,7 +953,10 @@ int sdw_cdns_init(struct sdw_cdns *cdns) cdns_writel(cdns, CDNS_MCP_CONFIG, val); - return 0; + /* commit changes */ + ret = cdns_update_config(cdns); + + return ret; } EXPORT_SYMBOL(sdw_cdns_init);
Per the hardware documentation, all changes to MCP_CONFIG, MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL need to be validated with a self-clearing write to MCP_CONFIG_UPDATE. For some reason, the existing code only does this write to CONFIG_UPDATE when enabling interrupts. Add a helper and do the update when the CONFIG is changed. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/soundwire/cadence_master.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)