[PATCH 1/4] pata_of_platform: Remove "electra-ide" quirk

[PATCH 1/4] pata_of_platform: Remove "electra-ide" quirk

[Alexander Shiyan]

"electra-ide" is not used anywhere in the kernel and could be
represented in devicetree in a normal way.
This patch removes specific quirk for "electra-ide".

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/ata/pata_of_platform.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
index a7e95a5..6af1c9b 100644
--- a/drivers/ata/pata_of_platform.c
+++ b/drivers/ata/pata_of_platform.c
@@ -35,20 +35,11 @@ static int pata_of_platform_probe(struct platform_device *ofdev)
 		return -EINVAL;
 	}
 
-	if (of_device_is_compatible(dn, "electra-ide")) {
-		/* Altstatus is really at offset 0x3f6 from the primary window
-		 * on electra-ide. Adjust ctl_res and io_res accordingly.
-		 */
-		ctl_res = io_res;
-		ctl_res.start = ctl_res.start+0x3f6;
-		io_res.end = ctl_res.start-1;
-	} else {
-		ret = of_address_to_resource(dn, 1, &ctl_res);
-		if (ret) {
-			dev_err(&ofdev->dev, "can't get CTL address from "
-				"device tree\n");
-			return -EINVAL;
-		}
+	ret = of_address_to_resource(dn, 1, &ctl_res);
+	if (ret) {
+		dev_err(&ofdev->dev, "can't get CTL address from "
+			"device tree\n");
+		return -EINVAL;
 	}
 
 	irq_res = platform_get_resource(ofdev, IORESOURCE_IRQ, 0);
@@ -79,8 +70,7 @@ static int pata_of_platform_probe(struct platform_device *ofdev)
 
 static struct of_device_id pata_of_platform_match[] = {
 	{ .compatible = "ata-generic", },
-	{ .compatible = "electra-ide", },
-	{},
+	{ },
 };
 MODULE_DEVICE_TABLE(of, pata_of_platform_match);
 
-- 
1.8.5.5

[PATCH 3/4] pata_platform: Use devm_ioremap_resource()

[Alexander Shiyan]

Use devm_ioremap_resource() calls.
This will provide a more reasonable exit codes in case of errors.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/ata/pata_platform.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index f8cff3e..ecbc3bf 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -145,19 +145,21 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
 	 * Handle the MMIO case
 	 */
 	if (mmio) {
-		ap->ioaddr.cmd_addr = devm_ioremap(dev, io_res->start,
-				resource_size(io_res));
-		ap->ioaddr.ctl_addr = devm_ioremap(dev, ctl_res->start,
-				resource_size(ctl_res));
+		ap->ioaddr.cmd_addr = devm_ioremap_resource(dev, io_res);
+		if (IS_ERR(ap->ioaddr.cmd_addr))
+			return PTR_ERR(ap->ioaddr.cmd_addr);
+		ap->ioaddr.ctl_addr = devm_ioremap_resource(dev, ctl_res);
+		if (IS_ERR(ap->ioaddr.ctl_addr))
+			return PTR_ERR(ap->ioaddr.ctl_addr);
 	} else {
 		ap->ioaddr.cmd_addr = devm_ioport_map(dev, io_res->start,
 				resource_size(io_res));
 		ap->ioaddr.ctl_addr = devm_ioport_map(dev, ctl_res->start,
 				resource_size(ctl_res));
-	}
-	if (!ap->ioaddr.cmd_addr || !ap->ioaddr.ctl_addr) {
-		dev_err(dev, "failed to map IO/CTL base\n");
-		return -ENOMEM;
+		if (!ap->ioaddr.cmd_addr || !ap->ioaddr.ctl_addr) {
+			dev_err(dev, "failed to map IO/CTL base\n");
+			return -ENOMEM;
+		}
 	}
 
 	ap->ioaddr.altstatus_addr = ap->ioaddr.ctl_addr;
-- 
1.8.5.5

Re: [PATCH 3/4] pata_platform: Use devm_ioremap_resource()

[Tejun Heo]

On Sat, Aug 23, 2014 at 02:46:11PM +0400, Alexander Shiyan wrote:
> Use devm_ioremap_resource() calls.
> This will provide a more reasonable exit codes in case of errors.

It also reserves the mem region.

Thanks.

-- 
tejun

[PATCH 4/4] pata_platform: Use 16-bit wide data transfer

[Alexander Shiyan]

In some cases, system bus can be configured for 16-bit mode, in this
case using of read/write functions for the 32 bit values causes two
cycles of 16 bits, which is incorrect.
The patch provides its own function to use the proper 16-bit mode.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/ata/pata_platform.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index ecbc3bf..79dd223 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -43,13 +43,28 @@ static int pata_platform_set_mode(struct ata_link *link, struct ata_device **unu
 	return 0;
 }
 
+static unsigned int pata_platform_xfer_noirq(struct ata_device *dev,
+					     unsigned char *buf,
+					     unsigned int buflen, int rw)
+{
+	unsigned long flags;
+	unsigned int consumed;
+
+	local_irq_save(flags);
+	/* Use 16-bit transfer */
+	consumed = ata_sff_data_xfer(dev, buf, buflen, rw);
+	local_irq_restore(flags);
+
+	return consumed;
+}
+
 static struct scsi_host_template pata_platform_sht = {
 	ATA_PIO_SHT(DRV_NAME),
 };
 
 static struct ata_port_operations pata_platform_port_ops = {
 	.inherits		= &ata_sff_port_ops,
-	.sff_data_xfer		= ata_sff_data_xfer_noirq,
+	.sff_data_xfer		= pata_platform_xfer_noirq,
 	.cable_detect		= ata_cable_unknown,
 	.set_mode		= pata_platform_set_mode,
 };
-- 
1.8.5.5

Re: [PATCH 4/4] pata_platform: Use 16-bit wide data transfer

[Tejun Heo]

Hello,

On Sat, Aug 23, 2014 at 02:46:12PM +0400, Alexander Shiyan wrote:
> In some cases, system bus can be configured for 16-bit mode, in this
> case using of read/write functions for the 32 bit values causes two
> cycles of 16 bits, which is incorrect.

Shouldn't we distinguish them instead of forcing 16bit transfer on
all?

> The patch provides its own function to use the proper 16-bit mode.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  drivers/ata/pata_platform.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> index ecbc3bf..79dd223 100644
> --- a/drivers/ata/pata_platform.c
> +++ b/drivers/ata/pata_platform.c
> @@ -43,13 +43,28 @@ static int pata_platform_set_mode(struct ata_link *link, struct ata_device **unu
>  	return 0;
>  }
>  
> +static unsigned int pata_platform_xfer_noirq(struct ata_device *dev,
> +					     unsigned char *buf,
> +					     unsigned int buflen, int rw)
> +{
> +	unsigned long flags;
> +	unsigned int consumed;
> +
> +	local_irq_save(flags);
> +	/* Use 16-bit transfer */
> +	consumed = ata_sff_data_xfer(dev, buf, buflen, rw);
> +	local_irq_restore(flags);
> +
> +	return consumed;
> +}

This doesn't seem like a good location for this function.  Please
rename the existing noirq function to xfer32_noriq and add generic
xfer_noirq for 16bit xfers.

Thanks.

-- 
tejun

Re: [PATCH 4/4] pata_platform: Use 16-bit wide data transfer

[Alexander Shiyan]

Sat, 23 Aug 2014 12:53:26 -0400 от Tejun Heo <tj@kernel.org>:
> Hello,
> 
> On Sat, Aug 23, 2014 at 02:46:12PM +0400, Alexander Shiyan wrote:
> > In some cases, system bus can be configured for 16-bit mode, in this
> > case using of read/write functions for the 32 bit values causes two
> > cycles of 16 bits, which is incorrect.
> 
> Shouldn't we distinguish them instead of forcing 16bit transfer on
> all?

I do not see any reason to distinguish. This embodiment can be used with
16-bit and 32-bit bus, and even for 64-bit ;)

...
> > +static unsigned int pata_platform_xfer_noirq(struct ata_device *dev,
> > +					     unsigned char *buf,
> > +					     unsigned int buflen, int rw)
> > +{
> > +	unsigned long flags;
> > +	unsigned int consumed;
> > +
> > +	local_irq_save(flags);
> > +	/* Use 16-bit transfer */
> > +	consumed = ata_sff_data_xfer(dev, buf, buflen, rw);
> > +	local_irq_restore(flags);
> > +
> > +	return consumed;
> > +}
> 
> This doesn't seem like a good location for this function.  Please
> rename the existing noirq function to xfer32_noriq and add generic
> xfer_noirq for 16bit xfers.

Ok.

---

Re: [PATCH 4/4] pata_platform: Use 16-bit wide data transfer

[Tejun Heo]

Hello,

On Sat, Aug 23, 2014 at 09:29:15PM +0400, Alexander Shiyan wrote:
> > Shouldn't we distinguish them instead of forcing 16bit transfer on
> > all?
> 
> I do not see any reason to distinguish. This embodiment can be used with
> 16-bit and 32-bit bus, and even for 64-bit ;)

Hmmm... we do care about doing 32bit when possible as there are cases
where PIO is the only data tarnsfer mode available (it's ancient at
this point but then again we're talking about pata_platform devices).
Are you saying that the controllers in question here wouldn't care
because they're all 16bit only anyway?  Otherwise, we do want to
distinguish them.

Thanks.

-- 
tejun

Re: [PATCH 4/4] pata_platform: Use 16-bit wide data transfer

[One Thousand Gnomes]

On Mon, 25 Aug 2014 15:34:54 -0400
Tejun Heo <tj@kernel.org> wrote:

> Hello,
> 
> On Sat, Aug 23, 2014 at 09:29:15PM +0400, Alexander Shiyan wrote:
> > > Shouldn't we distinguish them instead of forcing 16bit transfer on
> > > all?
> > 
> > I do not see any reason to distinguish. This embodiment can be used with
> > 16-bit and 32-bit bus, and even for 64-bit ;)
> 
> Hmmm... we do care about doing 32bit when possible as there are cases
> where PIO is the only data tarnsfer mode available 

Because some environments you get faster data transfers if you throw data
at the controller in 32bit chunks. In some cases that's because the
controller is clever about it and pipelining (eg some VLbus devices) in
others for the simple reason that the external bus cycles are either
16bit or converted that way and the CPU executes a 32bit move followed by
an add faster than 16bit/add/16bit/add.

(There are legacy memory mapped devices around where the fastest way
to read them on an older x86 is to use the FPU to do 64bit reads, but some
things should not be encouraged ;-) )


Alan

Re: [PATCH 4/4] pata_platform: Use 16-bit wide data transfer

[Alexander Shiyan]

Mon, 25 Aug 2014 20:48:17 +0100 от One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>:
> On Mon, 25 Aug 2014 15:34:54 -0400
> Tejun Heo <tj@kernel.org> wrote:
> 
> > Hello,
> > 
> > On Sat, Aug 23, 2014 at 09:29:15PM +0400, Alexander Shiyan wrote:
> > > > Shouldn't we distinguish them instead of forcing 16bit transfer on
> > > > all?
> > > 
> > > I do not see any reason to distinguish. This embodiment can be used with
> > > 16-bit and 32-bit bus, and even for 64-bit ;)
> > 
> > Hmmm... we do care about doing 32bit when possible as there are cases
> > where PIO is the only data tarnsfer mode available 
> 
> Because some environments you get faster data transfers if you throw data
> at the controller in 32bit chunks. In some cases that's because the
> controller is clever about it and pipelining (eg some VLbus devices) in
> others for the simple reason that the external bus cycles are either
> 16bit or converted that way and the CPU executes a 32bit move followed by
> an add faster than 16bit/add/16bit/add.
> 
> (There are legacy memory mapped devices around where the fastest way
> to read them on an older x86 is to use the FPU to do 64bit reads, but some
> things should not be encouraged ;-) )

I did not understand a bit of the final solution...
I hope I have clearly explained the original problem, which was written for
this patch? This is not theory.

---

Re: [PATCH 4/4] pata_platform: Use 16-bit wide data transfer

[Alexander Shiyan]

On Mon, 25 Aug 2014 20:48:17 +0100
One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:

> On Mon, 25 Aug 2014 15:34:54 -0400
> Tejun Heo <tj@kernel.org> wrote:
> 
> > Hello,
> > 
> > On Sat, Aug 23, 2014 at 09:29:15PM +0400, Alexander Shiyan wrote:
> > > > Shouldn't we distinguish them instead of forcing 16bit transfer on
> > > > all?
> > > 
> > > I do not see any reason to distinguish. This embodiment can be used with
> > > 16-bit and 32-bit bus, and even for 64-bit ;)
> > 
> > Hmmm... we do care about doing 32bit when possible as there are cases
> > where PIO is the only data tarnsfer mode available 
> 
> Because some environments you get faster data transfers if you throw data
> at the controller in 32bit chunks. In some cases that's because the
> controller is clever about it and pipelining (eg some VLbus devices) in
> others for the simple reason that the external bus cycles are either
> 16bit or converted that way and the CPU executes a 32bit move followed by
> an add faster than 16bit/add/16bit/add.
> 
> (There are legacy memory mapped devices around where the fastest way
> to read them on an older x86 is to use the FPU to do 64bit reads, but some
> things should not be encouraged ;-) )

Seems my response to this message was lost in space ...
Just to repeat:

I did not understand a bit of the final solution...
I hope I have clearly explained the original problem, which was written for
this patch? This is not theory.

-- 
Alexander Shiyan <shc_work@mail.ru>

Re: [PATCH 4/4] pata_platform: Use 16-bit wide data transfer

[Tejun Heo]

Hello, Alexander.

On Thu, Sep 18, 2014 at 08:40:20PM +0400, Alexander Shiyan wrote:
> I did not understand a bit of the final solution...
> I hope I have clearly explained the original problem, which was written for
> this patch? This is not theory.

The solution is implementing separate 16 and 32 bit operations and
apply 16 bit ones only to the controllers which require 16bit
operation for correctness.

Thanks.

-- 
tejun

Re: [PATCH 1/4] pata_of_platform: Remove "electra-ide" quirk

[Tejun Heo]

On Sat, Aug 23, 2014 at 02:46:10PM +0400, Alexander Shiyan wrote:
> "electra-ide" is not used anywhere in the kernel and could be
> represented in devicetree in a normal way.
> This patch removes specific quirk for "electra-ide".
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>

Applied to libata/for-3.18.

Thanks.

-- 
tejun