Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer
On Wed, Dec 06, 2017 at 07:32:43AM -0600, Pierre-Louis Bossart wrote:
> On 12/5/17 11:58 PM, Vinod Koul wrote:
> >On Tue, Dec 05, 2017 at 08:48:03AM -0600, Pierre-Louis Bossart wrote:
> >>On 12/5/17 7:43 AM, Pierre-Louis Bossart wrote:
> >>>On 12/5/17 12:31 AM, Vinod Koul wrote:
> On Sun, Dec 03, 2017 at 09:01:41PM -0600, Pierre-Louis Bossart wrote:
> >
> +static inline int do_transfer(struct sdw_bus *bus, struct
> sdw_msg *msg)
> +{
> + int retry = bus->prop.err_threshold;
> + enum sdw_command_response resp;
> + int ret = 0, i;
> +
> + for (i = 0; i <= retry; i++) {
> + resp = bus->ops->xfer_msg(bus, msg);
> + ret = find_response_code(resp);
> +
> + /* if cmd is ok or ignored return */
> + if (ret == 0 || ret == -ENODATA)
> >>>
> >>>Can you document why you don't retry on a CMD_IGNORED? I know
> >>>there was a
> >>>reason, I just can't remember it.
> >>
> >>CMD_IGNORED can be okay on broadcast. User of this API can retry all
> >>they
> >>want!
> >
> >So you retry if this is a CMD_FAILED but let higher levels retry for
> >CMD_IGNORED, sorry I don't see the logic.
>
> Yes that is right.
>
> If I am doing a broadcast read, lets say for Device Id registers, why in
> the
> world would I want to retry? CMD_IGNORED is a valid response and
> required to
> stop enumeration cycle in that case.
> >
> >Above is the clarfication
> >
> But if I am not expecting a CMD_IGNORED response, I can very well go
> ahead
> and retry from caller. The context is with caller and they can choose to
> do
> appropriate handling.
>
> And I have clarified this couple of times to you already, not sure how
> many
> more times I would have to do that.
> >>>
> >>>Until you clarify what you are doing.
> >
> >Let me try again, I think u missed that part of my reply above
> >
> >If I am doing a broadcast read, lets say for Device Id registers,
> >why in the world would I want to retry? CMD_IGNORED is a valid response
> >and required to stop enumeration cycle in that case.
> >
> >>>There is ONE case where IGNORED is a valid answer (reading the Prepare not
> >>>finished bits), and it should not only be documented but analyzed in more
> >>>details.
> >>I meant Read SCP_DevID registers from Device0... prepare bits should never
> >>return a CMD_IGNORED
> >
> >Precisely as I pointed out above.
> >
> >>>For a write an IGNORED is never OK.
> >
> >Agreed, but then transfer does both read and write. Write call can treat it
> >as error and read call leaves it upto caller.
> >
> >Does that sound okay for you?
>
> Not really.
> You have one case where it's ok and all others are not ok, to me this sounds
> like you should avoid the retry at the lowest level rather than pushing this
> to the caller
> And now that I think of it, the definitions for the DisCo spec were really
> meant for hardware-based retries available in some master IPs. If this is
> enabled, then the loops in software are not really needed at all. Can you
> please check this point?
Sure I will check that
--
~Vinod
Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer
On 12/5/17 11:58 PM, Vinod Koul wrote:
On Tue, Dec 05, 2017 at 08:48:03AM -0600, Pierre-Louis Bossart wrote:
On 12/5/17 7:43 AM, Pierre-Louis Bossart wrote:
On 12/5/17 12:31 AM, Vinod Koul wrote:
On Sun, Dec 03, 2017 at 09:01:41PM -0600, Pierre-Louis Bossart wrote:
+static inline int do_transfer(struct sdw_bus *bus, struct
sdw_msg *msg)
+{
+ int retry = bus->prop.err_threshold;
+ enum sdw_command_response resp;
+ int ret = 0, i;
+
+ for (i = 0; i <= retry; i++) {
+ resp = bus->ops->xfer_msg(bus, msg);
+ ret = find_response_code(resp);
+
+ /* if cmd is ok or ignored return */
+ if (ret == 0 || ret == -ENODATA)
Can you document why you don't retry on a CMD_IGNORED? I know
there was a
reason, I just can't remember it.
CMD_IGNORED can be okay on broadcast. User of this API can retry all
they
want!
So you retry if this is a CMD_FAILED but let higher levels retry for
CMD_IGNORED, sorry I don't see the logic.
Yes that is right.
If I am doing a broadcast read, lets say for Device Id registers, why in
the
world would I want to retry? CMD_IGNORED is a valid response and
required to
stop enumeration cycle in that case.
Above is the clarfication
But if I am not expecting a CMD_IGNORED response, I can very well go
ahead
and retry from caller. The context is with caller and they can choose to
do
appropriate handling.
And I have clarified this couple of times to you already, not sure how
many
more times I would have to do that.
Until you clarify what you are doing.
Let me try again, I think u missed that part of my reply above
If I am doing a broadcast read, lets say for Device Id registers,
why in the world would I want to retry? CMD_IGNORED is a valid response
and required to stop enumeration cycle in that case.
There is ONE case where IGNORED is a valid answer (reading the Prepare not
finished bits), and it should not only be documented but analyzed in more
details.
I meant Read SCP_DevID registers from Device0... prepare bits should never
return a CMD_IGNORED
Precisely as I pointed out above.
For a write an IGNORED is never OK.
Agreed, but then transfer does both read and write. Write call can treat it
as error and read call leaves it upto caller.
Does that sound okay for you?
Not really.
You have one case where it's ok and all others are not ok, to me this
sounds like you should avoid the retry at the lowest level rather than
pushing this to the caller
And now that I think of it, the definitions for the DisCo spec were
really meant for hardware-based retries available in some master IPs. If
this is enabled, then the loops in software are not really needed at
all. Can you please check this point?
Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer
On Tue, Dec 05, 2017 at 08:48:03AM -0600, Pierre-Louis Bossart wrote:
> On 12/5/17 7:43 AM, Pierre-Louis Bossart wrote:
> >On 12/5/17 12:31 AM, Vinod Koul wrote:
> >>On Sun, Dec 03, 2017 at 09:01:41PM -0600, Pierre-Louis Bossart wrote:
> >>+static inline int do_transfer(struct sdw_bus *bus, struct
> >>sdw_msg *msg)
> >>+{
> >>+ int retry = bus->prop.err_threshold;
> >>+ enum sdw_command_response resp;
> >>+ int ret = 0, i;
> >>+
> >>+ for (i = 0; i <= retry; i++) {
> >>+ resp = bus->ops->xfer_msg(bus, msg);
> >>+ ret = find_response_code(resp);
> >>+
> >>+ /* if cmd is ok or ignored return */
> >>+ if (ret == 0 || ret == -ENODATA)
> >
> >Can you document why you don't retry on a CMD_IGNORED? I know
> >there was a
> >reason, I just can't remember it.
>
> CMD_IGNORED can be okay on broadcast. User of this API can retry all
> they
> want!
> >>>
> >>>So you retry if this is a CMD_FAILED but let higher levels retry for
> >>>CMD_IGNORED, sorry I don't see the logic.
> >>
> >>Yes that is right.
> >>
> >>If I am doing a broadcast read, lets say for Device Id registers, why in
> >>the
> >>world would I want to retry? CMD_IGNORED is a valid response and
> >>required to
> >>stop enumeration cycle in that case.
Above is the clarfication
> >>But if I am not expecting a CMD_IGNORED response, I can very well go
> >>ahead
> >>and retry from caller. The context is with caller and they can choose to
> >>do
> >>appropriate handling.
> >>
> >>And I have clarified this couple of times to you already, not sure how
> >>many
> >>more times I would have to do that.
> >
> >Until you clarify what you are doing.
Let me try again, I think u missed that part of my reply above
If I am doing a broadcast read, lets say for Device Id registers,
why in the world would I want to retry? CMD_IGNORED is a valid response
and required to stop enumeration cycle in that case.
> >There is ONE case where IGNORED is a valid answer (reading the Prepare not
> >finished bits), and it should not only be documented but analyzed in more
> >details.
> I meant Read SCP_DevID registers from Device0... prepare bits should never
> return a CMD_IGNORED
Precisely as I pointed out above.
> >For a write an IGNORED is never OK.
Agreed, but then transfer does both read and write. Write call can treat it
as error and read call leaves it upto caller.
Does that sound okay for you?
--
~Vinod
Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer
On 12/5/17 7:43 AM, Pierre-Louis Bossart wrote:
On 12/5/17 12:31 AM, Vinod Koul wrote:
On Sun, Dec 03, 2017 at 09:01:41PM -0600, Pierre-Louis Bossart wrote:
On 12/3/17 11:04 AM, Vinod Koul wrote:
On Fri, Dec 01, 2017 at 05:27:31PM -0600, Pierre-Louis Bossart wrote:
Sorry looks like I missed replying to this one earlier.
+static inline int find_response_code(enum sdw_command_response resp)
+{
+ switch (resp) {
+ case SDW_CMD_OK:
+ return 0;
+
+ case SDW_CMD_IGNORED:
+ return -ENODATA;
+
+ case SDW_CMD_TIMEOUT:
+ return -ETIMEDOUT;
+
+ default:
+ return -EIO;
the 'default' case will handle both SDW_CMD_FAIL (which is a bus event
usually due to bus clash or parity issues) and SDW_CMD_FAIL_OTHER
(which is
an imp-def IP event).
Do they really belong in the same basket? From a debug perspective
there is
quite a bit of information lost.
at higher level the error handling is same. the information is not
lost as
it is expected that you would log it at error source.
I don't understand this. It's certainly not the same for me if you
detect an
electric problem or if the IP is in the weeds. Logging at the source
is fine
but this filtering prevents higher levels from doing anything different.
The point is higher levels like here cant do much than bail out and
complain.
Can you point out what would be different behaviour in each of these
cases?
+static inline int do_transfer(struct sdw_bus *bus, struct sdw_msg
*msg)
+{
+ int retry = bus->prop.err_threshold;
+ enum sdw_command_response resp;
+ int ret = 0, i;
+
+ for (i = 0; i <= retry; i++) {
+ resp = bus->ops->xfer_msg(bus, msg);
+ ret = find_response_code(resp);
+
+ /* if cmd is ok or ignored return */
+ if (ret == 0 || ret == -ENODATA)
Can you document why you don't retry on a CMD_IGNORED? I know there
was a
reason, I just can't remember it.
CMD_IGNORED can be okay on broadcast. User of this API can retry all
they
want!
So you retry if this is a CMD_FAILED but let higher levels retry for
CMD_IGNORED, sorry I don't see the logic.
Yes that is right.
If I am doing a broadcast read, lets say for Device Id registers, why
in the
world would I want to retry? CMD_IGNORED is a valid response and
required to
stop enumeration cycle in that case.
But if I am not expecting a CMD_IGNORED response, I can very well go
ahead
and retry from caller. The context is with caller and they can choose
to do
appropriate handling.
And I have clarified this couple of times to you already, not sure how
many
more times I would have to do that.
Until you clarify what you are doing.
There is ONE case where IGNORED is a valid answer (reading the Prepare
not finished bits), and it should not only be documented but analyzed in
more details.
I meant Read SCP_DevID registers from Device0... prepare bits should
never return a CMD_IGNORED
For a write an IGNORED is never OK.
Now that I think of it, the retry on TIMEOUT makes no sense to me.
The retry
was intended for bus-level issues, where maybe a single bit error
causes an
issue without consequences, but the TIMEOUT is a completely
different beast,
it's the master IP that doesn't answer really, a completely
different case.
well in those cases where you have blue wires, it actually helps :)
Blue wires are not supposed to change electrical behavior. TIMEOUT is
only
an internal SOC level issue, so no I don't get how this helps.
You have a retry count that is provided in the BIOS/firmware through
disco
properties and it's meant to bus errors. You are abusing the
definitions. A
command failed is supposed to be detected at the frame rate, which is
typically 20us. a timeout is likely a 100s of ms value, so if you
retry on
top it's going to lock up the bus.
The world is not perfect! A guy debugging setups needs all the help. I do
not see any reason for not to retry. Bus is anyway locked up while a
transfer is ongoing (we serialize transfers).
Now if you feel this should be abhorred, I can change this for timeout.
This TIMEOUT thing is your own definition, it's not part of the spec, so
I don't see how it can be lumped together with spec-related parts.
It's fine to keep a retry but please document what the expectations are
for the TIMEOUT case.
+enum sdw_command_response {
+ SDW_CMD_OK = 0,
+ SDW_CMD_IGNORED = 1,
+ SDW_CMD_FAIL = 2,
+ SDW_CMD_TIMEOUT = 4,
+ SDW_CMD_FAIL_OTHER = 8,
Humm, I can't recall if/why this is a mask? does it need to be?
mask, not following!
Taking a wild guess that you are asking about last error, which is
for SW
errors like malloc fail etc...
no, I was asking why this is declared as if it was used for a
bitmask, why
not 0,1,2,3,4?
Oh okay, I think it was something to do with bits for errors, but don
see it
helping so I can change it either way...
Unless you use bit-wise operators and combined responses there is no
reason to keep
Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer
On 12/5/17 12:31 AM, Vinod Koul wrote:
On Sun, Dec 03, 2017 at 09:01:41PM -0600, Pierre-Louis Bossart wrote:
On 12/3/17 11:04 AM, Vinod Koul wrote:
On Fri, Dec 01, 2017 at 05:27:31PM -0600, Pierre-Louis Bossart wrote:
Sorry looks like I missed replying to this one earlier.
+static inline int find_response_code(enum sdw_command_response resp)
+{
+ switch (resp) {
+ case SDW_CMD_OK:
+ return 0;
+
+ case SDW_CMD_IGNORED:
+ return -ENODATA;
+
+ case SDW_CMD_TIMEOUT:
+ return -ETIMEDOUT;
+
+ default:
+ return -EIO;
the 'default' case will handle both SDW_CMD_FAIL (which is a bus event
usually due to bus clash or parity issues) and SDW_CMD_FAIL_OTHER (which is
an imp-def IP event).
Do they really belong in the same basket? From a debug perspective there is
quite a bit of information lost.
at higher level the error handling is same. the information is not lost as
it is expected that you would log it at error source.
I don't understand this. It's certainly not the same for me if you detect an
electric problem or if the IP is in the weeds. Logging at the source is fine
but this filtering prevents higher levels from doing anything different.
The point is higher levels like here cant do much than bail out and complain.
Can you point out what would be different behaviour in each of these cases?
+static inline int do_transfer(struct sdw_bus *bus, struct sdw_msg *msg)
+{
+ int retry = bus->prop.err_threshold;
+ enum sdw_command_response resp;
+ int ret = 0, i;
+
+ for (i = 0; i <= retry; i++) {
+ resp = bus->ops->xfer_msg(bus, msg);
+ ret = find_response_code(resp);
+
+ /* if cmd is ok or ignored return */
+ if (ret == 0 || ret == -ENODATA)
Can you document why you don't retry on a CMD_IGNORED? I know there was a
reason, I just can't remember it.
CMD_IGNORED can be okay on broadcast. User of this API can retry all they
want!
So you retry if this is a CMD_FAILED but let higher levels retry for
CMD_IGNORED, sorry I don't see the logic.
Yes that is right.
If I am doing a broadcast read, lets say for Device Id registers, why in the
world would I want to retry? CMD_IGNORED is a valid response and required to
stop enumeration cycle in that case.
But if I am not expecting a CMD_IGNORED response, I can very well go ahead
and retry from caller. The context is with caller and they can choose to do
appropriate handling.
And I have clarified this couple of times to you already, not sure how many
more times I would have to do that.
Until you clarify what you are doing.
There is ONE case where IGNORED is a valid answer (reading the Prepare
not finished bits), and it should not only be documented but analyzed in
more details.
For a write an IGNORED is never OK.
Now that I think of it, the retry on TIMEOUT makes no sense to me. The retry
was intended for bus-level issues, where maybe a single bit error causes an
issue without consequences, but the TIMEOUT is a completely different beast,
it's the master IP that doesn't answer really, a completely different case.
well in those cases where you have blue wires, it actually helps :)
Blue wires are not supposed to change electrical behavior. TIMEOUT is only
an internal SOC level issue, so no I don't get how this helps.
You have a retry count that is provided in the BIOS/firmware through disco
properties and it's meant to bus errors. You are abusing the definitions. A
command failed is supposed to be detected at the frame rate, which is
typically 20us. a timeout is likely a 100s of ms value, so if you retry on
top it's going to lock up the bus.
The world is not perfect! A guy debugging setups needs all the help. I do
not see any reason for not to retry. Bus is anyway locked up while a
transfer is ongoing (we serialize transfers).
Now if you feel this should be abhorred, I can change this for timeout.
This TIMEOUT thing is your own definition, it's not part of the spec, so
I don't see how it can be lumped together with spec-related parts.
It's fine to keep a retry but please document what the expectations are
for the TIMEOUT case.
+enum sdw_command_response {
+ SDW_CMD_OK = 0,
+ SDW_CMD_IGNORED = 1,
+ SDW_CMD_FAIL = 2,
+ SDW_CMD_TIMEOUT = 4,
+ SDW_CMD_FAIL_OTHER = 8,
Humm, I can't recall if/why this is a mask? does it need to be?
mask, not following!
Taking a wild guess that you are asking about last error, which is for SW
errors like malloc fail etc...
no, I was asking why this is declared as if it was used for a bitmask, why
not 0,1,2,3,4?
Oh okay, I think it was something to do with bits for errors, but don see it
helping so I can change it either way...
Unless you use bit-wise operators and combined responses there is no
reason to keep the current definitions.
Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer
On Sun, Dec 03, 2017 at 09:01:41PM -0600, Pierre-Louis Bossart wrote:
> On 12/3/17 11:04 AM, Vinod Koul wrote:
> >On Fri, Dec 01, 2017 at 05:27:31PM -0600, Pierre-Louis Bossart wrote:
Sorry looks like I missed replying to this one earlier.
> >>>+static inline int find_response_code(enum sdw_command_response resp)
> >>>+{
> >>>+ switch (resp) {
> >>>+ case SDW_CMD_OK:
> >>>+ return 0;
> >>>+
> >>>+ case SDW_CMD_IGNORED:
> >>>+ return -ENODATA;
> >>>+
> >>>+ case SDW_CMD_TIMEOUT:
> >>>+ return -ETIMEDOUT;
> >>>+
> >>>+ default:
> >>>+ return -EIO;
> >>
> >>the 'default' case will handle both SDW_CMD_FAIL (which is a bus event
> >>usually due to bus clash or parity issues) and SDW_CMD_FAIL_OTHER (which is
> >>an imp-def IP event).
> >>
> >>Do they really belong in the same basket? From a debug perspective there is
> >>quite a bit of information lost.
> >
> >at higher level the error handling is same. the information is not lost as
> >it is expected that you would log it at error source.
>
> I don't understand this. It's certainly not the same for me if you detect an
> electric problem or if the IP is in the weeds. Logging at the source is fine
> but this filtering prevents higher levels from doing anything different.
The point is higher levels like here cant do much than bail out and complain.
Can you point out what would be different behaviour in each of these cases?
> >>>+static inline int do_transfer(struct sdw_bus *bus, struct sdw_msg *msg)
> >>>+{
> >>>+ int retry = bus->prop.err_threshold;
> >>>+ enum sdw_command_response resp;
> >>>+ int ret = 0, i;
> >>>+
> >>>+ for (i = 0; i <= retry; i++) {
> >>>+ resp = bus->ops->xfer_msg(bus, msg);
> >>>+ ret = find_response_code(resp);
> >>>+
> >>>+ /* if cmd is ok or ignored return */
> >>>+ if (ret == 0 || ret == -ENODATA)
> >>
> >>Can you document why you don't retry on a CMD_IGNORED? I know there was a
> >>reason, I just can't remember it.
> >
> >CMD_IGNORED can be okay on broadcast. User of this API can retry all they
> >want!
>
> So you retry if this is a CMD_FAILED but let higher levels retry for
> CMD_IGNORED, sorry I don't see the logic.
Yes that is right.
If I am doing a broadcast read, lets say for Device Id registers, why in the
world would I want to retry? CMD_IGNORED is a valid response and required to
stop enumeration cycle in that case.
But if I am not expecting a CMD_IGNORED response, I can very well go ahead
and retry from caller. The context is with caller and they can choose to do
appropriate handling.
And I have clarified this couple of times to you already, not sure how many
more times I would have to do that.
> >>Now that I think of it, the retry on TIMEOUT makes no sense to me. The retry
> >>was intended for bus-level issues, where maybe a single bit error causes an
> >>issue without consequences, but the TIMEOUT is a completely different beast,
> >>it's the master IP that doesn't answer really, a completely different case.
> >
> >well in those cases where you have blue wires, it actually helps :)
>
> Blue wires are not supposed to change electrical behavior. TIMEOUT is only
> an internal SOC level issue, so no I don't get how this helps.
>
> You have a retry count that is provided in the BIOS/firmware through disco
> properties and it's meant to bus errors. You are abusing the definitions. A
> command failed is supposed to be detected at the frame rate, which is
> typically 20us. a timeout is likely a 100s of ms value, so if you retry on
> top it's going to lock up the bus.
The world is not perfect! A guy debugging setups needs all the help. I do
not see any reason for not to retry. Bus is anyway locked up while a
transfer is ongoing (we serialize transfers).
Now if you feel this should be abhorred, I can change this for timeout.
> >>>+enum sdw_command_response {
> >>>+ SDW_CMD_OK = 0,
> >>>+ SDW_CMD_IGNORED = 1,
> >>>+ SDW_CMD_FAIL = 2,
> >>>+ SDW_CMD_TIMEOUT = 4,
> >>>+ SDW_CMD_FAIL_OTHER = 8,
> >>
> >>Humm, I can't recall if/why this is a mask? does it need to be?
> >
> >mask, not following!
> >
> >Taking a wild guess that you are asking about last error, which is for SW
> >errors like malloc fail etc...
>
> no, I was asking why this is declared as if it was used for a bitmask, why
> not 0,1,2,3,4?
Oh okay, I think it was something to do with bits for errors, but don see it
helping so I can change it either way...
--
~Vinod
Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer
On 12/3/17 11:04 AM, Vinod Koul wrote:
On Fri, Dec 01, 2017 at 05:27:31PM -0600, Pierre-Louis Bossart wrote:
+static inline int find_response_code(enum sdw_command_response resp)
+{
+ switch (resp) {
+ case SDW_CMD_OK:
+ return 0;
+
+ case SDW_CMD_IGNORED:
+ return -ENODATA;
+
+ case SDW_CMD_TIMEOUT:
+ return -ETIMEDOUT;
+
+ default:
+ return -EIO;
the 'default' case will handle both SDW_CMD_FAIL (which is a bus event
usually due to bus clash or parity issues) and SDW_CMD_FAIL_OTHER (which is
an imp-def IP event).
Do they really belong in the same basket? From a debug perspective there is
quite a bit of information lost.
at higher level the error handling is same. the information is not lost as
it is expected that you would log it at error source.
I don't understand this. It's certainly not the same for me if you
detect an electric problem or if the IP is in the weeds. Logging at the
source is fine but this filtering prevents higher levels from doing
anything different.
+static inline int do_transfer(struct sdw_bus *bus, struct sdw_msg *msg)
+{
+ int retry = bus->prop.err_threshold;
+ enum sdw_command_response resp;
+ int ret = 0, i;
+
+ for (i = 0; i <= retry; i++) {
+ resp = bus->ops->xfer_msg(bus, msg);
+ ret = find_response_code(resp);
+
+ /* if cmd is ok or ignored return */
+ if (ret == 0 || ret == -ENODATA)
Can you document why you don't retry on a CMD_IGNORED? I know there was a
reason, I just can't remember it.
CMD_IGNORED can be okay on broadcast. User of this API can retry all they
want!
So you retry if this is a CMD_FAILED but let higher levels retry for
CMD_IGNORED, sorry I don't see the logic.
Now that I think of it, the retry on TIMEOUT makes no sense to me. The retry
was intended for bus-level issues, where maybe a single bit error causes an
issue without consequences, but the TIMEOUT is a completely different beast,
it's the master IP that doesn't answer really, a completely different case.
well in those cases where you have blue wires, it actually helps :)
Blue wires are not supposed to change electrical behavior. TIMEOUT is
only an internal SOC level issue, so no I don't get how this helps.
You have a retry count that is provided in the BIOS/firmware through
disco properties and it's meant to bus errors. You are abusing the
definitions. A command failed is supposed to be detected at the frame
rate, which is typically 20us. a timeout is likely a 100s of ms value,
so if you retry on top it's going to lock up the bus.
+/**
+ * sdw_transfer() - Synchronous transfer message to a SDW Slave device
+ * @bus: SDW bus
+ * @slave: SDW Slave
is this just me or this argument is not used?
That's what happens where API gets reworked umpteen times, thanks for
pointing. Earlier slave was required to get the page address calculation,
now that it is removed, it is no longer required !
+int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
+ u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf)
+{
+ memset(msg, 0, sizeof(*msg));
+ msg->addr = addr;
add comment on implicit truncation to 16-bit address
Sure..
+ msg->len = count;
+ msg->dev_num = dev_num;
+ msg->flags = flags;
+ msg->buf = buf;
+ msg->ssp_sync = false;
+ msg->page = false;
+
+ if (addr < SDW_REG_NO_PAGE) { /* no paging area */
+ return 0;
+ } else if (addr >= SDW_REG_MAX) { /* illegal addr */
+ pr_err("SDW: Invalid address %x passed\n", addr);
+ return -EINVAL;
+ }
+
+ if (addr < SDW_REG_OPTIONAL_PAGE) { /* 32k but no page */
+ if (slave && !slave->prop.paging_support)
+ return 0;
+ /* no need for else as that will fall thru to paging */
+ }
+
+ /* paging madatory */
mandatory
thanks for spotting
+ if (dev_num == SDW_ENUM_DEV_NUM || dev_num == SDW_BROADCAST_DEV_NUM) {
+ pr_err("SDW: Invalid device for paging :%d\n", dev_num);
+ return -EINVAL;
+ }
+
+ if (!slave) {
+ pr_err("SDW: No slave for paging addr\n");
+ return -EINVAL;
I would move this test up, since if you have a NULL slave you should return
an error in all case, otherwise there will be an oops in the code below ...
naah, this fn is called for all IO, like broadcast where we have no slave.
So it is really optional for API, but for paging it is mandatory!
+ } else if (!slave->prop.paging_support) {
this wont oops as slave null would never come here
+ dev_err(&slave->dev,
+ "address %x needs paging but no support", addr);
+ return -EINVAL;
+ }
+
+ msg->addr_page1 = (addr >> SDW_REG_SHIFT(SDW_SCP_ADDRPAGE1_M
Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer
On Fri, Dec 01, 2017 at 05:27:31PM -0600, Pierre-Louis Bossart wrote:
> >+static inline int find_response_code(enum sdw_command_response resp)
> >+{
> >+switch (resp) {
> >+case SDW_CMD_OK:
> >+return 0;
> >+
> >+case SDW_CMD_IGNORED:
> >+return -ENODATA;
> >+
> >+case SDW_CMD_TIMEOUT:
> >+return -ETIMEDOUT;
> >+
> >+default:
> >+return -EIO;
>
> the 'default' case will handle both SDW_CMD_FAIL (which is a bus event
> usually due to bus clash or parity issues) and SDW_CMD_FAIL_OTHER (which is
> an imp-def IP event).
>
> Do they really belong in the same basket? From a debug perspective there is
> quite a bit of information lost.
at higher level the error handling is same. the information is not lost as
it is expected that you would log it at error source.
> >+static inline int do_transfer(struct sdw_bus *bus, struct sdw_msg *msg)
> >+{
> >+int retry = bus->prop.err_threshold;
> >+enum sdw_command_response resp;
> >+int ret = 0, i;
> >+
> >+for (i = 0; i <= retry; i++) {
> >+resp = bus->ops->xfer_msg(bus, msg);
> >+ret = find_response_code(resp);
> >+
> >+/* if cmd is ok or ignored return */
> >+if (ret == 0 || ret == -ENODATA)
>
> Can you document why you don't retry on a CMD_IGNORED? I know there was a
> reason, I just can't remember it.
CMD_IGNORED can be okay on broadcast. User of this API can retry all they
want!
>
> Now that I think of it, the retry on TIMEOUT makes no sense to me. The retry
> was intended for bus-level issues, where maybe a single bit error causes an
> issue without consequences, but the TIMEOUT is a completely different beast,
> it's the master IP that doesn't answer really, a completely different case.
well in those cases where you have blue wires, it actually helps :)
> >+/**
> >+ * sdw_transfer() - Synchronous transfer message to a SDW Slave device
> >+ * @bus: SDW bus
> >+ * @slave: SDW Slave
>
> is this just me or this argument is not used?
That's what happens where API gets reworked umpteen times, thanks for
pointing. Earlier slave was required to get the page address calculation,
now that it is removed, it is no longer required !
> >+int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
> >+u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf)
> >+{
> >+memset(msg, 0, sizeof(*msg));
> >+msg->addr = addr;
>
> add comment on implicit truncation to 16-bit address
Sure..
> >+msg->len = count;
> >+msg->dev_num = dev_num;
> >+msg->flags = flags;
> >+msg->buf = buf;
> >+msg->ssp_sync = false;
> >+msg->page = false;
> >+
> >+if (addr < SDW_REG_NO_PAGE) { /* no paging area */
> >+return 0;
> >+} else if (addr >= SDW_REG_MAX) { /* illegal addr */
> >+pr_err("SDW: Invalid address %x passed\n", addr);
> >+return -EINVAL;
> >+}
> >+
> >+if (addr < SDW_REG_OPTIONAL_PAGE) { /* 32k but no page */
> >+if (slave && !slave->prop.paging_support)
> >+return 0;
> >+/* no need for else as that will fall thru to paging */
> >+}
> >+
> >+/* paging madatory */
>
> mandatory
thanks for spotting
>
> >+if (dev_num == SDW_ENUM_DEV_NUM || dev_num == SDW_BROADCAST_DEV_NUM) {
> >+pr_err("SDW: Invalid device for paging :%d\n", dev_num);
> >+return -EINVAL;
> >+}
> >+
> >+if (!slave) {
> >+pr_err("SDW: No slave for paging addr\n");
> >+return -EINVAL;
>
> I would move this test up, since if you have a NULL slave you should return
> an error in all case, otherwise there will be an oops in the code below ...
naah, this fn is called for all IO, like broadcast where we have no slave.
So it is really optional for API, but for paging it is mandatory!
>
> >+} else if (!slave->prop.paging_support) {
this wont oops as slave null would never come here
> >+dev_err(&slave->dev,
> >+"address %x needs paging but no support", addr);
> >+return -EINVAL;
> >+}
> >+
> >+msg->addr_page1 = (addr >> SDW_REG_SHIFT(SDW_SCP_ADDRPAGE1_MASK));
> >+msg->addr_page2 = (addr >> SDW_REG_SHIFT(SDW_SCP_ADDRPAGE2_MASK));
> >+msg->addr |= BIT(15);
> >+msg->page = true;
>
> looks ok :-)
finally !!! yeah the paging and IO code has given me most headache till now!
> >+int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
> >+{
> >+struct sdw_msg msg;
> >+int ret;
> >+
> >+ret = sdw_fill_msg(&msg, slave, addr, count,
> >+slave->dev_num, SDW_MSG_FLAG_READ, val);
> >+if (ret < 0)
> >+return ret;
> >+
>
> ... if you don't test for the slave argument in the sdw_fill_msg but the
> address is correct then the rest of the code will bomb out.
I dont think so..
> >+struct sdw_msg {
> >+u16 addr;
> >+u16 len;
> >+u16 dev_num;
>
> was th
Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer
On 12/1/17 3:56 AM, Vinod Koul wrote:
SoundWire bus supports read or write register(s) for SoundWire Slave
device. sdw_read() and sdw_write() APIs are provided for single
register read/write. sdw_nread() and sdw_nwrite() for operations on
contiguous registers.
Signed-off-by: Sanyog Kale
Signed-off-by: Vinod Koul
---
drivers/soundwire/bus.c | 275 ++
drivers/soundwire/bus.h | 37 ++
include/linux/soundwire/sdw.h | 57 +
3 files changed, 369 insertions(+)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 507ae85ad58e..2f108f162905 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -3,6 +3,8 @@
#include
#include
+#include
+#include
#include
#include "bus.h"
@@ -22,6 +24,12 @@ int sdw_add_bus_master(struct sdw_bus *bus)
return -ENODEV;
}
+ if (!bus->ops) {
+ dev_err(bus->dev, "SoundWire Bus ops are not set");
+ return -EINVAL;
+ }
+
+ mutex_init(&bus->msg_lock);
mutex_init(&bus->bus_lock);
INIT_LIST_HEAD(&bus->slaves);
@@ -97,6 +105,273 @@ void sdw_delete_bus_master(struct sdw_bus *bus)
}
EXPORT_SYMBOL(sdw_delete_bus_master);
+/*
+ * SDW IO Calls
+ */
+
+static inline int find_response_code(enum sdw_command_response resp)
+{
+ switch (resp) {
+ case SDW_CMD_OK:
+ return 0;
+
+ case SDW_CMD_IGNORED:
+ return -ENODATA;
+
+ case SDW_CMD_TIMEOUT:
+ return -ETIMEDOUT;
+
+ default:
+ return -EIO;
the 'default' case will handle both SDW_CMD_FAIL (which is a bus event
usually due to bus clash or parity issues) and SDW_CMD_FAIL_OTHER (which
is an imp-def IP event).
Do they really belong in the same basket? From a debug perspective there
is quite a bit of information lost.
+ }
+}
+
+static inline int do_transfer(struct sdw_bus *bus, struct sdw_msg *msg)
+{
+ int retry = bus->prop.err_threshold;
+ enum sdw_command_response resp;
+ int ret = 0, i;
+
+ for (i = 0; i <= retry; i++) {
+ resp = bus->ops->xfer_msg(bus, msg);
+ ret = find_response_code(resp);
+
+ /* if cmd is ok or ignored return */
+ if (ret == 0 || ret == -ENODATA)
Can you document why you don't retry on a CMD_IGNORED? I know there was
a reason, I just can't remember it.
Now that I think of it, the retry on TIMEOUT makes no sense to me. The
retry was intended for bus-level issues, where maybe a single bit error
causes an issue without consequences, but the TIMEOUT is a completely
different beast, it's the master IP that doesn't answer really, a
completely different case.
+ return ret;
+ }
+
+ return ret;
+}
+
+static inline int do_transfer_defer(struct sdw_bus *bus,
+ struct sdw_msg *msg, struct sdw_defer *defer)
+{
+ int retry = bus->prop.err_threshold;
+ enum sdw_command_response resp;
+ int ret = 0, i;
+
+ defer->msg = msg;
+ defer->length = msg->len;
+
+ for (i = 0; i <= retry; i++) {
+ resp = bus->ops->xfer_msg_defer(bus, msg, defer);
+ ret = find_response_code(resp);
+ /* if cmd is ok or ignored return */
+ if (ret == 0 || ret == -ENODATA)
+ return ret;
+ }
+
+ return ret;
+}
+
+static int sdw_reset_page(struct sdw_bus *bus, u16 dev_num)
+{
+ int retry = bus->prop.err_threshold;
+ enum sdw_command_response resp;
+ int ret = 0, i;
+
+ for (i = 0; i <= retry; i++) {
+ resp = bus->ops->reset_page_addr(bus, dev_num);
+ ret = find_response_code(resp);
+ /* if cmd is ok or ignored return */
+ if (ret == 0 || ret == -ENODATA)
+ return ret;
+ }
+
+ return ret;
+}
+
+/**
+ * sdw_transfer() - Synchronous transfer message to a SDW Slave device
+ * @bus: SDW bus
+ * @slave: SDW Slave
is this just me or this argument is not used?
+ * @msg: SDW message to be xfered
+ */
+int sdw_transfer(struct sdw_bus *bus,
+ struct sdw_slave *slave, struct sdw_msg *msg)
+{
+ int ret;
+
+ mutex_lock(&bus->msg_lock);
+
+ ret = do_transfer(bus, msg);
+ if (ret != 0 && ret != -ENODATA)
+ dev_err(bus->dev, "trf on Slave %d failed:%d\n",
+ msg->dev_num, ret);
+
+ if (msg->page)
+ sdw_reset_page(bus, msg->dev_num);
+
+ mutex_unlock(&bus->msg_lock);
+
+ return ret;
+}
+
+/**
+ * sdw_transfer_defer() - Asynchronously transfer message to a SDW Slave device
+ * @bus: SDW bus
+ * @slave: SDW Slave
same, this argument is not used in the code below.
+ * @msg: SDW message to be xfered
+ * @defer: Defer block for signal completion
+ *
+ * Caller needs to hold the msg_lo
