* [PATCH 0/4] pata_parport: fix EXP Computer CD-865 with MC-1285B EPP cable
@ 2023-09-30 19:15 Ondrej Zary
2023-09-30 19:15 ` [PATCH 1/4] pata_parport: fix pata_parport_devchk Ondrej Zary
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Ondrej Zary @ 2023-09-30 19:15 UTC (permalink / raw
To: Damien Le Moal, Sudip Mukherjee
Cc: Christoph Hellwig, Sergey Shtylyov, Jens Axboe, Tim Waugh,
linux-block, linux-parport, linux-ide, linux-kernel
Two bugfixes, one workaround and IDE command set registers support for fit3
driver. All needed for EXP Computer CD-865 drive with MC-1285B EPP cable to
work.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/4] pata_parport: fix pata_parport_devchk
2023-09-30 19:15 [PATCH 0/4] pata_parport: fix EXP Computer CD-865 with MC-1285B EPP cable Ondrej Zary
@ 2023-09-30 19:15 ` Ondrej Zary
2023-10-02 18:43 ` Sergey Shtylyov
2023-09-30 19:15 ` [PATCH 2/4] pata_parport: implement set_devctl Ondrej Zary
` (3 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Ondrej Zary @ 2023-09-30 19:15 UTC (permalink / raw
To: Damien Le Moal, Sudip Mukherjee
Cc: Christoph Hellwig, Sergey Shtylyov, Jens Axboe, Tim Waugh,
linux-block, linux-parport, linux-ide, linux-kernel
There's a 'x' missing in 0x55 in pata_parport_devchk(), causing the
detection to always fail. Fix it.
Signed-off-by: Ondrej Zary <linux@zary.sk>
---
drivers/ata/pata_parport/pata_parport.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
index 1af64d435d3c..258d189f42e5 100644
--- a/drivers/ata/pata_parport/pata_parport.c
+++ b/drivers/ata/pata_parport/pata_parport.c
@@ -64,7 +64,7 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0xaa);
pi->proto->write_regr(pi, 0, ATA_REG_LBAL, 0x55);
- pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 055);
+ pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0x55);
pi->proto->write_regr(pi, 0, ATA_REG_LBAL, 0xaa);
nsect = pi->proto->read_regr(pi, 0, ATA_REG_NSECT);
--
Ondrej Zary
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/4] pata_parport: implement set_devctl
2023-09-30 19:15 [PATCH 0/4] pata_parport: fix EXP Computer CD-865 with MC-1285B EPP cable Ondrej Zary
2023-09-30 19:15 ` [PATCH 1/4] pata_parport: fix pata_parport_devchk Ondrej Zary
@ 2023-09-30 19:15 ` Ondrej Zary
2023-10-02 19:52 ` Sergey Shtylyov
2023-09-30 19:15 ` [PATCH 3/4] pata_parport: add custom version of wait_after_reset Ondrej Zary
` (2 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Ondrej Zary @ 2023-09-30 19:15 UTC (permalink / raw
To: Damien Le Moal, Sudip Mukherjee
Cc: Christoph Hellwig, Sergey Shtylyov, Jens Axboe, Tim Waugh,
linux-block, linux-parport, linux-ide, linux-kernel
Add missing ops->sff_set_devctl implementation.
Signed-off-by: Ondrej Zary <linux@zary.sk>
---
drivers/ata/pata_parport/pata_parport.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
index 258d189f42e5..cf87bbb52f1f 100644
--- a/drivers/ata/pata_parport/pata_parport.c
+++ b/drivers/ata/pata_parport/pata_parport.c
@@ -51,6 +51,13 @@ static void pata_parport_dev_select(struct ata_port *ap, unsigned int device)
ata_sff_pause(ap);
}
+static void pata_parport_set_devctl(struct ata_port *ap, u8 ctl)
+{
+ struct pi_adapter *pi = ap->host->private_data;
+
+ pi->proto->write_regr(pi, 1, 6, ctl);
+}
+
static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
{
struct pi_adapter *pi = ap->host->private_data;
@@ -252,6 +259,7 @@ static struct ata_port_operations pata_parport_port_ops = {
.hardreset = NULL,
.sff_dev_select = pata_parport_dev_select,
+ .sff_set_devctl = pata_parport_set_devctl,
.sff_check_status = pata_parport_check_status,
.sff_check_altstatus = pata_parport_check_altstatus,
.sff_tf_load = pata_parport_tf_load,
--
Ondrej Zary
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/4] pata_parport: add custom version of wait_after_reset
2023-09-30 19:15 [PATCH 0/4] pata_parport: fix EXP Computer CD-865 with MC-1285B EPP cable Ondrej Zary
2023-09-30 19:15 ` [PATCH 1/4] pata_parport: fix pata_parport_devchk Ondrej Zary
2023-09-30 19:15 ` [PATCH 2/4] pata_parport: implement set_devctl Ondrej Zary
@ 2023-09-30 19:15 ` Ondrej Zary
2023-10-02 20:48 ` Sergey Shtylyov
2023-10-03 0:55 ` Damien Le Moal
2023-09-30 19:15 ` [PATCH 4/4] pata_parport-fit3: implement IDE command set registers Ondrej Zary
2023-10-03 0:34 ` [PATCH 0/4] pata_parport: fix EXP Computer CD-865 with MC-1285B EPP cable Damien Le Moal
4 siblings, 2 replies; 19+ messages in thread
From: Ondrej Zary @ 2023-09-30 19:15 UTC (permalink / raw
To: Damien Le Moal, Sudip Mukherjee
Cc: Christoph Hellwig, Sergey Shtylyov, Jens Axboe, Tim Waugh,
linux-block, linux-parport, linux-ide, linux-kernel
Some parallel adapters (e.g. EXP Computer MC-1285B EPP Cable) return
bogus values when there's no master device present. This can cause
reset to fail, preventing the lone slave device (such as EXP Computer
CD-865) from working.
Add custom version of wait_after_reset that ignores master failure when
a slave device is present. The custom version is also needed because
the generic ata_sff_wait_after_reset uses direct port I/O for slave
device detection.
Signed-off-by: Ondrej Zary <linux@zary.sk>
---
drivers/ata/pata_parport/pata_parport.c | 65 ++++++++++++++++++++++++-
1 file changed, 64 insertions(+), 1 deletion(-)
diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
index cf87bbb52f1f..b3db953e615a 100644
--- a/drivers/ata/pata_parport/pata_parport.c
+++ b/drivers/ata/pata_parport/pata_parport.c
@@ -80,6 +80,69 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
return (nsect == 0x55) && (lbal == 0xaa);
}
+static int pata_parport_wait_after_reset(struct ata_link *link,
+ unsigned int devmask,
+ unsigned long deadline)
+{
+ struct ata_port *ap = link->ap;
+ struct pi_adapter *pi = ap->host->private_data;
+ unsigned int dev0 = devmask & (1 << 0);
+ unsigned int dev1 = devmask & (1 << 1);
+ int rc, ret = 0;
+
+ ata_msleep(ap, ATA_WAIT_AFTER_RESET);
+
+ /* always check readiness of the master device */
+ rc = ata_sff_wait_ready(link, deadline);
+ /* some adapters return bogus values if master device is not present,
+ * so don't abort now if a slave device is present
+ */
+ if (rc) {
+ if (!dev1)
+ return rc;
+ ret = -ENODEV;
+ }
+
+ /* if device 1 was found in ata_devchk, wait for register
+ * access briefly, then wait for BSY to clear.
+ */
+ if (dev1) {
+ int i;
+
+ pata_parport_dev_select(ap, 1);
+
+ /* Wait for register access. Some ATAPI devices fail
+ * to set nsect/lbal after reset, so don't waste too
+ * much time on it. We're gonna wait for !BSY anyway.
+ */
+ for (i = 0; i < 2; i++) {
+ u8 nsect, lbal;
+
+ nsect = pi->proto->read_regr(pi, 0, ATA_REG_NSECT);
+ lbal = pi->proto->read_regr(pi, 0, ATA_REG_LBAL);
+ if ((nsect == 1) && (lbal == 1))
+ break;
+ ata_msleep(ap, 50); /* give drive a breather */
+ }
+
+ rc = ata_sff_wait_ready(link, deadline);
+ if (rc) {
+ if (rc != -ENODEV)
+ return rc;
+ ret = rc;
+ }
+ }
+
+ /* is all this really necessary? */
+ pata_parport_dev_select(ap, 0);
+ if (dev1)
+ pata_parport_dev_select(ap, 1);
+ if (dev0)
+ pata_parport_dev_select(ap, 0);
+
+ return ret;
+}
+
static int pata_parport_bus_softreset(struct ata_port *ap, unsigned int devmask,
unsigned long deadline)
{
@@ -94,7 +157,7 @@ static int pata_parport_bus_softreset(struct ata_port *ap, unsigned int devmask,
ap->last_ctl = ap->ctl;
/* wait the port to become ready */
- return ata_sff_wait_after_reset(&ap->link, devmask, deadline);
+ return pata_parport_wait_after_reset(&ap->link, devmask, deadline);
}
static int pata_parport_softreset(struct ata_link *link, unsigned int *classes,
--
Ondrej Zary
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/4] pata_parport-fit3: implement IDE command set registers
2023-09-30 19:15 [PATCH 0/4] pata_parport: fix EXP Computer CD-865 with MC-1285B EPP cable Ondrej Zary
` (2 preceding siblings ...)
2023-09-30 19:15 ` [PATCH 3/4] pata_parport: add custom version of wait_after_reset Ondrej Zary
@ 2023-09-30 19:15 ` Ondrej Zary
2023-10-02 20:06 ` Sergey Shtylyov
2023-10-03 0:56 ` Damien Le Moal
2023-10-03 0:34 ` [PATCH 0/4] pata_parport: fix EXP Computer CD-865 with MC-1285B EPP cable Damien Le Moal
4 siblings, 2 replies; 19+ messages in thread
From: Ondrej Zary @ 2023-09-30 19:15 UTC (permalink / raw
To: Damien Le Moal, Sudip Mukherjee
Cc: Christoph Hellwig, Sergey Shtylyov, Jens Axboe, Tim Waugh,
linux-block, linux-parport, linux-ide, linux-kernel
fit3 protocol driver does not support accessing IDE control registers
(device control/altstatus). The DOS driver does not use these registers
either (as observed from DOSEMU trace). But the HW seems to be capable
of accessing these registers - I simply tried bit 3 and it works!
The control register is required to properly reset ATAPI devices or
they will be detected only once (after a power cycle).
Tested with EXP Computer CD-865 with MC-1285B EPP cable and
TransDisk 3000.
Signed-off-by: Ondrej Zary <linux@zary.sk>
---
drivers/ata/pata_parport/fit3.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/ata/pata_parport/fit3.c b/drivers/ata/pata_parport/fit3.c
index bad7aa920cdc..86b39966755b 100644
--- a/drivers/ata/pata_parport/fit3.c
+++ b/drivers/ata/pata_parport/fit3.c
@@ -9,11 +9,6 @@
*
* The TD-2000 and certain older devices use a different protocol.
* Try the fit2 protocol module with them.
- *
- * NB: The FIT adapters do not appear to support the control
- * registers. So, we map ALT_STATUS to STATUS and NO-OP writes
- * to the device control register - this means that IDE reset
- * will not work on these devices.
*/
#include <linux/module.h>
@@ -35,10 +30,11 @@
* cont = 1 - access the IDE command set
*/
+static int cont_map[] = { 0x00, 0x08 };
+
static void fit3_write_regr(struct pi_adapter *pi, int cont, int regr, int val)
{
- if (cont == 1)
- return;
+ regr += cont_map[cont];
switch (pi->mode) {
case 0:
@@ -59,11 +55,7 @@ static int fit3_read_regr(struct pi_adapter *pi, int cont, int regr)
{
int a, b;
- if (cont) {
- if (regr != 6)
- return 0xff;
- regr = 7;
- }
+ regr += cont_map[cont];
switch (pi->mode) {
case 0:
--
Ondrej Zary
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] pata_parport: fix pata_parport_devchk
2023-09-30 19:15 ` [PATCH 1/4] pata_parport: fix pata_parport_devchk Ondrej Zary
@ 2023-10-02 18:43 ` Sergey Shtylyov
2023-10-02 19:08 ` Sergey Shtylyov
2023-10-03 17:07 ` Ondrej Zary
0 siblings, 2 replies; 19+ messages in thread
From: Sergey Shtylyov @ 2023-10-02 18:43 UTC (permalink / raw
To: Ondrej Zary, Damien Le Moal, Sudip Mukherjee
Cc: Christoph Hellwig, Jens Axboe, Tim Waugh, linux-block,
linux-parport, linux-ide, linux-kernel
Hello!
On 9/30/23 10:15 PM, Ondrej Zary wrote:
> There's a 'x' missing in 0x55 in pata_parport_devchk(), causing the
> detection to always fail. Fix it.
>
> Signed-off-by: Ondrej Zary <linux@zary.sk>
I think we need a Fixes: tag here...
> ---
> drivers/ata/pata_parport/pata_parport.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> index 1af64d435d3c..258d189f42e5 100644
> --- a/drivers/ata/pata_parport/pata_parport.c
> +++ b/drivers/ata/pata_parport/pata_parport.c
> @@ -64,7 +64,7 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
> pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0xaa);
> pi->proto->write_regr(pi, 0, ATA_REG_LBAL, 0x55);
>
> - pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 055);
> + pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0x55);
Oh, Gawd! How did this ever work?! :-/
This bug seems to predate the Big PARIDE move...
> pi->proto->write_regr(pi, 0, ATA_REG_LBAL, 0xaa);
>
> nsect = pi->proto->read_regr(pi, 0, ATA_REG_NSECT);
>
MBR, Sergey
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] pata_parport: fix pata_parport_devchk
2023-10-02 18:43 ` Sergey Shtylyov
@ 2023-10-02 19:08 ` Sergey Shtylyov
2023-10-03 17:07 ` Ondrej Zary
1 sibling, 0 replies; 19+ messages in thread
From: Sergey Shtylyov @ 2023-10-02 19:08 UTC (permalink / raw
To: Ondrej Zary, Damien Le Moal, Sudip Mukherjee
Cc: Christoph Hellwig, Jens Axboe, Tim Waugh, linux-block,
linux-parport, linux-ide, linux-kernel
On 10/2/23 9:43 PM, Sergey Shtylyov wrote:
[...]
>> There's a 'x' missing in 0x55 in pata_parport_devchk(), causing the
>> detection to always fail. Fix it.
>>
>> Signed-off-by: Ondrej Zary <linux@zary.sk>
>
> I think we need a Fixes: tag here...
Fixes: 246a1c4c6b7f ("ata: pata_parport: add driver (PARIDE replacement)")
[...]
MBR, Sergey
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] pata_parport: implement set_devctl
2023-09-30 19:15 ` [PATCH 2/4] pata_parport: implement set_devctl Ondrej Zary
@ 2023-10-02 19:52 ` Sergey Shtylyov
0 siblings, 0 replies; 19+ messages in thread
From: Sergey Shtylyov @ 2023-10-02 19:52 UTC (permalink / raw
To: Ondrej Zary, Damien Le Moal, Sudip Mukherjee
Cc: Christoph Hellwig, Jens Axboe, Tim Waugh, linux-block,
linux-parport, linux-ide, linux-kernel
On 9/30/23 10:15 PM, Ondrej Zary wrote:
> Add missing ops->sff_set_devctl implementation.
This is also a bug fix, right?
> Signed-off-by: Ondrej Zary <linux@zary.sk>
Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
Fixes: 246a1c4c6b7f ("ata: pata_parport: add driver (PARIDE replacement)")
[...]
MBR, Sergey
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] pata_parport-fit3: implement IDE command set registers
2023-09-30 19:15 ` [PATCH 4/4] pata_parport-fit3: implement IDE command set registers Ondrej Zary
@ 2023-10-02 20:06 ` Sergey Shtylyov
2023-10-03 17:13 ` Ondrej Zary
2023-10-03 0:56 ` Damien Le Moal
1 sibling, 1 reply; 19+ messages in thread
From: Sergey Shtylyov @ 2023-10-02 20:06 UTC (permalink / raw
To: Ondrej Zary, Damien Le Moal, Sudip Mukherjee
Cc: Christoph Hellwig, Jens Axboe, Tim Waugh, linux-block,
linux-parport, linux-ide, linux-kernel
On 9/30/23 10:15 PM, Ondrej Zary wrote:
> fit3 protocol driver does not support accessing IDE control registers
> (device control/altstatus). The DOS driver does not use these registers
> either (as observed from DOSEMU trace). But the HW seems to be capable
> of accessing these registers - I simply tried bit 3 and it works!
>
> The control register is required to properly reset ATAPI devices or
> they will be detected only once (after a power cycle).
>
> Tested with EXP Computer CD-865 with MC-1285B EPP cable and
> TransDisk 3000.
>
> Signed-off-by: Ondrej Zary <linux@zary.sk>
> ---
> drivers/ata/pata_parport/fit3.c | 16 ++++------------
> 1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/ata/pata_parport/fit3.c b/drivers/ata/pata_parport/fit3.c
> index bad7aa920cdc..86b39966755b 100644
> --- a/drivers/ata/pata_parport/fit3.c
> +++ b/drivers/ata/pata_parport/fit3.c
[...]
> @@ -35,10 +30,11 @@
> * cont = 1 - access the IDE command set
> */
>
> +static int cont_map[] = { 0x00, 0x08 };
const?
> +
> static void fit3_write_regr(struct pi_adapter *pi, int cont, int regr, int val)
> {
> - if (cont == 1)
> - return;
> + regr += cont_map[cont];
Perhaps regr += cont << 3 instead?
>
> switch (pi->mode) {
> case 0:
> @@ -59,11 +55,7 @@ static int fit3_read_regr(struct pi_adapter *pi, int cont, int regr)
> {
> int a, b;
>
> - if (cont) {
> - if (regr != 6)
> - return 0xff;
> - regr = 7;
> - }
> + regr += cont_map[cont];
Likewise here?
[...]
MBR, Sergey
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] pata_parport: add custom version of wait_after_reset
2023-09-30 19:15 ` [PATCH 3/4] pata_parport: add custom version of wait_after_reset Ondrej Zary
@ 2023-10-02 20:48 ` Sergey Shtylyov
2023-10-03 17:20 ` Ondrej Zary
2023-10-03 0:55 ` Damien Le Moal
1 sibling, 1 reply; 19+ messages in thread
From: Sergey Shtylyov @ 2023-10-02 20:48 UTC (permalink / raw
To: Ondrej Zary, Damien Le Moal, Sudip Mukherjee
Cc: Christoph Hellwig, Jens Axboe, Tim Waugh, linux-block,
linux-parport, linux-ide, linux-kernel
On 9/30/23 10:15 PM, Ondrej Zary wrote:
> Some parallel adapters (e.g. EXP Computer MC-1285B EPP Cable) return
> bogus values when there's no master device present. This can cause
> reset to fail, preventing the lone slave device (such as EXP Computer
> CD-865) from working.
>
> Add custom version of wait_after_reset that ignores master failure when
> a slave device is present. The custom version is also needed because
> the generic ata_sff_wait_after_reset uses direct port I/O for slave
> device detection.
>
> Signed-off-by: Ondrej Zary <linux@zary.sk>
> ---
> drivers/ata/pata_parport/pata_parport.c | 65 ++++++++++++++++++++++++-
> 1 file changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> index cf87bbb52f1f..b3db953e615a 100644
> --- a/drivers/ata/pata_parport/pata_parport.c
> +++ b/drivers/ata/pata_parport/pata_parport.c
> @@ -80,6 +80,69 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
> return (nsect == 0x55) && (lbal == 0xaa);
> }
>
> +static int pata_parport_wait_after_reset(struct ata_link *link,
> + unsigned int devmask,
> + unsigned long deadline)
> +{
> + struct ata_port *ap = link->ap;
> + struct pi_adapter *pi = ap->host->private_data;
> + unsigned int dev0 = devmask & (1 << 0);
> + unsigned int dev1 = devmask & (1 << 1);
> + int rc, ret = 0;
> +
> + ata_msleep(ap, ATA_WAIT_AFTER_RESET);
> +
> + /* always check readiness of the master device */
> + rc = ata_sff_wait_ready(link, deadline);
> + /* some adapters return bogus values if master device is not present,
The multiline comments should start with /* on its own line.
Have you run scripts/checkpatch.pl on the patches?
> + * so don't abort now if a slave device is present
> + */
> + if (rc) {
> + if (!dev1)
> + return rc;
> + ret = -ENODEV;
> + }
> +
> + /* if device 1 was found in ata_devchk, wait for register
Likewise here...
> + * access briefly, then wait for BSY to clear.
> + */
> + if (dev1) {
> + int i;
> +
> + pata_parport_dev_select(ap, 1);
> +
> + /* Wait for register access. Some ATAPI devices fail
And here...
[...]
MBR, Sergey
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] pata_parport: fix EXP Computer CD-865 with MC-1285B EPP cable
2023-09-30 19:15 [PATCH 0/4] pata_parport: fix EXP Computer CD-865 with MC-1285B EPP cable Ondrej Zary
` (3 preceding siblings ...)
2023-09-30 19:15 ` [PATCH 4/4] pata_parport-fit3: implement IDE command set registers Ondrej Zary
@ 2023-10-03 0:34 ` Damien Le Moal
4 siblings, 0 replies; 19+ messages in thread
From: Damien Le Moal @ 2023-10-03 0:34 UTC (permalink / raw
To: Ondrej Zary, Sudip Mukherjee
Cc: Christoph Hellwig, Sergey Shtylyov, Jens Axboe, Tim Waugh,
linux-parport, linux-ide, linux-kernel
On 10/1/23 04:15, Ondrej Zary wrote:
>
> Two bugfixes, one workaround and IDE command set registers support for fit3
> driver. All needed for EXP Computer CD-865 drive with MC-1285B EPP cable to
> work.
>
Please use my email address listed by get_maintainer.pl: dlemoal@kernel.org
And these patches do not involve linux-block and Jens, so no need to send there
(that reduces noise on the block list).
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] pata_parport: add custom version of wait_after_reset
2023-09-30 19:15 ` [PATCH 3/4] pata_parport: add custom version of wait_after_reset Ondrej Zary
2023-10-02 20:48 ` Sergey Shtylyov
@ 2023-10-03 0:55 ` Damien Le Moal
2023-10-03 16:55 ` Sergey Shtylyov
1 sibling, 1 reply; 19+ messages in thread
From: Damien Le Moal @ 2023-10-03 0:55 UTC (permalink / raw
To: Ondrej Zary, Sudip Mukherjee
Cc: Christoph Hellwig, Sergey Shtylyov, Tim Waugh, linux-parport,
linux-ide, linux-kernel
On 10/1/23 04:15, Ondrej Zary wrote:
> Some parallel adapters (e.g. EXP Computer MC-1285B EPP Cable) return
> bogus values when there's no master device present. This can cause
> reset to fail, preventing the lone slave device (such as EXP Computer
> CD-865) from working.
>
> Add custom version of wait_after_reset that ignores master failure when
> a slave device is present. The custom version is also needed because
> the generic ata_sff_wait_after_reset uses direct port I/O for slave
> device detection.
>
> Signed-off-by: Ondrej Zary <linux@zary.sk>
> ---
> drivers/ata/pata_parport/pata_parport.c | 65 ++++++++++++++++++++++++-
> 1 file changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> index cf87bbb52f1f..b3db953e615a 100644
> --- a/drivers/ata/pata_parport/pata_parport.c
> +++ b/drivers/ata/pata_parport/pata_parport.c
> @@ -80,6 +80,69 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
> return (nsect == 0x55) && (lbal == 0xaa);
> }
>
> +static int pata_parport_wait_after_reset(struct ata_link *link,
> + unsigned int devmask,
> + unsigned long deadline)
> +{
> + struct ata_port *ap = link->ap;
> + struct pi_adapter *pi = ap->host->private_data;
> + unsigned int dev0 = devmask & (1 << 0);
> + unsigned int dev1 = devmask & (1 << 1);
> + int rc, ret = 0;
> +
> + ata_msleep(ap, ATA_WAIT_AFTER_RESET);
> +
> + /* always check readiness of the master device */
> + rc = ata_sff_wait_ready(link, deadline);
> + /* some adapters return bogus values if master device is not present,
> + * so don't abort now if a slave device is present
> + */
In addition to Sergey's comment, please move this comment inside the "if", or
even better, merge it with the otherwise not very useful "always check
readiness..." comment.
> + if (rc) {
> + if (!dev1)
> + return rc;
> + ret = -ENODEV;
> + }
> +
> + /* if device 1 was found in ata_devchk, wait for register
> + * access briefly, then wait for BSY to clear.
> + */
> + if (dev1) {
> + int i;
> +
> + pata_parport_dev_select(ap, 1);
> +
> + /* Wait for register access. Some ATAPI devices fail
> + * to set nsect/lbal after reset, so don't waste too
> + * much time on it. We're gonna wait for !BSY anyway.
> + */
> + for (i = 0; i < 2; i++) {
> + u8 nsect, lbal;
> +
> + nsect = pi->proto->read_regr(pi, 0, ATA_REG_NSECT);
> + lbal = pi->proto->read_regr(pi, 0, ATA_REG_LBAL);
> + if ((nsect == 1) && (lbal == 1))
> + break;
> + ata_msleep(ap, 50); /* give drive a breather */
Please move the comment on its own line above the sleep call.
> + }
> +
> + rc = ata_sff_wait_ready(link, deadline);
> + if (rc) {
> + if (rc != -ENODEV)
> + return rc;
> + ret = rc;
> + }
> + }
> +
> + /* is all this really necessary? */
I don't know. It is your driver... So either drop this comment, or clearly
explain why this is done.
> + pata_parport_dev_select(ap, 0);
> + if (dev1)
> + pata_parport_dev_select(ap, 1);
> + if (dev0)
> + pata_parport_dev_select(ap, 0);
Can you have dev1 && dev0 == true ? This seems like the second if should be an
"else if", but it is not clear what this is doing.
> +
> + return ret;
> +}
> +
> static int pata_parport_bus_softreset(struct ata_port *ap, unsigned int devmask,
> unsigned long deadline)
> {
> @@ -94,7 +157,7 @@ static int pata_parport_bus_softreset(struct ata_port *ap, unsigned int devmask,
> ap->last_ctl = ap->ctl;
>
> /* wait the port to become ready */
> - return ata_sff_wait_after_reset(&ap->link, devmask, deadline);
> + return pata_parport_wait_after_reset(&ap->link, devmask, deadline);
> }
>
> static int pata_parport_softreset(struct ata_link *link, unsigned int *classes,
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] pata_parport-fit3: implement IDE command set registers
2023-09-30 19:15 ` [PATCH 4/4] pata_parport-fit3: implement IDE command set registers Ondrej Zary
2023-10-02 20:06 ` Sergey Shtylyov
@ 2023-10-03 0:56 ` Damien Le Moal
1 sibling, 0 replies; 19+ messages in thread
From: Damien Le Moal @ 2023-10-03 0:56 UTC (permalink / raw
To: Ondrej Zary, Damien Le Moal, Sudip Mukherjee
Cc: Christoph Hellwig, Sergey Shtylyov, Jens Axboe, Tim Waugh,
linux-block, linux-parport, linux-ide, linux-kernel
On 10/1/23 04:15, Ondrej Zary wrote:
> fit3 protocol driver does not support accessing IDE control registers
> (device control/altstatus). The DOS driver does not use these registers
> either (as observed from DOSEMU trace). But the HW seems to be capable
> of accessing these registers - I simply tried bit 3 and it works!
>
> The control register is required to properly reset ATAPI devices or
> they will be detected only once (after a power cycle).
>
> Tested with EXP Computer CD-865 with MC-1285B EPP cable and
> TransDisk 3000.
>
> Signed-off-by: Ondrej Zary <linux@zary.sk>
For all patches of this series: please change the commit title to:
ata: pata_parport: xxx
> ---
> drivers/ata/pata_parport/fit3.c | 16 ++++------------
> 1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/ata/pata_parport/fit3.c b/drivers/ata/pata_parport/fit3.c
> index bad7aa920cdc..86b39966755b 100644
> --- a/drivers/ata/pata_parport/fit3.c
> +++ b/drivers/ata/pata_parport/fit3.c
> @@ -9,11 +9,6 @@
> *
> * The TD-2000 and certain older devices use a different protocol.
> * Try the fit2 protocol module with them.
> - *
> - * NB: The FIT adapters do not appear to support the control
> - * registers. So, we map ALT_STATUS to STATUS and NO-OP writes
> - * to the device control register - this means that IDE reset
> - * will not work on these devices.
> */
>
> #include <linux/module.h>
> @@ -35,10 +30,11 @@
> * cont = 1 - access the IDE command set
> */
>
> +static int cont_map[] = { 0x00, 0x08 };
> +
> static void fit3_write_regr(struct pi_adapter *pi, int cont, int regr, int val)
> {
> - if (cont == 1)
> - return;
> + regr += cont_map[cont];
>
> switch (pi->mode) {
> case 0:
> @@ -59,11 +55,7 @@ static int fit3_read_regr(struct pi_adapter *pi, int cont, int regr)
> {
> int a, b;
>
> - if (cont) {
> - if (regr != 6)
> - return 0xff;
> - regr = 7;
> - }
> + regr += cont_map[cont];
>
> switch (pi->mode) {
> case 0:
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] pata_parport: add custom version of wait_after_reset
2023-10-03 0:55 ` Damien Le Moal
@ 2023-10-03 16:55 ` Sergey Shtylyov
2023-10-03 23:44 ` Damien Le Moal
0 siblings, 1 reply; 19+ messages in thread
From: Sergey Shtylyov @ 2023-10-03 16:55 UTC (permalink / raw
To: Damien Le Moal, Ondrej Zary, Sudip Mukherjee
Cc: Christoph Hellwig, Tim Waugh, linux-parport, linux-ide,
linux-kernel
On 10/3/23 3:55 AM, Damien Le Moal wrote:
[...]
>> Some parallel adapters (e.g. EXP Computer MC-1285B EPP Cable) return
>> bogus values when there's no master device present. This can cause
>> reset to fail, preventing the lone slave device (such as EXP Computer
>> CD-865) from working.
>>
>> Add custom version of wait_after_reset that ignores master failure when
>> a slave device is present. The custom version is also needed because
>> the generic ata_sff_wait_after_reset uses direct port I/O for slave
>> device detection.
>>
>> Signed-off-by: Ondrej Zary <linux@zary.sk>
>> ---
>> drivers/ata/pata_parport/pata_parport.c | 65 ++++++++++++++++++++++++-
>> 1 file changed, 64 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
>> index cf87bbb52f1f..b3db953e615a 100644
>> --- a/drivers/ata/pata_parport/pata_parport.c
>> +++ b/drivers/ata/pata_parport/pata_parport.c
>> @@ -80,6 +80,69 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
>> return (nsect == 0x55) && (lbal == 0xaa);
>> }
>>
>> +static int pata_parport_wait_after_reset(struct ata_link *link,
>> + unsigned int devmask,
>> + unsigned long deadline)
>> +{
>> + struct ata_port *ap = link->ap;
>> + struct pi_adapter *pi = ap->host->private_data;
>> + unsigned int dev0 = devmask & (1 << 0);
>> + unsigned int dev1 = devmask & (1 << 1);
>> + int rc, ret = 0;
>> +
>> + ata_msleep(ap, ATA_WAIT_AFTER_RESET);
>> +
>> + /* always check readiness of the master device */
>> + rc = ata_sff_wait_ready(link, deadline);
>> + /* some adapters return bogus values if master device is not present,
>> + * so don't abort now if a slave device is present
>> + */
>
> In addition to Sergey's comment, please move this comment inside the "if", or
> even better, merge it with the otherwise not very useful "always check
> readiness..." comment.
That comment was copied from ata_sff_wait_after_reset(), I think...
[...]
>> + /* if device 1 was found in ata_devchk, wait for register
>> + * access briefly, then wait for BSY to clear.
>> + */
>> + if (dev1) {
>> + int i;
>> +
>> + pata_parport_dev_select(ap, 1);
>> +
>> + /* Wait for register access. Some ATAPI devices fail
>> + * to set nsect/lbal after reset, so don't waste too
>> + * much time on it. We're gonna wait for !BSY anyway.
>> + */
>> + for (i = 0; i < 2; i++) {
>> + u8 nsect, lbal;
>> +
>> + nsect = pi->proto->read_regr(pi, 0, ATA_REG_NSECT);
>> + lbal = pi->proto->read_regr(pi, 0, ATA_REG_LBAL);
>> + if ((nsect == 1) && (lbal == 1))
>> + break;
>> + ata_msleep(ap, 50); /* give drive a breather */
>
> Please move the comment on its own line above the sleep call.
Again, copied verbatim from ata_sff_wait_after_reset()...
>> + }
>> +
>> + rc = ata_sff_wait_ready(link, deadline);
>> + if (rc) {
>> + if (rc != -ENODEV)
>> + return rc;
>> + ret = rc;
>> + }
>> + }
>> +
>> + /* is all this really necessary? */
>
> I don't know. It is your driver... So either drop this comment, or clearly
> explain why this is done.
And again, copied verbatim from ata_sff_wait_after_reset()...
>> + pata_parport_dev_select(ap, 0);
>> + if (dev1)
>> + pata_parport_dev_select(ap, 1);
>> + if (dev0)
>> + pata_parport_dev_select(ap, 0);
>
> Can you have dev1 && dev0 == true ? This seems like the second if should be an
> "else if", but it is not clear what this is doing.
I guess this tries to leave the valid taskfile regs readable on a channel, instead
of just 0xff...
[...]
MBR, Sergey
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] pata_parport: fix pata_parport_devchk
2023-10-02 18:43 ` Sergey Shtylyov
2023-10-02 19:08 ` Sergey Shtylyov
@ 2023-10-03 17:07 ` Ondrej Zary
2023-10-03 17:18 ` Sergei Shtylyov
1 sibling, 1 reply; 19+ messages in thread
From: Ondrej Zary @ 2023-10-03 17:07 UTC (permalink / raw
To: Sergey Shtylyov
Cc: Damien Le Moal, Sudip Mukherjee, Christoph Hellwig, Tim Waugh,
linux-parport, linux-ide, linux-kernel
On Monday 02 October 2023 20:43:09 Sergey Shtylyov wrote:
> Hello!
>
> On 9/30/23 10:15 PM, Ondrej Zary wrote:
>
> > There's a 'x' missing in 0x55 in pata_parport_devchk(), causing the
> > detection to always fail. Fix it.
> >
> > Signed-off-by: Ondrej Zary <linux@zary.sk>
>
> I think we need a Fixes: tag here...
>
> > ---
> > drivers/ata/pata_parport/pata_parport.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> > index 1af64d435d3c..258d189f42e5 100644
> > --- a/drivers/ata/pata_parport/pata_parport.c
> > +++ b/drivers/ata/pata_parport/pata_parport.c
> > @@ -64,7 +64,7 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
> > pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0xaa);
> > pi->proto->write_regr(pi, 0, ATA_REG_LBAL, 0x55);
> >
> > - pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 055);
> > + pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0x55);
>
> Oh, Gawd! How did this ever work?! :-/
> This bug seems to predate the Big PARIDE move...
This code was not present in PARIDE - it's my bug.
The function is a clone of ata_devchk() without direct port access.
It's called only from softreset so nobody notices the breakage until something goes wrong. The CD-865 drive needs a reset to start working.
--
Ondrej Zary
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] pata_parport-fit3: implement IDE command set registers
2023-10-02 20:06 ` Sergey Shtylyov
@ 2023-10-03 17:13 ` Ondrej Zary
0 siblings, 0 replies; 19+ messages in thread
From: Ondrej Zary @ 2023-10-03 17:13 UTC (permalink / raw
To: Sergey Shtylyov
Cc: Damien Le Moal, Sudip Mukherjee, Christoph Hellwig, Tim Waugh,
linux-parport, linux-ide, linux-kernel
On Monday 02 October 2023 22:06:52 Sergey Shtylyov wrote:
> On 9/30/23 10:15 PM, Ondrej Zary wrote:
>
> > fit3 protocol driver does not support accessing IDE control registers
> > (device control/altstatus). The DOS driver does not use these registers
> > either (as observed from DOSEMU trace). But the HW seems to be capable
> > of accessing these registers - I simply tried bit 3 and it works!
> >
> > The control register is required to properly reset ATAPI devices or
> > they will be detected only once (after a power cycle).
> >
> > Tested with EXP Computer CD-865 with MC-1285B EPP cable and
> > TransDisk 3000.
> >
> > Signed-off-by: Ondrej Zary <linux@zary.sk>
> > ---
> > drivers/ata/pata_parport/fit3.c | 16 ++++------------
> > 1 file changed, 4 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/ata/pata_parport/fit3.c b/drivers/ata/pata_parport/fit3.c
> > index bad7aa920cdc..86b39966755b 100644
> > --- a/drivers/ata/pata_parport/fit3.c
> > +++ b/drivers/ata/pata_parport/fit3.c
> [...]
> > @@ -35,10 +30,11 @@
> > * cont = 1 - access the IDE command set
> > */
> >
> > +static int cont_map[] = { 0x00, 0x08 };
>
> const?
There's no 'const' in other protocol drivers. Maybe it should be added to all of them.
> > +
> > static void fit3_write_regr(struct pi_adapter *pi, int cont, int regr, int val)
> > {
> > - if (cont == 1)
> > - return;
> > + regr += cont_map[cont];
>
> Perhaps regr += cont << 3 instead?
I matched the style used by other protocol drivers.
> >
> > switch (pi->mode) {
> > case 0:
> > @@ -59,11 +55,7 @@ static int fit3_read_regr(struct pi_adapter *pi, int cont, int regr)
> > {
> > int a, b;
> >
> > - if (cont) {
> > - if (regr != 6)
> > - return 0xff;
> > - regr = 7;
> > - }
> > + regr += cont_map[cont];
>
> Likewise here?
>
> [...]
>
> MBR, Sergey
>
--
Ondrej Zary
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] pata_parport: fix pata_parport_devchk
2023-10-03 17:07 ` Ondrej Zary
@ 2023-10-03 17:18 ` Sergei Shtylyov
0 siblings, 0 replies; 19+ messages in thread
From: Sergei Shtylyov @ 2023-10-03 17:18 UTC (permalink / raw
To: Ondrej Zary, Sergey Shtylyov
Cc: Damien Le Moal, Sudip Mukherjee, Christoph Hellwig, Tim Waugh,
linux-parport, linux-ide, linux-kernel
On 10/3/23 8:07 PM, Ondrej Zary wrote:
[...]
>>> There's a 'x' missing in 0x55 in pata_parport_devchk(), causing the
>>> detection to always fail. Fix it.
>>>
>>> Signed-off-by: Ondrej Zary <linux@zary.sk>
>>
>> I think we need a Fixes: tag here...
>>
>>> ---
>>> drivers/ata/pata_parport/pata_parport.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
>>> index 1af64d435d3c..258d189f42e5 100644
>>> --- a/drivers/ata/pata_parport/pata_parport.c
>>> +++ b/drivers/ata/pata_parport/pata_parport.c
>>> @@ -64,7 +64,7 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
>>> pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0xaa);
>>> pi->proto->write_regr(pi, 0, ATA_REG_LBAL, 0x55);
>>>
>>> - pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 055);
>>> + pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0x55);
>>
>> Oh, Gawd! How did this ever work?! :-/
>> This bug seems to predate the Big PARIDE move...
>
> This code was not present in PARIDE - it's my bug.
Yes, I finally figured -- hence the Fixes: tag I suggested later....
> The function is a clone of ata_devchk() without direct port access.
The libata's taskfile methods suck big time -- I even used to have
the plans to clean this stuff up at some point...
> It's called only from softreset so nobody notices the breakage until something goes wrong. The CD-865 drive needs a reset to start working.
I thought the SRST reset is used at the initial detection phase as well...
MBR, Sergey
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] pata_parport: add custom version of wait_after_reset
2023-10-02 20:48 ` Sergey Shtylyov
@ 2023-10-03 17:20 ` Ondrej Zary
0 siblings, 0 replies; 19+ messages in thread
From: Ondrej Zary @ 2023-10-03 17:20 UTC (permalink / raw
To: Sergey Shtylyov
Cc: Damien Le Moal, Sudip Mukherjee, Christoph Hellwig, Tim Waugh,
linux-parport, linux-ide, linux-kernel
On Monday 02 October 2023 22:48:59 Sergey Shtylyov wrote:
> On 9/30/23 10:15 PM, Ondrej Zary wrote:
>
> > Some parallel adapters (e.g. EXP Computer MC-1285B EPP Cable) return
> > bogus values when there's no master device present. This can cause
> > reset to fail, preventing the lone slave device (such as EXP Computer
> > CD-865) from working.
> >
> > Add custom version of wait_after_reset that ignores master failure when
> > a slave device is present. The custom version is also needed because
> > the generic ata_sff_wait_after_reset uses direct port I/O for slave
> > device detection.
> >
> > Signed-off-by: Ondrej Zary <linux@zary.sk>
> > ---
> > drivers/ata/pata_parport/pata_parport.c | 65 ++++++++++++++++++++++++-
> > 1 file changed, 64 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> > index cf87bbb52f1f..b3db953e615a 100644
> > --- a/drivers/ata/pata_parport/pata_parport.c
> > +++ b/drivers/ata/pata_parport/pata_parport.c
> > @@ -80,6 +80,69 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
> > return (nsect == 0x55) && (lbal == 0xaa);
> > }
> >
> > +static int pata_parport_wait_after_reset(struct ata_link *link,
> > + unsigned int devmask,
> > + unsigned long deadline)
> > +{
> > + struct ata_port *ap = link->ap;
> > + struct pi_adapter *pi = ap->host->private_data;
> > + unsigned int dev0 = devmask & (1 << 0);
> > + unsigned int dev1 = devmask & (1 << 1);
> > + int rc, ret = 0;
> > +
> > + ata_msleep(ap, ATA_WAIT_AFTER_RESET);
> > +
> > + /* always check readiness of the master device */
> > + rc = ata_sff_wait_ready(link, deadline);
> > + /* some adapters return bogus values if master device is not present,
>
> The multiline comments should start with /* on its own line.
> Have you run scripts/checkpatch.pl on the patches?
Checkpatch doesn't complain.
> > + * so don't abort now if a slave device is present
> > + */
> > + if (rc) {
> > + if (!dev1)
> > + return rc;
> > + ret = -ENODEV;
> > + }
> > +
> > + /* if device 1 was found in ata_devchk, wait for register
>
> Likewise here...
>
> > + * access briefly, then wait for BSY to clear.
> > + */
> > + if (dev1) {
> > + int i;
> > +
> > + pata_parport_dev_select(ap, 1);
> > +
> > + /* Wait for register access. Some ATAPI devices fail
>
> And here...
>
> [...]
>
> MBR, Sergey
>
--
Ondrej Zary
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] pata_parport: add custom version of wait_after_reset
2023-10-03 16:55 ` Sergey Shtylyov
@ 2023-10-03 23:44 ` Damien Le Moal
0 siblings, 0 replies; 19+ messages in thread
From: Damien Le Moal @ 2023-10-03 23:44 UTC (permalink / raw
To: Sergey Shtylyov, Ondrej Zary, Sudip Mukherjee
Cc: Christoph Hellwig, Tim Waugh, linux-ide, linux-kernel
On 10/4/23 01:55, Sergey Shtylyov wrote:
> On 10/3/23 3:55 AM, Damien Le Moal wrote:
>
> [...]
>>> Some parallel adapters (e.g. EXP Computer MC-1285B EPP Cable) return
>>> bogus values when there's no master device present. This can cause
>>> reset to fail, preventing the lone slave device (such as EXP Computer
>>> CD-865) from working.
>>>
>>> Add custom version of wait_after_reset that ignores master failure when
>>> a slave device is present. The custom version is also needed because
>>> the generic ata_sff_wait_after_reset uses direct port I/O for slave
>>> device detection.
>>>
>>> Signed-off-by: Ondrej Zary <linux@zary.sk>
>>> ---
>>> drivers/ata/pata_parport/pata_parport.c | 65 ++++++++++++++++++++++++-
>>> 1 file changed, 64 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
>>> index cf87bbb52f1f..b3db953e615a 100644
>>> --- a/drivers/ata/pata_parport/pata_parport.c
>>> +++ b/drivers/ata/pata_parport/pata_parport.c
>>> @@ -80,6 +80,69 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
>>> return (nsect == 0x55) && (lbal == 0xaa);
>>> }
>>>
>>> +static int pata_parport_wait_after_reset(struct ata_link *link,
>>> + unsigned int devmask,
>>> + unsigned long deadline)
>>> +{
>>> + struct ata_port *ap = link->ap;
>>> + struct pi_adapter *pi = ap->host->private_data;
>>> + unsigned int dev0 = devmask & (1 << 0);
>>> + unsigned int dev1 = devmask & (1 << 1);
>>> + int rc, ret = 0;
>>> +
>>> + ata_msleep(ap, ATA_WAIT_AFTER_RESET);
>>> +
>>> + /* always check readiness of the master device */
>>> + rc = ata_sff_wait_ready(link, deadline);
>>> + /* some adapters return bogus values if master device is not present,
>>> + * so don't abort now if a slave device is present
>>> + */
>>
>> In addition to Sergey's comment, please move this comment inside the "if", or
>> even better, merge it with the otherwise not very useful "always check
>> readiness..." comment.
>
> That comment was copied from ata_sff_wait_after_reset(), I think...
Even if that is the case. let's not copy bad stuff as is but rather improve it.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-10-03 23:44 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-30 19:15 [PATCH 0/4] pata_parport: fix EXP Computer CD-865 with MC-1285B EPP cable Ondrej Zary
2023-09-30 19:15 ` [PATCH 1/4] pata_parport: fix pata_parport_devchk Ondrej Zary
2023-10-02 18:43 ` Sergey Shtylyov
2023-10-02 19:08 ` Sergey Shtylyov
2023-10-03 17:07 ` Ondrej Zary
2023-10-03 17:18 ` Sergei Shtylyov
2023-09-30 19:15 ` [PATCH 2/4] pata_parport: implement set_devctl Ondrej Zary
2023-10-02 19:52 ` Sergey Shtylyov
2023-09-30 19:15 ` [PATCH 3/4] pata_parport: add custom version of wait_after_reset Ondrej Zary
2023-10-02 20:48 ` Sergey Shtylyov
2023-10-03 17:20 ` Ondrej Zary
2023-10-03 0:55 ` Damien Le Moal
2023-10-03 16:55 ` Sergey Shtylyov
2023-10-03 23:44 ` Damien Le Moal
2023-09-30 19:15 ` [PATCH 4/4] pata_parport-fit3: implement IDE command set registers Ondrej Zary
2023-10-02 20:06 ` Sergey Shtylyov
2023-10-03 17:13 ` Ondrej Zary
2023-10-03 0:56 ` Damien Le Moal
2023-10-03 0:34 ` [PATCH 0/4] pata_parport: fix EXP Computer CD-865 with MC-1285B EPP cable Damien Le Moal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).