[PATCH] ata: Add Intel SCH PATA support

[PATCH] ata: Add Intel SCH PATA support

[Alek Du]

Seems I need to post the patch here instead of LKML

[PATCH] ata: Add Intel SCH PATA support

This patch adds Intel SCH chipsets (US15W, US15L, UL11L) PATA controller
support.

Signed-off-by: Alek Du <alek.du@intel.com>
---
 include/linux/pci_ids.h |    2 ++
 drivers/ata/ata_piix.c  |   40 ++++++++++++++++++++++++++++++++++++++--
 drivers/ide/pci/piix.c  |    2 ++
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 70eb3c8..b72b3b4 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2413,6 +2413,8 @@
 #define PCI_DEVICE_ID_INTEL_82443GX_0  0x71a0
 #define PCI_DEVICE_ID_INTEL_82443GX_2  0x71a2
 #define PCI_DEVICE_ID_INTEL_82372FB_1  0x7601
+#define PCI_DEVICE_ID_INTEL_SCH_LPC    0x8119
+#define PCI_DEVICE_ID_INTEL_SCH_IDE    0x811a
 #define PCI_DEVICE_ID_INTEL_82454GX    0x84c4
 #define PCI_DEVICE_ID_INTEL_82450GX    0x84c5
 #define PCI_DEVICE_ID_INTEL_82451NX    0x84ca
diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index b7c38ee..7f95a9a 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -214,6 +214,8 @@ static const struct pci_device_id piix_pci_tbl[] = {
        /* ICH7/7-R (i945, i975) UDMA 100*/
        { 0x8086, 0x27DF, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
        { 0x8086, 0x269E, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
+       /* INTEL SCH UDMA 100 */
+       { 0x8086, 0x811A, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
        /* ICH8 Mobile PATA Controller */
        { 0x8086, 0x2850, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },

@@ -561,6 +563,10 @@ struct ich_laptop {
        u16 subdevice;
 };

+struct sch_80 {
+       u16 device;
+};
+
 /*
  *     List of laptops that use short cables rather than 80 wire
  */
@@ -577,6 +583,17 @@ static const struct ich_laptop ich_laptop[] = {
        { 0, }
 };

+/*
+ *     List of chipsets whose port IOCFG and enable bit registers are reserved
+ */
+
+static const struct sch_80 sch_80[] = {
+       /* devid */
+       { 0x811A },     /* Intel SCH chipset */
+       /* end marker */
+       { 0, }
+};
+
 /**
  *     ich_pata_cable_detect - Probe host controller cable detect info
  *     @ap: Port for which cable detect info is desired
@@ -592,6 +609,7 @@ static int ich_pata_cable_detect(struct ata_port *ap)
 {
        struct pci_dev *pdev = to_pci_dev(ap->host->dev);
        const struct ich_laptop *lap = &ich_laptop[0];
+       const struct sch_80 *sch = &sch_80[0];
        u8 tmp, mask;

        /* Check for specials - Acer Aspire 5602WLMi */
@@ -604,6 +622,13 @@ static int ich_pata_cable_detect(struct ata_port *ap)
                lap++;
        }

+       /* Check for specials - Intel SCH chipset */
+       while (sch->device) {
+               if (sch->device == pdev->device)
+                       return ATA_CBL_PATA80;
+               sch++;
+       }
+
        /* check BIOS cable detect results */
        mask = ap->port_no == 0 ? PIIX_80C_PRI : PIIX_80C_SEC;
        pci_read_config_byte(pdev, PIIX_IOCFG, &tmp);
@@ -624,9 +649,20 @@ static int piix_pata_prereset(struct ata_link *link, unsigned long deadline)
 {
        struct ata_port *ap = link->ap;
        struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+       const struct sch_80 *sch = &sch_80[0];
+       int skip_check = 0;

-       if (!pci_test_config_bits(pdev, &piix_enable_bits[ap->port_no]))
-               return -ENOENT;
+       /* Check for specials - Intel SCH chipset */
+       while (sch->device) {
+               if (sch->device == pdev->device) {
+                       skip_check = 1;
+                       break;
+               }
+               sch++;
+       }
+       if (!skip_check)
+               if (!pci_test_config_bits(pdev, &piix_enable_bits[ap->port_no]))
+                       return -ENOENT;
        return ata_sff_prereset(link, deadline);
 }

diff --git a/drivers/ide/pci/piix.c b/drivers/ide/pci/piix.c
index decef0f..dce830b 100644
--- a/drivers/ide/pci/piix.c
+++ b/drivers/ide/pci/piix.c
@@ -377,6 +377,7 @@ static const struct ide_port_info piix_pci_info[] __devinitdata = {
        /* 22 */ DECLARE_ICH_DEV("ICH4",        ATA_UDMA5),
        /* 23 */ DECLARE_ICH_DEV("ESB2",        ATA_UDMA5),
        /* 24 */ DECLARE_ICH_DEV("ICH8M",       ATA_UDMA5),
+       /* 25 */ DECLARE_ICH_DEV("SCH",         ATA_UDMA5),
 };

 /**
@@ -450,6 +451,7 @@ static const struct pci_device_id piix_pci_tbl[] = {
        { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_82801DB_1),  22 },
        { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ESB2_18),    23 },
        { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ICH8_6),     24 },
+       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SCH_IDE),      25 },
        { 0, },
 };
 MODULE_DEVICE_TABLE(pci, piix_pci_tbl);
--
1.5.2.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ata: Add Intel SCH PATA support

[Alan Cox]

> +struct sch_80 {
> +       u16 device;
> +};

That seems somewhat overkill

> +       /* Check for specials - Intel SCH chipset */
> +       while (sch->device) {
> +               if (sch->device == pdev->device)
> +                       return ATA_CBL_PATA80;
> +               sch++;

PATA_80 means "controller *knows* we are 80 wire". PATA_UNK means
"controller can't do detect."

> +       /* Check for specials - Intel SCH chipset */
> +       while (sch->device) {
> +               if (sch->device == pdev->device) {
> +                       skip_check = 1;
> +                       break;
> +               }
> +               sch++;

This is horrible. If you just defined a new set of methods for your new
controller (remembering we now have inheritance anyway) you'd just use
the ata_sff_prereset and remove all the special casing code. At the very
least write the routine once only;)

> +       }
> +       if (!skip_check)
> +               if (!pci_test_config_bits(pdev, &piix_enable_bits[ap->port_no]))
> +                       return -ENOENT;
>         return ata_sff_prereset(link, deadline);
>  }
> 

Re: [PATCH] ata: Add Intel SCH PATA support

[Sergei Shtylyov]

Hello.

Alek Du wrote:

> Seems I need to post the patch here instead of LKML

    Your patch is whitespace damaged -- all tabs converted to spaces.

> [PATCH] ata: Add Intel SCH PATA support

> This patch adds Intel SCH chipsets (US15W, US15L, UL11L) PATA controller
> support.

> Signed-off-by: Alek Du <alek.du@intel.com>

> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> index b7c38ee..7f95a9a 100644
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
> @@ -214,6 +214,8 @@ static const struct pci_device_id piix_pci_tbl[] = {
>         /* ICH7/7-R (i945, i975) UDMA 100*/
>         { 0x8086, 0x27DF, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
>         { 0x8086, 0x269E, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
> +       /* INTEL SCH UDMA 100 */
> +       { 0x8086, 0x811A, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
>         /* ICH8 Mobile PATA Controller */
>         { 0x8086, 0x2850, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
> 
> @@ -561,6 +563,10 @@ struct ich_laptop {
>         u16 subdevice;
>  };
> 
> +struct sch_80 {
> +       u16 device;
> +};
> +

    Why create a structure which has only single field?

MBR, Sergei

Re: [PATCH] ata: Add Intel SCH PATA support

[Alek Du]

Sergei,

On Sun, 27 Apr 2008 00:46:14 +0800
"Sergei Shtylyov" <sshtylyov@ru.mvista.com> wrote:

> Re: [PATCH] ata: Add Intel SCH PATA support
> 
> Hello.
> 
> Alek Du wrote:
> 
> > Seems I need to post the patch here instead of LKML
> 
>     Your patch is whitespace damaged -- all tabs converted to spaces.
> 

I forward the patch from LKML and seems my clawsmail client eat all tabs ....

> > [PATCH] ata: Add Intel SCH PATA support
> 
> > This patch adds Intel SCH chipsets (US15W, US15L, UL11L) PATA controller
> > support.
> 
> > Signed-off-by: Alek Du <alek.du@intel.com>
> 
> > diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> > index b7c38ee..7f95a9a 100644
> > --- a/drivers/ata/ata_piix.c
> > +++ b/drivers/ata/ata_piix.c
> > @@ -214,6 +214,8 @@ static const struct pci_device_id piix_pci_tbl[] = {
> >         /* ICH7/7-R (i945, i975) UDMA 100*/
> >         { 0x8086, 0x27DF, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
> >         { 0x8086, 0x269E, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
> > +       /* INTEL SCH UDMA 100 */
> > +       { 0x8086, 0x811A, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
> >         /* ICH8 Mobile PATA Controller */
> >         { 0x8086, 0x2850, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
> >
> > @@ -561,6 +563,10 @@ struct ich_laptop {
> >         u16 subdevice;
> >  };
> >
> > +struct sch_80 {
> > +       u16 device;
> > +};
> > +
> 
>     Why create a structure which has only single field?
> 
Currently, it only use device ID to decide if force to go SCH way (force
to PATA_80 and skip port enabling bit check), but it would be easier for expanding
if I define a structure here. (looks like ich_laptop)

> MBR, Sergei

Re: [PATCH] ata: Add Intel SCH PATA support

[Alan Cox]

> Currently, it only use device ID to decide if force to go SCH way (force
> to PATA_80 and skip port enabling bit check), but it would be easier for expanding
> if I define a structure here. (looks like ich_laptop)

Just define an sch_pata type for those controller types and fix up the
relevant methods accordingly. Much cleaner

Re: [PATCH] ata: Add Intel SCH PATA support

[Alek Du]

Alan,
	Thanks for your comments:

On Sat, 26 Apr 2008 17:30:12 +0800
"Alan Cox" <alan@lxorguk.ukuu.org.uk> wrote:

> Re: [PATCH] ata: Add Intel SCH PATA support
> 
> > +struct sch_80 {
> > +       u16 device;
> > +};
> 
> That seems somewhat overkill

I have to consider there would be other PATA controllers work this way.
> 
> > +       /* Check for specials - Intel SCH chipset */
> > +       while (sch->device) {
> > +               if (sch->device == pdev->device)
> > +                       return ATA_CBL_PATA80;
> > +               sch++;
> 
> PATA_80 means "controller *knows* we are 80 wire". PATA_UNK means
> "controller can't do detect."

I need to force SCH to use PATA 80 pins to enable UDMA5, just like the way ich_laptop force
to use 40 pins.
> 
> > +       /* Check for specials - Intel SCH chipset */
> > +       while (sch->device) {
> > +               if (sch->device == pdev->device) {
> > +                       skip_check = 1;
> > +                       break;
> > +               }
> > +               sch++;
> 
> This is horrible. If you just defined a new set of methods for your new
> controller (remembering we now have inheritance anyway) you'd just use
> the ata_sff_prereset and remove all the special casing code. At the very
> least write the routine once only;)
> 
hmmm... for non-sch chipsets, there is no impact, this will only affect sch. You mean
I should create a ata_sch.c file instead of patching ata_piix.c ?

> > +       }
> > +       if (!skip_check)
> > +               if (!pci_test_config_bits(pdev, &piix_enable_bits[ap->port_no]))
> > +                       return -ENOENT;
> >         return ata_sff_prereset(link, deadline);
> >  }
> >

Re: [PATCH] ata: Add Intel SCH PATA support

[Alan Cox]

> > PATA_80 means "controller *knows* we are 80 wire". PATA_UNK means
> > "controller can't do detect."
> 
> I need to force SCH to use PATA 80 pins to enable UDMA5, just like the way ich_laptop force
> to use 40 pins.

What if I plug in a non 80 wire device ?

> hmmm... for non-sch chipsets, there is no impact, this will only affect sch. You mean
> I should create a ata_sch.c file instead of patching ata_piix.c ?

No an sch subtype in the driver just as we have types for ich/piix etc.

Re: [PATCH] ata: Add Intel SCH PATA support

[Alek Du]

Alan,

One problem I still do not understand, if I return PATA_UNK for cable detection result, could the
driver finally set the disk to UDMA5 mode if I plug in 80 wire disk?

Thanks,
Alek

On Sun, 27 Apr 2008 09:34:34 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > > PATA_80 means "controller *knows* we are 80 wire". PATA_UNK means
> > > "controller can't do detect."
> > 
> > I need to force SCH to use PATA 80 pins to enable UDMA5, just like the way ich_laptop force
> > to use 40 pins.
> 
> What if I plug in a non 80 wire device ?
> 
> > hmmm... for non-sch chipsets, there is no impact, this will only affect sch. You mean
> > I should create a ata_sch.c file instead of patching ata_piix.c ?
> 
> No an sch subtype in the driver just as we have types for ich/piix etc.
> 

Re: [PATCH] ata: Add Intel SCH PATA support

[Alan Cox]

On Sun, 27 Apr 2008 22:03:15 +0800
Alek Du <alek.du@intel.com> wrote:

> Alan,
> 
> One problem I still do not understand, if I return PATA_UNK for cable detection result, could the
> driver finally set the disk to UDMA5 mode if I plug in 80 wire disk?

You tell me - you've got the hardware documentation I assume. Assuming the
system implements detection then it should. If a PATA controller doesn't
implement any detection (cable or host side) and it does UDMA5 then I
believe the correct description for it is usually "broken" as people will
have horrible problems trying to use things like flash drives with it.

Alan

Re: [PATCH] ata: Add Intel SCH PATA support

[Alek Du]

On Sun, 27 Apr 2008 22:31:11 +0800
"Alan Cox" <alan@lxorguk.ukuu.org.uk> wrote:

> Re: [PATCH] ata: Add Intel SCH PATA support
> 
> On Sun, 27 Apr 2008 22:03:15 +0800
> Alek Du <alek.du@intel.com> wrote:
> 
> > Alan,
> >
> > One problem I still do not understand, if I return PATA_UNK for cable detection result, could the
> > driver finally set the disk to UDMA5 mode if I plug in 80 wire disk?
> 
> You tell me - you've got the hardware documentation I assume. Assuming the
> system implements detection then it should. If a PATA controller doesn't
> implement any detection (cable or host side) and it does UDMA5 then I
> believe the correct description for it is usually "broken" as people will
> have horrible problems trying to use things like flash drives with it.
> 
> Alan

The data sheet of SCH (http://download.intel.com/design/chipsets/embedded/datashts/319537.pdf) page 347 shows
the only supported PATA registers, for those not shown should be treated as "Reserved". 
Unfortunately, IOCFG (0x54) and port enable bits (0x41, 0x43) are all reserved.

Let's regard SCH PATA controller is an usually "broken" one :-), what do you suggest me to work around it?
Force it to 40-wire pin? or use a KConfig option to select 40-wire or 80-wire?

Thanks,
Alek

[PATCH] ata: Add Intel SCH PATA support (revised)

[Alek Du]

Hi Alan, Jeff

I re-write the patch according to the kind comments from Alan, I also did the test on my SCH machine. Please review it again. Thanks a lot.

This patch adds Intel SCH chipsets (US15W, US15L, UL11L) PATA controller
support.

Signed-off-by: Alek Du <alek.du@intel.com>
---
 drivers/ata/ata_piix.c  |   29 +++++++++++++++++++++++++++++
 drivers/ide/pci/piix.c  |    2 ++
 include/linux/pci_ids.h |    2 ++
 3 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index ea2c764..863c473 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -136,6 +136,7 @@ enum piix_controller_ids {
 	ich_pata_33,		/* ICH up to UDMA 33 only */
 	ich_pata_66,		/* ICH up to 66 Mhz */
 	ich_pata_100,		/* ICH up to UDMA 100 */
+	sch_pata_100,		/* SCH up to UDMA 100 */
 	ich5_sata,
 	ich6_sata,
 	ich6m_sata,
@@ -164,6 +165,7 @@ static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev);
 static void piix_set_dmamode(struct ata_port *ap, struct ata_device *adev);
 static void ich_set_dmamode(struct ata_port *ap, struct ata_device *adev);
 static int ich_pata_cable_detect(struct ata_port *ap);
+static int sch_pata_cable_detect(struct ata_port *ap);
 static u8 piix_vmw_bmdma_status(struct ata_port *ap);
 static int piix_sidpr_scr_read(struct ata_port *ap, unsigned int reg, u32 *val);
 static int piix_sidpr_scr_write(struct ata_port *ap, unsigned int reg, u32 val);
@@ -216,6 +218,8 @@ static const struct pci_device_id piix_pci_tbl[] = {
 	{ 0x8086, 0x269E, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
 	/* ICH8 Mobile PATA Controller */
 	{ 0x8086, 0x2850, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
+	/* Intel SCH PATA Controller */
+	{ 0x8086, 0x811A, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sch_pata_100 },
 
 	/* NOTE: The following PCI ids must be kept in sync with the
 	 * list in drivers/pci/quirks.c.
@@ -311,6 +315,13 @@ static struct ata_port_operations ich_pata_ops = {
 	.set_dmamode		= ich_set_dmamode,
 };
 
+static struct ata_port_operations sch_pata_ops = {
+	.inherits		= &piix_pata_ops,
+	.cable_detect		= sch_pata_cable_detect,
+	.prereset		= ata_sff_prereset,
+	.set_dmamode		= ich_set_dmamode,
+};
+
 static struct ata_port_operations piix_sata_ops = {
 	.inherits		= &ata_bmdma_port_ops,
 };
@@ -470,6 +481,15 @@ static struct ata_port_info piix_port_info[] = {
 		.port_ops	= &ich_pata_ops,
 	},
 
+	[sch_pata_100] =
+	{
+		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_CHECKINTR,
+		.pio_mask	= 0x1f,	/* pio0-4 */
+		.mwdma_mask	= 0x06, /* mwdma1-2 */
+		.udma_mask	= ATA_UDMA5, /* udma0-5 */
+		.port_ops	= &sch_pata_ops,
+	},
+
 	[ich5_sata] =
 	{
 		.flags		= PIIX_SATA_FLAGS,
@@ -614,6 +634,15 @@ static int ich_pata_cable_detect(struct ata_port *ap)
 }
 
 /**
+ * 	Intel SCH PATA controller could not detect cable type
+ * 	and always return ATA_CBL_PATA_UNK here.
+ */
+static int sch_pata_cable_detect(struct ata_port *ap)
+{
+	return ATA_CBL_PATA_UNK;
+}
+
+/**
  *	piix_pata_prereset - prereset for PATA host controller
  *	@link: Target link
  *	@deadline: deadline jiffies for the operation
diff --git a/drivers/ide/pci/piix.c b/drivers/ide/pci/piix.c
index 21c5dd2..31607fe 100644
--- a/drivers/ide/pci/piix.c
+++ b/drivers/ide/pci/piix.c
@@ -380,6 +380,7 @@ static const struct ide_port_info piix_pci_info[] __devinitdata = {
 	/* 22 */ DECLARE_ICH_DEV("ICH4",	ATA_UDMA5),
 	/* 23 */ DECLARE_ICH_DEV("ESB2",	ATA_UDMA5),
 	/* 24 */ DECLARE_ICH_DEV("ICH8M",	ATA_UDMA5),
+	/* 25 */ DECLARE_ICH_DEV("SCH",		ATA_UDMA5),
 };
 
 /**
@@ -453,6 +454,7 @@ static const struct pci_device_id piix_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_82801DB_1),  22 },
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ESB2_18),    23 },
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ICH8_6),     24 },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SCH_IDE),    25 },
 	{ 0, },
 };
 MODULE_DEVICE_TABLE(pci, piix_pci_tbl);
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 70eb3c8..e5a53da 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2413,6 +2413,8 @@
 #define PCI_DEVICE_ID_INTEL_82443GX_0	0x71a0
 #define PCI_DEVICE_ID_INTEL_82443GX_2	0x71a2
 #define PCI_DEVICE_ID_INTEL_82372FB_1	0x7601
+#define PCI_DEVICE_ID_INTEL_SCH_LPC	0x8119
+#define PCI_DEVICE_ID_INTEL_SCH_IDE	0x811a
 #define PCI_DEVICE_ID_INTEL_82454GX	0x84c4
 #define PCI_DEVICE_ID_INTEL_82450GX	0x84c5
 #define PCI_DEVICE_ID_INTEL_82451NX	0x84ca
-- 
1.5.2.5

Thanks,
Alek


On Sun, 27 Apr 2008 22:31:11 +0800
"Alan Cox" <alan@lxorguk.ukuu.org.uk> wrote:

> Re: [PATCH] ata: Add Intel SCH PATA support
> 
> On Sun, 27 Apr 2008 22:03:15 +0800
> Alek Du <alek.du@intel.com> wrote:
> 
> > Alan,
> >
> > One problem I still do not understand, if I return PATA_UNK for cable detection result, could the
> > driver finally set the disk to UDMA5 mode if I plug in 80 wire disk?
> 
> You tell me - you've got the hardware documentation I assume. Assuming the
> system implements detection then it should. If a PATA controller doesn't
> implement any detection (cable or host side) and it does UDMA5 then I
> believe the correct description for it is usually "broken" as people will
> have horrible problems trying to use things like flash drives with it.
> 
> Alan

Re: [PATCH] ata: Add Intel SCH PATA support (revised)

[Alan Cox]

On Mon, 28 Apr 2008 12:05:17 +0800
Alek Du <alek.du@intel.com> wrote:

> Hi Alan, Jeff
> 
> I re-write the patch according to the kind comments from Alan, I also did the test on my SCH machine. Please review it again. Thanks a lot.

Much nicer. We have a standard library helper for cable types you can use

ata_cable_unknown()

does the same as your private routine.

Alan

Re: [PATCH] ata: Add Intel SCH PATA support (revised)

[Alek Du]

Alan,

Fixed and tested, please review it:

This patch adds Intel SCH chipsets (US15W, US15L, UL11L) PATA controller
support.

Signed-off-by: Alek Du <alek.du@intel.com>
---
 drivers/ata/ata_piix.c  |   19 +++++++++++++++++++
 drivers/ide/pci/piix.c  |    2 ++
 include/linux/pci_ids.h |    2 ++
 3 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index ea2c764..4ec4178 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -136,6 +136,7 @@ enum piix_controller_ids {
 	ich_pata_33,		/* ICH up to UDMA 33 only */
 	ich_pata_66,		/* ICH up to 66 Mhz */
 	ich_pata_100,		/* ICH up to UDMA 100 */
+	sch_pata_100,		/* SCH up to UDMA 100 */
 	ich5_sata,
 	ich6_sata,
 	ich6m_sata,
@@ -216,6 +217,8 @@ static const struct pci_device_id piix_pci_tbl[] = {
 	{ 0x8086, 0x269E, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
 	/* ICH8 Mobile PATA Controller */
 	{ 0x8086, 0x2850, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
+	/* Intel SCH PATA Controller */
+	{ 0x8086, 0x811A, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sch_pata_100 },
 
 	/* NOTE: The following PCI ids must be kept in sync with the
 	 * list in drivers/pci/quirks.c.
@@ -311,6 +314,13 @@ static struct ata_port_operations ich_pata_ops = {
 	.set_dmamode		= ich_set_dmamode,
 };
 
+static struct ata_port_operations sch_pata_ops = {
+	.inherits		= &piix_pata_ops,
+	.cable_detect		= ata_cable_unknown,
+	.prereset		= ata_sff_prereset,
+	.set_dmamode		= ich_set_dmamode,
+};
+
 static struct ata_port_operations piix_sata_ops = {
 	.inherits		= &ata_bmdma_port_ops,
 };
@@ -470,6 +480,15 @@ static struct ata_port_info piix_port_info[] = {
 		.port_ops	= &ich_pata_ops,
 	},
 
+	[sch_pata_100] =
+	{
+		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_CHECKINTR,
+		.pio_mask	= 0x1f,	/* pio0-4 */
+		.mwdma_mask	= 0x06, /* mwdma1-2 */
+		.udma_mask	= ATA_UDMA5, /* udma0-5 */
+		.port_ops	= &sch_pata_ops,
+	},
+
 	[ich5_sata] =
 	{
 		.flags		= PIIX_SATA_FLAGS,
diff --git a/drivers/ide/pci/piix.c b/drivers/ide/pci/piix.c
index 21c5dd2..31607fe 100644
--- a/drivers/ide/pci/piix.c
+++ b/drivers/ide/pci/piix.c
@@ -380,6 +380,7 @@ static const struct ide_port_info piix_pci_info[] __devinitdata = {
 	/* 22 */ DECLARE_ICH_DEV("ICH4",	ATA_UDMA5),
 	/* 23 */ DECLARE_ICH_DEV("ESB2",	ATA_UDMA5),
 	/* 24 */ DECLARE_ICH_DEV("ICH8M",	ATA_UDMA5),
+	/* 25 */ DECLARE_ICH_DEV("SCH",		ATA_UDMA5),
 };
 
 /**
@@ -453,6 +454,7 @@ static const struct pci_device_id piix_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_82801DB_1),  22 },
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ESB2_18),    23 },
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ICH8_6),     24 },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SCH_IDE),    25 },
 	{ 0, },
 };
 MODULE_DEVICE_TABLE(pci, piix_pci_tbl);
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 70eb3c8..e5a53da 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2413,6 +2413,8 @@
 #define PCI_DEVICE_ID_INTEL_82443GX_0	0x71a0
 #define PCI_DEVICE_ID_INTEL_82443GX_2	0x71a2
 #define PCI_DEVICE_ID_INTEL_82372FB_1	0x7601
+#define PCI_DEVICE_ID_INTEL_SCH_LPC	0x8119
+#define PCI_DEVICE_ID_INTEL_SCH_IDE	0x811a
 #define PCI_DEVICE_ID_INTEL_82454GX	0x84c4
 #define PCI_DEVICE_ID_INTEL_82450GX	0x84c5
 #define PCI_DEVICE_ID_INTEL_82451NX	0x84ca
-- 
1.5.2.5


Thanks,
Alek

On Mon, 28 Apr 2008 16:41:23 +0800
"Alan Cox" <alan@lxorguk.ukuu.org.uk> wrote:

> Re: [PATCH] ata: Add Intel SCH PATA support (revised)
> 
> On Mon, 28 Apr 2008 12:05:17 +0800
> Alek Du <alek.du@intel.com> wrote:
> 
> > Hi Alan, Jeff
> >
> > I re-write the patch according to the kind comments from Alan, I also did the test on my SCH machine. Please review it again. Thanks a lot.
> 
> Much nicer. We have a standard library helper for cable types you can use
> 
> ata_cable_unknown()
> 
> does the same as your private routine.
> 
> Alan
> 
> 

Re: [PATCH] ata: Add Intel SCH PATA support

[Alan Cox]

> Let's regard SCH PATA controller is an usually "broken" one :-), what do you suggest me to work around it?
> Force it to 40-wire pin? or use a KConfig option to select 40-wire or 80-wire?

The drive side detect depends upon hardware features. If the correct
capacitors are present in the design then the drive side detect should
work. 

Alan

Re: [PATCH] ata: Add Intel SCH PATA support (revised)

[Alan Cox]

On Mon, 28 Apr 2008 17:07:51 +0800
Alek Du <alek.du@intel.com> wrote:

> Alan,
> 
> Fixed and tested, please review it:
> 
> This patch adds Intel SCH chipsets (US15W, US15L, UL11L) PATA controller
> support.
> 
> Signed-off-by: Alek Du <alek.du@intel.com>

Acked-by: Alan Cox <alan@redhat.com>

Re: [PATCH] ata: Add Intel SCH PATA support (revised)

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday 28 April 2008, Alek Du wrote:
> Alan,
> 
> Fixed and tested, please review it:
> 
> This patch adds Intel SCH chipsets (US15W, US15L, UL11L) PATA controller
> support.
> 
> Signed-off-by: Alek Du <alek.du@intel.com>
> ---
>  drivers/ata/ata_piix.c  |   19 +++++++++++++++++++
>  drivers/ide/pci/piix.c  |    2 ++
>  include/linux/pci_ids.h |    2 ++
>  3 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> index ea2c764..4ec4178 100644
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
> @@ -136,6 +136,7 @@ enum piix_controller_ids {
>  	ich_pata_33,		/* ICH up to UDMA 33 only */
>  	ich_pata_66,		/* ICH up to 66 Mhz */
>  	ich_pata_100,		/* ICH up to UDMA 100 */
> +	sch_pata_100,		/* SCH up to UDMA 100 */
>  	ich5_sata,
>  	ich6_sata,
>  	ich6m_sata,
> @@ -216,6 +217,8 @@ static const struct pci_device_id piix_pci_tbl[] = {
>  	{ 0x8086, 0x269E, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
>  	/* ICH8 Mobile PATA Controller */
>  	{ 0x8086, 0x2850, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
> +	/* Intel SCH PATA Controller */
> +	{ 0x8086, 0x811A, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sch_pata_100 },
>  
>  	/* NOTE: The following PCI ids must be kept in sync with the
>  	 * list in drivers/pci/quirks.c.
> @@ -311,6 +314,13 @@ static struct ata_port_operations ich_pata_ops = {
>  	.set_dmamode		= ich_set_dmamode,
>  };
>  
> +static struct ata_port_operations sch_pata_ops = {
> +	.inherits		= &piix_pata_ops,
> +	.cable_detect		= ata_cable_unknown,
> +	.prereset		= ata_sff_prereset,
> +	.set_dmamode		= ich_set_dmamode,
> +};
> +
>  static struct ata_port_operations piix_sata_ops = {
>  	.inherits		= &ata_bmdma_port_ops,
>  };
> @@ -470,6 +480,15 @@ static struct ata_port_info piix_port_info[] = {
>  		.port_ops	= &ich_pata_ops,
>  	},
>  
> +	[sch_pata_100] =
> +	{
> +		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_CHECKINTR,
> +		.pio_mask	= 0x1f,	/* pio0-4 */
> +		.mwdma_mask	= 0x06, /* mwdma1-2 */
> +		.udma_mask	= ATA_UDMA5, /* udma0-5 */
> +		.port_ops	= &sch_pata_ops,
> +	},
> +
>  	[ich5_sata] =
>  	{
>  		.flags		= PIIX_SATA_FLAGS,
> diff --git a/drivers/ide/pci/piix.c b/drivers/ide/pci/piix.c
> index 21c5dd2..31607fe 100644
> --- a/drivers/ide/pci/piix.c
> +++ b/drivers/ide/pci/piix.c
> @@ -380,6 +380,7 @@ static const struct ide_port_info piix_pci_info[] __devinitdata = {
>  	/* 22 */ DECLARE_ICH_DEV("ICH4",	ATA_UDMA5),
>  	/* 23 */ DECLARE_ICH_DEV("ESB2",	ATA_UDMA5),
>  	/* 24 */ DECLARE_ICH_DEV("ICH8M",	ATA_UDMA5),
> +	/* 25 */ DECLARE_ICH_DEV("SCH",		ATA_UDMA5),

DECLARE_ICH_DEV() cannot be used here - seems like DEFINE_SCH_DEV()
needs to be defined (which will lack .enablebits field but otherwise
be identical to DECLARE_ICH_DEV)

also seems that piix_cable_detect() should be updated to check for
dev->device == PCI_DEVICE_ID_INTEL_SCH_IDE and return ATA_CBL_PATA80
(it is OK to use it instead of ATA_CBL_UNK in drivers/ide/ - generic
cable detection code won't override drive side cable detection).

otherwise everything looks good

Thanks,
Bart

Re: [PATCH] ata: Add Intel SCH PATA support (revised)

[Alek Du]

Hi Bartlomiej,

On Tue, 29 Apr 2008 03:01:32 +0800
"Bartlomiej Zolnierkiewicz" <bzolnier@gmail.com> wrote:

> Re: [PATCH] ata: Add Intel SCH PATA support (revised)
> 
> Hi,
> 
> On Monday 28 April 2008, Alek Du wrote:
> > Alan,
> >
> > Fixed and tested, please review it:
> >
> > This patch adds Intel SCH chipsets (US15W, US15L, UL11L) PATA controller
> > support.
> >
> > Signed-off-by: Alek Du <alek.du@intel.com>
> > ---
> >  drivers/ata/ata_piix.c  |   19 +++++++++++++++++++
> >  drivers/ide/pci/piix.c  |    2 ++
> >  include/linux/pci_ids.h |    2 ++
> >  3 files changed, 23 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> > index ea2c764..4ec4178 100644
> > --- a/drivers/ata/ata_piix.c
> > +++ b/drivers/ata/ata_piix.c
> > @@ -136,6 +136,7 @@ enum piix_controller_ids {
> >       ich_pata_33,            /* ICH up to UDMA 33 only */
> >       ich_pata_66,            /* ICH up to 66 Mhz */
> >       ich_pata_100,           /* ICH up to UDMA 100 */
> > +     sch_pata_100,           /* SCH up to UDMA 100 */
> >       ich5_sata,
> >       ich6_sata,
> >       ich6m_sata,
> > @@ -216,6 +217,8 @@ static const struct pci_device_id piix_pci_tbl[] = {
> >       { 0x8086, 0x269E, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
> >       /* ICH8 Mobile PATA Controller */
> >       { 0x8086, 0x2850, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
> > +     /* Intel SCH PATA Controller */
> > +     { 0x8086, 0x811A, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sch_pata_100 },
> > 
> >       /* NOTE: The following PCI ids must be kept in sync with the
> >        * list in drivers/pci/quirks.c.
> > @@ -311,6 +314,13 @@ static struct ata_port_operations ich_pata_ops = {
> >       .set_dmamode            = ich_set_dmamode,
> >  };
> > 
> > +static struct ata_port_operations sch_pata_ops = {
> > +     .inherits               = &piix_pata_ops,
> > +     .cable_detect           = ata_cable_unknown,
> > +     .prereset               = ata_sff_prereset,
> > +     .set_dmamode            = ich_set_dmamode,
> > +};
> > +
> >  static struct ata_port_operations piix_sata_ops = {
> >       .inherits               = &ata_bmdma_port_ops,
> >  };
> > @@ -470,6 +480,15 @@ static struct ata_port_info piix_port_info[] = {
> >               .port_ops       = &ich_pata_ops,
> >       },
> > 
> > +     [sch_pata_100] =
> > +     {
> > +             .flags          = PIIX_PATA_FLAGS | PIIX_FLAG_CHECKINTR,
> > +             .pio_mask       = 0x1f, /* pio0-4 */
> > +             .mwdma_mask     = 0x06, /* mwdma1-2 */
> > +             .udma_mask      = ATA_UDMA5, /* udma0-5 */
> > +             .port_ops       = &sch_pata_ops,
> > +     },
> > +
> >       [ich5_sata] =
> >       {
> >               .flags          = PIIX_SATA_FLAGS,
> > diff --git a/drivers/ide/pci/piix.c b/drivers/ide/pci/piix.c
> > index 21c5dd2..31607fe 100644
> > --- a/drivers/ide/pci/piix.c
> > +++ b/drivers/ide/pci/piix.c
> > @@ -380,6 +380,7 @@ static const struct ide_port_info piix_pci_info[] __devinitdata = {
> >       /* 22 */ DECLARE_ICH_DEV("ICH4",        ATA_UDMA5),
> >       /* 23 */ DECLARE_ICH_DEV("ESB2",        ATA_UDMA5),
> >       /* 24 */ DECLARE_ICH_DEV("ICH8M",       ATA_UDMA5),
> > +     /* 25 */ DECLARE_ICH_DEV("SCH",         ATA_UDMA5),
> 
> DECLARE_ICH_DEV() cannot be used here - seems like DEFINE_SCH_DEV()
> needs to be defined (which will lack .enablebits field but otherwise
> be identical to DECLARE_ICH_DEV)
> 
I do not understand here, where DECLARE_ICH_DEV() used here? I just defined a new sub_type
sch_pata_100, and inherit most ops from piix instead of cable_detect and prereset. 

> also seems that piix_cable_detect() should be updated to check for
> dev->device == PCI_DEVICE_ID_INTEL_SCH_IDE and return ATA_CBL_PATA80
> (it is OK to use it instead of ATA_CBL_UNK in drivers/ide/ - generic
> cable detection code won't override drive side cable detection).
>
My first version of patch is to modify piix_cable_detect like your way. But Alan suggested
use this way -- it is much cleaner. And according to my test, return ATA_CBL_UNK do no harm.
The driver is set to UDMA5 when 80 cable is used.
 
> otherwise everything looks good
> 
> Thanks,
> Bart

Thanks,
Alek

Re: [PATCH] ata: Add Intel SCH PATA support (revised)

[Alek Du]

Hi Alan and Bartlomiej,

Modified the patch again. Add and use DECLARE_SCH_DEV instead of DECLARE_ICH_DEV
to avoid enable_bits. But I still use ATA_CBL_UNK, because according to my test,
it works fine -- it could set the disk to UDMA5 mode when using 80 wire cable.
Please review it again, thanks.

This patch adds Intel SCH chipsets (US15W, US15L, UL11L) PATA controller
support.

Signed-off-by: Alek Du <alek.du@intel.com>
---
 drivers/ata/ata_piix.c  |   19 +++++++++++++++++++
 drivers/ide/pci/piix.c  |   15 +++++++++++++++
 include/linux/pci_ids.h |    2 ++
 3 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index ea2c764..4ec4178 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -136,6 +136,7 @@ enum piix_controller_ids {
 	ich_pata_33,		/* ICH up to UDMA 33 only */
 	ich_pata_66,		/* ICH up to 66 Mhz */
 	ich_pata_100,		/* ICH up to UDMA 100 */
+	sch_pata_100,		/* SCH up to UDMA 100 */
 	ich5_sata,
 	ich6_sata,
 	ich6m_sata,
@@ -216,6 +217,8 @@ static const struct pci_device_id piix_pci_tbl[] = {
 	{ 0x8086, 0x269E, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
 	/* ICH8 Mobile PATA Controller */
 	{ 0x8086, 0x2850, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
+	/* Intel SCH PATA Controller */
+	{ 0x8086, 0x811A, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sch_pata_100 },
 
 	/* NOTE: The following PCI ids must be kept in sync with the
 	 * list in drivers/pci/quirks.c.
@@ -311,6 +314,13 @@ static struct ata_port_operations ich_pata_ops = {
 	.set_dmamode		= ich_set_dmamode,
 };
 
+static struct ata_port_operations sch_pata_ops = {
+	.inherits		= &piix_pata_ops,
+	.cable_detect		= ata_cable_unknown,
+	.prereset		= ata_sff_prereset,
+	.set_dmamode		= ich_set_dmamode,
+};
+
 static struct ata_port_operations piix_sata_ops = {
 	.inherits		= &ata_bmdma_port_ops,
 };
@@ -470,6 +480,15 @@ static struct ata_port_info piix_port_info[] = {
 		.port_ops	= &ich_pata_ops,
 	},
 
+	[sch_pata_100] =
+	{
+		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_CHECKINTR,
+		.pio_mask	= 0x1f,	/* pio0-4 */
+		.mwdma_mask	= 0x06, /* mwdma1-2 */
+		.udma_mask	= ATA_UDMA5, /* udma0-5 */
+		.port_ops	= &sch_pata_ops,
+	},
+
 	[ich5_sata] =
 	{
 		.flags		= PIIX_SATA_FLAGS,
diff --git a/drivers/ide/pci/piix.c b/drivers/ide/pci/piix.c
index 21c5dd2..d224a67 100644
--- a/drivers/ide/pci/piix.c
+++ b/drivers/ide/pci/piix.c
@@ -340,6 +340,19 @@ static const struct ide_port_ops piix_port_ops = {
 		.udma_mask	= udma, \
 	}
 
+#define DECLARE_SCH_DEV(name_str, udma) \
+	{ \
+		.name		= name_str, \
+		.init_chipset	= init_chipset_ich, \
+		.init_hwif	= init_hwif_ich, \
+		.port_ops	= &piix_port_ops, \
+		.host_flags	= IDE_HFLAGS_PIIX, \
+		.pio_mask	= ATA_PIO4, \
+		.swdma_mask	= ATA_SWDMA2_ONLY, \
+		.mwdma_mask	= ATA_MWDMA12_ONLY, \
+		.udma_mask	= udma, \
+	}
+
 static const struct ide_port_info piix_pci_info[] __devinitdata = {
 	/*  0 */ DECLARE_PIIX_DEV("PIIXa",	0x00),	/* no udma */
 	/*  1 */ DECLARE_PIIX_DEV("PIIXb",	0x00),	/* no udma */
@@ -380,6 +393,7 @@ static const struct ide_port_info piix_pci_info[] __devinitdata = {
 	/* 22 */ DECLARE_ICH_DEV("ICH4",	ATA_UDMA5),
 	/* 23 */ DECLARE_ICH_DEV("ESB2",	ATA_UDMA5),
 	/* 24 */ DECLARE_ICH_DEV("ICH8M",	ATA_UDMA5),
+	/* 25 */ DECLARE_SCH_DEV("SCH",		ATA_UDMA5),
 };
 
 /**
@@ -453,6 +467,7 @@ static const struct pci_device_id piix_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_82801DB_1),  22 },
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ESB2_18),    23 },
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ICH8_6),     24 },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SCH_IDE),    25 },
 	{ 0, },
 };
 MODULE_DEVICE_TABLE(pci, piix_pci_tbl);
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 70eb3c8..e5a53da 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2413,6 +2413,8 @@
 #define PCI_DEVICE_ID_INTEL_82443GX_0	0x71a0
 #define PCI_DEVICE_ID_INTEL_82443GX_2	0x71a2
 #define PCI_DEVICE_ID_INTEL_82372FB_1	0x7601
+#define PCI_DEVICE_ID_INTEL_SCH_LPC	0x8119
+#define PCI_DEVICE_ID_INTEL_SCH_IDE	0x811a
 #define PCI_DEVICE_ID_INTEL_82454GX	0x84c4
 #define PCI_DEVICE_ID_INTEL_82450GX	0x84c5
 #define PCI_DEVICE_ID_INTEL_82451NX	0x84ca
-- 
1.5.2.5




On Tue, 29 Apr 2008 03:01:32 +0800
"Bartlomiej Zolnierkiewicz" <bzolnier@gmail.com> wrote:

> > diff --git a/drivers/ide/pci/piix.c b/drivers/ide/pci/piix.c
> > index 21c5dd2..31607fe 100644
> > --- a/drivers/ide/pci/piix.c
> > +++ b/drivers/ide/pci/piix.c
> > @@ -380,6 +380,7 @@ static const struct ide_port_info piix_pci_info[] __devinitdata = {
> >       /* 22 */ DECLARE_ICH_DEV("ICH4",        ATA_UDMA5),
> >       /* 23 */ DECLARE_ICH_DEV("ESB2",        ATA_UDMA5),
> >       /* 24 */ DECLARE_ICH_DEV("ICH8M",       ATA_UDMA5),
> > +     /* 25 */ DECLARE_ICH_DEV("SCH",         ATA_UDMA5),
> 
> DECLARE_ICH_DEV() cannot be used here - seems like DEFINE_SCH_DEV()
> needs to be defined (which will lack .enablebits field but otherwise
> be identical to DECLARE_ICH_DEV)
> 
> also seems that piix_cable_detect() should be updated to check for
> dev->device == PCI_DEVICE_ID_INTEL_SCH_IDE and return ATA_CBL_PATA80
> (it is OK to use it instead of ATA_CBL_UNK in drivers/ide/ - generic
> cable detection code won't override drive side cable detection).
> 
> otherwise everything looks good
> 
> Thanks,
> Bart

Re: [PATCH] ata: Add Intel SCH PATA support (revised)

[Alek Du]

On Tue, 29 Apr 2008 08:14:11 +0800
Alek Du <alek.du@intel.com> wrote:

> > needs to be defined (which will lack .enablebits field but otherwise
> > be identical to DECLARE_ICH_DEV)
> > 
> I do not understand here, where DECLARE_ICH_DEV() used here? I just defined a new sub_type
> sch_pata_100, and inherit most ops from piix instead of cable_detect and prereset. 
>

I see now. I posted a new patch which defined DECLARE_SCH_DEV. Thanks a lot.

> > also seems that piix_cable_detect() should be updated to check for
> > dev->device == PCI_DEVICE_ID_INTEL_SCH_IDE and return ATA_CBL_PATA80
> > (it is OK to use it instead of ATA_CBL_UNK in drivers/ide/ - generic
> > cable detection code won't override drive side cable detection).
> >
> My first version of patch is to modify piix_cable_detect like your way. But Alan suggested
> use this way -- it is much cleaner. And according to my test, return ATA_CBL_UNK do no harm.
> The driver is set to UDMA5 when 80 cable is used.
>  
> > otherwise everything looks good
> > 
> > Thanks,
> > Bart
> 
> Thanks,
> Alek

Re: [PATCH] ata: Add Intel SCH PATA support (revised)

[Sergei Shtylyov]

Hello.

Alek Du wrote:

> Modified the patch again. Add and use DECLARE_SCH_DEV instead of DECLARE_ICH_DEV
> to avoid enable_bits. But I still use ATA_CBL_UNK, because according to my test,
> it works fine -- it could set the disk to UDMA5 mode when using 80 wire cable.
> Please review it again, thanks.

> This patch adds Intel SCH chipsets (US15W, US15L, UL11L) PATA controller
> support.

> Signed-off-by: Alek Du <alek.du@intel.com>

    NAK this patch completely -- both libata and IDE parts.
    Luckily, I have just had a quick look at the datasheet to which you've 
linked and that was enough to conclude that SCH is *not* compatible to 
ICH/PIIX family, so absolutely needs a separate driver!

> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> index ea2c764..4ec4178 100644
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
> @@ -136,6 +136,7 @@ enum piix_controller_ids {
>  	ich_pata_33,		/* ICH up to UDMA 33 only */
>  	ich_pata_66,		/* ICH up to 66 Mhz */
>  	ich_pata_100,		/* ICH up to UDMA 100 */
> +	sch_pata_100,		/* SCH up to UDMA 100 */
>  	ich5_sata,
>  	ich6_sata,
>  	ich6m_sata,
> @@ -216,6 +217,8 @@ static const struct pci_device_id piix_pci_tbl[] = {
>  	{ 0x8086, 0x269E, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
>  	/* ICH8 Mobile PATA Controller */
>  	{ 0x8086, 0x2850, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
> +	/* Intel SCH PATA Controller */
> +	{ 0x8086, 0x811A, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sch_pata_100 },
>  
>  	/* NOTE: The following PCI ids must be kept in sync with the
>  	 * list in drivers/pci/quirks.c.
> @@ -311,6 +314,13 @@ static struct ata_port_operations ich_pata_ops = {
>  	.set_dmamode		= ich_set_dmamode,
>  };
>  
> +static struct ata_port_operations sch_pata_ops = {
> +	.inherits		= &piix_pata_ops,
> +	.cable_detect		= ata_cable_unknown,
> +	.prereset		= ata_sff_prereset,
> +	.set_dmamode		= ich_set_dmamode,

    The ich_set_dmamode() won't work with SCH.

> +};
> +
>  static struct ata_port_operations piix_sata_ops = {
>  	.inherits		= &ata_bmdma_port_ops,
>  };
> @@ -470,6 +480,15 @@ static struct ata_port_info piix_port_info[] = {
>  		.port_ops	= &ich_pata_ops,
>  	},
>  
> +	[sch_pata_100] =
> +	{
> +		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_CHECKINTR,
> +		.pio_mask	= 0x1f,	/* pio0-4 */
> +		.mwdma_mask	= 0x06, /* mwdma1-2 */

    Wrong, SCH does support MWDMA0, so the mask would be 0x07...

> diff --git a/drivers/ide/pci/piix.c b/drivers/ide/pci/piix.c
> index 21c5dd2..d224a67 100644
> --- a/drivers/ide/pci/piix.c
> +++ b/drivers/ide/pci/piix.c
> @@ -340,6 +340,19 @@ static const struct ide_port_ops piix_port_ops = {
>  		.udma_mask	= udma, \
>  	}
>  
> +#define DECLARE_SCH_DEV(name_str, udma) \

    Well, a single instance hardly deserved its own macro, could just put the 
whole initializer in its array slot like for MPIIX (which probably shouldn't 
be in this driver anyway though :-)...

> +	{ \
> +		.name		= name_str, \
> +		.init_chipset	= init_chipset_ich, \

    This method read/writes IOCFG register which as you've yourself noted is 
missing on SCH (among with any ICH compatible registers ;-).

> +		.init_hwif	= init_hwif_ich, \
> +		.port_ops	= &piix_port_ops, \
> +		.host_flags	= IDE_HFLAGS_PIIX, \
> +		.pio_mask	= ATA_PIO4, \
> +		.swdma_mask	= ATA_SWDMA2_ONLY, \

    Wrong, no SWDMA support in SCH.

> +		.mwdma_mask	= ATA_MWDMA12_ONLY, \

    Shoud have been ATA_MWDMA2.

> +		.udma_mask	= udma, \

    This is fixed to AATA_UDMA5 so there was no need to parametrize...

MBR, Sergei

Re: [PATCH] ata: Add Intel SCH PATA support

[Sergei Shtylyov]

Alek Du wrote:

>>Re: [PATCH] ata: Add Intel SCH PATA support

>>On Sun, 27 Apr 2008 22:03:15 +0800
>>Alek Du <alek.du@intel.com> wrote:

>>>Alan,

>>>One problem I still do not understand, if I return PATA_UNK for cable detection result, could the
>>>driver finally set the disk to UDMA5 mode if I plug in 80 wire disk?

>>You tell me - you've got the hardware documentation I assume. Assuming the
>>system implements detection then it should. If a PATA controller doesn't
>>implement any detection (cable or host side) and it does UDMA5 then I
>>believe the correct description for it is usually "broken" as people will
>>have horrible problems trying to use things like flash drives with it.

>>Alan
> 
> 
> The data sheet of SCH (http://download.intel.com/design/chipsets/embedded/datashts/319537.pdf) page 347 shows
> the only supported PATA registers, for those not shown should be treated as "Reserved". 
> Unfortunately, IOCFG (0x54) and port enable bits (0x41, 0x43) are all reserved.

    If you'd actually compared the datasheet to the driver and/or the PIIX/ICH 
datasheets, you would have seen that SCH is not compatible to PIIX/ICH. It 
doesn't have PIO/DMA and UDMA timing and UDMA enable registers at config. 
space offsets 0x4x but instead has pair of drive timing registers (SCH PATA 
controller is single-channel) controlling PIO/DMA/UDMA speeds each at 0x8x, 
laid out completely differently: PIIX/ICH timing registers fields encoded 
cycle counts which SCH registers fileds encode mode numbers themselves.

> Let's regard SCH PATA controller is an usually "broken" one :-), what do you suggest me to work around it?

    Let's regard it as the one deserving its own driver. ;-)

> Thanks,
> Alek

MBR, Sergei

Re: [PATCH] ata: Add Intel SCH PATA support (revised)

[Alek Du]

Hi Sergei,

On Wed, 30 Apr 2008 00:38:17 +0800
"Sergei Shtylyov" <sshtylyov@ru.mvista.com> wrote:

>
> 
>     NAK this patch completely -- both libata and IDE parts.
>     Luckily, I have just had a quick look at the datasheet to which you've
> linked and that was enough to conclude that SCH is *not* compatible to
> ICH/PIIX family, so absolutely needs a separate driver!
>

Most registers are same except for port enable bit, IOCFG and DMA timing. Now
I know why my patch just works -- the BIOS set the timing correctly. Thanks
for point this out. I will post a separate driver patch later.


Thanks,
Alek

Re: [PATCH] ata: Add Intel SCH PATA support (revised)

[Sergei Shtylyov]

Hello.

Alek Du wrote:

>>    NAK this patch completely -- both libata and IDE parts.

   In the future it would be worth to post 2 separate patches, one for IDE and 
one for libata -- since they should go thru the different maintainer trees.

>>    Luckily, I have just had a quick look at the datasheet to which you've
>>linked and that was enough to conclude that SCH is *not* compatible to
>>ICH/PIIX family, so absolutely needs a separate driver!

> Most registers are same

    Well, if you mean the standard BMDMA and standard PCI config. space 
registers (0x00 thru 0x3f), then those are mostly the same for the majority of 
the PCI IDE controllers.  Why not stick your code into one of the other driver 
instead? ;-)

> except for port enable bit, IOCFG and DMA timing. Now

    And also PIO and UDMA timings/enable. Actually, all the vendor-specific 
PCI config. registers are different from PIIX/ICH -- and that's what makes the 
PATA drivers different from each other.

> I know why my patch just works -- the BIOS set the timing correctly.

    Yeah, and we have the special drivers which rely on the BIOS doing all the 
mode programming...

> Thanks for point this out. I will post a separate driver patch later.

    In the meantwhile the only viable solution would be to stick your code 
into drivers/ide/pci/generic.c and drivers/ata/ata_generic.c.  No more hacks, 
please. :-)

> Thanks,
> Alek

MBR, Sergei