| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <58614969-46e3-cb76-6f5e-139c555dcca1@opensource.wdc.com>
Date: Fri, 16 Sep 2022 15:35:00 +0100
From: Damien Le Moal <damien.lemoal@...nsource.wdc.com>
To: MD Lin <mdlin@...cron.com>
Cc: linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org,
kevinliu@...cron.com, charonchen@...cron.com,
corahuang@...cron.com, mhchen@...cron.com, georgechao@...cron.com,
banks@...cron.com, tzuwei@...cron.com
Subject: Re: [PATCH] ata: ahci: Add initialization quirk for JMicron
JMB585/JMB582 controllers
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@...cron.com>
> ---
> 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
Powered by blists - more mailing lists