[11/14] libqos/ahci: Functional register helpers

Introduce a set of "static inline" register helpers that are intended to
replace the current set of macros with more functional versions that are
better suited to inclusion in libqos than porcelain macros.

As a stopgap measure before eliminating the porcelain macros, define them
to use the new functions defined in the ahci.h header.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/ahci-test.c   | 25 ++++++++++-----------
 tests/libqos/ahci.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 14 deletions(-)

On 13/01/2015 04:34, John Snow wrote:
> Introduce a set of "static inline" register helpers that are intended to
> replace the current set of macros with more functional versions that are
> better suited to inclusion in libqos than porcelain macros.
> 
> As a stopgap measure before eliminating the porcelain macros, define them
> to use the new functions defined in the ahci.h header.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/ahci-test.c   | 25 ++++++++++-----------
>  tests/libqos/ahci.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+), 14 deletions(-)
> 
> diff --git a/tests/ahci-test.c b/tests/ahci-test.c
> index b84f942..ef751a4 100644
> --- a/tests/ahci-test.c
> +++ b/tests/ahci-test.c
> @@ -47,22 +47,19 @@ static char tmp_path[] = "/tmp/qtest.XXXXXX";
>  static bool ahci_pedantic;
>  
>  /*** IO macros for the AHCI memory registers. ***/
> -#define AHCI_READ(OFST) qpci_io_readl(ahci->dev, ahci->hba_base + (OFST))
> -#define AHCI_WRITE(OFST, VAL) qpci_io_writel(ahci->dev,                 \
> -                                             ahci->hba_base + (OFST), (VAL))
> -#define AHCI_RREG(regno)      AHCI_READ(4 * (regno))
> -#define AHCI_WREG(regno, val) AHCI_WRITE(4 * (regno), (val))
> -#define AHCI_SET(regno, mask) AHCI_WREG((regno), AHCI_RREG(regno) | (mask))
> -#define AHCI_CLR(regno, mask) AHCI_WREG((regno), AHCI_RREG(regno) & ~(mask))
> +#define AHCI_READ(OFST)       ahci_mread(ahci, (OFST))
> +#define AHCI_WRITE(OFST, VAL) ahci_mwrite(ahci, (OFST), (VAL))
> +#define AHCI_RREG(regno)      ahci_rreg(ahci, (regno))
> +#define AHCI_WREG(regno, val) ahci_wreg(ahci, (regno), (val))
> +#define AHCI_SET(regno, mask) ahci_set(ahci, (regno), (mask))
> +#define AHCI_CLR(regno, mask) ahci_clr(ahci, (regno), (mask))
>  
>  /*** IO macros for port-specific offsets inside of AHCI memory. ***/
> -#define PX_OFST(port, regno) (HBA_PORT_NUM_REG * (port) + AHCI_PORTS + (regno))
> -#define PX_RREG(port, regno)      AHCI_RREG(PX_OFST((port), (regno)))
> -#define PX_WREG(port, regno, val) AHCI_WREG(PX_OFST((port), (regno)), (val))
> -#define PX_SET(port, reg, mask)   PX_WREG((port), (reg),                \
> -                                          PX_RREG((port), (reg)) | (mask));
> -#define PX_CLR(port, reg, mask)   PX_WREG((port), (reg),                \
> -                                          PX_RREG((port), (reg)) & ~(mask));
> +#define PX_OFST(port, regno)      ahci_px_ofst((port), (regno))
> +#define PX_RREG(port, regno)      ahci_px_rreg(ahci, (port), (regno))
> +#define PX_WREG(port, regno, val) ahci_px_wreg(ahci, (port), (regno), (val))
> +#define PX_SET(port, reg, mask)   ahci_px_set(ahci, (port), (reg), (mask))
> +#define PX_CLR(port, reg, mask)   ahci_px_clr(ahci, (port), (reg), (mask))
>  
>  /*** Function Declarations ***/
>  static QPCIDevice *get_ahci_device(uint32_t *fingerprint);
> diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
> index 8e92385..56b1dfc 100644
> --- a/tests/libqos/ahci.h
> +++ b/tests/libqos/ahci.h
> @@ -354,4 +354,67 @@ typedef struct PRD {
>  /* For calculating how big the PRD table needs to be: */
>  #define CMD_TBL_SIZ(n) ((0x80 + ((n) * sizeof(PRD)) + 0x7F) & ~0x7F)
>  
> +/* Helpers for reading/writing AHCI HBA register values */
> +
> +static inline uint32_t ahci_mread(AHCIQState *ahci, size_t offset)
> +{
> +    return qpci_io_readl(ahci->dev, ahci->hba_base + offset);
> +}
> +
> +static inline void ahci_mwrite(AHCIQState *ahci, size_t offset, uint32_t value)
> +{
> +    qpci_io_writel(ahci->dev, ahci->hba_base + offset, value);
> +}
> +
> +static inline uint32_t ahci_rreg(AHCIQState *ahci, uint32_t reg_num)
> +{
> +    return ahci_mread(ahci, 4 * reg_num);
> +}
> +
> +static inline void ahci_wreg(AHCIQState *ahci, uint32_t reg_num, uint32_t value)
> +{
> +    ahci_mwrite(ahci, 4 * reg_num, value);
> +}
> +
> +static inline void ahci_set(AHCIQState *ahci, uint32_t reg_num, uint32_t mask)
> +{
> +    ahci_wreg(ahci, reg_num, ahci_rreg(ahci, reg_num) | mask);
> +}
> +
> +static inline void ahci_clr(AHCIQState *ahci, uint32_t reg_num, uint32_t mask)
> +{
> +    ahci_wreg(ahci, reg_num, ahci_rreg(ahci, reg_num) & ~mask);
> +}
> +
> +static inline size_t ahci_px_offset(uint8_t port, uint32_t reg_num)
> +{
> +    return AHCI_PORTS + (HBA_PORT_NUM_REG * port) + reg_num;
> +} __attribute__((__pure__))

This attribute applies to ahci_px_rreg, which I think is not what you want.

The compiler should figure out the "purity" anyway for a static inline
function, so my advice is to just drop it.

Paolo

> +static inline uint32_t ahci_px_rreg(AHCIQState *ahci, uint8_t port,
> +                                    uint32_t reg_num)
> +{
> +    return ahci_rreg(ahci, ahci_px_offset(port, reg_num));
> +}
> +
> +static inline void ahci_px_wreg(AHCIQState *ahci, uint8_t port,
> +                                uint32_t reg_num, uint32_t value)
> +{
> +    ahci_wreg(ahci, ahci_px_offset(port, reg_num), value);
> +}
> +
> +static inline void ahci_px_set(AHCIQState *ahci, uint8_t port,
> +                               uint32_t reg_num, uint32_t mask)
> +{
> +    ahci_px_wreg(ahci, port, reg_num,
> +                 ahci_px_rreg(ahci, port, reg_num) | mask);
> +}
> +
> +static inline void ahci_px_clr(AHCIQState *ahci, uint8_t port,
> +                               uint32_t reg_num, uint32_t mask)
> +{
> +    ahci_px_wreg(ahci, port, reg_num,
> +                 ahci_px_rreg(ahci, port, reg_num) & ~mask);
> +}
> +
>  #endif
>

On 01/19/2015 12:09 PM, Paolo Bonzini wrote:
>
>
> On 13/01/2015 04:34, John Snow wrote:
>> Introduce a set of "static inline" register helpers that are intended to
>> replace the current set of macros with more functional versions that are
>> better suited to inclusion in libqos than porcelain macros.
>>
>> As a stopgap measure before eliminating the porcelain macros, define them
>> to use the new functions defined in the ahci.h header.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   tests/ahci-test.c   | 25 ++++++++++-----------
>>   tests/libqos/ahci.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 74 insertions(+), 14 deletions(-)
>>
>> diff --git a/tests/ahci-test.c b/tests/ahci-test.c
>> index b84f942..ef751a4 100644
>> --- a/tests/ahci-test.c
>> +++ b/tests/ahci-test.c
>> @@ -47,22 +47,19 @@ static char tmp_path[] = "/tmp/qtest.XXXXXX";
>>   static bool ahci_pedantic;
>>
>>   /*** IO macros for the AHCI memory registers. ***/
>> -#define AHCI_READ(OFST) qpci_io_readl(ahci->dev, ahci->hba_base + (OFST))
>> -#define AHCI_WRITE(OFST, VAL) qpci_io_writel(ahci->dev,                 \
>> -                                             ahci->hba_base + (OFST), (VAL))
>> -#define AHCI_RREG(regno)      AHCI_READ(4 * (regno))
>> -#define AHCI_WREG(regno, val) AHCI_WRITE(4 * (regno), (val))
>> -#define AHCI_SET(regno, mask) AHCI_WREG((regno), AHCI_RREG(regno) | (mask))
>> -#define AHCI_CLR(regno, mask) AHCI_WREG((regno), AHCI_RREG(regno) & ~(mask))
>> +#define AHCI_READ(OFST)       ahci_mread(ahci, (OFST))
>> +#define AHCI_WRITE(OFST, VAL) ahci_mwrite(ahci, (OFST), (VAL))
>> +#define AHCI_RREG(regno)      ahci_rreg(ahci, (regno))
>> +#define AHCI_WREG(regno, val) ahci_wreg(ahci, (regno), (val))
>> +#define AHCI_SET(regno, mask) ahci_set(ahci, (regno), (mask))
>> +#define AHCI_CLR(regno, mask) ahci_clr(ahci, (regno), (mask))
>>
>>   /*** IO macros for port-specific offsets inside of AHCI memory. ***/
>> -#define PX_OFST(port, regno) (HBA_PORT_NUM_REG * (port) + AHCI_PORTS + (regno))
>> -#define PX_RREG(port, regno)      AHCI_RREG(PX_OFST((port), (regno)))
>> -#define PX_WREG(port, regno, val) AHCI_WREG(PX_OFST((port), (regno)), (val))
>> -#define PX_SET(port, reg, mask)   PX_WREG((port), (reg),                \
>> -                                          PX_RREG((port), (reg)) | (mask));
>> -#define PX_CLR(port, reg, mask)   PX_WREG((port), (reg),                \
>> -                                          PX_RREG((port), (reg)) & ~(mask));
>> +#define PX_OFST(port, regno)      ahci_px_ofst((port), (regno))
>> +#define PX_RREG(port, regno)      ahci_px_rreg(ahci, (port), (regno))
>> +#define PX_WREG(port, regno, val) ahci_px_wreg(ahci, (port), (regno), (val))
>> +#define PX_SET(port, reg, mask)   ahci_px_set(ahci, (port), (reg), (mask))
>> +#define PX_CLR(port, reg, mask)   ahci_px_clr(ahci, (port), (reg), (mask))
>>
>>   /*** Function Declarations ***/
>>   static QPCIDevice *get_ahci_device(uint32_t *fingerprint);
>> diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
>> index 8e92385..56b1dfc 100644
>> --- a/tests/libqos/ahci.h
>> +++ b/tests/libqos/ahci.h
>> @@ -354,4 +354,67 @@ typedef struct PRD {
>>   /* For calculating how big the PRD table needs to be: */
>>   #define CMD_TBL_SIZ(n) ((0x80 + ((n) * sizeof(PRD)) + 0x7F) & ~0x7F)
>>
>> +/* Helpers for reading/writing AHCI HBA register values */
>> +
>> +static inline uint32_t ahci_mread(AHCIQState *ahci, size_t offset)
>> +{
>> +    return qpci_io_readl(ahci->dev, ahci->hba_base + offset);
>> +}
>> +
>> +static inline void ahci_mwrite(AHCIQState *ahci, size_t offset, uint32_t value)
>> +{
>> +    qpci_io_writel(ahci->dev, ahci->hba_base + offset, value);
>> +}
>> +
>> +static inline uint32_t ahci_rreg(AHCIQState *ahci, uint32_t reg_num)
>> +{
>> +    return ahci_mread(ahci, 4 * reg_num);
>> +}
>> +
>> +static inline void ahci_wreg(AHCIQState *ahci, uint32_t reg_num, uint32_t value)
>> +{
>> +    ahci_mwrite(ahci, 4 * reg_num, value);
>> +}
>> +
>> +static inline void ahci_set(AHCIQState *ahci, uint32_t reg_num, uint32_t mask)
>> +{
>> +    ahci_wreg(ahci, reg_num, ahci_rreg(ahci, reg_num) | mask);
>> +}
>> +
>> +static inline void ahci_clr(AHCIQState *ahci, uint32_t reg_num, uint32_t mask)
>> +{
>> +    ahci_wreg(ahci, reg_num, ahci_rreg(ahci, reg_num) & ~mask);
>> +}
>> +
>> +static inline size_t ahci_px_offset(uint8_t port, uint32_t reg_num)
>> +{
>> +    return AHCI_PORTS + (HBA_PORT_NUM_REG * port) + reg_num;
>> +} __attribute__((__pure__))
>
> This attribute applies to ahci_px_rreg, which I think is not what you want.
>
> The compiler should figure out the "purity" anyway for a static inline
> function, so my advice is to just drop it.
>
> Paolo
>

No, whoops. Was thinking of the struct syntax. I am surprised it doesn't 
complain. I'll just remove it.

>> +static inline uint32_t ahci_px_rreg(AHCIQState *ahci, uint8_t port,
>> +                                    uint32_t reg_num)
>> +{
>> +    return ahci_rreg(ahci, ahci_px_offset(port, reg_num));
>> +}
>> +
>> +static inline void ahci_px_wreg(AHCIQState *ahci, uint8_t port,
>> +                                uint32_t reg_num, uint32_t value)
>> +{
>> +    ahci_wreg(ahci, ahci_px_offset(port, reg_num), value);
>> +}
>> +
>> +static inline void ahci_px_set(AHCIQState *ahci, uint8_t port,
>> +                               uint32_t reg_num, uint32_t mask)
>> +{
>> +    ahci_px_wreg(ahci, port, reg_num,
>> +                 ahci_px_rreg(ahci, port, reg_num) | mask);
>> +}
>> +
>> +static inline void ahci_px_clr(AHCIQState *ahci, uint8_t port,
>> +                               uint32_t reg_num, uint32_t mask)
>> +{
>> +    ahci_px_wreg(ahci, port, reg_num,
>> +                 ahci_px_rreg(ahci, port, reg_num) & ~mask);
>> +}
>> +
>>   #endif
>>
>