| Message ID | 1686901766-4928-1-git-send-email-RunaGuo-oc@zhaoxin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.com> | expand |
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. > > Add ZhaoXin Serial ATA support for ZhaoXin CUPs. What is "ZhaoXin" ? And what is "CUPs" ? Please spell this out. > > Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.com> > --- > 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. > 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... > + > +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. > +}; > + > +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. > +static int zx_hardreset(struct ata_link *link, unsigned int *class, > + unsigned long deadline) Please align the arguments together. > +{ > + 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. > + > + 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. > + } > + 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. > + 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. > + > + 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. > + 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. > + } > + 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. > + 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: /* * ... * ... */ > + 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. > + 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@zhaoxin.com"); > +MODULE_DESCRIPTION("SCSI low-level driver for ZX SATA controllers"); This is not a scsi driver... > +MODULE_LICENSE("GPL"); > +MODULE_DEVICE_TABLE(pci, zx_pci_tbl); > +MODULE_VERSION(DRV_VERSION);
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@zhaoxin.com> >> --- >> 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@zhaoxin.com"); >> +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); >
On 6/26/23 20:29, Runa Guo-oc wrote: > 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/. A company marketing message is not very informative, technically speaking. What is this chipset and on what board/machine can it be found ? That is the more relevant information we need in this commit message. > >> And what is "CUPs" ? Please spell this out. >> > > Yes, this is a spelling error. > >>> >>> Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.com> >>> --- >>> 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" Same here. If ZhaoXin is only a company name, we need also a chipset model to be informative regarding which HW this driver serves. >>> + 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 Yes. >>> + >>> + 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); > You did not understand my point. Doing: rc = sata_std_hardreset(link, class, deadline); if (rc && rc != -EAGAIN) { ata_link_err(link, "COMRESET failed (errno=%d) ap=%d link %d\n", rc, link->ap->port_no, link->pmp); return rc; } /* rc == 0 || rc == -EAGAIN case */ ... Makes the code much nicer. [...] >>> +MODULE_AUTHOR("Yanchen:YanchenSun@zhaoxin.com"); >>> +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)." Again, this is not a scsi driver. Even if in Linux all ATA drives seat under the scsi layer, this is an ATA subsytem driver for an ATA drive. Not SCSI.
On 2023/6/26 20:40, Damien Le Moal wrote: > On 6/26/23 20:29, Runa Guo-oc wrote: >> 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/. > > A company marketing message is not very informative, technically speaking. What > is this chipset and on what board/machine can it be found ? That is the more > relevant information we need in this commit message. > The reason why it is called Zhaoxin SATA is actually to express that it applies to all Zhaoxin support of its separate chipset/SOC, for example, ZX-100S/ZX-200 chipsets. >> >>> And what is "CUPs" ? Please spell this out. >>> >> >> Yes, this is a spelling error. >> >>>> >>>> Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.com> >>>> --- >>>> 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" > > Same here. If ZhaoXin is only a company name, we need also a chipset model to be > informative regarding which HW this driver serves. > As mentioned before, the chipset models this driver serves are ZX-100S and ZX-200 currently. >>>> + 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 > > Yes. > >>>> + >>>> + 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); >> > > You did not understand my point. Doing: > > rc = sata_std_hardreset(link, class, deadline); > if (rc && rc != -EAGAIN) { > ata_link_err(link, "COMRESET failed (errno=%d) ap=%d link %d\n", > rc, link->ap->port_no, link->pmp); > return rc; > } > > /* rc == 0 || rc == -EAGAIN case */ > ... > > Makes the code much nicer. > > > [...] > Okay, I'll adjust it like this? ... struct ata_port *ap = link->ap; int pmp = link->pmp; int tmprc; rc = sata_std_hardreset(link, class, deadline); if (rc && rc != -EAGAIN) { ata_link_err(link, "COMRESET failed (errno=%d) ap=%d link %d\n", rc, link->ap->port_no, link->pmp); return rc; } 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); return rc; >>>> +MODULE_AUTHOR("Yanchen:YanchenSun@zhaoxin.com"); >>>> +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)." > > Again, this is not a scsi driver. Even if in Linux all ATA drives seat under the > scsi layer, this is an ATA subsytem driver for an ATA drive. Not SCSI. > Okay, I'll remove this inaccurate description next time. Thank you.
On 6/27/23 16:17, Runa Guo-oc wrote: > On 2023/6/26 20:40, Damien Le Moal wrote: >> On 6/26/23 20:29, Runa Guo-oc wrote: >>> 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/. >> >> A company marketing message is not very informative, technically speaking. What >> is this chipset and on what board/machine can it be found ? That is the more >> relevant information we need in this commit message. >> > > The reason why it is called Zhaoxin SATA is actually to express that it > applies > to all Zhaoxin support of its separate chipset/SOC, for example, > ZX-100S/ZX-200 > chipsets. That is fine. I do not need a reason for the name. what I would like to see is information about which product/board/soc this driver will be needed for. So something like the above is actually fine (may be a little more details if you have). > >>> >>>> And what is "CUPs" ? Please spell this out. >>>> >>> >>> Yes, this is a spelling error. >>> >>>>> >>>>> Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.com> >>>>> --- >>>>> 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" >> >> Same here. If ZhaoXin is only a company name, we need also a chipset model to be >> informative regarding which HW this driver serves. >> > > As mentioned before, the chipset models this driver serves are ZX-100S > and ZX-200 > currently. Fine. Just say so in the Kconfig entry then. If in the future your company produces a different chipset model that needs a different driver, then the entries can be clearly differentiated even if they use the same company name (ZhaoXin). E.g. "ZhaoXin ZX-100S and ZX-200 chipset support" vs "ZhaoXin XYZ-newgen chipset support". Adding entries should not require modifying existing entries.
On 2023/6/27 16:02, Damien Le Moal wrote: > On 6/27/23 16:17, Runa Guo-oc wrote: >> On 2023/6/26 20:40, Damien Le Moal wrote: >>> On 6/26/23 20:29, Runa Guo-oc wrote: >>>> 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/. >>> >>> A company marketing message is not very informative, technically speaking. What >>> is this chipset and on what board/machine can it be found ? That is the more >>> relevant information we need in this commit message. >>> >> >> The reason why it is called Zhaoxin SATA is actually to express that it >> applies >> to all Zhaoxin support of its separate chipset/SOC, for example, >> ZX-100S/ZX-200 >> chipsets. > > That is fine. I do not need a reason for the name. what I would like to see is > information about which product/board/soc this driver will be needed for. So > something like the above is actually fine (may be a little more details if you > have). > For more details, take ZX-200 as an example to illustrate. ZX-200 is an TSMC 40nm chip that integrates PCIE EP/RC and internal SB controllers (Serial ATA, Universal Serial Bus Controller, Network Interface Controller, SPI, and so on). And information about SATA is as follows: • Serial ATA • Compliant with Serial ATA Specification Revision 3.2 • Support AHCI compliant with AHCI Specification 1.3.1 • Internal SATA PHY supports 1.5G, 3G and 6G speed. Up to 4 ports • Support 4 SATA ports for one SATA bus master, primary channel and secondary channel • Support Partial/Slumber/Auto Partial to Slumber/HIPM/DIPM, support DEVSLP • Support Esata/SATA Express • Support M.2 • Support Listen Mode (both AHCI controller and legacy SATA controller ) • Support MPS/DevSleep(AHCI controller only) • Do not support compatible mode • Do not support port multiplier • Do not support SGPIO • Do not support CPD • Support Hot-plug and MSI-X • Support FLR >> >>>> >>>>> And what is "CUPs" ? Please spell this out. >>>>> >>>> >>>> Yes, this is a spelling error. >>>> >>>>>> >>>>>> Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.com> >>>>>> --- >>>>>> 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" >>> >>> Same here. If ZhaoXin is only a company name, we need also a chipset model to be >>> informative regarding which HW this driver serves. >>> >> >> As mentioned before, the chipset models this driver serves are ZX-100S >> and ZX-200 >> currently. > > Fine. Just say so in the Kconfig entry then. > If in the future your company produces a different chipset model that needs a > different driver, then the entries can be clearly differentiated even if they > use the same company name (ZhaoXin). E.g. "ZhaoXin ZX-100S and ZX-200 chipset > support" vs "ZhaoXin XYZ-newgen chipset support". Adding entries should not > require modifying existing entries. > > > I got it. I'll add these information in Kconfig entry briefly next time. Thank you for your advice and detailed explanation.
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 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" + +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 }, + + { } /* terminate list */ +}; + +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, +}; + + +static int zx_hardreset(struct ata_link *link, unsigned int *class, + unsigned long deadline) +{ + 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"); + + 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); + } + 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; + + 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); + + 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); + + + 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; + } + 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; + + 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 */ + 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 ... */ + 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@zhaoxin.com"); +MODULE_DESCRIPTION("SCSI low-level driver for ZX SATA controllers"); +MODULE_LICENSE("GPL"); +MODULE_DEVICE_TABLE(pci, zx_pci_tbl); +MODULE_VERSION(DRV_VERSION);
[PATCH] ata:sata_zhaoxin: Add support for ZhaoXin Serial ATA Add ZhaoXin Serial ATA support for ZhaoXin CUPs. Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.com> --- 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