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 the current definitions.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
