RE: [PATCH] libata/ahci: quirk for JMB585/JMB582

[Md Lin]

Hi Damien,
Thank you very much for the detailed suggestion. I will modify the patch, commit message and send to you again.

Best regards,
MD Lin (林明德)

JMicron Technology
Tel : 03-5797389  Ext. 8727
Fax : 03-5799566
E-mail : mdlin@xxxxxxxxxxx

-----Original Message-----
From: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx> 
Sent: Thursday, September 08, 2022 6:13 AM
To: Md Lin <mdlin@xxxxxxxxxxx>; axboe@xxxxxxxxx
Cc: linux-ide@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Kevin Liu <kevinliu@xxxxxxxxxxx>; Charon Chen <charonchen@xxxxxxxxxxx>; Cora Huang <corahuang@xxxxxxxxxxx>; Ming Chen <mhchen@xxxxxxxxxxx>; George Chao <georgechao@xxxxxxxxxxx>; Banks <banks@xxxxxxxxxxx>; Tzuwei Hung <tzuwei@xxxxxxxxxxx>
Subject: Re: [PATCH] libata/ahci: quirk for JMB585/JMB582

On 9/7/22 19:51, MD Lin wrote:
> This patch adds a quirk, which enable error bit handling functions and 
> SATA configuration for JMicron JMB585/JMB582.
> 
> Signed-off-by: MD Lin <mdlin@xxxxxxxxxxx>

Please use scripts/get_maintainer.pl to check to whom ata patches should be addressed. If you do not send patches to me, there is a high chance that I will miss them.

The patch title should be:

ata: ahci: Add initialization quirk for Jmicro 585/582 controllers

Or equivalent, that is, a little more descriptive.

The commit message also does not explain WHY this quirk is necessary (the problem is not described) and there is also no description of HOW your patch address the issue. Please be a little more verbose with the commit message to better describe the patch.

> ---
>  drivers/ata/ahci.c | 65 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c index 
> 505920d45..b0768fae3 100755
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1657,6 +1657,68 @@ static void ahci_intel_pcs_quirk(struct pci_dev *pdev, struct ahci_host_priv *hp
>  	}
>  }
>  
> +static void ahci_jmb585_write_sata_phy(void __iomem *mmio, u32 addr, 
> +u32 data)

The patch title said jmb585 and jmb582. So should this be called
ahci_jmb58x_write_sata_phy() ?

> +{
> +	writel((addr & 0x01FFFUL) + (1UL << 18UL), mmio + 0xC0);
> +	writel(data, mmio + 0xC8);
> +}
> +
> +static void ahci_jmicron_585_quirk(void __iomem *mmio)

Same here. Also, please be consistent with the names: spell out jmicron or use jmb, either is fine with me, but once you choose one, stick with it.
So:

ahci_jmb58x_write_sata_phy()
ahci_jmb58x_quirk()

or

ahci_jmicron_58x_write_sata_phy()
ahci_jmicron_58x_quirk()

> +{
> +	u32 pi = readl(mmio + HOST_PORTS_IMPL);
> +	u32 b8_data;
> +
> +	/*
> +	 * enable error bit handling functions, these might overwrite
> +	 * the setting which loads from external SPI flash
> +	 */
> +	b8_data = (pi > 3) ? 0x13 : 0x92;
> +	writel(0x03060004+b8_data, mmio + 0xB8);

Spaces missing around the "+" in the first argument.

> +	writel(0x00FF0B01,         mmio + 0x30);
> +	writel(0x0000003F,         mmio + 0x34);
> +	writel(0x0000001F,         mmio + 0x38);
> +	writel(0x03060000+b8_data, mmio + 0xB8);

Same here.

> +	writel(0xF9E4EFBF,         mmio + 0xB0);

And what are all these magic values ? Where do they come from ?
It would be nice to have these defined as macros with descriptive names in drivers/ata/ahci.h.

> +
> +	/*
> +	 * set SATA configuration, these might overwrite
> +	 * the setting which loads from external SPI flash
> +	 */
> +	ahci_jmb585_write_sata_phy(mmio, 0x06, 0x70005BE3); /* port0 */
> +	ahci_jmb585_write_sata_phy(mmio, 0x13, 0x70005BE3); /* port1 */
> +	ahci_jmb585_write_sata_phy(mmio, 0x73, 0x000001E5); /* port0 */
> +	ahci_jmb585_write_sata_phy(mmio, 0x75, 0x000001E5); /* port1 */
> +	ahci_jmb585_write_sata_phy(mmio, 0x74, 0x00000024); /* port0 */
> +	ahci_jmb585_write_sata_phy(mmio, 0x80, 0x250B0003); /* port1 */
> +	if (pi > 3) {
> +		ahci_jmb585_write_sata_phy(mmio, 0x20, 0x70005BE3); /* port2 */
> +		ahci_jmb585_write_sata_phy(mmio, 0x2D, 0x70005BE3); /* port3 */
> +		ahci_jmb585_write_sata_phy(mmio, 0x3A, 0x70005BE3); /* port4 */
> +		ahci_jmb585_write_sata_phy(mmio, 0x79, 0x000001E5); /* port3 */
> +		ahci_jmb585_write_sata_phy(mmio, 0x83, 0x250B0003); /* port3 */
> +		ahci_jmb585_write_sata_phy(mmio, 0x7A, 0x00000024); /* port3 */
> +		ahci_jmb585_write_sata_phy(mmio, 0x84, 0x250B0003); /* port3 */
> +	}

Same here, lots of "magic" values that cannot be checked. Please use macros and add comments describing where these come from (adapter specs ?).

> +}
> +
> +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: /* check if the chip is JMB585 */

The comment can be removed. This is obvious.

Also, there is no case for jmb582 model which the patch title mentions. Is it the same number for both models ? If that is the case, then please add a comment above the switch() describing that.

> +		tmp8 = readb(mmio + 0x44);
> +		if (tmp8)
> +			ahci_jmicron_585_quirk(mmio);

The tmp8 variable is not necessary.

> +		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 +1837,9 @@ 
> static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	 */
>  	ahci_intel_pcs_quirk(pdev, hpriv);
>  
> +	/* set JMicron configuration */

This is obvious from the function name. So you can drop this comment.

> +	ahci_jmicron_quirk(pdev, hpriv);
> +
>  	/* prepare host */
>  	if (hpriv->cap & HOST_CAP_NCQ) {
>  		pi.flags |= ATA_FLAG_NCQ;

--
Damien Le Moal
Western Digital Research