Re: [PATCH 13/13] hw/net/tulip: Use ld/st_endian_pci_dma() API

[Richard Henderson <richard.henderson@linaro.org>]

On 9/30/24 00:34, Philippe Mathieu-Daudé wrote:
> Refactor to use the recently introduced ld/st_endian_pci_dma()
> API. No logical change intended.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/net/tulip.c | 32 ++++++++++----------------------
>   1 file changed, 10 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/net/tulip.c b/hw/net/tulip.c
> index 9df3e17162..6c67958da7 100644
> --- a/hw/net/tulip.c
> +++ b/hw/net/tulip.c
> @@ -71,36 +71,24 @@ static void tulip_desc_read(TULIPState *s, hwaddr p,
>           struct tulip_descriptor *desc)
>   {
>       const MemTxAttrs attrs = { .memory = true };
> +    bool use_big_endian = s->csr[0] & CSR0_DBO;
>   
> -    if (s->csr[0] & CSR0_DBO) {
> -        ldl_be_pci_dma(&s->dev, p, &desc->status, attrs);
> -        ldl_be_pci_dma(&s->dev, p + 4, &desc->control, attrs);
> -        ldl_be_pci_dma(&s->dev, p + 8, &desc->buf_addr1, attrs);
> -        ldl_be_pci_dma(&s->dev, p + 12, &desc->buf_addr2, attrs);
> -    } else {
> -        ldl_le_pci_dma(&s->dev, p, &desc->status, attrs);
> -        ldl_le_pci_dma(&s->dev, p + 4, &desc->control, attrs);
> -        ldl_le_pci_dma(&s->dev, p + 8, &desc->buf_addr1, attrs);
> -        ldl_le_pci_dma(&s->dev, p + 12, &desc->buf_addr2, attrs);
> -    }
> +    ldl_endian_pci_dma(use_big_endian, &s->dev, p, &desc->status, attrs);
> +    ldl_endian_pci_dma(use_big_endian, &s->dev, p + 4, &desc->control, attrs);
> +    ldl_endian_pci_dma(use_big_endian, &s->dev, p + 8, &desc->buf_addr1, attrs);
> +    ldl_endian_pci_dma(use_big_endian, &s->dev, p + 12, &desc->buf_addr2, attrs);

I don't know if I'm keen on replacing 1 conditional with 4.
I had the same thought vs patch 3, in target/arm/ptw.c.

I suppose it's not exactly performance sensative code, and the compiler might be able to 
do something, given that the conditional is invariant, but it strikes me as untidy.

Anyone else care to weigh in?


r~