Re: [alsa-devel] [RFC PATCH 23/40] soundwire: stream: fix disable sequence
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
---
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
Re: [alsa-devel] [RFC PATCH 23/40] soundwire: stream: fix disable sequence
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
> > > ---
> > > 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
> > >
> >
--
Re: [alsa-devel] [RFC PATCH 23/40] soundwire: stream: fix disable sequence
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
---
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
Re: [alsa-devel] [RFC PATCH 23/40] soundwire: stream: fix disable sequence
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
---
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;
Re: [alsa-devel] [RFC PATCH 23/40] soundwire: stream: fix disable sequence
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
> ---
> 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-de...@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Re: [alsa-devel] [RFC PATCH 23/40] soundwire: stream: fix disable sequence
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.
