| Message ID | 20190725234032.21152-24-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: updates for 5.4 | expand |
On 2019-07-26 01:40, Pierre-Louis Bossart wrote: > > - return do_bank_switch(stream); > + ret = do_bank_switch(stream); > + if (ret < 0) { > + dev_err(bus->dev, "Bank switch failed: %d\n", ret); > + return ret; > + } > + > + /* make sure alternate bank (previous current) is also disabled */ > + list_for_each_entry(m_rt, &stream->master_list, stream_node) { > + bus = m_rt->bus; > + /* Disable port(s) */ > + ret = sdw_enable_disable_ports(m_rt, false); > + if (ret < 0) { > + dev_err(bus->dev, "Disable port(s) failed: %d\n", ret); > + return ret; > + } > + } > + > + return 0; > } > > /** > While not directly connected to this commit, I see that you do: link_for_each_entry(m_rt, &stream->master_list, stream_node) quite often in /stream.c code. Helpful macro would prove useful.
On 7/26/19 5:14 AM, Cezary Rojewski wrote: > On 2019-07-26 01:40, Pierre-Louis Bossart wrote: >> - return do_bank_switch(stream); >> + ret = do_bank_switch(stream); >> + if (ret < 0) { >> + dev_err(bus->dev, "Bank switch failed: %d\n", ret); >> + return ret; >> + } >> + >> + /* make sure alternate bank (previous current) is also disabled */ >> + list_for_each_entry(m_rt, &stream->master_list, stream_node) { >> + bus = m_rt->bus; >> + /* Disable port(s) */ >> + ret = sdw_enable_disable_ports(m_rt, false); >> + if (ret < 0) { >> + dev_err(bus->dev, "Disable port(s) failed: %d\n", ret); >> + return ret; >> + } >> + } >> + >> + return 0; >> } >> /** >> > > While not directly connected to this commit, I see that you do: > link_for_each_entry(m_rt, &stream->master_list, stream_node) > > quite often in /stream.c code. Helpful macro would prove useful. Yes, but the flip side is that people need to look at what the macro does to figure it out, while everyone knows what list_for_each_entry() means. Not sure about this one. And on top of this we'll probably have to rework this code to have a background copy of the current bank in the alternate bank so it'd rather leave it simple for now.
Unrelated to this specific patch, but I looked at _sdw_disable_stream() and I see this there (again, maybe my version is outdated already): struct sdw_master_runtime *m_rt = NULL; struct sdw_bus *bus = NULL; where both those initialisations are redundant. Moreover: On Thu, Jul 25, 2019 at 06:40:15PM -0500, Pierre-Louis Bossart wrote: > When we disable the stream and then call hw_free, two bank switches > will be handled and as a result we re-enable the stream on hw_free. > > Make sure the stream is disabled on both banks. > > TODO: we need to completely revisit all this and make sure we have a > mirroring mechanism between current and alternate banks. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/stream.c | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c > index 53f5e790fcd7..75b9ad1fb1a6 100644 > --- a/drivers/soundwire/stream.c > +++ b/drivers/soundwire/stream.c > @@ -1637,7 +1637,24 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream) > } > } > > - return do_bank_switch(stream); > + ret = do_bank_switch(stream); > + if (ret < 0) { > + dev_err(bus->dev, "Bank switch failed: %d\n", ret); > + return ret; > + } > + > + /* make sure alternate bank (previous current) is also disabled */ > + list_for_each_entry(m_rt, &stream->master_list, stream_node) { > + bus = m_rt->bus; "bus" is only used below, so at least take that line under "if (ret < 0)" or even just do "dev_err(m_rt->bus->dev,...)" everywhere in this function and remove the variable altogether... Thanks Guennadi > + /* Disable port(s) */ > + ret = sdw_enable_disable_ports(m_rt, false); > + if (ret < 0) { > + dev_err(bus->dev, "Disable port(s) failed: %d\n", ret); > + return ret; > + } > + } > + > + return 0; > } > > /** > -- > 2.20.1 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> Unrelated to this specific patch, but I looked at _sdw_disable_stream() > and I see this there (again, maybe my version is outdated already): > > struct sdw_master_runtime *m_rt = NULL; > struct sdw_bus *bus = NULL; > > where both those initialisations are redundant. Moreover: will look at this, thanks. > > On Thu, Jul 25, 2019 at 06:40:15PM -0500, Pierre-Louis Bossart wrote: >> When we disable the stream and then call hw_free, two bank switches >> will be handled and as a result we re-enable the stream on hw_free. >> >> Make sure the stream is disabled on both banks. >> >> TODO: we need to completely revisit all this and make sure we have a >> mirroring mechanism between current and alternate banks. >> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> --- >> drivers/soundwire/stream.c | 19 ++++++++++++++++++- >> 1 file changed, 18 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c >> index 53f5e790fcd7..75b9ad1fb1a6 100644 >> --- a/drivers/soundwire/stream.c >> +++ b/drivers/soundwire/stream.c >> @@ -1637,7 +1637,24 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream) >> } >> } >> >> - return do_bank_switch(stream); >> + ret = do_bank_switch(stream); >> + if (ret < 0) { >> + dev_err(bus->dev, "Bank switch failed: %d\n", ret); >> + return ret; >> + } >> + >> + /* make sure alternate bank (previous current) is also disabled */ >> + list_for_each_entry(m_rt, &stream->master_list, stream_node) { >> + bus = m_rt->bus; > > "bus" is only used below, so at least take that line under "if (ret < 0)" > or even just do "dev_err(m_rt->bus->dev,...)" everywhere in this function > and remove the variable altogether... will look at this, thanks Guennadi > > Thanks > Guennadi > >> + /* Disable port(s) */ >> + ret = sdw_enable_disable_ports(m_rt, false); >> + if (ret < 0) { >> + dev_err(bus->dev, "Disable port(s) failed: %d\n", ret); >> + return ret;
On Thu, Jul 25, 2019 at 06:40:15PM -0500, Pierre-Louis Bossart wrote: > When we disable the stream and then call hw_free, two bank switches > will be handled and as a result we re-enable the stream on hw_free. > I didnt quite get why there will be two bank switches as part of disable flow which leads to enabling of stream? > Make sure the stream is disabled on both banks. > > TODO: we need to completely revisit all this and make sure we have a > mirroring mechanism between current and alternate banks. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/stream.c | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c > index 53f5e790fcd7..75b9ad1fb1a6 100644 > --- a/drivers/soundwire/stream.c > +++ b/drivers/soundwire/stream.c > @@ -1637,7 +1637,24 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream) > } > } > > - return do_bank_switch(stream); > + ret = do_bank_switch(stream); > + if (ret < 0) { > + dev_err(bus->dev, "Bank switch failed: %d\n", ret); > + return ret; > + } > + > + /* make sure alternate bank (previous current) is also disabled */ > + list_for_each_entry(m_rt, &stream->master_list, stream_node) { > + bus = m_rt->bus; > + /* Disable port(s) */ > + ret = sdw_enable_disable_ports(m_rt, false); > + if (ret < 0) { > + dev_err(bus->dev, "Disable port(s) failed: %d\n", ret); > + return ret; > + } > + } > + > + return 0; > } > > /** > -- > 2.20.1 >
On 8/5/19 4:56 AM, Sanyog Kale wrote: > On Thu, Jul 25, 2019 at 06:40:15PM -0500, Pierre-Louis Bossart wrote: >> When we disable the stream and then call hw_free, two bank switches >> will be handled and as a result we re-enable the stream on hw_free. >> > > I didnt quite get why there will be two bank switches as part of disable flow > which leads to enabling of stream? You have two bank switches, one to stop streaming and on in de-prepare. It's symmetrical with the start sequence, where we do a bank switch to prepare and another to enable. Let's assume we are using bank0 when streaming. Before the first bank switch, the channel_enable is set to false in the alternate bank1. When the bank switch happens, bank1 become active and the streaming stops. But bank0 registers have not been modified so when we do the second bank switch in de-prepare we make bank0 active, and the ch_enable bits are still set so streaming will restart... When we stop streaming, we need to make sure the ch_enable bits are cleared in the two banks. > >> Make sure the stream is disabled on both banks. >> >> TODO: we need to completely revisit all this and make sure we have a >> mirroring mechanism between current and alternate banks. >> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> --- >> drivers/soundwire/stream.c | 19 ++++++++++++++++++- >> 1 file changed, 18 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c >> index 53f5e790fcd7..75b9ad1fb1a6 100644 >> --- a/drivers/soundwire/stream.c >> +++ b/drivers/soundwire/stream.c >> @@ -1637,7 +1637,24 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream) >> } >> } >> >> - return do_bank_switch(stream); >> + ret = do_bank_switch(stream); >> + if (ret < 0) { >> + dev_err(bus->dev, "Bank switch failed: %d\n", ret); >> + return ret; >> + } >> + >> + /* make sure alternate bank (previous current) is also disabled */ >> + list_for_each_entry(m_rt, &stream->master_list, stream_node) { >> + bus = m_rt->bus; >> + /* Disable port(s) */ >> + ret = sdw_enable_disable_ports(m_rt, false); >> + if (ret < 0) { >> + dev_err(bus->dev, "Disable port(s) failed: %d\n", ret); >> + return ret; >> + } >> + } >> + >> + return 0; >> } >> >> /** >> -- >> 2.20.1 >> >
On Mon, Aug 05, 2019 at 10:33:25AM -0500, Pierre-Louis Bossart wrote: > > > On 8/5/19 4:56 AM, Sanyog Kale wrote: > > On Thu, Jul 25, 2019 at 06:40:15PM -0500, Pierre-Louis Bossart wrote: > > > When we disable the stream and then call hw_free, two bank switches > > > will be handled and as a result we re-enable the stream on hw_free. > > > > > > > I didnt quite get why there will be two bank switches as part of disable flow > > which leads to enabling of stream? > > You have two bank switches, one to stop streaming and on in de-prepare. It's > symmetrical with the start sequence, where we do a bank switch to prepare > and another to enable. Got it. I misunderstood it that two bank switches are performed as part of disable_stream. > > Let's assume we are using bank0 when streaming. > > Before the first bank switch, the channel_enable is set to false in the > alternate bank1. When the bank switch happens, bank1 become active and the > streaming stops. But bank0 registers have not been modified so when we do > the second bank switch in de-prepare we make bank0 active, and the ch_enable > bits are still set so streaming will restart... When we stop streaming, we > need to make sure the ch_enable bits are cleared in the two banks. This is clear. Even though the channels remains enabled, i believe there won't be any data pushed on lines as stream will be closed. Regarding mirroring approach, I assume after bank switch we will take snapshot of active bank and program same in inactive bank. > > > > > > > Make sure the stream is disabled on both banks. > > > > > > TODO: we need to completely revisit all this and make sure we have a > > > mirroring mechanism between current and alternate banks. > > > > > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > > --- > > > drivers/soundwire/stream.c | 19 ++++++++++++++++++- > > > 1 file changed, 18 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c > > > index 53f5e790fcd7..75b9ad1fb1a6 100644 > > > --- a/drivers/soundwire/stream.c > > > +++ b/drivers/soundwire/stream.c > > > @@ -1637,7 +1637,24 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream) > > > } > > > } > > > - return do_bank_switch(stream); > > > + ret = do_bank_switch(stream); > > > + if (ret < 0) { > > > + dev_err(bus->dev, "Bank switch failed: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + /* make sure alternate bank (previous current) is also disabled */ > > > + list_for_each_entry(m_rt, &stream->master_list, stream_node) { > > > + bus = m_rt->bus; > > > + /* Disable port(s) */ > > > + ret = sdw_enable_disable_ports(m_rt, false); > > > + if (ret < 0) { > > > + dev_err(bus->dev, "Disable port(s) failed: %d\n", ret); > > > + return ret; > > > + } > > > + } > > > + > > > + return 0; > > > } > > > /** > > > -- > > > 2.20.1 > > > > >
On 8/5/19 11:32 AM, Sanyog Kale wrote: > On Mon, Aug 05, 2019 at 10:33:25AM -0500, Pierre-Louis Bossart wrote: >> >> >> On 8/5/19 4:56 AM, Sanyog Kale wrote: >>> On Thu, Jul 25, 2019 at 06:40:15PM -0500, Pierre-Louis Bossart wrote: >>>> When we disable the stream and then call hw_free, two bank switches >>>> will be handled and as a result we re-enable the stream on hw_free. >>>> >>> >>> I didnt quite get why there will be two bank switches as part of disable flow >>> which leads to enabling of stream? >> >> You have two bank switches, one to stop streaming and on in de-prepare. It's >> symmetrical with the start sequence, where we do a bank switch to prepare >> and another to enable. > > Got it. I misunderstood it that two bank switches are performed as part of > disable_stream. > >> >> Let's assume we are using bank0 when streaming. >> >> Before the first bank switch, the channel_enable is set to false in the >> alternate bank1. When the bank switch happens, bank1 become active and the >> streaming stops. But bank0 registers have not been modified so when we do >> the second bank switch in de-prepare we make bank0 active, and the ch_enable >> bits are still set so streaming will restart... When we stop streaming, we >> need to make sure the ch_enable bits are cleared in the two banks. > > This is clear. Even though the channels remains enabled, i believe there > won't be any data pushed on lines as stream will be closed. Actually the link remains active. We tested this by setting the PRBS data mode and the Slave device detects when we artificially inject errors. There is however no data consumption on the DMA side of the Master IP since the DMA is indeed stopped. > > Regarding mirroring approach, I assume after bank switch we will take > snapshot of active bank and program same in inactive bank. That should be the approach yes. > >> >> >>> >>>> Make sure the stream is disabled on both banks. >>>> >>>> TODO: we need to completely revisit all this and make sure we have a >>>> mirroring mechanism between current and alternate banks. >>>> >>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >>>> --- >>>> drivers/soundwire/stream.c | 19 ++++++++++++++++++- >>>> 1 file changed, 18 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c >>>> index 53f5e790fcd7..75b9ad1fb1a6 100644 >>>> --- a/drivers/soundwire/stream.c >>>> +++ b/drivers/soundwire/stream.c >>>> @@ -1637,7 +1637,24 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream) >>>> } >>>> } >>>> - return do_bank_switch(stream); >>>> + ret = do_bank_switch(stream); >>>> + if (ret < 0) { >>>> + dev_err(bus->dev, "Bank switch failed: %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + /* make sure alternate bank (previous current) is also disabled */ >>>> + list_for_each_entry(m_rt, &stream->master_list, stream_node) { >>>> + bus = m_rt->bus; >>>> + /* Disable port(s) */ >>>> + ret = sdw_enable_disable_ports(m_rt, false); >>>> + if (ret < 0) { >>>> + dev_err(bus->dev, "Disable port(s) failed: %d\n", ret); >>>> + return ret; >>>> + } >>>> + } >>>> + >>>> + return 0; >>>> } >>>> /** >>>> -- >>>> 2.20.1 >>>> >>> >
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 53f5e790fcd7..75b9ad1fb1a6 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1637,7 +1637,24 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream) } } - return do_bank_switch(stream); + ret = do_bank_switch(stream); + if (ret < 0) { + dev_err(bus->dev, "Bank switch failed: %d\n", ret); + return ret; + } + + /* make sure alternate bank (previous current) is also disabled */ + list_for_each_entry(m_rt, &stream->master_list, stream_node) { + bus = m_rt->bus; + /* Disable port(s) */ + ret = sdw_enable_disable_ports(m_rt, false); + if (ret < 0) { + dev_err(bus->dev, "Disable port(s) failed: %d\n", ret); + return ret; + } + } + + return 0; } /**
When we disable the stream and then call hw_free, two bank switches will be handled and as a result we re-enable the stream on hw_free. Make sure the stream is disabled on both banks. TODO: we need to completely revisit all this and make sure we have a mirroring mechanism between current and alternate banks. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/soundwire/stream.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)