Re: [PATCH v10 0/4] ata: Add APM X-Gene SoC SATA host controller support

Re: [PATCH v10 0/4] ata: Add APM X-Gene SoC SATA host controller support

[Loc Ho]

Hi Tejun,

As it had being awhile, any issue with this version of the SATA
drivers before I post the follow on errata patches?

-Loc

On Thu, Jan 16, 2014 at 8:11 AM, Loc Ho <lho@apm.com> wrote:
> This patch adds support for the APM X-Gene SoC SATA host controller. In
> order for the host controller to work, the corresponding PHY driver
> musts also be available.
>
> v10:
>  * Update binding documentation
>
> v9:
>  * Remove ACPI/EFI include files
>  * Remove the IO flush support, interrupt routine, and DTS resources
>  * Remove function xgene_rd, xgene_wr, and xgene_wr_flush
>  * Remove PMP support (function xgene_ahci_qc_issue, xgene_ahci_qc_prep,
>    xgene_ahci_qc_fill_rtf, xgene_ahci_softreset, and xgene_ahci_do_softreset)
>  * Rename function xgene_ahci_enable_phy to xgene_ahci_force_phy_rdy
>  * Clean up hardreset functions
>  * Require v7 of the PHY driver
>
> v8:
>  * Remove _ADDR from defines
>  * Remove define MSTAWAUX_COHERENT_BYPASS_SET and
>    STARAUX_COHERENT_BYPASS_SET and use direct coding
>  * Remove the un-necessary check for DTS boot with built in ACPI table
>  * Switch to use dma_set_mask_and_coherent for setting DMA mask
>  * Remove ACPI table matching code
>  * Update clock-names for sata01clk, sata23clk, and sata45clk
>
> v7:
>  * Update the clock code by toggle the clock
>  * Update the DTS clock mask values due to the clock spilt between host and
>    v5 of the PHY drivers
>
> v6:
>  * Update binding documentation
>  * Change select PHY_XGENE_SATA to PHY_XGENE
>  * Add ULL to constants
>  * Change indentation and comments
>  * Clean up the probe functions a bit more
>  * Remove xgene_ahci_remove function
>  * Add the flush register to DTS
>  * Remove the interrupt-parent from DTS
>
> v5:
>  * Sync up to v3 of the PHY driver
>  * Remove MSLIM wrapper functions
>  * Change the memory shutdown loop to use usleep_range
>  * Use devm_ioremap_resource instead devm_ioremap
>  * Remove suspend/resume functions as not needed
>
> v4:
>  * Remove the ID property in DT
>  * Remove the temporary PHY direct function call and use PHY function
>  * Change printk to pr_debug
>  * Move the IOB flush addresses into the DT
>  * Remove the parameters retrieval function as no longer needed
>  * Remove the header file as no longer needed
>  * Require v2 patch of the SATA PHY driver. Require slightly modification
>    in the Kconfig as it is moved to folder driver/phy and use Kconfig
>    PHY_XGENE_SATA instead SATA_XGENE_PHY.
>
> v3:
>  * Move out the SATA PHY to another driver
>  * Remove the clock-cells entry from DTS
>  * Remove debug wrapper
>  * Remove delay functions wrapper
>  * Clean up resource and IRQ query
>  * Remove query clock name
>  * Switch to use dma_set_mask/dma_coherent_mask
>  * Remove un-necessary devm_kfree
>  * Update GPL license header to v2
>  * Spilt up function xgene_ahci_hardreset
>  * Spilt up function xgene_ahci_probe
>  * Remove all reference of CONFIG_ARCH_MSLIM
>  * Clean up chip revision code
>
> v2:
>  * Clean up file sata_xgene.c with Lindent and etc
>  * Clean up file sata_xgene_serdes.c with Lindent and etc
>  * Add description to each patch
>
> v1:
>  * inital version
>
> Signed-off-by: Loc Ho <lho@apm.com>
> Signed-off-by: Tuan Phan <tphan@apm.com>
> Signed-off-by: Suman Tripathi <stripathi@apm.com>
> ---
> Loc Ho (4):
>   ata: Export required functions by APM X-Gene SATA driver
>   Documentation: Add documentation for APM X-Gene SoC SATA host
>     controller DTS binding
>   ata: Add APM X-Gene SoC SATA host controller driver
>   arm64: Add APM X-Gene SoC SATA host controller DTS entries
>
>  .../devicetree/bindings/ata/apm-xgene.txt          |   70 +++
>  arch/arm64/boot/dts/apm-storm.dtsi                 |   75 +++
>  drivers/ata/Kconfig                                |    8 +
>  drivers/ata/Makefile                               |    1 +
>  drivers/ata/ahci.h                                 |    9 +
>  drivers/ata/libahci.c                              |   16 +-
>  drivers/ata/sata_xgene.c                           |  630 ++++++++++++++++++++
>  7 files changed, 803 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/ata/apm-xgene.txt
>  create mode 100644 drivers/ata/sata_xgene.c
>

Re: [PATCH v10 0/4] ata: Add APM X-Gene SoC SATA host controller support

[Loc Ho]

Hi Tejun,

With regard to make use of the standard ahci_platform and recent Hans'
PHY works, the original version of the driver was an ahci_platform
driver. Unfortunately, the APM X-Gene SATA controller is more
complicate than an standard AHCI controller. In order to make it works
properly, the follow additional programming sequences are required:

1. There are a number of errata that require workaround. Some can be
fixed by adding broken flags while others are better to just wrap
around the existent libahci library routines and not overly polluting
the libahci routines.
2. There are additional controller programming sequences to configure.
2a. By default, RAM are powered down and require brought out of shutdown.
2b. The controller has an additional corresponding PHY part that needs
to be programmed after PHY configuration.
2c. The controller requires extra programming sequence for the
hardreset due to errata.
2d. For the IO flush, it requires additional memory resources.
3. With the current AHCI platform data handing and ARM64 arch support,
if I were to add custom AHCI platform data routine, I would need
equivalent functions and that would make the ARCH support having
knowledge of the SATA code which I don't want. Also, the DTS
processing/scanning in ARCH64 is NULL for platform data and would have
make X-Gene to have its own version which I also don't want.

-Loc

> On Thu, Jan 16, 2014 at 8:11 AM, Loc Ho <lho@apm.com> wrote:
>> This patch adds support for the APM X-Gene SoC SATA host controller. In
>> order for the host controller to work, the corresponding PHY driver
>> musts also be available.
>>
>> v10:
>>  * Update binding documentation
>>
>> v9:
>>  * Remove ACPI/EFI include files
>>  * Remove the IO flush support, interrupt routine, and DTS resources
>>  * Remove function xgene_rd, xgene_wr, and xgene_wr_flush
>>  * Remove PMP support (function xgene_ahci_qc_issue, xgene_ahci_qc_prep,
>>    xgene_ahci_qc_fill_rtf, xgene_ahci_softreset, and xgene_ahci_do_softreset)
>>  * Rename function xgene_ahci_enable_phy to xgene_ahci_force_phy_rdy
>>  * Clean up hardreset functions
>>  * Require v7 of the PHY driver
>>
>> v8:
>>  * Remove _ADDR from defines
>>  * Remove define MSTAWAUX_COHERENT_BYPASS_SET and
>>    STARAUX_COHERENT_BYPASS_SET and use direct coding
>>  * Remove the un-necessary check for DTS boot with built in ACPI table
>>  * Switch to use dma_set_mask_and_coherent for setting DMA mask
>>  * Remove ACPI table matching code
>>  * Update clock-names for sata01clk, sata23clk, and sata45clk
>>
>> v7:
>>  * Update the clock code by toggle the clock
>>  * Update the DTS clock mask values due to the clock spilt between host and
>>    v5 of the PHY drivers
>>
>> v6:
>>  * Update binding documentation
>>  * Change select PHY_XGENE_SATA to PHY_XGENE
>>  * Add ULL to constants
>>  * Change indentation and comments
>>  * Clean up the probe functions a bit more
>>  * Remove xgene_ahci_remove function
>>  * Add the flush register to DTS
>>  * Remove the interrupt-parent from DTS
>>
>> v5:
>>  * Sync up to v3 of the PHY driver
>>  * Remove MSLIM wrapper functions
>>  * Change the memory shutdown loop to use usleep_range
>>  * Use devm_ioremap_resource instead devm_ioremap
>>  * Remove suspend/resume functions as not needed
>>
>> v4:
>>  * Remove the ID property in DT
>>  * Remove the temporary PHY direct function call and use PHY function
>>  * Change printk to pr_debug
>>  * Move the IOB flush addresses into the DT
>>  * Remove the parameters retrieval function as no longer needed
>>  * Remove the header file as no longer needed
>>  * Require v2 patch of the SATA PHY driver. Require slightly modification
>>    in the Kconfig as it is moved to folder driver/phy and use Kconfig
>>    PHY_XGENE_SATA instead SATA_XGENE_PHY.
>>
>> v3:
>>  * Move out the SATA PHY to another driver
>>  * Remove the clock-cells entry from DTS
>>  * Remove debug wrapper
>>  * Remove delay functions wrapper
>>  * Clean up resource and IRQ query
>>  * Remove query clock name
>>  * Switch to use dma_set_mask/dma_coherent_mask
>>  * Remove un-necessary devm_kfree
>>  * Update GPL license header to v2
>>  * Spilt up function xgene_ahci_hardreset
>>  * Spilt up function xgene_ahci_probe
>>  * Remove all reference of CONFIG_ARCH_MSLIM
>>  * Clean up chip revision code
>>
>> v2:
>>  * Clean up file sata_xgene.c with Lindent and etc
>>  * Clean up file sata_xgene_serdes.c with Lindent and etc
>>  * Add description to each patch
>>
>> v1:
>>  * inital version
>>
>> Signed-off-by: Loc Ho <lho@apm.com>
>> Signed-off-by: Tuan Phan <tphan@apm.com>
>> Signed-off-by: Suman Tripathi <stripathi@apm.com>
>> ---
>> Loc Ho (4):
>>   ata: Export required functions by APM X-Gene SATA driver
>>   Documentation: Add documentation for APM X-Gene SoC SATA host
>>     controller DTS binding
>>   ata: Add APM X-Gene SoC SATA host controller driver
>>   arm64: Add APM X-Gene SoC SATA host controller DTS entries
>>
>>  .../devicetree/bindings/ata/apm-xgene.txt          |   70 +++
>>  arch/arm64/boot/dts/apm-storm.dtsi                 |   75 +++
>>  drivers/ata/Kconfig                                |    8 +
>>  drivers/ata/Makefile                               |    1 +
>>  drivers/ata/ahci.h                                 |    9 +
>>  drivers/ata/libahci.c                              |   16 +-
>>  drivers/ata/sata_xgene.c                           |  630 ++++++++++++++++++++
>>  7 files changed, 803 insertions(+), 6 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/ata/apm-xgene.txt
>>  create mode 100644 drivers/ata/sata_xgene.c
>>

Re: [PATCH v10 0/4] ata: Add APM X-Gene SoC SATA host controller support

[Tejun Heo]

Hello, Loc.

On Thu, Feb 13, 2014 at 03:28:01PM -0800, Loc Ho wrote:
> 1. There are a number of errata that require workaround. Some can be
> fixed by adding broken flags while others are better to just wrap
> around the existent libahci library routines and not overly polluting
> the libahci routines.
> 2. There are additional controller programming sequences to configure.
> 2a. By default, RAM are powered down and require brought out of shutdown.
> 2b. The controller has an additional corresponding PHY part that needs
> to be programmed after PHY configuration.

Have you looked at the latest patchset Hans posted?  He added multiple
PHY support and split up init to three steps so that each platform
driver can mix and match as they see fit.  Looking at xgene driver,
sure there are things specific to the driver but there also are
non-insignificant amount of boilerplate code and that's what I'm
primarily concerned about.  It may be okay when you have two or three
drivers duplicating some code but it looks like we could have many
more and I *really* want to avoid the situation where the same piece
of code is copied over N times.  In addition, frankly, not many people
except yourself would care about these drivers once they're merged and
many of these are gonna be painful to test making later refactoring a
lot harder.

> 2c. The controller requires extra programming sequence for the
> hardreset due to errata.
> 2d. For the IO flush, it requires additional memory resources.

Sure, you'll need to override good parts of the driver.  What I'm
saying is please try to reuse whatever you can.  If that takes
refactoring and librarize ahci_platform, please do so and I do see
healthy chunk of duplicated code in the init path.  Please take a look
at Hans' patches and if necessary work with him so that your driver
can be part of the refactoring.

Thanks.

-- 
tejun

Re: [PATCH v10 0/4] ata: Add APM X-Gene SoC SATA host controller support

[Loc Ho]

Hi Tejun,

>
> On Thu, Feb 13, 2014 at 03:28:01PM -0800, Loc Ho wrote:
>> 1. There are a number of errata that require workaround. Some can be
>> fixed by adding broken flags while others are better to just wrap
>> around the existent libahci library routines and not overly polluting
>> the libahci routines.
>> 2. There are additional controller programming sequences to configure.
>> 2a. By default, RAM are powered down and require brought out of shutdown.
>> 2b. The controller has an additional corresponding PHY part that needs
>> to be programmed after PHY configuration.
>
> Have you looked at the latest patchset Hans posted?  He added multiple
> PHY support and split up init to three steps so that each platform
> driver can mix and match as they see fit.  Looking at xgene driver,
> sure there are things specific to the driver but there also are
> non-insignificant amount of boilerplate code and that's what I'm
> primarily concerned about.  It may be okay when you have two or three
> drivers duplicating some code but it looks like we could have many
> more and I *really* want to avoid the situation where the same piece
> of code is copied over N times.  In addition, frankly, not many people
> except yourself would care about these drivers once they're merged and
> many of these are gonna be painful to test making later refactoring a
> lot harder.
>

Ok... I will look into Hans' patch again. To be sure that I am looking
at the correct patch, are you referring to this one:

http://www.spinics.net/lists/linux-ide/msg47628.html

-Loc

Re: [PATCH v10 0/4] ata: Add APM X-Gene SoC SATA host controller support

[Tejun Heo]

On Fri, Feb 14, 2014 at 01:44:08PM -0800, Loc Ho wrote:
> Ok... I will look into Hans' patch again. To be sure that I am looking
> at the correct patch, are you referring to this one:
> 
> http://www.spinics.net/lists/linux-ide/msg47628.html

Yeah, v5 is the latest.

Thanks!

-- 
tejun