Re: VT8251 SATA controller improvements for viaide(4)

On Mon, Apr 14, 2025 at 12:18 AM Andrius V <[email protected]> wrote:
>
> Hi,
>
> I am working on improvements to the viaide(4) driver, specifically for VIA
> controllers (despite the name, it also supports other vendors like AMD/NVIDIA).
>
> Recent changes include more complete support for CX700/VX800, VT6415 IDE
> controller support, basic VT8261 support (unfortunately, the motherboard died,
> so further work is unlikely), VT8237S IDE identification, VX900 RAID mode
> support, and general VIA RAID improvements.
>
> My final "big" goal is to improve support for the VT8251 chipset controller in
> IDE/RAID modes (AHCI mode already works fine). Current support is incomplete and
> has several issues:
>
> 1) A bogus wd0 attaches if no drive is present on the first SATA port.
> 2) No drives are detected on SATA ports other than the first.
> 3) PCI ID 0x3349 is defined as a VT8237R integrated controller, but is actually
>    a VT8251 integrated controller.
>
> The VT8251 integrated controller has 2 SATA channels with 2 ports each, and 2
> PATA channels. The SATA controller uses PCI ID 0x3349 in IDE/RAID modes
> (at least on the motherboard I have). It may use either 0x3349 or 0x6287 in
> AHCI mode (I believe this depends on the CD/CE revisions of the southbridge,
> visible in chip markings).
>

Forgot to mention that there's also 0x5287 PCI ID, which is used in
IDE mode in CE revision.
CE revision uses 0x3349 only on RAID mode.

> The PATA controller appears under PCI ID 0x0571, which is reused by many
> [...]
>
> However, simply switching to 'via_chip_map()` is not enough. Some problems
> remain:
>
> 1) Attaching a drive on port 4 (second channel, second port) may cause
>    never-ending timeouts.

Even with the fix below, I still get timeouts sometimes on older
revision chipset, but only with one SSD model out of few drives tested
(and only if nothing attach to port 3).

> 2) Attaching a SATA disk to any second channel port (typically ports 3/4)
>    causes PATA drives to fail to attach at all.
>
> [...]
>
> Now I am considering how best to implement a fix. I see a few options:
>
> 1) Add a new flag in 'via_chip_map()` indicating whether it is a SATA
> controller,
>    and assign the proper setup function:
>      sc->sc_wdcdev.sc_atac.atac_set_modes =
>          sata_channel ? sata_setup_channel : via_setup_channel;
>
> 2) Add a sata_channel flag to the ata_channel struct and skip the
>    problematic writes:
>      if (!chp->sata_channel) {
>          pci_conf_write(...);
>          pci_conf_write(...);
>      }
>    Alternatively, call sata_setup_channel() early inside via_setup_channel():
>      if (chp->sata_channel) {
>          sata_setup_channel();
>          return;
>      }
>
> 3) Use ide_flags instead to achieve the same goal:
> https://nxr.netbsd.org/xref/src/sys/dev/pci/pciidevar.h#180
>
> I'd appreciate any feedback on which direction is preferred or what
> are other alternatives.
>
> I can also prepare patches or give additional details if needed.

I also came up with one more option to set atac_set_modes to
sata_setup_channel together with udma option and use null check to set
via_setup_channel otherwise.

@@ -501,12 +515,18 @@ via_chip_map(struct pciide_softc *sc, const
struct pci_attach_args *pa)
  no_ideconf = 1;
  /* FALLTHROUGH */
  case PCI_PRODUCT_VIATECH_CX700_IDE:
+ sc->sc_wdcdev.sc_atac.atac_udma_cap = 6;
+ break;
+ case PCI_PRODUCT_VIATECH_VT8251_SATA:
+ /* FALLTHROUGH */
+ case PCI_PRODUCT_VIATECH_VT8251_SATA_2:
  /* FALLTHROUGH */
  case PCI_PRODUCT_VIATECH_VT8261_SATA:
  /* FALLTHROUGH */
  case PCI_PRODUCT_VIATECH_VX900_IDE:
  /* FALLTHROUGH */
  case PCI_PRODUCT_VIATECH_VX900_RAID:
+ sc->sc_wdcdev.sc_atac.atac_set_modes = sata_setup_channel;
  sc->sc_wdcdev.sc_atac.atac_udma_cap = 6;
  break;
  default:
@@ -662,7 +682,8 @@ via_chip_map(struct pciide_softc *sc, const struct
pci_attach_args *pa)
  }
  sc->sc_wdcdev.sc_atac.atac_pio_cap = 4;
  sc->sc_wdcdev.sc_atac.atac_dma_cap = 2;
- sc->sc_wdcdev.sc_atac.atac_set_modes = via_setup_channel;
+ if (sc->sc_wdcdev.sc_atac.atac_set_modes == NULL)
+ sc->sc_wdcdev.sc_atac.atac_set_modes = via_setup_channel;
  sc->sc_wdcdev.sc_atac.atac_channels = sc->wdc_chanarray;
  if (single_channel)



Prepared the full patch for review and comments
https://netbsd.org/~andvar/viaide-vt8251.diff

>
> Thank you.
>
> Regards,
> Andrius V