Re: [patch 5/7] xenon: add SATA support

Re: [patch 5/7] xenon: add SATA support

[Sergei Shtylyov]

Felix Domke wrote:
> This adds support for the HDD and DVD SATA controller on the xenon southbridge.

   Pleas post this to linux-ide@vger.kernel.org in the future.

> It also disables ATA_TFLAG_POLLING in libata-core, which prevented the DVD drive
> from being detected. It needs to be investigated what exactly is wrong here. 

> Signed-off-by: Felix Domke <tmbinc@elitedvb.net>

> ---
>  drivers/ata/Kconfig       |    8 +
>  drivers/ata/Makefile      |    1 
>  drivers/ata/libata-core.c |    2 
>  drivers/ata/sata_xenon.c  |  272 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 282 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.20/drivers/ata/Kconfig
> ===================================================================
> --- linux-2.6.20.orig/drivers/ata/Kconfig	2007-03-07 19:01:12.000000000 +0100
> +++ linux-2.6.20/drivers/ata/Kconfig	2007-03-07 19:01:22.000000000 +0100
> @@ -123,6 +123,14 @@
>  
>  	  If unsure, say N.
>  
> +config SATA_XENON
> +	tristate "Xenon SATA support"
> +	depends on PCI
> +	help
> +	  This option enables support for Xenon southbridge.
> +
> +	  If unsure, say N.
> +
>  config SATA_ULI
>  	tristate "ULi Electronics SATA support"
>  	depends on PCI
> Index: linux-2.6.20/drivers/ata/Makefile
> ===================================================================
> --- linux-2.6.20.orig/drivers/ata/Makefile	2007-03-07 19:01:12.000000000 +0100
> +++ linux-2.6.20/drivers/ata/Makefile	2007-03-07 19:01:22.000000000 +0100
> @@ -11,6 +11,7 @@
>  obj-$(CONFIG_SATA_VIA)		+= sata_via.o
>  obj-$(CONFIG_SATA_VITESSE)	+= sata_vsc.o
>  obj-$(CONFIG_SATA_SIS)		+= sata_sis.o
> +obj-$(CONFIG_SATA_XENON)	+= sata_xenon.o
>  obj-$(CONFIG_SATA_SX4)		+= sata_sx4.o
>  obj-$(CONFIG_SATA_NV)		+= sata_nv.o
>  obj-$(CONFIG_SATA_ULI)		+= sata_uli.o
> Index: linux-2.6.20/drivers/ata/libata-core.c
> ===================================================================
> --- linux-2.6.20.orig/drivers/ata/libata-core.c	2007-03-07 19:01:12.000000000 +0100
> +++ linux-2.6.20/drivers/ata/libata-core.c	2007-03-07 19:01:22.000000000 +0100
> @@ -1478,7 +1478,7 @@
>  	}
>  
>  	tf.protocol = ATA_PROT_PIO;
> -	tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */
> +//	tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */

    I doubt that this could *ever* get accepted.

>  	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
>  				     id, sizeof(id[0]) * ATA_ID_WORDS);
> Index: linux-2.6.20/drivers/ata/sata_xenon.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.20/drivers/ata/sata_xenon.c	2007-03-07 19:01:22.000000000 +0100
> @@ -0,0 +1,272 @@
> +/*
> + *  sata_xenon.c - SATA support for xenon southbridge
> + *
> + *  based on sata_sis.c, modifications by anonymous xbox360 hacker,
> + *
> + *  		    Please ALWAYS copy linux-ide@vger.kernel.org
> + *		    on emails.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2, or (at your option)
> + *  any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; see the file COPYING.  If not, write to
> + *  the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
> + *
> + *
> + *  libata documentation is available via 'make {ps|pdf}docs',
> + *  as Documentation/DocBook/libata.*
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/init.h>
> +#include <linux/blkdev.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <scsi/scsi_host.h>
> +#include <linux/libata.h>
> +
> +#define DRV_NAME	"sata_xenon"
> +#define DRV_VERSION	"0.1"
> +
> +	/* small note: it's completely unknown whether the xenon southbridge sata
> +	   is really based on SiS technology.
> +	   Most of SATA is standardized anyway.
> +
> +
> +	   So, we have these two pci devices, one for each port.
> +
> +	   They have two BARs, one for the IDE registers (0..7,
> +	   altstatus/devctl is +0xA), and one for the BMDMA.
> +
> +	   SCR seem to be sis-like in pci config space, but that should
> +	   be verified!
> +
> +	   Note on the DVD-ROM part:
> +
> +	   The drives usually require some tweaks to be usable under linux.
> +
> +	   You either need to hack the scsi layer, or, in case of the GDR3120L,
> +	   set 'modeB' in the bootloader.
> +	*/
> +
> +enum {
> +	/* PCI configuration registers */
> +	SIS_SCR_BASE		= 0xc0, /* sata0 phy SCR registers */
> +};
> +
> +extern struct ata_probe_ent *ata_probe_ent_alloc(struct device *dev,
> +						 const struct ata_port_info *port);
> +
> +static int xenon_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
> +static u32 xenon_scr_read (struct ata_port *ap, unsigned int sc_reg);
> +static void xenon_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
> +
> +static const struct pci_device_id xenon_pci_tbl[] = {
> +	{ PCI_VDEVICE(MICROSOFT, 0x5803), 0 },
> +	{ PCI_VDEVICE(MICROSOFT, 0x5802), 0 },
> +
> +	{ }	/* terminate list */
> +};
> +
> +static struct pci_driver xenon_pci_driver = {
> +	.name			= DRV_NAME,
> +	.id_table		= xenon_pci_tbl,
> +	.probe			= xenon_init_one,
> +	.remove			= ata_pci_remove_one,
> +};
> +
> +static struct scsi_host_template xenon_sht = {
> +	.module			= THIS_MODULE,
> +	.name			= DRV_NAME,
> +	.ioctl			= ata_scsi_ioctl,
> +	.queuecommand		= ata_scsi_queuecmd,
> +	.can_queue		= ATA_DEF_QUEUE,
> +	.this_id		= ATA_SHT_THIS_ID,
> +	.sg_tablesize		= ATA_MAX_PRD,
> +	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
> +	.emulated		= ATA_SHT_EMULATED,
> +	.use_clustering		= ATA_SHT_USE_CLUSTERING,
> +	.proc_name		= DRV_NAME,
> +	.dma_boundary		= ATA_DMA_BOUNDARY,
> +	.slave_configure	= ata_scsi_slave_config,
> +	.slave_destroy		= ata_scsi_slave_destroy,
> +	.bios_param		= ata_std_bios_param,
> +};
> +
> +static const struct ata_port_operations xenon_ops = {
> +	.port_disable		= ata_port_disable,
> +	.tf_load		= ata_tf_load,
> +	.tf_read		= ata_tf_read,
> +	.check_status		= ata_check_status,
> +	.exec_command		= ata_exec_command,
> +	.dev_select		= ata_std_dev_select,
> +	.bmdma_setup            = ata_bmdma_setup,
> +	.bmdma_start            = ata_bmdma_start,
> +	.bmdma_stop		= ata_bmdma_stop,
> +	.bmdma_status		= ata_bmdma_status,
> +	.qc_prep		= ata_qc_prep,
> +	.qc_issue		= ata_qc_issue_prot,
> +	.data_xfer		= ata_pio_data_xfer,
> +	.freeze			= ata_bmdma_freeze,
> +	.thaw			= ata_bmdma_thaw,
> +	.error_handler		= ata_bmdma_error_handler,
> +	.post_internal_cmd	= ata_bmdma_post_internal_cmd,
> +	.irq_handler		= ata_interrupt,
> +	.irq_clear		= ata_bmdma_irq_clear,
> +	.scr_read		= xenon_scr_read,
> +	.scr_write		= xenon_scr_write,
> +	.port_start		= ata_port_start,
> +	.port_stop		= ata_port_stop,
> +	.host_stop		= ata_host_stop,
> +};
> +
> +static struct ata_port_info xenon_port_info = {
> +	.sht		= &xenon_sht,
> +	.flags		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY,
> +	.pio_mask	= 0x1f,
> +	.mwdma_mask	= 0x7,
> +	.udma_mask	= 0x7f,
> +	.port_ops	= &xenon_ops,
> +};
> +
> +
> +MODULE_DESCRIPTION("low-level driver for Xenon Southbridge SATA controller");
> +MODULE_LICENSE("GPL");
> +MODULE_DEVICE_TABLE(pci, xenon_pci_tbl);
> +MODULE_VERSION(DRV_VERSION);
> +
> +static unsigned int get_scr_cfg_addr(unsigned int port_no, unsigned int sc_reg, int device)
> +{
> +	unsigned int addr = SIS_SCR_BASE + (4 * sc_reg);
> +

    Why we need device and port_no arguments here?

> +	return addr;
> +}
> +
> +static u32 xenon_scr_cfg_read (struct ata_port *ap, unsigned int sc_reg)
> +{
> +	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
> +	unsigned int cfg_addr = get_scr_cfg_addr(ap->port_no, sc_reg, pdev->device);
> +	u32 val;
> +
> +	if (sc_reg == SCR_ERROR) /* doesn't exist in PCI cfg space */
> +		return 0; /* assume no error */

    Since SCR_ERROR == 1, this check seems broken.

> +
> +	pci_read_config_dword(pdev, cfg_addr, &val);
> +
> +	return val;
> +}
> +
> +static void xenon_scr_cfg_write (struct ata_port *ap, unsigned int scr, u32 val)
> +{
> +	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
> +	unsigned int cfg_addr = get_scr_cfg_addr(ap->port_no, scr, pdev->device);
> +
> +	if (scr == SCR_ERROR) /* doesn't exist in PCI cfg space */
> +		return;

    Same here.

> +
> +	pci_write_config_dword(pdev, cfg_addr, val);
> +}
> +
> +static u32 xenon_scr_read (struct ata_port *ap, unsigned int sc_reg)
> +{
> +	if (sc_reg > SCR_CONTROL)
> +		return 0xffffffffU;
> +
> +	return xenon_scr_cfg_read(ap, sc_reg);
> +}
> +
> +static void xenon_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val)
> +{
> +	if (sc_reg > SCR_CONTROL)
> +		return;
> +
> +	xenon_scr_cfg_write(ap, sc_reg, val);
> +}
> +

    Reading SATA regs via config. space... isn't that ugly? :-]

> +static int xenon_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> +	static int printed_version;
> +	struct ata_probe_ent *probe_ent = NULL;
> +	int rc;
> +	int pci_dev_busy = 0;
> +
> +	if (!printed_version++)
> +		dev_printk(KERN_INFO, &pdev->dev, "version " DRV_VERSION "\n");
> +
> +	rc = pci_enable_device(pdev);
> +	if (rc)
> +		return rc;
> +
> +	rc = pci_request_regions(pdev, DRV_NAME);
> +	if (rc) {
> +		pci_dev_busy = 1;
> +		goto err_out;
> +	}
> +
> +	rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
> +	if (rc)
> +		goto err_out_regions;
> +	rc = pci_set_consistent_dma_mask(pdev, ATA_DMA_MASK);
> +	if (rc)
> +		goto err_out_regions;
> +
> +	probe_ent = ata_probe_ent_alloc(pci_dev_to_dev(pdev), &xenon_port_info);
> +	if (!probe_ent) {
> +		rc = -ENOMEM;
> +		goto err_out_regions;
> +	}
> +
> +	probe_ent->irq = pdev->irq;
> +	probe_ent->irq_flags = IRQF_SHARED;
> +
> +	probe_ent->port->cmd_addr = (long)ioremap(pci_resource_start(pdev, 0), PAGE_SIZE);
> +	probe_ent->port->altstatus_addr = probe_ent->port->cmd_addr + 0xa;
> +	probe_ent->port->ctl_addr = probe_ent->port->cmd_addr + 0xa;
> +	probe_ent->port->bmdma_addr = (long)ioremap(pci_resource_start(pdev, 1), PAGE_SIZE);
> +	ata_std_ports(probe_ent->port);
> +	probe_ent->n_ports = 1;
> +
> +	pci_set_master(pdev);
> +	pci_intx(pdev, 1);
> +
> +	/* FIXME: check ata_device_add return value */
> +	ata_device_add(probe_ent);
> +	kfree(probe_ent);
> +
> +	return 0;
> +
> +err_out_regions:
> +	pci_release_regions(pdev);
> +
> +err_out:
> +	if (!pci_dev_busy)
> +		pci_disable_device(pdev);
> +	return rc;
> +
> +}
> +
> +static int __init xenon_init(void)
> +{
> +	return pci_register_driver(&xenon_pci_driver);
> +}
> +
> +static void __exit xenon_exit(void)
> +{
> +	pci_unregister_driver(&xenon_pci_driver);
> +}
> +
> +module_init(xenon_init);
> +module_exit(xenon_exit);
> +

MBR, Sergei

Re: [patch 5/7] xenon: add SATA support

[Felix Domke]

Sergei Shtylyov wrote:
> Felix Domke wrote:
>> This adds support for the HDD and DVD SATA controller on the xenon
>> southbridge.
>   Pleas post this to linux-ide@vger.kernel.org in the future.
I'll do.

>> It also disables ATA_TFLAG_POLLING in libata-core, which prevented the
>> DVD drive
>> from being detected. It needs to be investigated what exactly is wrong
>> here. 

>>      tf.protocol = ATA_PROT_PIO;
>> -    tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */
>> +//    tf.flags |= ATA_TFLAG_POLLING; /* for polling presence
>> detection */
>    I doubt that this could *ever* get accepted.
It was not meant to be accepted, but to work around the problem which
was introduced in 2.6.19. I have far too less knowledge of the SATA
layer to understand why this flag exactly causes the detection of the
DVD-Drive fail.

As said, this needs to be investigated, and fixed (in the driver, of
course, not in the subsystem).

>> +static unsigned int get_scr_cfg_addr(unsigned int port_no, unsigned
>> int sc_reg, int device)
>> +{
>> +    unsigned int addr = SIS_SCR_BASE + (4 * sc_reg);
>> +
>    Why we need device and port_no arguments here?
As there is only one device/port per PCI device, the arguments should
probably be removed from this function, right.

>> +    if (sc_reg == SCR_ERROR) /* doesn't exist in PCI cfg space */
>> +        return 0; /* assume no error */
>    Since SCR_ERROR == 1, this check seems broken.
Why?


Felix

Re: [patch 5/7] xenon: add SATA support

[Sergei Shtylyov]

Felix Domke wrote:
>>Felix Domke wrote:

>>>This adds support for the HDD and DVD SATA controller on the xenon
>>>southbridge.

>>  Pleas post this to linux-ide@vger.kernel.org in the future.

> I'll do.

>>>It also disables ATA_TFLAG_POLLING in libata-core, which prevented the
>>>DVD drive
>>>from being detected. It needs to be investigated what exactly is wrong
>>>here. 

>>>     tf.protocol = ATA_PROT_PIO;
>>>-    tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */
>>>+//    tf.flags |= ATA_TFLAG_POLLING; /* for polling presence
>>>detection */

>>   I doubt that this could *ever* get accepted.

> It was not meant to be accepted, but to work around the problem which
> was introduced in 2.6.19. I have far too less knowledge of the SATA
> layer to understand why this flag exactly causes the detection of the
> DVD-Drive fail.

> As said, this needs to be investigated, and fixed (in the driver, of
> course, not in the subsystem).

    Yeah, I've missed it in the patch description. :-<

>>>+static unsigned int get_scr_cfg_addr(unsigned int port_no, unsigned
>>>int sc_reg, int device)
>>>+{
>>>+    unsigned int addr = SIS_SCR_BASE + (4 * sc_reg);
>>>+

>>   Why we need device and port_no arguments here?

> As there is only one device/port per PCI device, the arguments should
> probably be removed from this function, right.

>>>+    if (sc_reg == SCR_ERROR) /* doesn't exist in PCI cfg space */
>>>+        return 0; /* assume no error */
>>
>>   Since SCR_ERROR == 1, this check seems broken.

> Why?

    Since get_scr_cfg_addr() will never return 1... oh wait, we're checking 
another variable... :-<
    Maybe worth merging to get_scr_cfg_addr() then, along with (sc_reg > 
SCR_CONTROL) checks, to avoid duplicate code.

> Felix

MBR, Sergei

[patch 5/7] [RFC] xenon: add SATA support

[Felix Domke]

This adds support for the HDD and DVD SATA controller on the xenon
southbridge.

It also disables ATA_TFLAG_POLLING in libata-core, which prevented the
DVD drive
from being detected. It needs to be investigated what exactly is wrong
here.

Signed-off-by: Felix Domke <tmbinc@elitedvb.net>

---
 drivers/ata/Kconfig       |    8 +
 drivers/ata/Makefile      |    1
 drivers/ata/libata-core.c |    2
 drivers/ata/sata_xenon.c  |  272
++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 282 insertions(+), 1 deletion(-)

Index: linux-2.6.20/drivers/ata/Kconfig
===================================================================
--- linux-2.6.20.orig/drivers/ata/Kconfig	2007-03-07 19:01:12.000000000
+0100
+++ linux-2.6.20/drivers/ata/Kconfig	2007-03-07 19:01:22.000000000 +0100
@@ -123,6 +123,14 @@

 	  If unsure, say N.

+config SATA_XENON
+	tristate "Xenon SATA support"
+	depends on PCI
+	help
+	  This option enables support for Xenon southbridge.
+
+	  If unsure, say N.
+
 config SATA_ULI
 	tristate "ULi Electronics SATA support"
 	depends on PCI
Index: linux-2.6.20/drivers/ata/Makefile
===================================================================
--- linux-2.6.20.orig/drivers/ata/Makefile	2007-03-07 19:01:12.000000000
+0100
+++ linux-2.6.20/drivers/ata/Makefile	2007-03-07 19:01:22.000000000 +0100
@@ -11,6 +11,7 @@
 obj-$(CONFIG_SATA_VIA)		+= sata_via.o
 obj-$(CONFIG_SATA_VITESSE)	+= sata_vsc.o
 obj-$(CONFIG_SATA_SIS)		+= sata_sis.o
+obj-$(CONFIG_SATA_XENON)	+= sata_xenon.o
 obj-$(CONFIG_SATA_SX4)		+= sata_sx4.o
 obj-$(CONFIG_SATA_NV)		+= sata_nv.o
 obj-$(CONFIG_SATA_ULI)		+= sata_uli.o
Index: linux-2.6.20/drivers/ata/libata-core.c
===================================================================
--- linux-2.6.20.orig/drivers/ata/libata-core.c	2007-03-07
19:01:12.000000000 +0100
+++ linux-2.6.20/drivers/ata/libata-core.c	2007-03-07 19:01:22.000000000
+0100
@@ -1478,7 +1478,7 @@
 	}

 	tf.protocol = ATA_PROT_PIO;
-	tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */
+//	tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */

 	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
 				     id, sizeof(id[0]) * ATA_ID_WORDS);
Index: linux-2.6.20/drivers/ata/sata_xenon.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.20/drivers/ata/sata_xenon.c	2007-03-07 19:01:22.000000000
+0100
@@ -0,0 +1,272 @@
+/*
+ *  sata_xenon.c - SATA support for xenon southbridge
+ *
+ *  based on sata_sis.c, modifications by anonymous xbox360 hacker,
+ *
+ *  		    Please ALWAYS copy linux-ide@vger.kernel.org
+ *		    on emails.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2, or (at your option)
+ *  any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; see the file COPYING.  If not, write to
+ *  the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ *
+ *  libata documentation is available via 'make {ps|pdf}docs',
+ *  as Documentation/DocBook/libata.*
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/init.h>
+#include <linux/blkdev.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <scsi/scsi_host.h>
+#include <linux/libata.h>
+
+#define DRV_NAME	"sata_xenon"
+#define DRV_VERSION	"0.1"
+
+	/* small note: it's completely unknown whether the xenon southbridge sata
+	   is really based on SiS technology.
+	   Most of SATA is standardized anyway.
+
+
+	   So, we have these two pci devices, one for each port.
+
+	   They have two BARs, one for the IDE registers (0..7,
+	   altstatus/devctl is +0xA), and one for the BMDMA.
+
+	   SCR seem to be sis-like in pci config space, but that should
+	   be verified!
+
+	   Note on the DVD-ROM part:
+
+	   The drives usually require some tweaks to be usable under linux.
+
+	   You either need to hack the scsi layer, or, in case of the GDR3120L,
+	   set 'modeB' in the bootloader.
+	*/
+
+enum {
+	/* PCI configuration registers */
+	SIS_SCR_BASE		= 0xc0, /* sata0 phy SCR registers */
+};
+
+extern struct ata_probe_ent *ata_probe_ent_alloc(struct device *dev,
+						 const struct ata_port_info *port);
+
+static int xenon_init_one (struct pci_dev *pdev, const struct
pci_device_id *ent);
+static u32 xenon_scr_read (struct ata_port *ap, unsigned int sc_reg);
+static void xenon_scr_write (struct ata_port *ap, unsigned int sc_reg,
u32 val);
+
+static const struct pci_device_id xenon_pci_tbl[] = {
+	{ PCI_VDEVICE(MICROSOFT, 0x5803), 0 },
+	{ PCI_VDEVICE(MICROSOFT, 0x5802), 0 },
+
+	{ }	/* terminate list */
+};
+
+static struct pci_driver xenon_pci_driver = {
+	.name			= DRV_NAME,
+	.id_table		= xenon_pci_tbl,
+	.probe			= xenon_init_one,
+	.remove			= ata_pci_remove_one,
+};
+
+static struct scsi_host_template xenon_sht = {
+	.module			= THIS_MODULE,
+	.name			= DRV_NAME,
+	.ioctl			= ata_scsi_ioctl,
+	.queuecommand		= ata_scsi_queuecmd,
+	.can_queue		= ATA_DEF_QUEUE,
+	.this_id		= ATA_SHT_THIS_ID,
+	.sg_tablesize		= ATA_MAX_PRD,
+	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
+	.emulated		= ATA_SHT_EMULATED,
+	.use_clustering		= ATA_SHT_USE_CLUSTERING,
+	.proc_name		= DRV_NAME,
+	.dma_boundary		= ATA_DMA_BOUNDARY,
+	.slave_configure	= ata_scsi_slave_config,
+	.slave_destroy		= ata_scsi_slave_destroy,
+	.bios_param		= ata_std_bios_param,
+};
+
+static const struct ata_port_operations xenon_ops = {
+	.port_disable		= ata_port_disable,
+	.tf_load		= ata_tf_load,
+	.tf_read		= ata_tf_read,
+	.check_status		= ata_check_status,
+	.exec_command		= ata_exec_command,
+	.dev_select		= ata_std_dev_select,
+	.bmdma_setup            = ata_bmdma_setup,
+	.bmdma_start            = ata_bmdma_start,
+	.bmdma_stop		= ata_bmdma_stop,
+	.bmdma_status		= ata_bmdma_status,
+	.qc_prep		= ata_qc_prep,
+	.qc_issue		= ata_qc_issue_prot,
+	.data_xfer		= ata_pio_data_xfer,
+	.freeze			= ata_bmdma_freeze,
+	.thaw			= ata_bmdma_thaw,
+	.error_handler		= ata_bmdma_error_handler,
+	.post_internal_cmd	= ata_bmdma_post_internal_cmd,
+	.irq_handler		= ata_interrupt,
+	.irq_clear		= ata_bmdma_irq_clear,
+	.scr_read		= xenon_scr_read,
+	.scr_write		= xenon_scr_write,
+	.port_start		= ata_port_start,
+	.port_stop		= ata_port_stop,
+	.host_stop		= ata_host_stop,
+};
+
+static struct ata_port_info xenon_port_info = {
+	.sht		= &xenon_sht,
+	.flags		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY,
+	.pio_mask	= 0x1f,
+	.mwdma_mask	= 0x7,
+	.udma_mask	= 0x7f,
+	.port_ops	= &xenon_ops,
+};
+
+
+MODULE_DESCRIPTION("low-level driver for Xenon Southbridge SATA
controller");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(pci, xenon_pci_tbl);
+MODULE_VERSION(DRV_VERSION);
+
+static unsigned int get_scr_cfg_addr(unsigned int port_no, unsigned int
sc_reg, int device)
+{
+	unsigned int addr = SIS_SCR_BASE + (4 * sc_reg);
+
+	return addr;
+}
+
+static u32 xenon_scr_cfg_read (struct ata_port *ap, unsigned int sc_reg)
+{
+	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	unsigned int cfg_addr = get_scr_cfg_addr(ap->port_no, sc_reg,
pdev->device);
+	u32 val;
+
+	if (sc_reg == SCR_ERROR) /* doesn't exist in PCI cfg space */
+		return 0; /* assume no error */
+
+	pci_read_config_dword(pdev, cfg_addr, &val);
+
+	return val;
+}
+
+static void xenon_scr_cfg_write (struct ata_port *ap, unsigned int scr,
u32 val)
+{
+	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	unsigned int cfg_addr = get_scr_cfg_addr(ap->port_no, scr, pdev->device);
+
+	if (scr == SCR_ERROR) /* doesn't exist in PCI cfg space */
+		return;
+
+	pci_write_config_dword(pdev, cfg_addr, val);
+}
+
+static u32 xenon_scr_read (struct ata_port *ap, unsigned int sc_reg)
+{
+	if (sc_reg > SCR_CONTROL)
+		return 0xffffffffU;
+
+	return xenon_scr_cfg_read(ap, sc_reg);
+}
+
+static void xenon_scr_write (struct ata_port *ap, unsigned int sc_reg,
u32 val)
+{
+	if (sc_reg > SCR_CONTROL)
+		return;
+
+	xenon_scr_cfg_write(ap, sc_reg, val);
+}
+
+static int xenon_init_one (struct pci_dev *pdev, const struct
pci_device_id *ent)
+{
+	static int printed_version;
+	struct ata_probe_ent *probe_ent = NULL;
+	int rc;
+	int pci_dev_busy = 0;
+
+	if (!printed_version++)
+		dev_printk(KERN_INFO, &pdev->dev, "version " DRV_VERSION "\n");
+
+	rc = pci_enable_device(pdev);
+	if (rc)
+		return rc;
+
+	rc = pci_request_regions(pdev, DRV_NAME);
+	if (rc) {
+		pci_dev_busy = 1;
+		goto err_out;
+	}
+
+	rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
+	if (rc)
+		goto err_out_regions;
+	rc = pci_set_consistent_dma_mask(pdev, ATA_DMA_MASK);
+	if (rc)
+		goto err_out_regions;
+
+	probe_ent = ata_probe_ent_alloc(pci_dev_to_dev(pdev), &xenon_port_info);
+	if (!probe_ent) {
+		rc = -ENOMEM;
+		goto err_out_regions;
+	}
+
+	probe_ent->irq = pdev->irq;
+	probe_ent->irq_flags = IRQF_SHARED;
+
+	probe_ent->port->cmd_addr = (long)ioremap(pci_resource_start(pdev, 0),
PAGE_SIZE);
+	probe_ent->port->altstatus_addr = probe_ent->port->cmd_addr + 0xa;
+	probe_ent->port->ctl_addr = probe_ent->port->cmd_addr + 0xa;
+	probe_ent->port->bmdma_addr = (long)ioremap(pci_resource_start(pdev,
1), PAGE_SIZE);
+	ata_std_ports(probe_ent->port);
+	probe_ent->n_ports = 1;
+
+	pci_set_master(pdev);
+	pci_intx(pdev, 1);
+
+	/* FIXME: check ata_device_add return value */
+	ata_device_add(probe_ent);
+	kfree(probe_ent);
+
+	return 0;
+
+err_out_regions:
+	pci_release_regions(pdev);
+
+err_out:
+	if (!pci_dev_busy)
+		pci_disable_device(pdev);
+	return rc;
+
+}
+
+static int __init xenon_init(void)
+{
+	return pci_register_driver(&xenon_pci_driver);
+}
+
+static void __exit xenon_exit(void)
+{
+	pci_unregister_driver(&xenon_pci_driver);
+}
+
+module_init(xenon_init);
+module_exit(xenon_exit);
+

--

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [patch 5/7] [RFC] xenon: add SATA support

[Tejun Heo]

Hello,

Generally looks good.  Some questions and suggestions follow.

Felix Domke wrote:
> Index: linux-2.6.20/drivers/ata/libata-core.c
> ===================================================================
> --- linux-2.6.20.orig/drivers/ata/libata-core.c	2007-03-07
> 19:01:12.000000000 +0100
> +++ linux-2.6.20/drivers/ata/libata-core.c	2007-03-07 19:01:22.000000000
> +0100
> @@ -1478,7 +1478,7 @@
>  	}
> 
>  	tf.protocol = ATA_PROT_PIO;
> -	tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */
> +//	tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */

Polling IDENTIFY doesn't work for you?  Care to explain how it's broken?

> Index: linux-2.6.20/drivers/ata/sata_xenon.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.20/drivers/ata/sata_xenon.c	2007-03-07 19:01:22.000000000
> +0100
> +	   Note on the DVD-ROM part:
> +
> +	   The drives usually require some tweaks to be usable under linux.
> +
> +	   You either need to hack the scsi layer, or, in case of the GDR3120L,
> +	   set 'modeB' in the bootloader.

Care to explain it further?  Or is it some cryptic embedded stuff?

> +extern struct ata_probe_ent *ata_probe_ent_alloc(struct device *dev,
> +						 const struct ata_port_info *port);

This is an internal function and if you do this, your driver won't build
as a kernel module.  Please do as other drivers do.  probe_ent is in the
process of being removed, so looking ugly is okay for the time being.

> +static u32 xenon_scr_cfg_read (struct ata_port *ap, unsigned int sc_reg)
> +{
> +	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
> +	unsigned int cfg_addr = get_scr_cfg_addr(ap->port_no, sc_reg,
> pdev->device);
> +	u32 val;
> +
> +	if (sc_reg == SCR_ERROR) /* doesn't exist in PCI cfg space */
> +		return 0; /* assume no error */

Interesting, SError isn't there while other regs are?

> +static u32 xenon_scr_read (struct ata_port *ap, unsigned int sc_reg)
> +{
> +	if (sc_reg > SCR_CONTROL)
> +		return 0xffffffffU;
> +
> +	return xenon_scr_cfg_read(ap, sc_reg);
> +}

Just collapse xenon_scr_cfg_read() into xenon_scr_read().

> +static void xenon_scr_write (struct ata_port *ap, unsigned int sc_reg,
> u32 val)
> +{
> +	if (sc_reg > SCR_CONTROL)
> +		return;
> +
> +	xenon_scr_cfg_write(ap, sc_reg, val);
> +}

Ditto for write.

> +static int xenon_init_one (struct pci_dev *pdev, const struct
> pci_device_id *ent)
> +{
> +	static int printed_version;
> +	struct ata_probe_ent *probe_ent = NULL;
> +	int rc;
> +	int pci_dev_busy = 0;
> +
> +	if (!printed_version++)
> +		dev_printk(KERN_INFO, &pdev->dev, "version " DRV_VERSION "\n");
> +
> +	rc = pci_enable_device(pdev);
> +	if (rc)
> +		return rc;

New drivers are generally accepted into libata-dev#upstream branch, and
it looks a bit different in the init path.  If you're a gitter, please
generate patch against libata-dev#upstream, if not the latest -mm should
be good enough.

Thanks.

-- 
tejun

Re: [patch 5/7] [RFC] xenon: add SATA support

[Jeff Garzik]

Felix Domke wrote:
> This adds support for the HDD and DVD SATA controller on the xenon
> southbridge.
> 
> It also disables ATA_TFLAG_POLLING in libata-core, which prevented the
> DVD drive
> from being detected. It needs to be investigated what exactly is wrong
> here.

It sounds like everyone agrees this needs to be investigated, so I won't 
belabor the point :)


> ---
>  drivers/ata/Kconfig       |    8 +
>  drivers/ata/Makefile      |    1
>  drivers/ata/libata-core.c |    2
>  drivers/ata/sata_xenon.c  |  272
> ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 282 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.20/drivers/ata/Kconfig
> ===================================================================
> --- linux-2.6.20.orig/drivers/ata/Kconfig	2007-03-07 19:01:12.000000000
> +0100
> +++ linux-2.6.20/drivers/ata/Kconfig	2007-03-07 19:01:22.000000000 +0100
> @@ -123,6 +123,14 @@
> 
>  	  If unsure, say N.
> 
> +config SATA_XENON
> +	tristate "Xenon SATA support"
> +	depends on PCI
> +	help
> +	  This option enables support for Xenon southbridge.
> +
> +	  If unsure, say N.
> +
>  config SATA_ULI
>  	tristate "ULi Electronics SATA support"
>  	depends on PCI
> Index: linux-2.6.20/drivers/ata/Makefile
> ===================================================================
> --- linux-2.6.20.orig/drivers/ata/Makefile	2007-03-07 19:01:12.000000000
> +0100
> +++ linux-2.6.20/drivers/ata/Makefile	2007-03-07 19:01:22.000000000 +0100
> @@ -11,6 +11,7 @@
>  obj-$(CONFIG_SATA_VIA)		+= sata_via.o
>  obj-$(CONFIG_SATA_VITESSE)	+= sata_vsc.o
>  obj-$(CONFIG_SATA_SIS)		+= sata_sis.o
> +obj-$(CONFIG_SATA_XENON)	+= sata_xenon.o
>  obj-$(CONFIG_SATA_SX4)		+= sata_sx4.o
>  obj-$(CONFIG_SATA_NV)		+= sata_nv.o
>  obj-$(CONFIG_SATA_ULI)		+= sata_uli.o
> Index: linux-2.6.20/drivers/ata/libata-core.c
> ===================================================================
> --- linux-2.6.20.orig/drivers/ata/libata-core.c	2007-03-07
> 19:01:12.000000000 +0100
> +++ linux-2.6.20/drivers/ata/libata-core.c	2007-03-07 19:01:22.000000000
> +0100
> @@ -1478,7 +1478,7 @@
>  	}
> 
>  	tf.protocol = ATA_PROT_PIO;
> -	tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */
> +//	tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */
> 
>  	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
>  				     id, sizeof(id[0]) * ATA_ID_WORDS);
> Index: linux-2.6.20/drivers/ata/sata_xenon.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.20/drivers/ata/sata_xenon.c	2007-03-07 19:01:22.000000000
> +0100
> @@ -0,0 +1,272 @@
> +/*
> + *  sata_xenon.c - SATA support for xenon southbridge
> + *
> + *  based on sata_sis.c, modifications by anonymous xbox360 hacker,
> + *
> + *  		    Please ALWAYS copy linux-ide@vger.kernel.org
> + *		    on emails.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2, or (at your option)
> + *  any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; see the file COPYING.  If not, write to
> + *  the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
> + *
> + *
> + *  libata documentation is available via 'make {ps|pdf}docs',
> + *  as Documentation/DocBook/libata.*
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/init.h>
> +#include <linux/blkdev.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <scsi/scsi_host.h>
> +#include <linux/libata.h>
> +
> +#define DRV_NAME	"sata_xenon"
> +#define DRV_VERSION	"0.1"
> +
> +	/* small note: it's completely unknown whether the xenon southbridge sata
> +	   is really based on SiS technology.
> +	   Most of SATA is standardized anyway.
> +
> +
> +	   So, we have these two pci devices, one for each port.
> +
> +	   They have two BARs, one for the IDE registers (0..7,
> +	   altstatus/devctl is +0xA), and one for the BMDMA.
> +
> +	   SCR seem to be sis-like in pci config space, but that should
> +	   be verified!
> +
> +	   Note on the DVD-ROM part:
> +
> +	   The drives usually require some tweaks to be usable under linux.
> +
> +	   You either need to hack the scsi layer, or, in case of the GDR3120L,
> +	   set 'modeB' in the bootloader.
> +	*/
> +
> +enum {
> +	/* PCI configuration registers */
> +	SIS_SCR_BASE		= 0xc0, /* sata0 phy SCR registers */
> +};
> +
> +extern struct ata_probe_ent *ata_probe_ent_alloc(struct device *dev,
> +						 const struct ata_port_info *port);
> +
> +static int xenon_init_one (struct pci_dev *pdev, const struct
> pci_device_id *ent);
> +static u32 xenon_scr_read (struct ata_port *ap, unsigned int sc_reg);
> +static void xenon_scr_write (struct ata_port *ap, unsigned int sc_reg,
> u32 val);
> +
> +static const struct pci_device_id xenon_pci_tbl[] = {
> +	{ PCI_VDEVICE(MICROSOFT, 0x5803), 0 },
> +	{ PCI_VDEVICE(MICROSOFT, 0x5802), 0 },
> +
> +	{ }	/* terminate list */
> +};
> +
> +static struct pci_driver xenon_pci_driver = {
> +	.name			= DRV_NAME,
> +	.id_table		= xenon_pci_tbl,
> +	.probe			= xenon_init_one,
> +	.remove			= ata_pci_remove_one,
> +};
> +
> +static struct scsi_host_template xenon_sht = {
> +	.module			= THIS_MODULE,
> +	.name			= DRV_NAME,
> +	.ioctl			= ata_scsi_ioctl,
> +	.queuecommand		= ata_scsi_queuecmd,
> +	.can_queue		= ATA_DEF_QUEUE,
> +	.this_id		= ATA_SHT_THIS_ID,
> +	.sg_tablesize		= ATA_MAX_PRD,
> +	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
> +	.emulated		= ATA_SHT_EMULATED,
> +	.use_clustering		= ATA_SHT_USE_CLUSTERING,
> +	.proc_name		= DRV_NAME,
> +	.dma_boundary		= ATA_DMA_BOUNDARY,
> +	.slave_configure	= ata_scsi_slave_config,
> +	.slave_destroy		= ata_scsi_slave_destroy,
> +	.bios_param		= ata_std_bios_param,
> +};
> +
> +static const struct ata_port_operations xenon_ops = {
> +	.port_disable		= ata_port_disable,
> +	.tf_load		= ata_tf_load,
> +	.tf_read		= ata_tf_read,
> +	.check_status		= ata_check_status,
> +	.exec_command		= ata_exec_command,
> +	.dev_select		= ata_std_dev_select,
> +	.bmdma_setup            = ata_bmdma_setup,
> +	.bmdma_start            = ata_bmdma_start,
> +	.bmdma_stop		= ata_bmdma_stop,
> +	.bmdma_status		= ata_bmdma_status,
> +	.qc_prep		= ata_qc_prep,
> +	.qc_issue		= ata_qc_issue_prot,
> +	.data_xfer		= ata_pio_data_xfer,
> +	.freeze			= ata_bmdma_freeze,
> +	.thaw			= ata_bmdma_thaw,
> +	.error_handler		= ata_bmdma_error_handler,
> +	.post_internal_cmd	= ata_bmdma_post_internal_cmd,
> +	.irq_handler		= ata_interrupt,
> +	.irq_clear		= ata_bmdma_irq_clear,
> +	.scr_read		= xenon_scr_read,
> +	.scr_write		= xenon_scr_write,
> +	.port_start		= ata_port_start,
> +	.port_stop		= ata_port_stop,
> +	.host_stop		= ata_host_stop,
> +};
> +
> +static struct ata_port_info xenon_port_info = {
> +	.sht		= &xenon_sht,
> +	.flags		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY,
> +	.pio_mask	= 0x1f,
> +	.mwdma_mask	= 0x7,
> +	.udma_mask	= 0x7f,
> +	.port_ops	= &xenon_ops,
> +};
> +
> +
> +MODULE_DESCRIPTION("low-level driver for Xenon Southbridge SATA
> controller");
> +MODULE_LICENSE("GPL");
> +MODULE_DEVICE_TABLE(pci, xenon_pci_tbl);
> +MODULE_VERSION(DRV_VERSION);
> +
> +static unsigned int get_scr_cfg_addr(unsigned int port_no, unsigned int
> sc_reg, int device)
> +{
> +	unsigned int addr = SIS_SCR_BASE + (4 * sc_reg);
> +
> +	return addr;
> +}
> +
> +static u32 xenon_scr_cfg_read (struct ata_port *ap, unsigned int sc_reg)
> +{
> +	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
> +	unsigned int cfg_addr = get_scr_cfg_addr(ap->port_no, sc_reg,
> pdev->device);
> +	u32 val;
> +
> +	if (sc_reg == SCR_ERROR) /* doesn't exist in PCI cfg space */
> +		return 0; /* assume no error */
> +
> +	pci_read_config_dword(pdev, cfg_addr, &val);
> +
> +	return val;
> +}
> +
> +static void xenon_scr_cfg_write (struct ata_port *ap, unsigned int scr,
> u32 val)
> +{
> +	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
> +	unsigned int cfg_addr = get_scr_cfg_addr(ap->port_no, scr, pdev->device);
> +
> +	if (scr == SCR_ERROR) /* doesn't exist in PCI cfg space */
> +		return;
> +
> +	pci_write_config_dword(pdev, cfg_addr, val);
> +}
> +
> +static u32 xenon_scr_read (struct ata_port *ap, unsigned int sc_reg)
> +{
> +	if (sc_reg > SCR_CONTROL)
> +		return 0xffffffffU;
> +
> +	return xenon_scr_cfg_read(ap, sc_reg);
> +}
> +
> +static void xenon_scr_write (struct ata_port *ap, unsigned int sc_reg,
> u32 val)
> +{
> +	if (sc_reg > SCR_CONTROL)
> +		return;
> +
> +	xenon_scr_cfg_write(ap, sc_reg, val);
> +}

as noted, there is no need for two levels of scr_read/write functions. 
collapse each into a single read and single write function.


> +static int xenon_init_one (struct pci_dev *pdev, const struct
> pci_device_id *ent)
> +{
> +	static int printed_version;
> +	struct ata_probe_ent *probe_ent = NULL;
> +	int rc;
> +	int pci_dev_busy = 0;
> +
> +	if (!printed_version++)
> +		dev_printk(KERN_INFO, &pdev->dev, "version " DRV_VERSION "\n");
> +
> +	rc = pci_enable_device(pdev);
> +	if (rc)
> +		return rc;
> +
> +	rc = pci_request_regions(pdev, DRV_NAME);
> +	if (rc) {
> +		pci_dev_busy = 1;
> +		goto err_out;
> +	}

would be nice for you to update these against iomap, as found in 2.6.21-rc


> +	rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
> +	if (rc)
> +		goto err_out_regions;
> +	rc = pci_set_consistent_dma_mask(pdev, ATA_DMA_MASK);
> +	if (rc)
> +		goto err_out_regions;

shouldn't be needed, this is default


> +	probe_ent = ata_probe_ent_alloc(pci_dev_to_dev(pdev), &xenon_port_info);
> +	if (!probe_ent) {
> +		rc = -ENOMEM;
> +		goto err_out_regions;
> +	}
> +
> +	probe_ent->irq = pdev->irq;
> +	probe_ent->irq_flags = IRQF_SHARED;
> +
> +	probe_ent->port->cmd_addr = (long)ioremap(pci_resource_start(pdev, 0),
> PAGE_SIZE);
> +	probe_ent->port->altstatus_addr = probe_ent->port->cmd_addr + 0xa;
> +	probe_ent->port->ctl_addr = probe_ent->port->cmd_addr + 0xa;
> +	probe_ent->port->bmdma_addr = (long)ioremap(pci_resource_start(pdev,
> 1), PAGE_SIZE);

this needs updating for 2.6.21-rc, to get rid of the nasty casts and 
lack of ioremap() error handling


otherwise, seems pretty small and straightforward