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

[Ani Sinha]

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.