[PATCH v1] ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

[PATCH v1] ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

[Tejun Heo]

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.

-- 
tejun

[PATCH v1] ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

[Tejun Heo]

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

[PATCH v1] ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

[Suman Tripathi]

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



-- 
Thanks,
with regards,
Suman Tripathi

[PATCH v1] ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v1] ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

[Tejun Heo]

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.

-- 
tejun

[PATCH v1] ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

[Catalin Marinas]

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 at 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).

-- 
Catalin

[PATCH v1] ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

[Mark Rutland]

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

[PATCH v1] ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

[Russell King - ARM Linux]

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 at 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. :)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v1] ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

[Catalin Marinas]

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.

-- 
Catalin

[PATCH v1] ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

[Mark Rutland]

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.