Re: [PATCH] ata: ahci: Add initialization quirk for JMicron JMB585/JMB582 controllers

[Damien Le Moal]

On 2022/09/15 1:11, MD Lin wrote:
> JMicron JMB585/JMB582 does not enable specific error bit handling functions
> by default so this patch enable these functions for better compatibility.
> Besides, these patches also adjust SATA RX/TX_GEN1/TX_GEN2 parameters for
> better compatibility. These patches had been tested in JMicron Test
> Laboratory and been implemented to our customers.
> 
> Signed-off-by: MD Lin <mdlin@xxxxxxxxxxx>
> ---
>  drivers/ata/ahci.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/ata/ahci.h | 23 +++++++++++++++
>  2 files changed, 94 insertions(+)
>  mode change 100755 => 100644 drivers/ata/ahci.h
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 505920d45..3e9e3b8f8 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1657,6 +1657,75 @@ static void ahci_intel_pcs_quirk(struct pci_dev *pdev, struct ahci_host_priv *hp
>  	}
>  }
>  
> +static void ahci_jmb58x_write_sata(void __iomem *mmio, u32 addr, u32 data)
> +{
> +	writel((addr & 0x01FFFUL) + (1UL << 18UL), mmio + 0xC0);
> +	writel(data, mmio + 0xC8);
> +}
> +
> +static void ahci_jmb58x_quirk(void __iomem *mmio)
> +{
> +	u32 pi = readl(mmio + HOST_PORTS_IMPL);
> +	u32 b8_data;
> +
> +	/*
> +	 * JMB582: PI is 0x03
> +	 * JMB585: PI is 0x1f
> +	 */

What is this comment for ?

> +
> +	/*
> +	 * enable error bit handling functions, these might overwrite
> +	 * the setting which loads from external SPI flash.
> +	 * the address and value are defined in adapter specs.
> +	 */
> +	b8_data = (pi > 3) ? 0x13 : 0x92;

This looks strange. If pi is fixed depending on the controller type, why not use
a switch-case here with the values in the comments above defined as macros ?
Something like:

	switch (pi) {
	case JMB582:
		b8_data = 0x92;
	case JMB585:
	default:
		b8_data = 0x13;
	}

This is a lot more readable.

> +	writel(JMB58X_EH_MODIFY_ON + b8_data,  mmio + 0xB8);
> +	writel(JMB58X_EH_GENERAL,              mmio + 0x30);
> +	writel(JMB58X_EH_CFIS_RETRY,           mmio + 0x34);
> +	writel(JMB58X_EH_DROP_D2H,             mmio + 0x38);
> +	writel(JMB58X_EH_MODIFY_OFF + b8_data, mmio + 0xB8);
> +	writel(JMB58X_EH_TX_LOCK,              mmio + 0xB0);

Why not define all these magic values as macros using the register names ?
Anyone comparing the code to the controller specs will more easily understand.

> +
> +	/*
> +	 * set SATA configuration, these might overwrite
> +	 * the setting which loads from external SPI flash.
> +	 * the address and value are defined in adapter specs.
> +	 */
> +	ahci_jmb58x_write_sata(mmio, 0x06, JMB58X_SATA0_RX);
> +	ahci_jmb58x_write_sata(mmio, 0x13, JMB58X_SATA1_RX);
> +	ahci_jmb58x_write_sata(mmio, 0x73, JMB58X_SATA0_TX_GEN2);
> +	ahci_jmb58x_write_sata(mmio, 0x75, JMB58X_SATA1_TX_GEN2);
> +	ahci_jmb58x_write_sata(mmio, 0x74, JMB58X_SATA0_TX_GEN1);
> +	ahci_jmb58x_write_sata(mmio, 0x80, JMB58X_SATA1_TX_GEN1);
> +	if (pi > 3) {
> +		ahci_jmb58x_write_sata(mmio, 0x20, JMB58X_SATA2_RX);
> +		ahci_jmb58x_write_sata(mmio, 0x2D, JMB58X_SATA3_RX);
> +		ahci_jmb58x_write_sata(mmio, 0x3A, JMB58X_SATA4_RX);
> +		ahci_jmb58x_write_sata(mmio, 0x79, JMB58X_SATA3_TX_GEN2);
> +		ahci_jmb58x_write_sata(mmio, 0x83, JMB58X_SATA3_TX_GEN2_EXT);
> +		ahci_jmb58x_write_sata(mmio, 0x7A, JMB58X_SATA3_TX_GEN1);
> +		ahci_jmb58x_write_sata(mmio, 0x84, JMB58X_SATA3_TX_GEN1_EXT);
> +	}

Same comment here.

> +}
> +
> +static void ahci_jmicron_quirk(struct pci_dev *pdev,
> +			struct ahci_host_priv *hpriv)
> +{
> +	void __iomem *mmio = hpriv->mmio;
> +	u8 tmp8;
> +
> +	if (pdev->vendor != PCI_VENDOR_ID_JMICRON)
> +		return;
> +
> +	switch (pdev->device) {
> +	case 0x585:
> +		tmp8 = readb(mmio + 0x44);
> +		if (tmp8)  /* check controller version */
> +			ahci_jmb58x_quirk(mmio);

The tmp8 variable is not needed:

		if (readb(mmio + 0x44))
			ahci_jmb58x_quirk(mmio);

is fine, with the magic value 0x44 defined as a macro with a descriptive name.

> +		break;
> +	}
> +}
> +
>  static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	unsigned int board_id = ent->driver_data;
> @@ -1775,6 +1844,8 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	 */
>  	ahci_intel_pcs_quirk(pdev, hpriv);
>  
> +	ahci_jmicron_quirk(pdev, hpriv);
> +
>  	/* prepare host */
>  	if (hpriv->cap & HOST_CAP_NCQ) {
>  		pi.flags |= ATA_FLAG_NCQ;
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> old mode 100755
> new mode 100644
> index 9290e787a..82ecc6f2c
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -52,6 +52,29 @@
>  #define EM_MSG_LED_VALUE_OFF          0xfff80000
>  #define EM_MSG_LED_VALUE_ON           0x00010000
>  
> +/* JMicron JMB585/JMB582 Error Bit Handling Register Value */
> +#define JMB58X_EH_MODIFY_ON           0x03060004
> +#define JMB58X_EH_MODIFY_OFF          0x03060000
> +#define JMB58X_EH_GENERAL             0x00FF0B01
> +#define JMB58X_EH_CFIS_RETRY          0x0000003F
> +#define JMB58X_EH_DROP_D2H            0x0000001F
> +#define JMB58X_EH_TX_LOCK             0xF9E4EFBF
> +
> +/* JMicron JMB585/JMB582 SATA PHY Register Value */
> +#define JMB58X_SATA0_RX               0x70005BE3
> +#define JMB58X_SATA1_RX               0x70005BE3
> +#define JMB58X_SATA2_RX               0x70005BE3
> +#define JMB58X_SATA3_RX               0x70005BE3
> +#define JMB58X_SATA4_RX               0x70005BE3
> +#define JMB58X_SATA0_TX_GEN1          0x00000024
> +#define JMB58X_SATA1_TX_GEN1          0x250B0003
> +#define JMB58X_SATA3_TX_GEN1          0x00000024
> +#define JMB58X_SATA3_TX_GEN1_EXT      0x250B0003
> +#define JMB58X_SATA0_TX_GEN2          0x000001E5
> +#define JMB58X_SATA1_TX_GEN2          0x000001E5
> +#define JMB58X_SATA3_TX_GEN2          0x000001E5
> +#define JMB58X_SATA3_TX_GEN2_EXT      0x250B0003
> +
>  enum {
>  	AHCI_MAX_PORTS		= 32,
>  	AHCI_MAX_CLKS		= 5,

-- 
Damien Le Moal
Western Digital Research