Ani, Michael,
An inline response at the bottom.
Thanks!
eric
On 1/26/22 01:05, Ani Sinha wrote:
On Tue, 25 Jan 2022, Eric DeVolder wrote:
Ani,
Thanks for the feedback! Inline responses below.
eric
On 1/25/22 04:53, Ani Sinha wrote:
+
+ action = ACTION_BEGIN_CLEAR_OPERATION;
+ BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
+
+ action = ACTION_END_OPERATION;
+ BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
+
+ action = ACTION_SET_RECORD_OFFSET;
+ BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0);
+ BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
+
+ action = ACTION_EXECUTE_OPERATION;
+ BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET,
+ ERST_EXECUTE_OPERATION_MAGIC);
except here, on all cases we have
BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
We should treat the above as special case and simplify the rest of the
calls (eliminate repeated common arguments).
OK, I created BUILD_WRITE_ACTION() to replace this occurrence. I've provided
what this section of code looks like with this and the other below change at
the end.
I have seen the comment from Michael and you about using inline functions, I
will respond to that in the other message.
+ BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
+
+ action = ACTION_CHECK_BUSY_STATUS;
+ BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
+ BUILD_READ_REGISTER_VALUE(32, ERST_VALUE_OFFSET, 0x01);
+
+ action = ACTION_GET_COMMAND_STATUS;
+ BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
+ BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET);
+
+ action = ACTION_GET_RECORD_IDENTIFIER;
+ BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
+ BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET);
+
+ action = ACTION_SET_RECORD_IDENTIFIER;
+ BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0);
+ BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
This one seems reverted. Should this be
BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0);
like others?
This is a SET operation, so the data is provided in VALUE register, then
the ACTION is written to perform the command, ie record the value.
Ok I see. makes sense.
+
+ action = ACTION_GET_RECORD_COUNT;
+ BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
+ BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET);
+
+ action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
+ BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
+
+ action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
+ BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
+ BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET);
+
+ action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
+ BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
+ BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET);
+
+ action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
+ BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
+ BUILD_READ_REGISTER(32, ERST_VALUE_OFFSET);
+
+ action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
+ BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
+ BUILD_READ_REGISTER(64, ERST_VALUE_OFFSET);
+
BUILD_READ_REGISTER() is always called with ERST_VALUE_OFFSET as second
argument. WE should eliminate this repeated passing of same argument.
The BUILD_READ_REGISTER is always against the VALUE register, as you point
out,
so I've s/BUILD_READ_REGISTER/BUILD_READ_VALUE/ and embedded the offset in the
macro now. You can see this below.
And here is what the main snippet looks like with the above changes (a diff
is quite messy):
/*
* Macros for use with construction of the action instructions
*/
#define BUILD_READ_VALUE(width_in_bits) \
build_serialization_instruction_entry(table_instruction_data, \
action, INST_READ_REGISTER, 0, width_in_bits, \
bar0 + ERST_VALUE_OFFSET, 0)
#define BUILD_READ_VALUE_VALUE(width_in_bits, value) \
build_serialization_instruction_entry(table_instruction_data, \
action, INST_READ_REGISTER_VALUE, 0, width_in_bits, \
bar0 + ERST_VALUE_OFFSET, value)
#define BUILD_WRITE_REGISTER(width_in_bits, reg, value) \
build_serialization_instruction_entry(table_instruction_data, \
action, INST_WRITE_REGISTER, 0, width_in_bits, \
bar0 + reg, value)
#define BUILD_WRITE_REGISTER_VALUE(width_in_bits, reg, value) \
build_serialization_instruction_entry(table_instruction_data, \
action, INST_WRITE_REGISTER_VALUE, 0, width_in_bits, \
bar0 + reg, value)
#define BUILD_WRITE_ACTION() \
BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action)
/* Serialization Instruction Entries */
action = ACTION_BEGIN_WRITE_OPERATION;
BUILD_WRITE_ACTION();
action = ACTION_BEGIN_READ_OPERATION;
BUILD_WRITE_ACTION();
action = ACTION_BEGIN_CLEAR_OPERATION;
BUILD_WRITE_ACTION();
action = ACTION_END_OPERATION;
BUILD_WRITE_ACTION();
action = ACTION_SET_RECORD_OFFSET;
BUILD_WRITE_REGISTER(32, ERST_VALUE_OFFSET, 0);
BUILD_WRITE_ACTION();
action = ACTION_EXECUTE_OPERATION;
BUILD_WRITE_REGISTER_VALUE(32, ERST_VALUE_OFFSET,
ERST_EXECUTE_OPERATION_MAGIC);
BUILD_WRITE_ACTION();
action = ACTION_CHECK_BUSY_STATUS;
BUILD_WRITE_ACTION();
BUILD_READ_VALUE_VALUE(32, 0x01);
action = ACTION_GET_COMMAND_STATUS;
BUILD_WRITE_ACTION();
BUILD_READ_VALUE(32);
action = ACTION_GET_RECORD_IDENTIFIER;
BUILD_WRITE_ACTION();
BUILD_READ_VALUE(64);
action = ACTION_SET_RECORD_IDENTIFIER;
BUILD_WRITE_REGISTER(64, ERST_VALUE_OFFSET, 0);
BUILD_WRITE_ACTION();
action = ACTION_GET_RECORD_COUNT;
BUILD_WRITE_ACTION();
BUILD_READ_VALUE(32);
action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
BUILD_WRITE_ACTION();
BUILD_WRITE_REGISTER_VALUE(32, ERST_ACTION_OFFSET, action);
action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
BUILD_WRITE_ACTION();
BUILD_READ_VALUE(64);
action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
BUILD_WRITE_ACTION();
BUILD_READ_VALUE(64);
action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
BUILD_WRITE_ACTION();
BUILD_READ_VALUE(32);
action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
BUILD_WRITE_ACTION();
BUILD_READ_VALUE(64);
/* Serialization Header */
Yes this looks a lot cleaner. Now as Michael suggested, we can convert
them to inline functions and pass a struct with the common params. Maybe
we can use a macro also to make things even more cleaner. Like calling
the inline function from the macro with the common struct. I am trying to
avoid repeated copy-paste code.
OK, I've converted the above to utilize a context structure that Michael
outlined, and a function call, rather than macro (as above), to generate
the table content.
Without using macros, I think this code is about as simplified as can be.
As this has significant changes, I'll post as v14. Note that the code
grew by about 35 lines, not too bad.
eric
