Re: [PATCH 2/3] ati-vga: Support unaligned access to GPIO DDC registers

[Marc-André Lureau]

On Tue, Oct 10, 2023 at 5:03 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> The GPIO_VGA_DDC and GPIO_DVI_DDC registers are used on Radeon for DDC
> access. Some drivers like the PPC Mac FCode ROM uses unaligned writes
> to these registers so implement this the same way as already done for
> GPIO_MONID which is used the same way for the Rage 128 Pro.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Acked-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  hw/display/ati.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index f0bf1d7493..ce63935ead 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -319,11 +319,13 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, 
> unsigned int size)
>      case DAC_CNTL:
>          val = s->regs.dac_cntl;
>          break;
> -    case GPIO_VGA_DDC:
> -        val = s->regs.gpio_vga_ddc;
> +    case GPIO_VGA_DDC ... GPIO_VGA_DDC + 3:
> +        val = ati_reg_read_offs(s->regs.gpio_vga_ddc,
> +                                addr - GPIO_VGA_DDC, size);
>          break;
> -    case GPIO_DVI_DDC:
> -        val = s->regs.gpio_dvi_ddc;
> +    case GPIO_DVI_DDC ... GPIO_DVI_DDC + 3:
> +        val = ati_reg_read_offs(s->regs.gpio_dvi_ddc,
> +                                addr - GPIO_DVI_DDC, size);
>          break;
>      case GPIO_MONID ... GPIO_MONID + 3:
>          val = ati_reg_read_offs(s->regs.gpio_monid,
> @@ -626,29 +628,34 @@ static void ati_mm_write(void *opaque, hwaddr addr,
>          s->regs.dac_cntl = data & 0xffffe3ff;
>          s->vga.dac_8bit = !!(data & DAC_8BIT_EN);
>          break;
> -    case GPIO_VGA_DDC:
> +    /*
> +     * GPIO regs for DDC access. Because some drivers access these via
> +     * multiple byte writes we have to be careful when we send bits to
> +     * avoid spurious changes in bitbang_i2c state. Only do it when either
> +     * the enable bits are changed or output bits changed while enabled.
> +     */
> +    case GPIO_VGA_DDC ... GPIO_VGA_DDC + 3:
>          if (s->dev_id != PCI_DEVICE_ID_ATI_RAGE128_PF) {
>              /* FIXME: Maybe add a property to select VGA or DVI port? */
>          }
>          break;
> -    case GPIO_DVI_DDC:
> +    case GPIO_DVI_DDC ... GPIO_DVI_DDC + 3:
>          if (s->dev_id != PCI_DEVICE_ID_ATI_RAGE128_PF) {
> -            s->regs.gpio_dvi_ddc = ati_i2c(&s->bbi2c, data, 0);
> +            ati_reg_write_offs(&s->regs.gpio_dvi_ddc,
> +                               addr - GPIO_DVI_DDC, data, size);
> +            if ((addr <= GPIO_DVI_DDC + 2 && addr + size > GPIO_DVI_DDC + 2) 
> ||
> +                (addr == GPIO_DVI_DDC && (s->regs.gpio_dvi_ddc & 0x30000))) {
> +                s->regs.gpio_dvi_ddc = ati_i2c(&s->bbi2c,
> +                                               s->regs.gpio_dvi_ddc, 0);
> +            }
>          }
>          break;
>      case GPIO_MONID ... GPIO_MONID + 3:
>          /* FIXME What does Radeon have here? */
>          if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
> +            /* Rage128p accesses DDC via MONID(1-2) with additional mask bit 
> */
>              ati_reg_write_offs(&s->regs.gpio_monid,
>                                 addr - GPIO_MONID, data, size);
> -            /*
> -             * Rage128p accesses DDC used to get EDID via these bits.
> -             * Because some drivers access this via multiple byte writes
> -             * we have to be careful when we send bits to avoid spurious
> -             * changes in bitbang_i2c state. So only do it when mask is set
> -             * and either the enable bits are changed or output bits changed
> -             * while enabled.
> -             */
>              if ((s->regs.gpio_monid & BIT(25)) &&
>                  ((addr <= GPIO_MONID + 2 && addr + size > GPIO_MONID + 2) ||
>                   (addr == GPIO_MONID && (s->regs.gpio_monid & 0x60000)))) {
> --
> 2.30.9
>
>


-- 
Marc-André Lureau