On 2023/6/16 16:34, Damien Le Moal wrote: > On 6/16/23 16:49, Runa Guo-oc wrote: >> [PATCH] ata:sata_zhaoxin: Add support for ZhaoXin Serial ATA > > Broken patch: the email subject is your SoB instead of the above line, which > should not be part of the message (it should be the subject). Please reformat > and resend. > Okay. >> >> Add ZhaoXin Serial ATA support for ZhaoXin CUPs. > > What is "ZhaoXin" ? Zhaoxin is a Chinese company that has mastered the core technology of independent general-purpose processors and its system platform chips, and is committed to providing users with efficient, compatible and secure independent general-purpose processors, chipsets and other products. for more information, you can visit here: https://www.zhaoxin.com/. > And what is "CUPs" ? Please spell this out. > Yes, this is a spelling error. >> >> Signed-off-by: Runa Guo-oc <RunaGuo-oc@xxxxxxxxxxx> >> --- >> drivers/ata/Kconfig | 8 + >> drivers/ata/Makefile | 1 + >> drivers/ata/sata_zhaoxin.c | 384 +++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 393 insertions(+) >> create mode 100644 drivers/ata/sata_zhaoxin.c >> >> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig >> index 42b51c9..ae327f3 100644 >> --- a/drivers/ata/Kconfig >> +++ b/drivers/ata/Kconfig >> @@ -553,6 +553,14 @@ config SATA_VITESSE >> >> If unsure, say N. >> >> +config SATA_ZHAOXIN >> + tristate "ZhaoXin SATA support" >> + depends on PCI >> + help >> + This option enables support for ZhaoXin Serial ATA. >> + >> + If unsure, say N. >> + >> comment "PATA SFF controllers with BMDMA" >> >> config PATA_ALI >> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile >> index 20e6645..4b84669 100644 >> --- a/drivers/ata/Makefile >> +++ b/drivers/ata/Makefile >> @@ -45,6 +45,7 @@ obj-$(CONFIG_SATA_SIL) += sata_sil.o >> obj-$(CONFIG_SATA_SIS) += sata_sis.o >> obj-$(CONFIG_SATA_SVW) += sata_svw.o >> obj-$(CONFIG_SATA_ULI) += sata_uli.o >> +obj-$(CONFIG_SATA_ZHAOXIN) += sata_zhaoxin.o > > Please sort this alphabetically. > Like this? obj-$(CONFIG_SATA_VITESSE) += sata_vsc.o obj-$(CONFIG_SATA_ZHAOXIN) += sata_zhaoxin.o >> obj-$(CONFIG_SATA_VIA) += sata_via.o >> obj-$(CONFIG_SATA_VITESSE) += sata_vsc.o >> >> diff --git a/drivers/ata/sata_zhaoxin.c b/drivers/ata/sata_zhaoxin.c >> new file mode 100644 >> index 0000000..ef8c73a >> --- /dev/null >> +++ b/drivers/ata/sata_zhaoxin.c >> @@ -0,0 +1,384 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * sata_zhaoxin.c - ZhaoXin Serial ATA controllers >> + */ >> + >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/pci.h> >> +#include <linux/blkdev.h> >> +#include <linux/delay.h> >> +#include <linux/device.h> >> +#include <scsi/scsi.h> >> +#include <scsi/scsi_cmnd.h> >> +#include <scsi/scsi_host.h> >> +#include <linux/libata.h> >> + >> +#define DRV_NAME "sata_zx" >> +#define DRV_VERSION "2.6.1" > > Version is not needed. The driver comes with the kernel... > Right, I'll remove it next time. >> + >> +enum board_ids_enum { >> + zx100s, >> +}; >> + >> +enum { >> + SATA_CHAN_ENAB = 0x40, /* SATA channel enable */ >> + SATA_INT_GATE = 0x41, /* SATA interrupt gating */ >> + SATA_NATIVE_MODE = 0x42, /* Native mode enable */ >> + PATA_UDMA_TIMING = 0xB3, /* PATA timing for DMA/ cable detect */ >> + PATA_PIO_TIMING = 0xAB, /* PATA timing register */ >> + >> + PORT0 = (1 << 1), >> + PORT1 = (1 << 0), >> + ALL_PORTS = PORT0 | PORT1, >> + >> + NATIVE_MODE_ALL = (1 << 7) | (1 << 6) | (1 << 5) | (1 << 4), >> + >> + SATA_EXT_PHY = (1 << 6), /* 0==use PATA, 1==ext phy */ >> +}; >> + >> +static int zx_init_one(struct pci_dev *pdev, const struct pci_device_id *ent); >> +static int zx_scr_read(struct ata_link *link, unsigned int scr, u32 *val); >> +static int zx_scr_write(struct ata_link *link, unsigned int scr, u32 val); >> +static int zx_hardreset(struct ata_link *link, unsigned int *class, >> + unsigned long deadline); >> + >> +static void zx_tf_load(struct ata_port *ap, const struct ata_taskfile *tf); >> + >> +static const struct pci_device_id zx_pci_tbl[] = { >> + { PCI_VDEVICE(ZHAOXIN, 0x9002), zx100s }, >> + { PCI_VDEVICE(ZHAOXIN, 0x9003), zx100s }, >> + > > Blank line not needed. >>> + { } /* terminate list */ > > Comment not needed. > The purpose of writing like this is convenient to add new device IDs in the future. Considering that this is not likely, remove it also fine. >> +}; >> + >> +static struct pci_driver zx_pci_driver = { >> + .name = DRV_NAME, >> + .id_table = zx_pci_tbl, >> + .probe = zx_init_one, >> +#ifdef CONFIG_PM_SLEEP >> + .suspend = ata_pci_device_suspend, >> + .resume = ata_pci_device_resume, >> +#endif >> + .remove = ata_pci_remove_one, >> +}; >> + >> +static struct scsi_host_template zx_sht = { >> + ATA_BMDMA_SHT(DRV_NAME), >> +}; >> + >> +static struct ata_port_operations zx_base_ops = { >> + .inherits = &ata_bmdma_port_ops, >> + .sff_tf_load = zx_tf_load, >> +}; >> + >> +static struct ata_port_operations zx_ops = { >> + .inherits = &zx_base_ops, >> + .hardreset = zx_hardreset, >> + .scr_read = zx_scr_read, >> + .scr_write = zx_scr_write, >> +}; >> + >> +static struct ata_port_info zx100s_port_info = { >> + .flags = ATA_FLAG_SATA | ATA_FLAG_SLAVE_POSS, >> + .pio_mask = ATA_PIO4, >> + .mwdma_mask = ATA_MWDMA2, >> + .udma_mask = ATA_UDMA6, >> + .port_ops = &zx_ops, >> +}; >> + >> + > > Extra blank line not needed. > I see >> +static int zx_hardreset(struct ata_link *link, unsigned int *class, >> + unsigned long deadline) > > Please align the arguments together. > Okay. >> +{ >> + int rc; >> + >> + rc = sata_std_hardreset(link, class, deadline); >> + if (!rc || rc == -EAGAIN) { >> + struct ata_port *ap = link->ap; >> + int pmp = link->pmp; >> + int tmprc; >> + >> + if (pmp) { >> + ap->ops->sff_dev_select(ap, pmp); >> + tmprc = ata_sff_wait_ready(&ap->link, deadline); >> + } else { >> + tmprc = ata_sff_wait_ready(link, deadline); >> + } >> + if (tmprc) >> + ata_link_err(link, "COMRESET failed for wait (errno=%d)\n", >> + rc); >> + else >> + ata_link_err(link, "wait for bsy success\n"); > > Remove this. If it worked, be silent. > Okay. >> + >> + ata_link_err(link, "COMRESET success (errno=%d) ap=%d link %d\n", >> + rc, link->ap->port_no, link->pmp); >> + } else { >> + ata_link_err(link, "COMRESET failed (errno=%d) ap=%d link %d\n", >> + rc, link->ap->port_no, link->pmp); > > Reverse the if condition and exit early for this case. That will make the > function code nicer. And you can drop the error message as well since > sata_std_hardreset() prints one. > Yes, I agree with your. I'll adjust the above codes like these? if(!rc || rc == -EAGAIN){ struct ata_port *ap = link->ap; int pmp = link->pmp; int tmprc; if(pmp){ ap->ops->sff_dev_select(ap,pmp); tmprc=ata_sff_wait_ready(&ap->link,deadline); }else tmprc=ata_sff_wait_ready(link,deadline); if(tmprc) ata_link_err(link,"COMRESET failed for wait(errno=%d)\n",rc); ata_link_err(link,"COMRESET success (errno=%d) ap=%d link%d\n", rc,link->ap->port_no,link->pmp); >> + } >> + return rc; >> +} >> + >> +static int zx_scr_read(struct ata_link *link, unsigned int scr, u32 *val) >> +{ >> + static const u8 ipm_tbl[] = { 1, 2, 6, 0 }; >> + struct pci_dev *pdev = to_pci_dev(link->ap->host->dev); >> + int slot = 2 * link->ap->port_no + link->pmp; >> + u32 v = 0; >> + u8 raw; >> + >> + switch (scr) { >> + case SCR_STATUS: >> + pci_read_config_byte(pdev, 0xA0 + slot, &raw); >> + >> + /* read the DET field, bit0 and 1 of the config byte */ >> + v |= raw & 0x03; >> + >> + /* read the SPD field, bit4 of the configure byte */ >> + v |= raw & 0x30; >> + >> + /* read the IPM field, bit2 and 3 of the config byte */ >> + v |= ((ipm_tbl[(raw >> 2) & 0x3])<<8); >> + break; >> + >> + case SCR_ERROR: >> + /* devices other than 5287 uses 0xA8 as base */ >> + WARN_ON(pdev->device != 0x9002 && pdev->device != 0x9003); >> + pci_write_config_byte(pdev, 0x42, slot); >> + pci_read_config_dword(pdev, 0xA8, &v); >> + break; >> + >> + case SCR_CONTROL: >> + pci_read_config_byte(pdev, 0xA4 + slot, &raw); >> + >> + /* read the DET field, bit0 and bit1 */ >> + v |= ((raw & 0x02) << 1) | (raw & 0x01); >> + >> + /* read the IPM field, bit2 and bit3 */ >> + v |= ((raw >> 2) & 0x03) << 8; >> + > > remove this blank line. > Okay. >> + break; >> + >> + default: >> + return -EINVAL; >> + } >> + >> + *val = v; >> + return 0; >> +} >> + >> +static int zx_scr_write(struct ata_link *link, unsigned int scr, u32 val) >> +{ >> + struct pci_dev *pdev = to_pci_dev(link->ap->host->dev); >> + int slot = 2 * link->ap->port_no + link->pmp; >> + u32 v = 0; >> + >> + WARN_ON(pdev == NULL); > > Warning about a null pointer and still dereferenceing it below is useless. The > kernel will crash... This should not happen, right ? So remove this. > Okay. >> + >> + switch (scr) { >> + case SCR_ERROR: >> + /* devices 0x9002 uses 0xA8 as base */ >> + WARN_ON(pdev->device != 0x9002 && pdev->device != 0x9003); >> + pci_write_config_byte(pdev, 0x42, slot); >> + pci_write_config_dword(pdev, 0xA8, val); >> + return 0; >> + >> + case SCR_CONTROL: >> + /* set the DET field */ >> + v |= ((val & 0x4) >> 1) | (val & 0x1); >> + >> + /* set the IPM field */ >> + v |= ((val >> 8) & 0x3) << 2; >> + >> + >> + pci_write_config_byte(pdev, 0xA4 + slot, v); >> + >> + > > Way too many blank lines. > I see >> + return 0; >> + >> + default: >> + return -EINVAL; >> + } >> +} >> + >> + >> +/** >> + * zx_tf_load - send taskfile registers to host controller >> + * @ap: Port to which output is sent >> + * @tf: ATA taskfile register set >> + * >> + * Outputs ATA taskfile to standard ATA host controller. >> + * >> + * This is to fix the internal bug of zx chipsets, which will >> + * reset the device register after changing the IEN bit on ctl >> + * register. >> + */ >> +static void zx_tf_load(struct ata_port *ap, const struct ata_taskfile *tf) >> +{ >> + struct ata_taskfile ttf; >> + >> + if (tf->ctl != ap->last_ctl) { >> + ttf = *tf; >> + ttf.flags |= ATA_TFLAG_DEVICE; >> + tf = &ttf; > > This is very strange... Why the need for the extra local copy ? A comment would > be nice. > tf, pointer to const, the content it pointed to is constant and cannot be changed directly. ttf, it is a variable. Firstly, we change its content based on *tf; Then, make tf pointed to it; Lastly, *tf's content will be changed undirectly. >> + } >> + ata_sff_tf_load(ap, tf); >> +} >> + >> +static const unsigned int zx_bar_sizes[] = { >> + 8, 4, 8, 4, 16, 256 >> +}; >> + >> +static const unsigned int zx100s_bar_sizes0[] = { >> + 8, 4, 8, 4, 16, 0 >> +}; >> + >> +static const unsigned int zx100s_bar_sizes1[] = { >> + 8, 4, 0, 0, 16, 0 >> +}; >> + >> +static int zx_prepare_host(struct pci_dev *pdev, struct ata_host **r_host) >> +{ >> + const struct ata_port_info *ppi0[] = { >> + &zx100s_port_info, NULL >> + }; >> + const struct ata_port_info *ppi1[] = { >> + &zx100s_port_info, &ata_dummy_port_info >> + }; >> + struct ata_host *host; >> + int i, rc; >> + >> + if (pdev->device == 0x9002) >> + rc = ata_pci_bmdma_prepare_host(pdev, ppi0, &host); >> + else if (pdev->device == 0x9003) >> + rc = ata_pci_bmdma_prepare_host(pdev, ppi1, &host); >> + else >> + rc = -EINVAL; >> + > > Remove the blank line here. > Okay. >> + if (rc) >> + return rc; >> + >> + *r_host = host; >> + >> + /* 9002 hosts four sata ports as M/S of the two channels */ >> + /* 9003 hosts two sata ports as M/S of the one channel */ > > Multi-line comment format: > > /* > * ... > * ... > */ > I got it. >> + for (i = 0; i < host->n_ports; i++) >> + ata_slave_link_init(host->ports[i]); >> + >> + return 0; >> +} >> + >> +static void zx_configure(struct pci_dev *pdev, int board_id) >> +{ >> + u8 tmp8; >> + >> + pci_read_config_byte(pdev, PCI_INTERRUPT_LINE, &tmp8); >> + dev_info(&pdev->dev, "routed to hard irq line %d\n", >> + (int) (tmp8 & 0xf0) == 0xf0 ? 0 : tmp8 & 0x0f); >> + >> + /* make sure SATA channels are enabled */ >> + pci_read_config_byte(pdev, SATA_CHAN_ENAB, &tmp8); >> + if ((tmp8 & ALL_PORTS) != ALL_PORTS) { >> + dev_dbg(&pdev->dev, "enabling SATA channels (0x%x)\n", >> + (int)tmp8); >> + tmp8 |= ALL_PORTS; >> + pci_write_config_byte(pdev, SATA_CHAN_ENAB, tmp8); >> + } >> + >> + /* make sure interrupts for each channel sent to us */ >> + pci_read_config_byte(pdev, SATA_INT_GATE, &tmp8); >> + if ((tmp8 & ALL_PORTS) != ALL_PORTS) { >> + dev_dbg(&pdev->dev, "enabling SATA channel interrupts (0x%x)\n", >> + (int) tmp8); >> + tmp8 |= ALL_PORTS; >> + pci_write_config_byte(pdev, SATA_INT_GATE, tmp8); >> + } >> + >> + /* make sure native mode is enabled */ >> + pci_read_config_byte(pdev, SATA_NATIVE_MODE, &tmp8); >> + if ((tmp8 & NATIVE_MODE_ALL) != NATIVE_MODE_ALL) { >> + dev_dbg(&pdev->dev, >> + "enabling SATA channel native mode (0x%x)\n", >> + (int) tmp8); >> + tmp8 |= NATIVE_MODE_ALL; >> + pci_write_config_byte(pdev, SATA_NATIVE_MODE, tmp8); >> + } >> +} >> + >> +static int zx_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) >> +{ >> + unsigned int i; >> + int rc; >> + struct ata_host *host = NULL; >> + int board_id = (int) ent->driver_data; >> + const unsigned int *bar_sizes; >> + int legacy_mode = 0; >> + >> + ata_print_version_once(&pdev->dev, DRV_VERSION); >> + >> + if (pdev->device == 0x9002 || pdev->device == 0x9003) { >> + if ((pdev->class >> 8) == PCI_CLASS_STORAGE_IDE) { >> + u8 tmp8, mask; >> + >> + /* TODO: What if one channel is in native mode ... */ > > I do not know... What about it ? If this is not expected to work/not supported, > then return an error. > Yes, you're right. Zhaoxin sata controllers do not support legacy mode. So we return an error here. Based on the latest kernel code, this part may be adjusted like these: u8 tmp8, mask = 0; pci_read_config_byte(pdev, PCI_CLASS_PROG, &tmp8); if (!ata_port_is_dummy(host->ports[0])) mask |= (1 << 0); if (!ata_port_is_dummy(host->ports[1])) mask |= (1 << 2); if ((tmp8 & mask) != mask) legacy_mode = 1; >> + pci_read_config_byte(pdev, PCI_CLASS_PROG, &tmp8); >> + mask = (1 << 2) | (1 << 0); >> + if ((tmp8 & mask) != mask) >> + legacy_mode = 1; >> + } >> + if (legacy_mode) >> + return -EINVAL; >> + } >> + >> + rc = pcim_enable_device(pdev); >> + if (rc) >> + return rc; >> + >> + if (board_id == zx100s && pdev->device == 0x9002) >> + bar_sizes = &zx100s_bar_sizes0[0]; >> + else if (board_id == zx100s && pdev->device == 0x9003) >> + bar_sizes = &zx100s_bar_sizes1[0]; >> + else >> + bar_sizes = &zx_bar_sizes[0]; >> + >> + for (i = 0; i < ARRAY_SIZE(zx_bar_sizes); i++) { >> + if ((pci_resource_start(pdev, i) == 0) || >> + (pci_resource_len(pdev, i) < bar_sizes[i])) { >> + if (bar_sizes[i] == 0) >> + continue; >> + >> + dev_err(&pdev->dev, >> + "invalid PCI BAR %u (sz 0x%llx, val 0x%llx)\n", >> + i, >> + (unsigned long long)pci_resource_start(pdev, i), >> + (unsigned long long)pci_resource_len(pdev, i)); >> + >> + return -ENODEV; >> + } >> + } >> + >> + switch (board_id) { >> + case zx100s: >> + rc = zx_prepare_host(pdev, &host); >> + break; >> + default: >> + rc = -EINVAL; >> + } >> + if (rc) >> + return rc; >> + >> + zx_configure(pdev, board_id); >> + >> + pci_set_master(pdev); >> + return ata_host_activate(host, pdev->irq, ata_bmdma_interrupt, >> + IRQF_SHARED, &zx_sht); >> +} >> + >> +module_pci_driver(zx_pci_driver); >> + >> +MODULE_AUTHOR("Yanchen:YanchenSun@xxxxxxxxxxx"); >> +MODULE_DESCRIPTION("SCSI low-level driver for ZX SATA controllers"); > > This is not a scsi driver... > I treat it as a scsi driver for the following reasons, which may be not accurate. 1, IO path: vfs->fs->block layer->scsi layer->this driver; 2, Extracted from the following link: "SCSI Lower level drivers (LLDs) are variously called host bus adapter (HBA) drivers and host drivers (HD)." https://www.kernel.org/doc/html/latest/scsi/scsi_mid_low_api.html Maybe I shall delete it next time. >> +MODULE_LICENSE("GPL"); >> +MODULE_DEVICE_TABLE(pci, zx_pci_tbl); >> +MODULE_VERSION(DRV_VERSION); >
