| Message ID | 1427899682-10912-2-git-send-email-stripathi@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 01, 2015 at 08:18:02PM +0530, Suman Tripathi wrote: > This patch enables full AHCI feature support for APM X-Gene SoC SATA host host > controller. The following errata's are removed: > > 1. 2a0bdff6b95 ("ahci-xgene: fix the dma state machine lockup for the > IDENTIFY DEVICE PIO mode command") > 2. 09c32aaa368 ("ahci_xgene: Fix the dma state machine lockup for the > ATA_CMD_SMART PIO mode command") > 3. 1540035da71 ("ahci_xgene: Implement the xgene_ahci_poll_reg_val to > support PMP") > 4. a3a84bc7c88 ("ahci_xgene: Implement the workaround to support PMP > enumeration and discovery") > 5. 1102407bb71 ("ahci_xgene: Fix the DMA state machine lockup for the > ATA_CMD_PACKET PIO mode command") > 6. 72f79f9e35b ("ahci_xgene: Removing NCQ support from the APM X-Gene SoC > AHCI SATA Host Controller driver") > > In addition, enable PMP support for second generation APM X-Gene SoC. > > Signed-off-by : Suman Tripathi <stripathi@apm.com> Applied to libata/for-4.1 w/ minor edit. > + if (xgene_ahci_version1()) { > + /* > + * Override the callbacks due to > + * XGENE1 ERRATA's. > + */ The column limit is 80 (more like 75), not 40. I collapsed the above into one line. Thanks.
On Wed, Apr 01, 2015 at 12:13:36PM -0400, Tejun Heo wrote: > > Signed-off-by : Suman Tripathi <stripathi@apm.com> > > Applied to libata/for-4.1 w/ minor edit. Reverted due to build failure from missing asm/cputype.h. Suman, you're the develper and familiar with how the prerequisite patches are flowing where. When sending patches to maintainers, please let them know how to deal with your patches. Having cross-tree dependencies are fine, there are multiple ways to deal with that but please don't just drop patches which will fail build and see what happens. Please help us do our job. Thanks.
On Wed, Apr 01, 2015 at 12:13:36PM -0400, Tejun Heo wrote: > > Signed-off-by : Suman Tripathi <stripathi@apm.com> > > Applied to libata/for-4.1 w/ minor edit. Reverted due to build failure from missing asm/cputype.h. Suman, you're the develper and familiar with how the prerequisite patches are flowing where. When sending patches to maintainers, please let them know how to deal with your patches. Having cross-tree dependencies are fine, there are multiple ways to deal with that but please don't just drop patches which will fail build and see what happens. Please help us do our job. Apologize for that . Really sorry . I will check it. Thanks. On Wed, Apr 1, 2015 at 10:01 PM, Tejun Heo <tj@kernel.org> wrote: > On Wed, Apr 01, 2015 at 12:13:36PM -0400, Tejun Heo wrote: >> > Signed-off-by : Suman Tripathi <stripathi@apm.com> >> >> Applied to libata/for-4.1 w/ minor edit. > > Reverted due to build failure from missing asm/cputype.h. Suman, > you're the develper and familiar with how the prerequisite patches are > flowing where. When sending patches to maintainers, please let them > know how to deal with your patches. Having cross-tree dependencies > are fine, there are multiple ways to deal with that but please don't > just drop patches which will fail build and see what happens. Please > help us do our job. > > Thanks. > > -- > tejun
On Wed, Apr 01, 2015 at 12:31:16PM -0400, Tejun Heo wrote: > On Wed, Apr 01, 2015 at 12:13:36PM -0400, Tejun Heo wrote: > > > Signed-off-by : Suman Tripathi <stripathi@apm.com> > > > > Applied to libata/for-4.1 w/ minor edit. > > Reverted due to build failure from missing asm/cputype.h. I'm guessing this is some kind of ARM driver, which is why linux-arm-kernel has been Cc'd - though I haven't seen the original patch. ARM has asm/cputype.h, and has had for a long time, so "missing asm/cputype.h" doesn't make sense as a reason to revert it. Maybe it's trying to be built on x86 when it should be restricted to only ARM? Dunno without seeing the original patch.
On Wed, Apr 01, 2015 at 05:39:56PM +0100, Russell King - ARM Linux wrote: > On Wed, Apr 01, 2015 at 12:31:16PM -0400, Tejun Heo wrote: > > On Wed, Apr 01, 2015 at 12:13:36PM -0400, Tejun Heo wrote: > > > > Signed-off-by : Suman Tripathi <stripathi@apm.com> > > > > > > Applied to libata/for-4.1 w/ minor edit. > > > > Reverted due to build failure from missing asm/cputype.h. > > I'm guessing this is some kind of ARM driver, which is why > linux-arm-kernel has been Cc'd - though I haven't seen the original > patch. It's ahci_xgene which seems to be specific to ARM. > ARM has asm/cputype.h, and has had for a long time, so "missing > asm/cputype.h" doesn't make sense as a reason to revert it. Maybe As it's a new build failure I assumed that was a new file. Apparently not. > it's trying to be built on x86 when it should be restricted to > only ARM? The build failure is from kbuild and it's on PPC64 w/ COMPILE_TEST. Urgh.. Suman, can you please update the patch to turn off COMPILE_TEST on the driver? Thanks.
On Wed, Apr 01, 2015 at 05:39:56PM +0100, Russell King - ARM Linux wrote: > On Wed, Apr 01, 2015 at 12:31:16PM -0400, Tejun Heo wrote: > > On Wed, Apr 01, 2015 at 12:13:36PM -0400, Tejun Heo wrote: > > > > Signed-off-by : Suman Tripathi <stripathi@apm.com> > > > > > > Applied to libata/for-4.1 w/ minor edit. > > > > Reverted due to build failure from missing asm/cputype.h. > > I'm guessing this is some kind of ARM driver, which is why > linux-arm-kernel has been Cc'd - though I haven't seen the original > patch. > > ARM has asm/cputype.h, and has had for a long time, so "missing > asm/cputype.h" doesn't make sense as a reason to revert it. Maybe > it's trying to be built on x86 when it should be restricted to > only ARM? > > Dunno without seeing the original patch. First search result leads here: https://www.mail-archive.com/devicetree@vger.kernel.org/msg67774.html The driver in general should not be ARM specific, though it runs on an ARMv8 platform. But looking at the patch it has some errata workarounds triggered based on the CPU id (MIDR). That looks dodgy as it doesn't even check the full ID, only the variant part. But if they don't have any other way of identifying the hardware version (ideally some SoC or device identification), the driver should at least be made dependent on ARM64. Looking at the Kconfig entries, PHY_XGENE depends on (ARM64 || COMPILE_TEST). The COMPILE_TEST part should be dropped (I would still prefer a different way to handle this than checking MIDR but I'm not familiar with the hardware).
On Wed, Apr 01, 2015 at 06:00:33PM +0100, Catalin Marinas wrote: > On Wed, Apr 01, 2015 at 05:39:56PM +0100, Russell King - ARM Linux wrote: > > On Wed, Apr 01, 2015 at 12:31:16PM -0400, Tejun Heo wrote: > > > On Wed, Apr 01, 2015 at 12:13:36PM -0400, Tejun Heo wrote: > > > > > Signed-off-by : Suman Tripathi <stripathi@apm.com> > > > > > > > > Applied to libata/for-4.1 w/ minor edit. > > > > > > Reverted due to build failure from missing asm/cputype.h. > > > > I'm guessing this is some kind of ARM driver, which is why > > linux-arm-kernel has been Cc'd - though I haven't seen the original > > patch. > > > > ARM has asm/cputype.h, and has had for a long time, so "missing > > asm/cputype.h" doesn't make sense as a reason to revert it. Maybe > > it's trying to be built on x86 when it should be restricted to > > only ARM? > > > > Dunno without seeing the original patch. > > First search result leads here: > > https://www.mail-archive.com/devicetree@vger.kernel.org/msg67774.html > > The driver in general should not be ARM specific, though it runs on an > ARMv8 platform. But looking at the patch it has some errata workarounds > triggered based on the CPU id (MIDR). That looks dodgy as it doesn't > even check the full ID, only the variant part. > > But if they don't have any other way of identifying the hardware > version (ideally some SoC or device identification), the driver should > at least be made dependent on ARM64. Looking at the Kconfig entries, > PHY_XGENE depends on (ARM64 || COMPILE_TEST). The COMPILE_TEST part > should be dropped (I would still prefer a different way to handle this > than checking MIDR but I'm not familiar with the hardware). We should just add a new compatible string for the new revision of the AHCI controller. Given the errata we're avoiding apply to the AHCI controller and not the CPU, looking at the CPU ID is nonsense. Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 01, 2015 at 06:00:33PM +0100, Catalin Marinas wrote: > On Wed, Apr 01, 2015 at 05:39:56PM +0100, Russell King - ARM Linux wrote: > > On Wed, Apr 01, 2015 at 12:31:16PM -0400, Tejun Heo wrote: > > > On Wed, Apr 01, 2015 at 12:13:36PM -0400, Tejun Heo wrote: > > > > > Signed-off-by : Suman Tripathi <stripathi@apm.com> > > > > > > > > Applied to libata/for-4.1 w/ minor edit. > > > > > > Reverted due to build failure from missing asm/cputype.h. > > > > I'm guessing this is some kind of ARM driver, which is why > > linux-arm-kernel has been Cc'd - though I haven't seen the original > > patch. > > > > ARM has asm/cputype.h, and has had for a long time, so "missing > > asm/cputype.h" doesn't make sense as a reason to revert it. Maybe > > it's trying to be built on x86 when it should be restricted to > > only ARM? > > > > Dunno without seeing the original patch. > > First search result leads here: > > https://www.mail-archive.com/devicetree@vger.kernel.org/msg67774.html Thanks. > The driver in general should not be ARM specific, though it runs on an > ARMv8 platform. But looking at the patch it has some errata workarounds > triggered based on the CPU id (MIDR). That looks dodgy as it doesn't > even check the full ID, only the variant part. Okay, it's not ARM but an ARM64 driver, so that's your territory, not mine. :)
On Wed, Apr 01, 2015 at 06:30:30PM +0100, Russell King - ARM Linux wrote: > On Wed, Apr 01, 2015 at 06:00:33PM +0100, Catalin Marinas wrote: > > The driver in general should not be ARM specific, though it runs on an > > ARMv8 platform. But looking at the patch it has some errata workarounds > > triggered based on the CPU id (MIDR). That looks dodgy as it doesn't > > even check the full ID, only the variant part. > > Okay, it's not ARM but an ARM64 driver, so that's your territory, not > mine. :) Well, feel free to enter this territory ;). Anyway, what I meant is that such driver should not rely on CPU identification at all, especially since it does not require some specific CPU features but it tries to guess which SoC revision it is and which device (not CPU) bugs have been fixed. Better if they follow Mark R's idea to pass such information in DT for the device. So maybe the revert is not too bad an idea.
On Wed, Apr 01, 2015 at 07:18:19PM +0100, Catalin Marinas wrote: > On Wed, Apr 01, 2015 at 06:30:30PM +0100, Russell King - ARM Linux wrote: > > On Wed, Apr 01, 2015 at 06:00:33PM +0100, Catalin Marinas wrote: > > > The driver in general should not be ARM specific, though it runs on an > > > ARMv8 platform. But looking at the patch it has some errata workarounds > > > triggered based on the CPU id (MIDR). That looks dodgy as it doesn't > > > even check the full ID, only the variant part. > > > > Okay, it's not ARM but an ARM64 driver, so that's your territory, not > > mine. :) > > Well, feel free to enter this territory ;). > > Anyway, what I meant is that such driver should not rely on CPU > identification at all, especially since it does not require some > specific CPU features but it tries to guess which SoC revision it is and > which device (not CPU) bugs have been fixed. Better if they follow Mark > R's idea to pass such information in DT for the device. > > So maybe the revert is not too bad an idea. The revert is completely sane. FWIW, NAK to the original patch using the MIDR, or any other patch which uses the MIDR for device (not CPU) errata workarounds. Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c index 2e8bb60..0be47b0 100644 --- a/drivers/ata/ahci_xgene.c +++ b/drivers/ata/ahci_xgene.c @@ -28,6 +28,8 @@ #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/phy/phy.h> +#include <asm/cputype.h> + #include "ahci.h" #define DRV_NAME "xgene-ahci" @@ -94,6 +96,11 @@ struct xgene_ahci_context { void __iomem *csr_mux; /* MUX CSR address of IP */ }; +static bool xgene_ahci_version1(void) +{ + return MIDR_VARIANT(read_cpuid_id()) == 0 ? true : false; +} + static int xgene_ahci_init_memram(struct xgene_ahci_context *ctx) { dev_dbg(ctx->dev, "Release memory from shutdown\n"); @@ -703,7 +710,19 @@ static int xgene_ahci_probe(struct platform_device *pdev) /* Configure the host controller */ xgene_ahci_hw_init(hpriv); skip_clk_phy: - hpriv->flags = AHCI_HFLAG_NO_PMP | AHCI_HFLAG_NO_NCQ; + + if (xgene_ahci_version1()) { + /* + * Override the callbacks due to + * XGENE1 ERRATA's. + */ + hpriv->flags |= AHCI_HFLAG_NO_NCQ; + xgene_ahci_ops.qc_issue = xgene_ahci_qc_issue; + xgene_ahci_ops.softreset = xgene_ahci_softreset; + xgene_ahci_ops.pmp_softreset = xgene_ahci_pmp_softreset; + } else { + hpriv->flags |= AHCI_HFLAG_YES_FBS; + } rc = ahci_platform_init_host(pdev, hpriv, &xgene_ahci_port_info, &ahci_platform_sht);
This patch enables full AHCI feature support for APM X-Gene SoC SATA host host controller. The following errata's are removed: 1. 2a0bdff6b95 ("ahci-xgene: fix the dma state machine lockup for the IDENTIFY DEVICE PIO mode command") 2. 09c32aaa368 ("ahci_xgene: Fix the dma state machine lockup for the ATA_CMD_SMART PIO mode command") 3. 1540035da71 ("ahci_xgene: Implement the xgene_ahci_poll_reg_val to support PMP") 4. a3a84bc7c88 ("ahci_xgene: Implement the workaround to support PMP enumeration and discovery") 5. 1102407bb71 ("ahci_xgene: Fix the DMA state machine lockup for the ATA_CMD_PACKET PIO mode command") 6. 72f79f9e35b ("ahci_xgene: Removing NCQ support from the APM X-Gene SoC AHCI SATA Host Controller driver") In addition, enable PMP support for second generation APM X-Gene SoC. Signed-off-by : Suman Tripathi <stripathi@apm.com> --- drivers/ata/ahci_xgene.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) -- 1.8.2.1 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html