Re: [PATCH v1 2/5] hw/ppc: SPI controller model - registers implementation

[Stefan Berger <stefanb@linux.ibm.com>]

On 3/7/24 13:54, Stefan Berger wrote:
> 
> 
> On 2/7/24 11:08, Chalapathi V wrote:
>> SPI controller device model supports a connection to a single SPI 
>> responder.
>> This provide access to SPI seeproms, TPM, flash device and an ADC 
>> controller.
>>
>> All SPI function control is mapped into the SPI register space to 
>> enable full
>> control by firmware. In this commit SPI configuration component is 
>> modelled
>> which contains all SPI configuration and status registers as well as 
>> the hold
>> registers for data to be sent or having been received.
>>
>> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
>
>> +    case SEQUENCER_OPERATION_REG:
>> +        for (int i = 0; i < SPI_CONTROLLER_REG_SIZE; i++) {
>> +            sc->sequencer_operation_reg[i] =
>> +                 (val & PPC_BITMASK(i * 8 , i * 8 + 7)) >> (63 - (i * 
>> 8 + 7));
> 
> To me it would be more obvious if you used a mask here like this:
> 
> mask = PPC_BIT_MASK(0, 7);
> mask = (0xff << 56);
> 
> for (...) {
>      sc->sequencer_operation_reg[i] = (val & mask) >> (56 - i * 8);
>      mask >>= 8;
> }
>

Actually simpler and even this masking is not necessary:

for (...) {
     sc->sequencer_operation_reg[i] = (val >> (56 - i * 8)) & 0xff;
}