[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 <***@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;

[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 <***@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,
};

[Tejun Heo]

Hello,

Post by 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.

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

Post by Alexander Shiyan
The patch provides its own function to use the proper 16-bit mode.
---
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.

[Alexander Shiyan]

Post by Tejun Heo
Hello,

Post by 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.

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 ;)

...

Post by Tejun Heo

Post by Alexander Shiyan
+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.

---

��{.n�+�������+%��lzwm��b�맲��r��zX���z)����w*jg��������ݢj/���z�ޖ��

[Tejun Heo]

Hello,

Post by Alexander Shiyan

Post by Tejun Heo
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.

[One Thousand Gnomes]

On Mon, 25 Aug 2014 15:34:54 -0400

Post by Tejun Heo
Hello,

Post by Alexander Shiyan

Post by Tejun Heo
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


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alexander Shiyan]

Post by One Thousand Gnomes
On Mon, 25 Aug 2014 15:34:54 -0400

Post by Tejun Heo
Hello,

Post by Alexander Shiyan

Post by Tejun Heo
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.

---

��칻�&�~�&���+-��ݶ��w��˛���m�b��bu觶��ܨ}���Ơz�&j:+v�������zZ+

[Alexander Shiyan]

On Mon, 25 Aug 2014 20:48:17 +0100

Post by One Thousand Gnomes
On Mon, 25 Aug 2014 15:34:54 -0400

Post by Tejun Heo
Hello,

Post by Alexander Shiyan

Post by Tejun Heo
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.

[Tejun Heo]

Hello, Alexander.

Post by Alexander Shiyan
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 Heo]

Post by Alexander Shiyan
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 Heo]

"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".

Applied to libata/for-3.18.

Thanks.