Re: [PATCH] ahci: enable GHC.AE bit before set GHC.HR - Kernel

This is a discussion on Re: [PATCH] ahci: enable GHC.AE bit before set GHC.HR - Kernel ; Peer Chen wrote: > According to the description of section 5.2.2.1 and 10.1.2 of AHCI specification rev1_1/rev1_2, GHC.HR shall only be set to 1 > by software when GHC.AE is set to 1. > > Signed-off-by: Peer Chen > --- ...

+ Reply to Thread
Results 1 to 7 of 7

Thread: Re: [PATCH] ahci: enable GHC.AE bit before set GHC.HR

  1. Re: [PATCH] ahci: enable GHC.AE bit before set GHC.HR

    Peer Chen wrote:
    > According to the description of section 5.2.2.1 and 10.1.2 of AHCI specification rev1_1/rev1_2, GHC.HR shall only be set to 1
    > by software when GHC.AE is set to 1.
    >
    > Signed-off-by: Peer Chen
    > ---
    > --- linux-2.6.23-rc7/drivers/ata/ahci.c.orig 2007-09-20 11:01:55.000000000 -0400
    > +++ linux-2.6.23-rc7/drivers/ata/ahci.c 2007-09-20 11:07:31.000000000 -0400
    > @@ -834,6 +834,10 @@ static int ahci_reset_controller(struct
    > void __iomem *mmio = host->iomap[AHCI_PCI_BAR];
    > u32 tmp;
    >
    > + /* turn on AHCI mode before controller reset*/
    > + writel(HOST_AHCI_EN, mmio + HOST_CTL);
    > + (void) readl(mmio + HOST_CTL); /* flush */


    applied the attached patch, inspired by yours.


    commit 5fca365d6109b51cfeb3515ca660cd2dc6e8822c
    Author: Jeff Garzik
    Date: Wed Sep 26 00:02:41 2007 -0400

    [libata] AHCI: enable AHCI mode, before using AHCI reset

    AHCI spec says host-reset bit may only be set when the ahci-enable bit
    is also set.

    Noticed by Peer Chen

    Signed-off-by: Jeff Garzik

    drivers/ata/ahci.c | 8 +++++++-
    1 file changed, 7 insertions(+), 1 deletion(-)

    5fca365d6109b51cfeb3515ca660cd2dc6e8822c
    diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
    index 9f3c591..b615390 100644
    --- a/drivers/ata/ahci.c
    +++ b/drivers/ata/ahci.c
    @@ -827,8 +827,14 @@ static int ahci_reset_controller(struct ata_host *host)
    void __iomem *mmio = host->iomap[AHCI_PCI_BAR];
    u32 tmp;

    - /* global controller reset */
    + /* we must be in AHCI mode, before using anything
    + * AHCI-specific, such as HOST_RESET.
    + */
    tmp = readl(mmio + HOST_CTL);
    + if (!(tmp & HOST_AHCI_EN))
    + writel(tmp | HOST_AHCI_EN, mmio + HOST_CTL);
    +
    + /* global controller reset */
    if ((tmp & HOST_RESET) == 0) {
    writel(tmp | HOST_RESET, mmio + HOST_CTL);
    readl(mmio + HOST_CTL); /* flush */


  2. Re: [PATCH] ahci: enable GHC.AE bit before set GHC.HR

    On Wed, 26 Sep 2007 00:03:19 -0400
    Jeff Garzik wrote:

    > Peer Chen wrote:
    > > According to the description of section 5.2.2.1 and 10.1.2 of AHCI specification rev1_1/rev1_2, GHC.HR shall only be set to ¡®1¡¯
    > > by software when GHC.AE is set to ¡®1¡¯.
    > >
    > > Signed-off-by: Peer Chen
    > > ---
    > > --- linux-2.6.23-rc7/drivers/ata/ahci.c.orig 2007-09-20 11:01:55.000000000 -0400
    > > +++ linux-2.6.23-rc7/drivers/ata/ahci.c 2007-09-20 11:07:31.000000000 -0400
    > > @@ -834,6 +834,10 @@ static int ahci_reset_controller(struct
    > > void __iomem *mmio = host->iomap[AHCI_PCI_BAR];
    > > u32 tmp;
    > >
    > > + /* turn on AHCI mode before controller reset*/
    > > + writel(HOST_AHCI_EN, mmio + HOST_CTL);
    > > + (void) readl(mmio + HOST_CTL); /* flush */

    >
    > applied the attached patch, inspired by yours.



    NAK - mmio is an iomap so writel and readl are the wrong things to use

    -
    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/

  3. Re: [PATCH] ahci: enable GHC.AE bit before set GHC.HR

    Alan Cox wrote:
    > On Wed, 26 Sep 2007 00:03:19 -0400
    > Jeff Garzik wrote:
    >
    >> Peer Chen wrote:
    >>> According to the description of section 5.2.2.1 and 10.1.2 of AHCI specification rev1_1/rev1_2, GHC.HR shall only be set to ¡®1¡¯
    >>> by software when GHC.AE is set to ¡®1¡¯.
    >>>
    >>> Signed-off-by: Peer Chen
    >>> ---
    >>> --- linux-2.6.23-rc7/drivers/ata/ahci.c.orig 2007-09-20 11:01:55.000000000 -0400
    >>> +++ linux-2.6.23-rc7/drivers/ata/ahci.c 2007-09-20 11:07:31.000000000 -0400
    >>> @@ -834,6 +834,10 @@ static int ahci_reset_controller(struct
    >>> void __iomem *mmio = host->iomap[AHCI_PCI_BAR];
    >>> u32 tmp;
    >>>
    >>> + /* turn on AHCI mode before controller reset*/
    >>> + writel(HOST_AHCI_EN, mmio + HOST_CTL);
    >>> + (void) readl(mmio + HOST_CTL); /* flush */

    >> applied the attached patch, inspired by yours.

    >
    >
    > NAK - mmio is an iomap so writel and readl are the wrong things to use


    The patch is consistent with the rest of the driver.

    You are welcome to submit a patch to convert ahci to using ioremap.

    Jeff



    -
    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/

  4. Re: [PATCH] ahci: enable GHC.AE bit before set GHC.HR

    > > NAK - mmio is an iomap so writel and readl are the wrong things to use
    >
    > The patch is consistent with the rest of the driver.
    > You are welcome to submit a patch to convert ahci to using ioremap.


    You could just flip the relevant function to use ioread while you are
    tidying it up, instead of spreading new bugs into the code.

    Alan
    -
    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/

  5. Re: [PATCH] ahci: enable GHC.AE bit before set GHC.HR

    Alan Cox wrote:
    >>> NAK - mmio is an iomap so writel and readl are the wrong things to use

    >> The patch is consistent with the rest of the driver.
    >> You are welcome to submit a patch to convert ahci to using ioremap.

    >
    > You could just flip the relevant function to use ioread while you are
    > tidying it up, instead of spreading new bugs into the code.


    No, as I just noted above, the proper fix for this driver is to use
    ioremap rather than pci_iomap.

    Adding support to ahci for legacy PIO is completely pointless.

    Jeff



    -
    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/

  6. Re: [PATCH] ahci: enable GHC.AE bit before set GHC.HR

    On Wed, 26 Sep 2007 10:33:28 -0400
    Jeff Garzik wrote:

    > Alan Cox wrote:
    > >>> NAK - mmio is an iomap so writel and readl are the wrong things to use
    > >> The patch is consistent with the rest of the driver.
    > >> You are welcome to submit a patch to convert ahci to using ioremap.

    > >
    > > You could just flip the relevant function to use ioread while you are
    > > tidying it up, instead of spreading new bugs into the code.

    >
    > No, as I just noted above, the proper fix for this driver is to use
    > ioremap rather than pci_iomap.
    >
    > Adding support to ahci for legacy PIO is completely pointless.


    iomap isn't just for legacy PIO. It allows us to handle future weird
    mappings in ways ioremap cannot.
    -
    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/

  7. Re: [PATCH] ahci: enable GHC.AE bit before set GHC.HR

    Alan Cox wrote:
    > On Wed, 26 Sep 2007 10:33:28 -0400
    > Jeff Garzik wrote:
    >
    >> Alan Cox wrote:
    >>>>> NAK - mmio is an iomap so writel and readl are the wrong things to use
    >>>> The patch is consistent with the rest of the driver.
    >>>> You are welcome to submit a patch to convert ahci to using ioremap.
    >>> You could just flip the relevant function to use ioread while you are
    >>> tidying it up, instead of spreading new bugs into the code.

    >> No, as I just noted above, the proper fix for this driver is to use
    >> ioremap rather than pci_iomap.
    >>
    >> Adding support to ahci for legacy PIO is completely pointless.

    >
    > iomap isn't just for legacy PIO. It allows us to handle future weird
    > mappings in ways ioremap cannot.


    Well, when needs dictate, we can re-evaluate.

    Until some future date arrives where it matters for all these MMIO-only
    drivers and hardware, it's just a bunch of pointless overhead for ahci
    and many other drivers. It's also just not the Linux way to punish
    everybody for some edge case that so far only exists in email conversations.

    The beauty of libata is that you don't have to enforce such a pogrom
    across all libata drivers. The libata high level API is completely free
    from ioread/iowrite junk, leaving each driver to make its own decision.

    Jeff



    -
    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/

+ Reply to Thread