Re: [PATCH v13 06/10] ACPI ERST: build the ACPI ERST table

[Eric DeVolder <eric.devolder@oracle.com>]

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