headers
Jiri Slaby | 1 Apr 2012 22:48
Picon
Gravatar

Re: [PATCH v2] libata: disable runtime pm for hotpluggable port

On 03/13/2012 02:57 AM, Lin Ming wrote:
> Currently, hotplug doesn't work if port is already runtime suspended.
> For now, we simply disable runtime pm for hotpluggable port.
> Later, we should add runtime pm support for hotpluggable port too.
> 
> Bug report:
> https://lkml.org/lkml/2012/2/19/70
> 
> v2:
> - Use bit 2 and 3 for flags ATA_FLAG_EXTERNAL and ATA_FLAG_PLUGGABLE.
> 
> TODO: add similar hotpluggable port check for controllers other than
> AHCI.

Any updates on this? The patches which introduced the bug are in -rc1
now. Unlike this patch.

> Reported-and-tested-by: Jiri Slaby <jslaby <at> suse.cz>
> Reported-and-tested-by: cwillu <at> cwillu.com
> Reported-and-tested-by: jackdachef <at> gmail.com
> Signed-off-by: Lin Ming <ming.m.lin <at> intel.com>
> ---
>  drivers/ata/ahci.c             |    3 +++
>  drivers/ata/ahci.h             |    3 +++
>  drivers/ata/libahci.c          |   20 ++++++++++++++++++++
>  drivers/ata/libata-transport.c |    6 ++++--
>  include/linux/libata.h         |    2 ++
>  5 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
(Continue reading)

Permalink | Reply | 1 comment |
headers
Rafal Prylowski | 2 Apr 2012 09:52
Picon

Re: [PATCH 1/3] PATA host controller driver for ep93xx

On 2012-03-30 22:18, Arnd Bergmann wrote:
>> +static u16 ep93xx_pata_read(struct ep93xx_pata_data *drv_data,
>> +			    void *addr_io,
>> +			    const struct ata_timing *t)
>> +{
>> +	unsigned long addr = (unsigned long) addr_io & 0x1f;
>> +	void __iomem *base = drv_data->ide_base;
>> +	u16 ret;
>> +
>> +	writel(IDECtrl_DIOWn | IDECtrl_DIORn | addr, base + IDECtrl);
>> +	ndelay(t->setup);
>> +	writel(IDECtrl_DIOWn | addr, base + IDECtrl);
> 
> The pointer arithmetic that you do here on addr_io looks really evil.
> Basically your registers are indirect and cannot be accessed through
> pointer dereference. Maybe you should not be trying to do that then
> and not use the ata_port->ioaddr structure.

Yes, I already prepared a version of the driver in which IDE register
addresses are coded as enums instead of using ata_port->ioaddr structure. 
I will post updated version, when I get feedback from Ryan if enums
or defines are preferred.

>> +	ndelay(t->act8b);
> 
> I'm not too familar with ata drivers, but I don't think you're supposed
> to have delays in the code for the timings, rather than programming
> the timings into the controller registers. Are you sure this is the
> right thing here?
(Continue reading)

Permalink | Reply | 5 comments |
headers
Arnd Bergmann | 2 Apr 2012 10:08
Picon
Gravatar

Re: [PATCH 1/3] PATA host controller driver for ep93xx

On Monday 02 April 2012, Rafal Prylowski wrote:
> > 
> > And I think this should be consequently be sorted below nonstandard ports, instead of the SFF list.
> > 
> >       Arnd
> 
> I selected "PATA SFF controllers with BMDMA" list because the driver
> inherits .ata_bmdma_port_ops (this is in libata-sff.c, which is compiled
> only if ATA_SFF is set).

Ok, I see. Is it actually the right one to inherit from though?

You end up overriding most of the opterations that are set there again,
the only ones that you use are:

ata_bmdma_error_handler, ata_bmdma_post_internal_cmd, ata_bmdma_qc_issue,
ata_sff_qc_fill_rtf, ata_sff_freeze, ata_sff_thaw, ata_sff_prereset,
ata_sff_postreset and ata_sff_error_handler.

Are you sure they are all doing the righ thing on your hardware? If
not, it might be better to explicitly just set the ones you need and
see if that still uses any sff functions in the end.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
(Continue reading)

Permalink | Reply | 2 comments |
headers
Rafal Prylowski | 2 Apr 2012 11:28
Picon

Re: [PATCH 1/3] PATA host controller driver for ep93xx

On 2012-04-02 10:08, Arnd Bergmann wrote:
> On Monday 02 April 2012, Rafal Prylowski wrote:
>>
>> I selected "PATA SFF controllers with BMDMA" list because the driver
>> inherits .ata_bmdma_port_ops (this is in libata-sff.c, which is compiled
>> only if ATA_SFF is set).
> 
> Ok, I see. Is it actually the right one to inherit from though?
> 
> You end up overriding most of the opterations that are set there again,
> the only ones that you use are:
> 
> ata_bmdma_error_handler, ata_bmdma_post_internal_cmd, ata_bmdma_qc_issue,
> ata_sff_qc_fill_rtf, ata_sff_freeze, ata_sff_thaw, ata_sff_prereset,
> ata_sff_postreset and ata_sff_error_handler.
> 
> Are you sure they are all doing the righ thing on your hardware? If
> not, it might be better to explicitly just set the ones you need and
> see if that still uses any sff functions in the end.
> 
> 	Arnd

I think that inheriting from .ata_bmdma_port_ops is quite reasonable.
Another option would be to inherit from .ata_sff_port_ops and implement
.qc_issue hook (like in pata_octeon_cf.c), but this way we end up
reimplementing the same things from libata-sff.c, IMHO. Also, I think
that it's not possible to write PATA driver without this SFF stuff
(at least for me - I'm not libata expert).
I reviewed code paths from all hooks used in ep93xx driver to make sure
that we use only ep93xx_pata_read/ep93xx_pata_write instead of ioread/iowrite.
(Continue reading)

Permalink | Reply | 1 comment |
headers
Arnd Bergmann | 2 Apr 2012 12:24
Picon
Gravatar

Re: [PATCH 1/3] PATA host controller driver for ep93xx

On Monday 02 April 2012, Rafal Prylowski wrote:
> I think that inheriting from .ata_bmdma_port_ops is quite reasonable.
> Another option would be to inherit from .ata_sff_port_ops and implement
> .qc_issue hook (like in pata_octeon_cf.c), but this way we end up
> reimplementing the same things from libata-sff.c, IMHO. Also, I think
> that it's not possible to write PATA driver without this SFF stuff
> (at least for me - I'm not libata expert).
> I reviewed code paths from all hooks used in ep93xx driver to make sure
> that we use only ep93xx_pata_read/ep93xx_pata_write instead of ioread/iowrite.
> 
Ok, thanks for the confirmation.

	Arnd

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 0 comments |
headers
Ryan Mallon | 3 Apr 2012 03:50
Picon

Re: [PATCH 1/3] PATA host controller driver for ep93xx

On 02/04/12 17:52, Rafal Prylowski wrote:

> On 2012-03-30 22:18, Arnd Bergmann wrote:
>>> +static u16 ep93xx_pata_read(struct ep93xx_pata_data *drv_data,
>>> +			    void *addr_io,
>>> +			    const struct ata_timing *t)
>>> +{
>>> +	unsigned long addr = (unsigned long) addr_io & 0x1f;
>>> +	void __iomem *base = drv_data->ide_base;
>>> +	u16 ret;
>>> +
>>> +	writel(IDECtrl_DIOWn | IDECtrl_DIORn | addr, base + IDECtrl);
>>> +	ndelay(t->setup);
>>> +	writel(IDECtrl_DIOWn | addr, base + IDECtrl);
>>
>> The pointer arithmetic that you do here on addr_io looks really evil.
>> Basically your registers are indirect and cannot be accessed through
>> pointer dereference. Maybe you should not be trying to do that then
>> and not use the ata_port->ioaddr structure.
> 
> Yes, I already prepared a version of the driver in which IDE register
> addresses are coded as enums instead of using ata_port->ioaddr structure. 
> I will post updated version, when I get feedback from Ryan if enums
> or defines are preferred.

I would prefer defines, simply because it looks odd having an enum which
has multiple names defined to the same value, and most drivers tend to
use defines rather than enums for registers. It's not a big deal though,
and certainly not a show stopper.
(Continue reading)

Permalink | Reply | 1 comment |
headers
Arnd Bergmann | 3 Apr 2012 09:41
Picon
Gravatar

Re: [PATCH 1/3] PATA host controller driver for ep93xx

On Tuesday 03 April 2012, Ryan Mallon wrote:
> On 02/04/12 17:52, Rafal Prylowski wrote:
>
> > Yes, I already prepared a version of the driver in which IDE register
> > addresses are coded as enums instead of using ata_port->ioaddr structure. 
> > I will post updated version, when I get feedback from Ryan if enums
> > or defines are preferred.
> 
> I would prefer defines, simply because it looks odd having an enum which
> has multiple names defined to the same value, and most drivers tend to
> use defines rather than enums for registers. It's not a big deal though,
> and certainly not a show stopper.

I tend to prefer enums, but I agree that it's a minor issue and I generally
don't even mention it in reviews.

	Arnd

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 0 comments |
headers
Sergei Shtylyov | 3 Apr 2012 13:56

Re: [PATCH 1/1] Add support 2 SATA ports for Maui and change filename from sata_dwc_460ex.c to sata_dwc_4xx.c

Hello.

On 03-04-2012 14:12, Thang Q. Nguyen wrote:

> Signed-off-by: Thang Q. Nguyen<tqnguyen <at> apm.com>
> ---
> Changes for v2:
> 	- Use git rename feature to change the driver to the newname and for
> 	  easier review.

>   arch/powerpc/boot/dts/bluestone.dts              |   21 +
>   drivers/ata/Makefile                             |    2 +-
>   drivers/ata/{sata_dwc_460ex.c =>  sata_dwc_4xx.c} | 1371 ++++++++++++++--------
>   3 files changed, 904 insertions(+), 490 deletions(-)
>   rename drivers/ata/{sata_dwc_460ex.c =>  sata_dwc_4xx.c} (56%)

    You submitted a magapatch doing several things at once (some even 
needlessly) and even in two areas of the kernel. This needs proper 
splitting/description.

> diff --git a/arch/powerpc/boot/dts/bluestone.dts b/arch/powerpc/boot/dts/bluestone.dts
> index cfa23bf..803fda6 100644
> --- a/arch/powerpc/boot/dts/bluestone.dts
> +++ b/arch/powerpc/boot/dts/bluestone.dts
>  <at>  <at>  -155,6 +155,27  <at>  <at> 
>   					/*RXDE*/  0x5 0x4>;
>   		};
>
> +		/* SATA DWC devices */
> +		SATA0: sata <at> bffd1000 {
(Continue reading)

Permalink | Reply | 8 comments |
headers
Rafal Prylowski | 3 Apr 2012 16:42
Picon

[PATCH v2 0/3] Add PATA host controller support for Cirrus Logic EP93xx CPU

Hi,

this is a second version of EP93xx PATA driver (PATCH v2 1/3),
IDE platform code for ep93xx (PATCH v2 2/3) and IDE support
for EDB93xx boards (PATCH v2 3/3).

Changes since first version:
- don't BUG() in ep93xx_pata_dma_start(), ep93xx_pata_dma_setup(),
- don't use ata_ioports structure for EP93xx IDECTRL register values,
- IORDY timeout handling using jiffies (ep93xx_pata_wait_for_iordy),
- IDE/GPIO pin muxing solved by adding ep93xx_ide_acquire/release_gpio,
- register the driver also for EDB9312 and EDB9315 boards,
- corrected many coding style issues,
- dma initialization changed: request and configure channels in
  ep93xx_pata_dma_init (was: request in ep93xx_pata_dma_init,
  configuration in ep93xx_pata_dma_setup - channels were needlessly
  configured to the same values before each transfer).

Changes were made based on comments from Hartley, Ryan and Arnd.
Thanks!

[PATCH v2 1/3] PATA host controller driver for ep93xx
[PATCH v2 2/3] ep93xx: IDE driver platform support code
[PATCH v2 3/3] ep93xx: Add IDE support to edb93xx boards

 arch/arm/mach-ep93xx/core.c                  |   85 +
 arch/arm/mach-ep93xx/edb93xx.c               |   24
 arch/arm/mach-ep93xx/include/mach/platform.h |    3
 arch/arm/mach-ep93xx/soc.h                   |    1
 drivers/ata/Kconfig                          |    9
(Continue reading)

Permalink | Reply | 25 comments |
headers
Rafal Prylowski | 3 Apr 2012 16:45
Picon

[PATCH v2 1/3] PATA host controller driver for ep93xx


Signed-off-by: Rafal Prylowski <prylowski <at> metasoft.pl>
Cc: Joao Ramos <joao.ramos <at> inov.pt>
Cc: H Hartley Sweeten <hsweeten <at> visionengravers.com>
Cc: Ryan Mallon <rmallon <at> gmail.com>
Cc: Sergei Shtylyov <sshtylyov <at> mvista.com>
Cc: Bartlomiej Zolnierkiewicz <bzolnier <at> gmail.com>

---
 drivers/ata/Kconfig       |    9
 drivers/ata/Makefile      |    1
 drivers/ata/pata_ep93xx.c |  976 ++++++++++++++++++++++++++++++++++++
 3 files changed, 986 insertions(+)

Index: linux-2.6/drivers/ata/pata_ep93xx.c
===================================================================
--- /dev/null
+++ linux-2.6/drivers/ata/pata_ep93xx.c
 <at>  <at>  -0,0 +1,976  <at>  <at> 
+/*
+ * EP93XX PATA controller driver.
+ *
+ * Copyright (c) 2012, Metasoft s.c.
+ *	Rafal Prylowski <prylowski <at> metasoft.pl>
+ *
+ * Based on pata_scc.c, pata_icside.c and on earlier version of EP93XX
+ * PATA driver by Lennert Buytenhek and Alessandro Zummo.
+ * Read/Write timings, resource management and other improvements
+ * from driver by Joao Ramos and Bartlomiej Zolnierkiewicz.
+ * DMA engine support based on spi-ep93xx.c by Mika Westerberg.
(Continue reading)

Permalink | Reply | 14 comments |

Gmane