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

[Stefan Berger]

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;
}