All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v17 0/4] ata: Add APM X-Gene SoC AHCI SATA host controller support
@ 2014-03-12 20:19 ` Loc Ho
  0 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-12 20:19 UTC (permalink / raw)
  To: olof, tj, arnd
  Cc: linux-scsi, linux-ide, devicetree, linux-arm-kernel, ddutile,
	jcm, patches, Loc Ho, Tuan Phan, Suman Tripathi

This patch adds support for the APM X-Gene SoC AHCI SATA host controller. In
order for the host controller to work, the corresponding PHY driver
musts also be available. Currently, only Gen3 disk is supported with this
initial version.

v17:
 * Add comment on no support for PM currently
 * Add xgene_ahci_host_stop function to support stopping the controller
 * Remove call to phy_exit as not necessary with new ahci_platform code

v16:
 * Rebase to libata-for-3.15
 * Pull in the PHY DTS patch as the host controller DTS patch depends on it

v15:
 * Rebase to libata next branch
 * Remove field plat_data and PHY from context structure
 * Fix comment on function xgene_ahci_read_id as well as using bit mask to
   clear DEVSLP bit
 * Remove function xgene_ahci_force_phy_rdy and xgene_ahci_phy_restart as not
   required since Gen1/Gen2 support remove for this initial version
 * Update function xgene_ahci_do_hardreset comment
 * Remove Gen1/Gen2 support from function xgene_ahci_do_hardreset
 * Change int to u32 for variable portcmd_saved in function
   xgene_ahci_hardreset
 * Change variable hplat_data to ctx in function xgene_ahci_probe
 * Remove PHY call and using ahci_platform_enable_resource instead
 * Add ahci_patlform_remove_one to driver function .remove
 * Change phy-name to "sata-phy"

v14:
 * Remove the shutdown already check and replace the while loop check with
   msleep in function xgene_ahci_init_memram

v13:
 * Add fully-winged style comment for function xgene_ahci_read_id and
   xgene_ahci_do_hardrest
 * Minor comments update for function xgene_ahci_read_id,
   xgene_ahci_do_hardrest, and xgene_ahci_hardreset
 * NOTE: There is no functional code change.

v12:
 * Remove function xgene_ahci_get_channel and use the ata_port field port_no
 * Update comment for function xgene_ahci_read_id to function comment style
   '/**'
 * Update comment for multiple lines to fully-winged style

v11:
 * Drop the export functions requirement with libachi
 * Change CONFIG_SATA_XGENE to CONFIG_AHCI_XGENE
 * Rename file sata_xgene.c to ahci_xgene.c
 * Convert to use Hans De Geode version 5 ahci_platform code re-factor changes
   to reduce code duplication. For extra context, use plat_data to store our
   context. The probe function follows the ahci_sunxi implementation. A number
   of code fragments update to reflect this change.
 * Update comment for function xgene_ahci_read_id
 * Minor code move around in function xgene_ahci_do_hardreset and use
   ATA_BUSY instead 0x80
 * Fix hardreset to use start_engine function pointer as required due to newer
   kernel rebased
 * Fix the set DMA mask for 32-bit as well

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):
  arm64: Add APM X-Gene SoC 15Gbps Multi-purpose PHY DTS entries
  Documentation: Add documentation for the APM X-Gene SoC SATA host
    controller DTS binding
  ata: Add APM X-Gene SoC AHCI SATA host controller driver
  arm64: Add APM X-Gene SoC AHCI SATA host controller DTS entries

 .../devicetree/bindings/ata/apm-xgene.txt          |   70 +++
 arch/arm64/boot/dts/apm-storm.dtsi                 |  150 ++++++
 drivers/ata/Kconfig                                |    7 +
 drivers/ata/Makefile                               |    1 +
 drivers/ata/ahci_xgene.c                           |  497 ++++++++++++++++++++
 5 files changed, 725 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ata/apm-xgene.txt
 create mode 100644 drivers/ata/ahci_xgene.c


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v17 0/4] ata: Add APM X-Gene SoC AHCI SATA host controller support
@ 2014-03-12 20:19 ` Loc Ho
  0 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-12 20:19 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds support for the APM X-Gene SoC AHCI SATA host controller. In
order for the host controller to work, the corresponding PHY driver
musts also be available. Currently, only Gen3 disk is supported with this
initial version.

v17:
 * Add comment on no support for PM currently
 * Add xgene_ahci_host_stop function to support stopping the controller
 * Remove call to phy_exit as not necessary with new ahci_platform code

v16:
 * Rebase to libata-for-3.15
 * Pull in the PHY DTS patch as the host controller DTS patch depends on it

v15:
 * Rebase to libata next branch
 * Remove field plat_data and PHY from context structure
 * Fix comment on function xgene_ahci_read_id as well as using bit mask to
   clear DEVSLP bit
 * Remove function xgene_ahci_force_phy_rdy and xgene_ahci_phy_restart as not
   required since Gen1/Gen2 support remove for this initial version
 * Update function xgene_ahci_do_hardreset comment
 * Remove Gen1/Gen2 support from function xgene_ahci_do_hardreset
 * Change int to u32 for variable portcmd_saved in function
   xgene_ahci_hardreset
 * Change variable hplat_data to ctx in function xgene_ahci_probe
 * Remove PHY call and using ahci_platform_enable_resource instead
 * Add ahci_patlform_remove_one to driver function .remove
 * Change phy-name to "sata-phy"

v14:
 * Remove the shutdown already check and replace the while loop check with
   msleep in function xgene_ahci_init_memram

v13:
 * Add fully-winged style comment for function xgene_ahci_read_id and
   xgene_ahci_do_hardrest
 * Minor comments update for function xgene_ahci_read_id,
   xgene_ahci_do_hardrest, and xgene_ahci_hardreset
 * NOTE: There is no functional code change.

v12:
 * Remove function xgene_ahci_get_channel and use the ata_port field port_no
 * Update comment for function xgene_ahci_read_id to function comment style
   '/**'
 * Update comment for multiple lines to fully-winged style

v11:
 * Drop the export functions requirement with libachi
 * Change CONFIG_SATA_XGENE to CONFIG_AHCI_XGENE
 * Rename file sata_xgene.c to ahci_xgene.c
 * Convert to use Hans De Geode version 5 ahci_platform code re-factor changes
   to reduce code duplication. For extra context, use plat_data to store our
   context. The probe function follows the ahci_sunxi implementation. A number
   of code fragments update to reflect this change.
 * Update comment for function xgene_ahci_read_id
 * Minor code move around in function xgene_ahci_do_hardreset and use
   ATA_BUSY instead 0x80
 * Fix hardreset to use start_engine function pointer as required due to newer
   kernel rebased
 * Fix the set DMA mask for 32-bit as well

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):
  arm64: Add APM X-Gene SoC 15Gbps Multi-purpose PHY DTS entries
  Documentation: Add documentation for the APM X-Gene SoC SATA host
    controller DTS binding
  ata: Add APM X-Gene SoC AHCI SATA host controller driver
  arm64: Add APM X-Gene SoC AHCI SATA host controller DTS entries

 .../devicetree/bindings/ata/apm-xgene.txt          |   70 +++
 arch/arm64/boot/dts/apm-storm.dtsi                 |  150 ++++++
 drivers/ata/Kconfig                                |    7 +
 drivers/ata/Makefile                               |    1 +
 drivers/ata/ahci_xgene.c                           |  497 ++++++++++++++++++++
 5 files changed, 725 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ata/apm-xgene.txt
 create mode 100644 drivers/ata/ahci_xgene.c

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v17 1/4] arm64: Add APM X-Gene SoC 15Gbps Multi-purpose PHY DTS entries
  2014-03-12 20:19 ` Loc Ho
@ 2014-03-12 20:19   ` Loc Ho
  -1 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-12 20:19 UTC (permalink / raw)
  To: olof, tj, arnd
  Cc: linux-scsi, linux-ide, devicetree, linux-arm-kernel, ddutile,
	jcm, patches, Loc Ho, Tuan Phan, Suman Tripathi

This patch adds the DTS entries for the APM X-Gene SoC 15Gbps Multi-purpose
PHY driver. The PHY for SATA controller 2 and 3 are enabled by default.

Signed-off-by: Loc Ho <lho@apm.com>
Signed-off-by: Tuan Phan <tphan@apm.com>
Signed-off-by: Suman Tripathi <stripathi@apm.com>
---
 arch/arm64/boot/dts/apm-storm.dtsi |   75 ++++++++++++++++++++++++++++++++++++
 1 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index d37d736..c78ddcf 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -176,6 +176,51 @@
 				reg-names = "csr-reg";
 				clock-output-names = "eth8clk";
 			};
+
+			sataphy1clk: sataphy1clk@1f21c000 {
+				compatible = "apm,xgene-device-clock";
+				#clock-cells = <1>;
+				clocks = <&socplldiv2 0>;
+				clock-names = "socplldiv2";
+				reg = <0x0 0x1f21c000 0x0 0x1000>;
+				reg-names = "csr-reg";
+				clock-output-names = "sataphy1clk";
+				status = "disabled";
+				csr-offset = <0x4>;
+				csr-mask = <0x00>;
+				enable-offset = <0x0>;
+				enable-mask = <0x06>;
+			};
+
+			sataphy2clk: sataphy1clk@1f22c000 {
+				compatible = "apm,xgene-device-clock";
+				#clock-cells = <1>;
+				clocks = <&socplldiv2 0>;
+				clock-names = "socplldiv2";
+				reg = <0x0 0x1f22c000 0x0 0x1000>;
+				reg-names = "csr-reg";
+				clock-output-names = "sataphy2clk";
+				status = "ok";
+				csr-offset = <0x4>;
+				csr-mask = <0x3a>;
+				enable-offset = <0x0>;
+				enable-mask = <0x06>;
+			};
+
+			sataphy3clk: sataphy1clk@1f23c000 {
+				compatible = "apm,xgene-device-clock";
+				#clock-cells = <1>;
+				clocks = <&socplldiv2 0>;
+				clock-names = "socplldiv2";
+				reg = <0x0 0x1f23c000 0x0 0x1000>;
+				reg-names = "csr-reg";
+				clock-output-names = "sataphy3clk";
+				status = "ok";
+				csr-offset = <0x4>;
+				csr-mask = <0x3a>;
+				enable-offset = <0x0>;
+				enable-mask = <0x06>;
+			};
 		};
 
 		serial0: serial@1c020000 {
@@ -187,5 +232,35 @@
 			interrupt-parent = <&gic>;
 			interrupts = <0x0 0x4c 0x4>;
 		};
+
+		phy1: phy@1f21a000 {
+			compatible = "apm,xgene-phy";
+			reg = <0x0 0x1f21a000 0x0 0x100>;
+			#phy-cells = <1>;
+			clocks = <&sataphy1clk 0>;
+			status = "disabled";
+			apm,tx-boost-gain = <30 30 30 30 30 30>;
+			apm,tx-eye-tuning = <2 10 10 2 10 10>;
+		};
+
+		phy2: phy@1f22a000 {
+			compatible = "apm,xgene-phy";
+			reg = <0x0 0x1f22a000 0x0 0x100>;
+			#phy-cells = <1>;
+			clocks = <&sataphy2clk 0>;
+			status = "ok";
+			apm,tx-boost-gain = <30 30 30 30 30 30>;
+			apm,tx-eye-tuning = <1 10 10 2 10 10>;
+		};
+
+		phy3: phy@1f23a000 {
+			compatible = "apm,xgene-phy";
+			reg = <0x0 0x1f23a000 0x0 0x100>;
+			#phy-cells = <1>;
+			clocks = <&sataphy3clk 0>;
+			status = "ok";
+			apm,tx-boost-gain = <31 31 31 31 31 31>;
+			apm,tx-eye-tuning = <2 10 10 2 10 10>;
+		};
 	};
 };
-- 
1.5.5


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v17 1/4] arm64: Add APM X-Gene SoC 15Gbps Multi-purpose PHY DTS entries
@ 2014-03-12 20:19   ` Loc Ho
  0 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-12 20:19 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds the DTS entries for the APM X-Gene SoC 15Gbps Multi-purpose
PHY driver. The PHY for SATA controller 2 and 3 are enabled by default.

Signed-off-by: Loc Ho <lho@apm.com>
Signed-off-by: Tuan Phan <tphan@apm.com>
Signed-off-by: Suman Tripathi <stripathi@apm.com>
---
 arch/arm64/boot/dts/apm-storm.dtsi |   75 ++++++++++++++++++++++++++++++++++++
 1 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index d37d736..c78ddcf 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -176,6 +176,51 @@
 				reg-names = "csr-reg";
 				clock-output-names = "eth8clk";
 			};
+
+			sataphy1clk: sataphy1clk at 1f21c000 {
+				compatible = "apm,xgene-device-clock";
+				#clock-cells = <1>;
+				clocks = <&socplldiv2 0>;
+				clock-names = "socplldiv2";
+				reg = <0x0 0x1f21c000 0x0 0x1000>;
+				reg-names = "csr-reg";
+				clock-output-names = "sataphy1clk";
+				status = "disabled";
+				csr-offset = <0x4>;
+				csr-mask = <0x00>;
+				enable-offset = <0x0>;
+				enable-mask = <0x06>;
+			};
+
+			sataphy2clk: sataphy1clk at 1f22c000 {
+				compatible = "apm,xgene-device-clock";
+				#clock-cells = <1>;
+				clocks = <&socplldiv2 0>;
+				clock-names = "socplldiv2";
+				reg = <0x0 0x1f22c000 0x0 0x1000>;
+				reg-names = "csr-reg";
+				clock-output-names = "sataphy2clk";
+				status = "ok";
+				csr-offset = <0x4>;
+				csr-mask = <0x3a>;
+				enable-offset = <0x0>;
+				enable-mask = <0x06>;
+			};
+
+			sataphy3clk: sataphy1clk at 1f23c000 {
+				compatible = "apm,xgene-device-clock";
+				#clock-cells = <1>;
+				clocks = <&socplldiv2 0>;
+				clock-names = "socplldiv2";
+				reg = <0x0 0x1f23c000 0x0 0x1000>;
+				reg-names = "csr-reg";
+				clock-output-names = "sataphy3clk";
+				status = "ok";
+				csr-offset = <0x4>;
+				csr-mask = <0x3a>;
+				enable-offset = <0x0>;
+				enable-mask = <0x06>;
+			};
 		};
 
 		serial0: serial at 1c020000 {
@@ -187,5 +232,35 @@
 			interrupt-parent = <&gic>;
 			interrupts = <0x0 0x4c 0x4>;
 		};
+
+		phy1: phy at 1f21a000 {
+			compatible = "apm,xgene-phy";
+			reg = <0x0 0x1f21a000 0x0 0x100>;
+			#phy-cells = <1>;
+			clocks = <&sataphy1clk 0>;
+			status = "disabled";
+			apm,tx-boost-gain = <30 30 30 30 30 30>;
+			apm,tx-eye-tuning = <2 10 10 2 10 10>;
+		};
+
+		phy2: phy at 1f22a000 {
+			compatible = "apm,xgene-phy";
+			reg = <0x0 0x1f22a000 0x0 0x100>;
+			#phy-cells = <1>;
+			clocks = <&sataphy2clk 0>;
+			status = "ok";
+			apm,tx-boost-gain = <30 30 30 30 30 30>;
+			apm,tx-eye-tuning = <1 10 10 2 10 10>;
+		};
+
+		phy3: phy at 1f23a000 {
+			compatible = "apm,xgene-phy";
+			reg = <0x0 0x1f23a000 0x0 0x100>;
+			#phy-cells = <1>;
+			clocks = <&sataphy3clk 0>;
+			status = "ok";
+			apm,tx-boost-gain = <31 31 31 31 31 31>;
+			apm,tx-eye-tuning = <2 10 10 2 10 10>;
+		};
 	};
 };
-- 
1.5.5

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v17 2/4] Documentation: Add documentation for the APM X-Gene SoC SATA host controller DTS binding
  2014-03-12 20:19   ` Loc Ho
@ 2014-03-12 20:19     ` Loc Ho
  -1 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-12 20:19 UTC (permalink / raw)
  To: olof, tj, arnd
  Cc: linux-scsi, linux-ide, devicetree, linux-arm-kernel, ddutile,
	jcm, patches, Loc Ho, Tuan Phan, Suman Tripathi

This patch adds documentation for the APM X-Gene SoC SATA host controller DTS
binding.

Signed-off-by: Loc Ho <lho@apm.com>
Signed-off-by: Tuan Phan <tphan@apm.com>
Signed-off-by: Suman Tripathi <stripathi@apm.com>
---
 .../devicetree/bindings/ata/apm-xgene.txt          |   70 ++++++++++++++++++++
 1 files changed, 70 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ata/apm-xgene.txt

diff --git a/Documentation/devicetree/bindings/ata/apm-xgene.txt b/Documentation/devicetree/bindings/ata/apm-xgene.txt
new file mode 100644
index 0000000..633eb3b
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/apm-xgene.txt
@@ -0,0 +1,70 @@
+* APM X-Gene 6.0 Gb/s SATA host controller nodes
+
+SATA host controller nodes are defined to describe on-chip Serial ATA
+controllers. Each SATA controller (pair of ports) have its own node.
+
+Required properties:
+- compatible		: Shall contain:
+  * "apm,xgene-ahci-sgmii" if mux'ed with SGMII
+  * "apm,xgene-ahci-pcie" if mux'ed with PCIe
+- reg			: First memory resource shall be the AHCI memory
+			  resource.
+			  Second memory resource shall be the host controller
+			  memory resource.
+- interrupts		: Interrupt-specifier for SATA host controller IRQ.
+- clocks		: Reference to the clock entry.
+- phys			: A list of phandles + phy-specifiers, one for each
+			  entry in phy-names.
+- phy-names		: Should contain:
+  * "sata-6g" for the SATA 6.0Gbps PHY
+
+Optional properties:
+- status		: Shall be "ok" if enabled or "disabled" if disabled.
+			  Default is "ok".
+- interrupt-parent	: Interrupt controller.
+
+Example:
+		sataclk: sataclk {
+			compatible = "fixed-clock";
+			#clock-cells = <1>;
+			clock-frequency = <100000000>;
+			clock-output-names = "sataclk";
+		};
+
+		phy2: phy@1f22a000 {
+			compatible = "apm,xgene-phy";
+			reg = <0x0 0x1f22a000 0x0 0x100>,
+			      <0x0 0x1f22c000 0x0 0x100>;
+			#phy-cells = <1>;
+		};
+
+		phy3: phy@1f23a000 {
+			compatible = "apm,xgene-phy";
+			reg = <0x0 0x1f23a000 0x0 0x100>,
+			      <0x0 0x1f23c000 0x0 0x100>;
+			#phy-cells = <1>;
+		};
+
+		sata2: sata@1a400000 {
+			compatible = "apm,xgene-ahci-sgmii";
+			reg = <0x0 0x1a400000 0x0 0x1000>,
+			      <0x0 0x1f220000 0x0 0x10000>;
+			interrupt-parent = <&gic>;
+			interrupts = <0x0 0x87 0x4>;
+			status = "ok";
+			clocks = <&sataclk 0>;
+			phys = <&phy2 0>;
+			phy-names = "sata-6g";
+		};
+
+		sata3: sata@1a800000 {
+			compatible = "apm,xgene-ahci-pcie";
+			reg = <0x0 0x1a800000 0x0 0x1000>,
+			      <0x0 0x1f230000 0x0 0x10000>;
+			interrupt-parent = <&gic>;
+			interrupts = <0x0 0x88 0x4>;
+			status = "ok";
+			clocks = <&sataclk 0>;
+			phys = <&phy3 0>;
+			phy-names = "sata-6g";
+		};
-- 
1.5.5


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v17 2/4] Documentation: Add documentation for the APM X-Gene SoC SATA host controller DTS binding
@ 2014-03-12 20:19     ` Loc Ho
  0 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-12 20:19 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds documentation for the APM X-Gene SoC SATA host controller DTS
binding.

Signed-off-by: Loc Ho <lho@apm.com>
Signed-off-by: Tuan Phan <tphan@apm.com>
Signed-off-by: Suman Tripathi <stripathi@apm.com>
---
 .../devicetree/bindings/ata/apm-xgene.txt          |   70 ++++++++++++++++++++
 1 files changed, 70 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ata/apm-xgene.txt

diff --git a/Documentation/devicetree/bindings/ata/apm-xgene.txt b/Documentation/devicetree/bindings/ata/apm-xgene.txt
new file mode 100644
index 0000000..633eb3b
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/apm-xgene.txt
@@ -0,0 +1,70 @@
+* APM X-Gene 6.0 Gb/s SATA host controller nodes
+
+SATA host controller nodes are defined to describe on-chip Serial ATA
+controllers. Each SATA controller (pair of ports) have its own node.
+
+Required properties:
+- compatible		: Shall contain:
+  * "apm,xgene-ahci-sgmii" if mux'ed with SGMII
+  * "apm,xgene-ahci-pcie" if mux'ed with PCIe
+- reg			: First memory resource shall be the AHCI memory
+			  resource.
+			  Second memory resource shall be the host controller
+			  memory resource.
+- interrupts		: Interrupt-specifier for SATA host controller IRQ.
+- clocks		: Reference to the clock entry.
+- phys			: A list of phandles + phy-specifiers, one for each
+			  entry in phy-names.
+- phy-names		: Should contain:
+  * "sata-6g" for the SATA 6.0Gbps PHY
+
+Optional properties:
+- status		: Shall be "ok" if enabled or "disabled" if disabled.
+			  Default is "ok".
+- interrupt-parent	: Interrupt controller.
+
+Example:
+		sataclk: sataclk {
+			compatible = "fixed-clock";
+			#clock-cells = <1>;
+			clock-frequency = <100000000>;
+			clock-output-names = "sataclk";
+		};
+
+		phy2: phy at 1f22a000 {
+			compatible = "apm,xgene-phy";
+			reg = <0x0 0x1f22a000 0x0 0x100>,
+			      <0x0 0x1f22c000 0x0 0x100>;
+			#phy-cells = <1>;
+		};
+
+		phy3: phy at 1f23a000 {
+			compatible = "apm,xgene-phy";
+			reg = <0x0 0x1f23a000 0x0 0x100>,
+			      <0x0 0x1f23c000 0x0 0x100>;
+			#phy-cells = <1>;
+		};
+
+		sata2: sata at 1a400000 {
+			compatible = "apm,xgene-ahci-sgmii";
+			reg = <0x0 0x1a400000 0x0 0x1000>,
+			      <0x0 0x1f220000 0x0 0x10000>;
+			interrupt-parent = <&gic>;
+			interrupts = <0x0 0x87 0x4>;
+			status = "ok";
+			clocks = <&sataclk 0>;
+			phys = <&phy2 0>;
+			phy-names = "sata-6g";
+		};
+
+		sata3: sata at 1a800000 {
+			compatible = "apm,xgene-ahci-pcie";
+			reg = <0x0 0x1a800000 0x0 0x1000>,
+			      <0x0 0x1f230000 0x0 0x10000>;
+			interrupt-parent = <&gic>;
+			interrupts = <0x0 0x88 0x4>;
+			status = "ok";
+			clocks = <&sataclk 0>;
+			phys = <&phy3 0>;
+			phy-names = "sata-6g";
+		};
-- 
1.5.5

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
  2014-03-12 20:19     ` Loc Ho
@ 2014-03-12 20:19       ` Loc Ho
  -1 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-12 20:19 UTC (permalink / raw)
  To: olof, tj, arnd
  Cc: linux-scsi, linux-ide, devicetree, linux-arm-kernel, ddutile,
	jcm, patches, Loc Ho, Tuan Phan, Suman Tripathi

This patch adds support for the APM X-Gene SoC AHCI SATA host controller
driver. It requires the corresponding APM X-Gene SoC PHY driver. This
initial version only supports Gen3 speed.

Signed-off-by: Loc Ho <lho@apm.com>
Signed-off-by: Tuan Phan <tphan@apm.com>
Signed-off-by: Suman Tripathi <stripathi@apm.com>
---
 drivers/ata/Kconfig      |    7 +
 drivers/ata/Makefile     |    1 +
 drivers/ata/ahci_xgene.c |  497 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 505 insertions(+), 0 deletions(-)
 create mode 100644 drivers/ata/ahci_xgene.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 93fc2f0..9de4ca5 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -115,6 +115,13 @@ config AHCI_SUNXI
 
 	  If unsure, say N.
 
+config AHCI_XGENE
+	tristate "APM X-Gene 6.0Gbps AHCI SATA host controller support"
+	depends on SATA_AHCI_PLATFORM && (ARM64 || COMPILE_TEST)
+	select PHY_XGENE
+	help
+	 This option enables support for APM X-Gene SoC SATA host controller.
+
 config SATA_FSL
 	tristate "Freescale 3.0Gbps SATA support"
 	depends on FSL_SOC
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 246050b..72b423b 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_SATA_DWC)		+= sata_dwc_460ex.o
 obj-$(CONFIG_SATA_HIGHBANK)	+= sata_highbank.o libahci.o
 obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o
 obj-$(CONFIG_AHCI_SUNXI)	+= ahci_sunxi.o
+obj-$(CONFIG_AHCI_XGENE)	+= ahci_xgene.o
 
 # SFF w/ custom DMA
 obj-$(CONFIG_PDC_ADMA)		+= pdc_adma.o
diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
new file mode 100644
index 0000000..e0e637a
--- /dev/null
+++ b/drivers/ata/ahci_xgene.c
@@ -0,0 +1,497 @@
+/*
+ * AppliedMicro X-Gene SoC SATA Host Controller Driver
+ *
+ * Copyright (c) 2014, Applied Micro Circuits Corporation
+ * Author: Loc Ho <lho@apm.com>
+ *         Tuan Phan <tphan@apm.com>
+ *         Suman Tripathi <stripathi@apm.com>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * NOTE: PM support is not currently available.
+ *
+ */
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/ahci_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/phy/phy.h>
+#include "ahci.h"
+
+/* Controller who PHY shared with SGMII Ethernet PHY */
+#define XGENE_AHCI_SGMII_DTS		"apm,xgene-ahci-sgmii"
+
+/* Controller who PHY (internal reference clock macro) shared with PCIe */
+#define XGENE_AHCI_PCIE_DTS		"apm,xgene-ahci-pcie"
+
+/* Max # of disk per a controller */
+#define MAX_AHCI_CHN_PERCTR		2
+
+#define SATA_ENET_MUX_OFFSET		0x00007000
+#define SATA_DIAG_OFFSET		0x0000D000
+#define SATA_GLB_OFFSET			0x0000D850
+#define SATA_SHIM_OFFSET		0x0000E000
+#define SATA_MASTER_OFFSET		0x0000F000
+#define SATA_PORT0_OFFSET		0x00000100
+#define SATA_PORT1_OFFSET		0x00000180
+
+/* MUX CSR */
+#define SATA_ENET_CONFIG_REG		0x00000000
+#define  CFG_SATA_ENET_SELECT_MASK	0x00000001
+
+/* SATA host controller CSR */
+#define SLVRDERRATTRIBUTES		0x00000000
+#define SLVWRERRATTRIBUTES		0x00000004
+#define MSTRDERRATTRIBUTES		0x00000008
+#define MSTWRERRATTRIBUTES		0x0000000c
+#define BUSCTLREG			0x00000014
+#define IOFMSTRWAUX			0x00000018
+#define INTSTATUSMASK			0x0000002c
+#define ERRINTSTATUS			0x00000030
+#define ERRINTSTATUSMASK		0x00000034
+
+/* SATA host AHCI CSR */
+#define PORTCFG				0x000000a4
+#define  PORTADDR_SET(dst, src) \
+		(((dst) & ~0x0000003f) | (((u32)(src)) & 0x0000003f))
+#define PORTPHY1CFG		0x000000a8
+#define PORTPHY1CFG_FRCPHYRDY_SET(dst, src) \
+		(((dst) & ~0x00100000) | (((u32)(src) << 0x14) & 0x00100000))
+#define PORTPHY2CFG			0x000000ac
+#define PORTPHY3CFG			0x000000b0
+#define PORTPHY4CFG			0x000000b4
+#define PORTPHY5CFG			0x000000b8
+#define SCTL0				0x0000012C
+#define PORTPHY5CFG_RTCHG_SET(dst, src) \
+		(((dst) & ~0xfff00000) | (((u32)(src) << 0x14) & 0xfff00000))
+#define PORTAXICFG_EN_CONTEXT_SET(dst, src) \
+		(((dst) & ~0x01000000) | (((u32)(src) << 0x18) & 0x01000000))
+#define PORTAXICFG			0x000000bc
+#define PORTAXICFG_OUTTRANS_SET(dst, src) \
+		(((dst) & ~0x00f00000) | (((u32)(src) << 0x14) & 0x00f00000))
+
+/* SATA host controller slave CSR */
+#define INT_SLV_TMOMASK			0x00000010
+
+/* SATA global diagnostic CSR */
+#define CFG_MEM_RAM_SHUTDOWN		0x00000070
+#define BLOCK_MEM_RDY			0x00000074
+
+struct xgene_ahci_context {
+	struct ahci_host_priv *hpriv;
+	struct device *dev;
+	void __iomem *csr_base;		/* CSR base address of IP */
+};
+
+static int xgene_ahci_init_memram(struct xgene_ahci_context *ctx)
+{
+	void __iomem *diagcsr = ctx->csr_base + SATA_DIAG_OFFSET;
+
+	dev_dbg(ctx->dev, "Release memory from shutdown\n");
+	writel(0x0, diagcsr + CFG_MEM_RAM_SHUTDOWN);
+	readl(diagcsr + CFG_MEM_RAM_SHUTDOWN); /* Force a barrier */
+	msleep(1);	/* reset may take up to 1ms */
+	if (readl(diagcsr + BLOCK_MEM_RDY) != 0xFFFFFFFF) {
+		dev_err(ctx->dev, "failed to release memory from shutdown\n");
+		return -ENODEV;
+	}
+	return 0;
+}
+
+/**
+ * xgene_ahci_read_id - Read ID data from the specified device
+ * @dev: device
+ * @tf: proposed taskfile
+ * @id: data buffer
+ *
+ * This custom read ID function is required due to the fact that the HW
+ * does not support DEVSLP and the controller state machine may get stuck
+ * after processing the ID query command.
+ */
+static unsigned int xgene_ahci_read_id(struct ata_device *dev,
+				       struct ata_taskfile *tf, u16 *id)
+{
+	u32 err_mask;
+	void __iomem *port_mmio = ahci_port_base(dev->link->ap);
+
+	err_mask = ata_do_dev_read_id(dev, tf, id);
+	if (err_mask)
+		return err_mask;
+
+	/*
+	 * Mask reserved area. Word78 spec of Link Power Management
+	 * bit15-8: reserved
+	 * bit7: NCQ autosence
+	 * bit6: Software settings preservation supported
+	 * bit5: reserved
+	 * bit4: In-order sata delivery supported
+	 * bit3: DIPM requests supported
+	 * bit2: DMA Setup FIS Auto-Activate optimization supported
+	 * bit1: DMA Setup FIX non-Zero buffer offsets supported
+	 * bit0: Reserved
+	 *
+	 * Clear reserved bit 8 (DEVSLP bit) as we don't support DEVSLP
+	 */
+	id[ATA_ID_FEATURE_SUPP] &= ~(1 << 8);
+
+	/*
+	 * Due to HW errata, restart the port if no other command active.
+	 * Otherwise the controller may get stuck.
+	 */
+	if (!readl(port_mmio + PORT_CMD_ISSUE)) {
+		writel(PORT_CMD_FIS_RX, port_mmio + PORT_CMD);
+		readl(port_mmio + PORT_CMD);	/* Force a barrier */
+		writel(PORT_CMD_FIS_RX | PORT_CMD_START, port_mmio + PORT_CMD);
+		readl(port_mmio + PORT_CMD);	/* Force a barrier */
+	}
+	return 0;
+}
+
+static void xgene_ahci_set_phy_cfg(struct xgene_ahci_context *ctx, int channel)
+{
+	void __iomem *mmio = ctx->hpriv->mmio;
+	u32 val;
+
+	dev_dbg(ctx->dev, "port configure mmio 0x%p channel %d\n",
+		mmio, channel);
+	val = readl(mmio + PORTCFG);
+	val = PORTADDR_SET(val, channel == 0 ? 2 : 3);
+	writel(val, mmio + PORTCFG);
+	readl(mmio + PORTCFG);  /* Force a barrier */
+	/* Disable fix rate */
+	writel(0x0001fffe, mmio + PORTPHY1CFG);
+	readl(mmio + PORTPHY1CFG); /* Force a barrier */
+	writel(0x5018461c, mmio + PORTPHY2CFG);
+	readl(mmio + PORTPHY2CFG); /* Force a barrier */
+	writel(0x1c081907, mmio + PORTPHY3CFG);
+	readl(mmio + PORTPHY3CFG); /* Force a barrier */
+	writel(0x1c080815, mmio + PORTPHY4CFG);
+	readl(mmio + PORTPHY4CFG); /* Force a barrier */
+	/* Set window negotiation */
+	val = readl(mmio + PORTPHY5CFG);
+	val = PORTPHY5CFG_RTCHG_SET(val, 0x300);
+	writel(val, mmio + PORTPHY5CFG);
+	readl(mmio + PORTPHY5CFG); /* Force a barrier */
+	val = readl(mmio + PORTAXICFG);
+	val = PORTAXICFG_EN_CONTEXT_SET(val, 0x1); /* Enable context mgmt */
+	val = PORTAXICFG_OUTTRANS_SET(val, 0xe); /* Set outstanding */
+	writel(val, mmio + PORTAXICFG);
+	readl(mmio + PORTAXICFG); /* Force a barrier */
+}
+
+/**
+ * xgene_ahci_do_hardreset - Issue the actual COMRESET
+ * @link: link to reset
+ * @deadline: deadline jiffies for the operation
+ * @online: Return value to indicate if device online
+ *
+ * Due to the limitation of the hardware PHY, a difference set of setting is
+ * required for each supported disk speed - Gen3 (6.0Gbps), Gen2 (3.0Gbps),
+ * and Gen1 (1.5Gbps). Otherwise during long IO stress test, the PHY will
+ * report disparity error and etc. In addition, during COMRESET, there can
+ * be error reported in the register PORT_SCR_ERR. For SERR_DISPARITY and
+ * SERR_10B_8B_ERR, the PHY receiver line must be reseted. The following
+ * algorithm is followed to proper configure the hardware PHY during COMRESET:
+ *
+ * Alg Part 1:
+ * 1. Start the PHY at Gen3 speed (default setting)
+ * 2. Issue the COMRESET
+ * 3. If no link, go to Alg Part 3
+ * 4. If link up, determine if the negotiated speed matches the PHY
+ *    configured speed
+ * 5. If they matched, go to Alg Part 2
+ * 6. If they do not matched and first time, configure the PHY for the linked
+ *    up disk speed and repeat step 2
+ * 7. Go to Alg Part 2
+ *
+ * Alg Part 2:
+ * 1. On link up, if there are any SERR_DISPARITY and SERR_10B_8B_ERR error
+ *    reported in the register PORT_SCR_ERR, then reset the PHY receiver line
+ * 2. Go to Alg Part 3
+ *
+ * Alg Part 3:
+ * 1. Clear any pending from register PORT_SCR_ERR.
+ *
+ * NOTE: First the initial version, we will NOT support Gen1/Gen2. In addition
+ *       and until the underlying PHY support an method to reset the receiver
+ *       line, on detection of SERR_DISPARITY or SERR_10B_8B_ERR error,
+ *       an warning message will be printed.
+ */
+static int xgene_ahci_do_hardreset(struct ata_link *link,
+				   unsigned long deadline, bool *online)
+{
+	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
+	struct ata_port *ap = link->ap;
+	struct ahci_host_priv *hpriv = ap->host->private_data;
+	struct xgene_ahci_context *ctx = hpriv->plat_data;
+	struct ahci_port_priv *pp = ap->private_data;
+	u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
+	void __iomem *port_mmio = ahci_port_base(ap);
+	struct ata_taskfile tf;
+	int rc;
+	u32 val;
+
+	/* clear D2H reception area to properly wait for D2H FIS */
+	ata_tf_init(link->device, &tf);
+	tf.command = ATA_BUSY;
+	ata_tf_to_fis(&tf, 0, 0, d2h_fis);
+	rc = sata_link_hardreset(link, timing, deadline, online,
+				 ahci_check_ready);
+
+	val = readl(port_mmio + PORT_SCR_ERR);
+	if (val & (SERR_DISPARITY | SERR_10B_8B_ERR))
+		dev_warn(ctx->dev, "link has error\n");
+
+	/* clear all errors if any pending */
+	val = readl(port_mmio + PORT_SCR_ERR);
+	writel(val, port_mmio + PORT_SCR_ERR);
+
+	return rc;
+}
+
+static int xgene_ahci_hardreset(struct ata_link *link, unsigned int *class,
+				unsigned long deadline)
+{
+	struct ata_port *ap = link->ap;
+        struct ahci_host_priv *hpriv = ap->host->private_data;
+	void __iomem *port_mmio = ahci_port_base(ap);
+	bool online;
+	int rc;
+	u32 portcmd_saved;
+	u32 portclb_saved;
+	u32 portclbhi_saved;
+	u32 portrxfis_saved;
+	u32 portrxfishi_saved;
+
+	/* As hardreset resets these CSR, save it to restore later */
+	portcmd_saved = readl(port_mmio + PORT_CMD);
+	portclb_saved = readl(port_mmio + PORT_LST_ADDR);
+	portclbhi_saved = readl(port_mmio + PORT_LST_ADDR_HI);
+	portrxfis_saved = readl(port_mmio + PORT_FIS_ADDR);
+	portrxfishi_saved = readl(port_mmio + PORT_FIS_ADDR_HI);
+
+	ahci_stop_engine(ap);
+
+	rc = xgene_ahci_do_hardreset(link, deadline, &online);
+
+	/* As controller hardreset clears them, restore them */
+	writel(portcmd_saved, port_mmio + PORT_CMD);
+	writel(portclb_saved, port_mmio + PORT_LST_ADDR);
+	writel(portclbhi_saved, port_mmio + PORT_LST_ADDR_HI);
+	writel(portrxfis_saved, port_mmio + PORT_FIS_ADDR);
+	writel(portrxfishi_saved, port_mmio + PORT_FIS_ADDR_HI);
+
+	hpriv->start_engine(ap);
+
+	if (online)
+		*class = ahci_dev_classify(ap);
+
+	return rc;
+}
+
+static void xgene_ahci_host_stop(struct ata_host *host)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+
+	ahci_platform_disable_resources(hpriv);
+}
+
+static struct ata_port_operations xgene_ahci_ops = {
+	.inherits = &ahci_ops,
+	.host_stop = xgene_ahci_host_stop,
+	.hardreset = xgene_ahci_hardreset,
+	.read_id = xgene_ahci_read_id,
+};
+
+static const struct ata_port_info xgene_ahci_port_info = {
+	AHCI_HFLAGS(AHCI_HFLAG_NO_PMP | AHCI_HFLAG_YES_NCQ),
+	.flags = AHCI_FLAG_COMMON | ATA_FLAG_NCQ,
+	.pio_mask = ATA_PIO4,
+	.udma_mask = ATA_UDMA6,
+	.port_ops = &xgene_ahci_ops,
+};
+
+static int xgene_ahci_hw_init(struct ahci_host_priv *hpriv)
+{
+	struct xgene_ahci_context *ctx = hpriv->plat_data;
+	int i;
+	int rc;
+	u32 val;
+
+	/* Remove IP RAM out of shutdown */
+	rc = xgene_ahci_init_memram(ctx);
+	if (rc)
+		return rc;
+
+	for (i = 0; i < MAX_AHCI_CHN_PERCTR; i++)
+		xgene_ahci_set_phy_cfg(ctx, i);
+
+	/* AXI disable Mask */
+	writel(0xffffffff, hpriv->mmio + HOST_IRQ_STAT);
+	readl(hpriv->mmio + HOST_IRQ_STAT); /* Force a barrier */
+	writel(0, ctx->csr_base + INTSTATUSMASK);
+	readl(ctx->csr_base + INTSTATUSMASK); /* Force a barrier */
+	dev_dbg(ctx->dev, "top level interrupt mask 0x%X value 0x%08X\n",
+		INTSTATUSMASK, val);
+
+	writel(0x0, ctx->csr_base + ERRINTSTATUSMASK);
+	readl(ctx->csr_base + ERRINTSTATUSMASK); /* Force a barrier */
+	writel(0x0, ctx->csr_base + SATA_SHIM_OFFSET + INT_SLV_TMOMASK);
+	readl(ctx->csr_base + SATA_SHIM_OFFSET + INT_SLV_TMOMASK);
+
+	/* Enable AXI Interrupt */
+	writel(0xffffffff, ctx->csr_base + SLVRDERRATTRIBUTES);
+	writel(0xffffffff, ctx->csr_base + SLVWRERRATTRIBUTES);
+	writel(0xffffffff, ctx->csr_base + MSTRDERRATTRIBUTES);
+	writel(0xffffffff, ctx->csr_base + MSTWRERRATTRIBUTES);
+
+	/* Enable coherency */
+	val = readl(ctx->csr_base + BUSCTLREG);
+	val &= ~0x00000002;     /* Enable write coherency */
+	val &= ~0x00000001;     /* Enable read coherency */
+	writel(val, ctx->csr_base + BUSCTLREG);
+
+	val = readl(ctx->csr_base + IOFMSTRWAUX);
+	val |= (1 << 3);        /* Enable read coherency */
+	val |= (1 << 9);        /* Enable write coherency */
+	writel(val, ctx->csr_base + IOFMSTRWAUX);
+	val = readl(ctx->csr_base + IOFMSTRWAUX);
+	dev_dbg(ctx->dev, "coherency 0x%X value 0x%08X\n",
+		IOFMSTRWAUX, val);
+
+	return rc;
+}
+
+static int xgene_ahci_mux_select(struct xgene_ahci_context *ctx)
+{
+	void *mux_csr = ctx->csr_base + SATA_ENET_MUX_OFFSET;
+	u32 val;
+
+	val = readl(mux_csr + SATA_ENET_CONFIG_REG);
+	val &= ~CFG_SATA_ENET_SELECT_MASK;
+	writel(val, mux_csr + SATA_ENET_CONFIG_REG);
+	val = readl(mux_csr + SATA_ENET_CONFIG_REG);
+	return val & CFG_SATA_ENET_SELECT_MASK ? -1 : 0;
+}
+
+static int xgene_ahci_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ahci_host_priv *hpriv;
+	struct xgene_ahci_context *ctx;
+	struct resource *res;
+	int rc;
+
+	hpriv = ahci_platform_get_resources(pdev);
+	if (IS_ERR(hpriv))
+		return PTR_ERR(hpriv);
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx) {
+		dev_err(dev, "can't allocate host context\n");
+		return -ENOMEM;
+	}
+	hpriv->plat_data = ctx;
+	ctx->hpriv = hpriv;
+	ctx->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res) {
+		dev_err(dev, "no csr space\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Can't use devm_ioremap_resource due to overlapping region.
+	 * 0xYYYY.0000 - host core
+	 * 0xYYYY.7000 - Mux (if applicable)
+	 * 0xYYYY.A000 - PHY indirect access
+	 * 0xYYYY.C000 - Clock
+	 * 0xYYYY.D000 - RAM shutdown removal
+	 * As we map the entire region as one, it overlaps with the PHY driver.
+	 */
+	ctx->csr_base = devm_ioremap(dev, res->start, resource_size(res));
+	if (!ctx->csr_base) {
+		dev_err(dev, "can't map %pR\n", res);
+		return -ENOMEM;
+	}
+
+	dev_dbg(dev, "VAddr 0x%p Mmio VAddr 0x%p\n", ctx->csr_base,
+		hpriv->mmio);
+
+	/* Select ATA */
+	if (of_device_is_compatible(pdev->dev.of_node,
+		XGENE_AHCI_SGMII_DTS)) {
+		if (xgene_ahci_mux_select(ctx)) {
+			dev_err(dev, "SATA mux selection failed\n");
+			return -ENODEV;
+		}
+	}
+
+	/* Due to errata, HW requires full toggle transition */
+	rc = ahci_platform_enable_clks(hpriv);
+	if (rc)
+		goto disable_resources;
+	ahci_platform_disable_clks(hpriv);
+
+	rc = ahci_platform_enable_resources(hpriv);
+	if (rc)
+		goto disable_resources;
+
+	/* Configure the host controller */
+	xgene_ahci_hw_init(hpriv);
+
+	/* Setup DMA mask - 32 for 32-bit system and 64 for 64-bit system */
+	rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(8*sizeof(void *)));
+	if (rc) {
+		dev_err(dev, "Unable to set dma mask\n");
+		goto disable_resources;
+	}
+
+	rc = ahci_platform_init_host(pdev, hpriv, &xgene_ahci_port_info, 0, 0);
+	if (rc)
+		goto disable_resources;
+
+	dev_dbg(dev, "X-Gene SATA host controller initialized\n");
+	return 0;
+
+disable_resources:
+	ahci_platform_disable_resources(hpriv);
+	return rc;
+}
+
+static const struct of_device_id xgene_ahci_of_match[] = {
+	{.compatible = XGENE_AHCI_SGMII_DTS,},
+	{.compatible = XGENE_AHCI_PCIE_DTS,},
+	{},
+};
+MODULE_DEVICE_TABLE(of, xgene_ahci_of_match);
+
+static struct platform_driver xgene_ahci_driver = {
+	.probe = xgene_ahci_probe,
+	.remove = ata_platform_remove_one,
+	.driver = {
+		.name = "xgene-ahci",
+		.owner = THIS_MODULE,
+		.of_match_table = xgene_ahci_of_match,
+	},
+};
+
+module_platform_driver(xgene_ahci_driver);
+
+MODULE_DESCRIPTION("APM X-Gene AHCI SATA driver");
+MODULE_AUTHOR("Loc Ho <lho@apm.com>");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("0.4");
-- 
1.5.5


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
@ 2014-03-12 20:19       ` Loc Ho
  0 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-12 20:19 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds support for the APM X-Gene SoC AHCI SATA host controller
driver. It requires the corresponding APM X-Gene SoC PHY driver. This
initial version only supports Gen3 speed.

Signed-off-by: Loc Ho <lho@apm.com>
Signed-off-by: Tuan Phan <tphan@apm.com>
Signed-off-by: Suman Tripathi <stripathi@apm.com>
---
 drivers/ata/Kconfig      |    7 +
 drivers/ata/Makefile     |    1 +
 drivers/ata/ahci_xgene.c |  497 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 505 insertions(+), 0 deletions(-)
 create mode 100644 drivers/ata/ahci_xgene.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 93fc2f0..9de4ca5 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -115,6 +115,13 @@ config AHCI_SUNXI
 
 	  If unsure, say N.
 
+config AHCI_XGENE
+	tristate "APM X-Gene 6.0Gbps AHCI SATA host controller support"
+	depends on SATA_AHCI_PLATFORM && (ARM64 || COMPILE_TEST)
+	select PHY_XGENE
+	help
+	 This option enables support for APM X-Gene SoC SATA host controller.
+
 config SATA_FSL
 	tristate "Freescale 3.0Gbps SATA support"
 	depends on FSL_SOC
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 246050b..72b423b 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_SATA_DWC)		+= sata_dwc_460ex.o
 obj-$(CONFIG_SATA_HIGHBANK)	+= sata_highbank.o libahci.o
 obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o
 obj-$(CONFIG_AHCI_SUNXI)	+= ahci_sunxi.o
+obj-$(CONFIG_AHCI_XGENE)	+= ahci_xgene.o
 
 # SFF w/ custom DMA
 obj-$(CONFIG_PDC_ADMA)		+= pdc_adma.o
diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
new file mode 100644
index 0000000..e0e637a
--- /dev/null
+++ b/drivers/ata/ahci_xgene.c
@@ -0,0 +1,497 @@
+/*
+ * AppliedMicro X-Gene SoC SATA Host Controller Driver
+ *
+ * Copyright (c) 2014, Applied Micro Circuits Corporation
+ * Author: Loc Ho <lho@apm.com>
+ *         Tuan Phan <tphan@apm.com>
+ *         Suman Tripathi <stripathi@apm.com>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * NOTE: PM support is not currently available.
+ *
+ */
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/ahci_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/phy/phy.h>
+#include "ahci.h"
+
+/* Controller who PHY shared with SGMII Ethernet PHY */
+#define XGENE_AHCI_SGMII_DTS		"apm,xgene-ahci-sgmii"
+
+/* Controller who PHY (internal reference clock macro) shared with PCIe */
+#define XGENE_AHCI_PCIE_DTS		"apm,xgene-ahci-pcie"
+
+/* Max # of disk per a controller */
+#define MAX_AHCI_CHN_PERCTR		2
+
+#define SATA_ENET_MUX_OFFSET		0x00007000
+#define SATA_DIAG_OFFSET		0x0000D000
+#define SATA_GLB_OFFSET			0x0000D850
+#define SATA_SHIM_OFFSET		0x0000E000
+#define SATA_MASTER_OFFSET		0x0000F000
+#define SATA_PORT0_OFFSET		0x00000100
+#define SATA_PORT1_OFFSET		0x00000180
+
+/* MUX CSR */
+#define SATA_ENET_CONFIG_REG		0x00000000
+#define  CFG_SATA_ENET_SELECT_MASK	0x00000001
+
+/* SATA host controller CSR */
+#define SLVRDERRATTRIBUTES		0x00000000
+#define SLVWRERRATTRIBUTES		0x00000004
+#define MSTRDERRATTRIBUTES		0x00000008
+#define MSTWRERRATTRIBUTES		0x0000000c
+#define BUSCTLREG			0x00000014
+#define IOFMSTRWAUX			0x00000018
+#define INTSTATUSMASK			0x0000002c
+#define ERRINTSTATUS			0x00000030
+#define ERRINTSTATUSMASK		0x00000034
+
+/* SATA host AHCI CSR */
+#define PORTCFG				0x000000a4
+#define  PORTADDR_SET(dst, src) \
+		(((dst) & ~0x0000003f) | (((u32)(src)) & 0x0000003f))
+#define PORTPHY1CFG		0x000000a8
+#define PORTPHY1CFG_FRCPHYRDY_SET(dst, src) \
+		(((dst) & ~0x00100000) | (((u32)(src) << 0x14) & 0x00100000))
+#define PORTPHY2CFG			0x000000ac
+#define PORTPHY3CFG			0x000000b0
+#define PORTPHY4CFG			0x000000b4
+#define PORTPHY5CFG			0x000000b8
+#define SCTL0				0x0000012C
+#define PORTPHY5CFG_RTCHG_SET(dst, src) \
+		(((dst) & ~0xfff00000) | (((u32)(src) << 0x14) & 0xfff00000))
+#define PORTAXICFG_EN_CONTEXT_SET(dst, src) \
+		(((dst) & ~0x01000000) | (((u32)(src) << 0x18) & 0x01000000))
+#define PORTAXICFG			0x000000bc
+#define PORTAXICFG_OUTTRANS_SET(dst, src) \
+		(((dst) & ~0x00f00000) | (((u32)(src) << 0x14) & 0x00f00000))
+
+/* SATA host controller slave CSR */
+#define INT_SLV_TMOMASK			0x00000010
+
+/* SATA global diagnostic CSR */
+#define CFG_MEM_RAM_SHUTDOWN		0x00000070
+#define BLOCK_MEM_RDY			0x00000074
+
+struct xgene_ahci_context {
+	struct ahci_host_priv *hpriv;
+	struct device *dev;
+	void __iomem *csr_base;		/* CSR base address of IP */
+};
+
+static int xgene_ahci_init_memram(struct xgene_ahci_context *ctx)
+{
+	void __iomem *diagcsr = ctx->csr_base + SATA_DIAG_OFFSET;
+
+	dev_dbg(ctx->dev, "Release memory from shutdown\n");
+	writel(0x0, diagcsr + CFG_MEM_RAM_SHUTDOWN);
+	readl(diagcsr + CFG_MEM_RAM_SHUTDOWN); /* Force a barrier */
+	msleep(1);	/* reset may take up to 1ms */
+	if (readl(diagcsr + BLOCK_MEM_RDY) != 0xFFFFFFFF) {
+		dev_err(ctx->dev, "failed to release memory from shutdown\n");
+		return -ENODEV;
+	}
+	return 0;
+}
+
+/**
+ * xgene_ahci_read_id - Read ID data from the specified device
+ * @dev: device
+ * @tf: proposed taskfile
+ * @id: data buffer
+ *
+ * This custom read ID function is required due to the fact that the HW
+ * does not support DEVSLP and the controller state machine may get stuck
+ * after processing the ID query command.
+ */
+static unsigned int xgene_ahci_read_id(struct ata_device *dev,
+				       struct ata_taskfile *tf, u16 *id)
+{
+	u32 err_mask;
+	void __iomem *port_mmio = ahci_port_base(dev->link->ap);
+
+	err_mask = ata_do_dev_read_id(dev, tf, id);
+	if (err_mask)
+		return err_mask;
+
+	/*
+	 * Mask reserved area. Word78 spec of Link Power Management
+	 * bit15-8: reserved
+	 * bit7: NCQ autosence
+	 * bit6: Software settings preservation supported
+	 * bit5: reserved
+	 * bit4: In-order sata delivery supported
+	 * bit3: DIPM requests supported
+	 * bit2: DMA Setup FIS Auto-Activate optimization supported
+	 * bit1: DMA Setup FIX non-Zero buffer offsets supported
+	 * bit0: Reserved
+	 *
+	 * Clear reserved bit 8 (DEVSLP bit) as we don't support DEVSLP
+	 */
+	id[ATA_ID_FEATURE_SUPP] &= ~(1 << 8);
+
+	/*
+	 * Due to HW errata, restart the port if no other command active.
+	 * Otherwise the controller may get stuck.
+	 */
+	if (!readl(port_mmio + PORT_CMD_ISSUE)) {
+		writel(PORT_CMD_FIS_RX, port_mmio + PORT_CMD);
+		readl(port_mmio + PORT_CMD);	/* Force a barrier */
+		writel(PORT_CMD_FIS_RX | PORT_CMD_START, port_mmio + PORT_CMD);
+		readl(port_mmio + PORT_CMD);	/* Force a barrier */
+	}
+	return 0;
+}
+
+static void xgene_ahci_set_phy_cfg(struct xgene_ahci_context *ctx, int channel)
+{
+	void __iomem *mmio = ctx->hpriv->mmio;
+	u32 val;
+
+	dev_dbg(ctx->dev, "port configure mmio 0x%p channel %d\n",
+		mmio, channel);
+	val = readl(mmio + PORTCFG);
+	val = PORTADDR_SET(val, channel == 0 ? 2 : 3);
+	writel(val, mmio + PORTCFG);
+	readl(mmio + PORTCFG);  /* Force a barrier */
+	/* Disable fix rate */
+	writel(0x0001fffe, mmio + PORTPHY1CFG);
+	readl(mmio + PORTPHY1CFG); /* Force a barrier */
+	writel(0x5018461c, mmio + PORTPHY2CFG);
+	readl(mmio + PORTPHY2CFG); /* Force a barrier */
+	writel(0x1c081907, mmio + PORTPHY3CFG);
+	readl(mmio + PORTPHY3CFG); /* Force a barrier */
+	writel(0x1c080815, mmio + PORTPHY4CFG);
+	readl(mmio + PORTPHY4CFG); /* Force a barrier */
+	/* Set window negotiation */
+	val = readl(mmio + PORTPHY5CFG);
+	val = PORTPHY5CFG_RTCHG_SET(val, 0x300);
+	writel(val, mmio + PORTPHY5CFG);
+	readl(mmio + PORTPHY5CFG); /* Force a barrier */
+	val = readl(mmio + PORTAXICFG);
+	val = PORTAXICFG_EN_CONTEXT_SET(val, 0x1); /* Enable context mgmt */
+	val = PORTAXICFG_OUTTRANS_SET(val, 0xe); /* Set outstanding */
+	writel(val, mmio + PORTAXICFG);
+	readl(mmio + PORTAXICFG); /* Force a barrier */
+}
+
+/**
+ * xgene_ahci_do_hardreset - Issue the actual COMRESET
+ * @link: link to reset
+ * @deadline: deadline jiffies for the operation
+ * @online: Return value to indicate if device online
+ *
+ * Due to the limitation of the hardware PHY, a difference set of setting is
+ * required for each supported disk speed - Gen3 (6.0Gbps), Gen2 (3.0Gbps),
+ * and Gen1 (1.5Gbps). Otherwise during long IO stress test, the PHY will
+ * report disparity error and etc. In addition, during COMRESET, there can
+ * be error reported in the register PORT_SCR_ERR. For SERR_DISPARITY and
+ * SERR_10B_8B_ERR, the PHY receiver line must be reseted. The following
+ * algorithm is followed to proper configure the hardware PHY during COMRESET:
+ *
+ * Alg Part 1:
+ * 1. Start the PHY at Gen3 speed (default setting)
+ * 2. Issue the COMRESET
+ * 3. If no link, go to Alg Part 3
+ * 4. If link up, determine if the negotiated speed matches the PHY
+ *    configured speed
+ * 5. If they matched, go to Alg Part 2
+ * 6. If they do not matched and first time, configure the PHY for the linked
+ *    up disk speed and repeat step 2
+ * 7. Go to Alg Part 2
+ *
+ * Alg Part 2:
+ * 1. On link up, if there are any SERR_DISPARITY and SERR_10B_8B_ERR error
+ *    reported in the register PORT_SCR_ERR, then reset the PHY receiver line
+ * 2. Go to Alg Part 3
+ *
+ * Alg Part 3:
+ * 1. Clear any pending from register PORT_SCR_ERR.
+ *
+ * NOTE: First the initial version, we will NOT support Gen1/Gen2. In addition
+ *       and until the underlying PHY support an method to reset the receiver
+ *       line, on detection of SERR_DISPARITY or SERR_10B_8B_ERR error,
+ *       an warning message will be printed.
+ */
+static int xgene_ahci_do_hardreset(struct ata_link *link,
+				   unsigned long deadline, bool *online)
+{
+	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
+	struct ata_port *ap = link->ap;
+	struct ahci_host_priv *hpriv = ap->host->private_data;
+	struct xgene_ahci_context *ctx = hpriv->plat_data;
+	struct ahci_port_priv *pp = ap->private_data;
+	u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
+	void __iomem *port_mmio = ahci_port_base(ap);
+	struct ata_taskfile tf;
+	int rc;
+	u32 val;
+
+	/* clear D2H reception area to properly wait for D2H FIS */
+	ata_tf_init(link->device, &tf);
+	tf.command = ATA_BUSY;
+	ata_tf_to_fis(&tf, 0, 0, d2h_fis);
+	rc = sata_link_hardreset(link, timing, deadline, online,
+				 ahci_check_ready);
+
+	val = readl(port_mmio + PORT_SCR_ERR);
+	if (val & (SERR_DISPARITY | SERR_10B_8B_ERR))
+		dev_warn(ctx->dev, "link has error\n");
+
+	/* clear all errors if any pending */
+	val = readl(port_mmio + PORT_SCR_ERR);
+	writel(val, port_mmio + PORT_SCR_ERR);
+
+	return rc;
+}
+
+static int xgene_ahci_hardreset(struct ata_link *link, unsigned int *class,
+				unsigned long deadline)
+{
+	struct ata_port *ap = link->ap;
+        struct ahci_host_priv *hpriv = ap->host->private_data;
+	void __iomem *port_mmio = ahci_port_base(ap);
+	bool online;
+	int rc;
+	u32 portcmd_saved;
+	u32 portclb_saved;
+	u32 portclbhi_saved;
+	u32 portrxfis_saved;
+	u32 portrxfishi_saved;
+
+	/* As hardreset resets these CSR, save it to restore later */
+	portcmd_saved = readl(port_mmio + PORT_CMD);
+	portclb_saved = readl(port_mmio + PORT_LST_ADDR);
+	portclbhi_saved = readl(port_mmio + PORT_LST_ADDR_HI);
+	portrxfis_saved = readl(port_mmio + PORT_FIS_ADDR);
+	portrxfishi_saved = readl(port_mmio + PORT_FIS_ADDR_HI);
+
+	ahci_stop_engine(ap);
+
+	rc = xgene_ahci_do_hardreset(link, deadline, &online);
+
+	/* As controller hardreset clears them, restore them */
+	writel(portcmd_saved, port_mmio + PORT_CMD);
+	writel(portclb_saved, port_mmio + PORT_LST_ADDR);
+	writel(portclbhi_saved, port_mmio + PORT_LST_ADDR_HI);
+	writel(portrxfis_saved, port_mmio + PORT_FIS_ADDR);
+	writel(portrxfishi_saved, port_mmio + PORT_FIS_ADDR_HI);
+
+	hpriv->start_engine(ap);
+
+	if (online)
+		*class = ahci_dev_classify(ap);
+
+	return rc;
+}
+
+static void xgene_ahci_host_stop(struct ata_host *host)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+
+	ahci_platform_disable_resources(hpriv);
+}
+
+static struct ata_port_operations xgene_ahci_ops = {
+	.inherits = &ahci_ops,
+	.host_stop = xgene_ahci_host_stop,
+	.hardreset = xgene_ahci_hardreset,
+	.read_id = xgene_ahci_read_id,
+};
+
+static const struct ata_port_info xgene_ahci_port_info = {
+	AHCI_HFLAGS(AHCI_HFLAG_NO_PMP | AHCI_HFLAG_YES_NCQ),
+	.flags = AHCI_FLAG_COMMON | ATA_FLAG_NCQ,
+	.pio_mask = ATA_PIO4,
+	.udma_mask = ATA_UDMA6,
+	.port_ops = &xgene_ahci_ops,
+};
+
+static int xgene_ahci_hw_init(struct ahci_host_priv *hpriv)
+{
+	struct xgene_ahci_context *ctx = hpriv->plat_data;
+	int i;
+	int rc;
+	u32 val;
+
+	/* Remove IP RAM out of shutdown */
+	rc = xgene_ahci_init_memram(ctx);
+	if (rc)
+		return rc;
+
+	for (i = 0; i < MAX_AHCI_CHN_PERCTR; i++)
+		xgene_ahci_set_phy_cfg(ctx, i);
+
+	/* AXI disable Mask */
+	writel(0xffffffff, hpriv->mmio + HOST_IRQ_STAT);
+	readl(hpriv->mmio + HOST_IRQ_STAT); /* Force a barrier */
+	writel(0, ctx->csr_base + INTSTATUSMASK);
+	readl(ctx->csr_base + INTSTATUSMASK); /* Force a barrier */
+	dev_dbg(ctx->dev, "top level interrupt mask 0x%X value 0x%08X\n",
+		INTSTATUSMASK, val);
+
+	writel(0x0, ctx->csr_base + ERRINTSTATUSMASK);
+	readl(ctx->csr_base + ERRINTSTATUSMASK); /* Force a barrier */
+	writel(0x0, ctx->csr_base + SATA_SHIM_OFFSET + INT_SLV_TMOMASK);
+	readl(ctx->csr_base + SATA_SHIM_OFFSET + INT_SLV_TMOMASK);
+
+	/* Enable AXI Interrupt */
+	writel(0xffffffff, ctx->csr_base + SLVRDERRATTRIBUTES);
+	writel(0xffffffff, ctx->csr_base + SLVWRERRATTRIBUTES);
+	writel(0xffffffff, ctx->csr_base + MSTRDERRATTRIBUTES);
+	writel(0xffffffff, ctx->csr_base + MSTWRERRATTRIBUTES);
+
+	/* Enable coherency */
+	val = readl(ctx->csr_base + BUSCTLREG);
+	val &= ~0x00000002;     /* Enable write coherency */
+	val &= ~0x00000001;     /* Enable read coherency */
+	writel(val, ctx->csr_base + BUSCTLREG);
+
+	val = readl(ctx->csr_base + IOFMSTRWAUX);
+	val |= (1 << 3);        /* Enable read coherency */
+	val |= (1 << 9);        /* Enable write coherency */
+	writel(val, ctx->csr_base + IOFMSTRWAUX);
+	val = readl(ctx->csr_base + IOFMSTRWAUX);
+	dev_dbg(ctx->dev, "coherency 0x%X value 0x%08X\n",
+		IOFMSTRWAUX, val);
+
+	return rc;
+}
+
+static int xgene_ahci_mux_select(struct xgene_ahci_context *ctx)
+{
+	void *mux_csr = ctx->csr_base + SATA_ENET_MUX_OFFSET;
+	u32 val;
+
+	val = readl(mux_csr + SATA_ENET_CONFIG_REG);
+	val &= ~CFG_SATA_ENET_SELECT_MASK;
+	writel(val, mux_csr + SATA_ENET_CONFIG_REG);
+	val = readl(mux_csr + SATA_ENET_CONFIG_REG);
+	return val & CFG_SATA_ENET_SELECT_MASK ? -1 : 0;
+}
+
+static int xgene_ahci_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ahci_host_priv *hpriv;
+	struct xgene_ahci_context *ctx;
+	struct resource *res;
+	int rc;
+
+	hpriv = ahci_platform_get_resources(pdev);
+	if (IS_ERR(hpriv))
+		return PTR_ERR(hpriv);
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx) {
+		dev_err(dev, "can't allocate host context\n");
+		return -ENOMEM;
+	}
+	hpriv->plat_data = ctx;
+	ctx->hpriv = hpriv;
+	ctx->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res) {
+		dev_err(dev, "no csr space\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Can't use devm_ioremap_resource due to overlapping region.
+	 * 0xYYYY.0000 - host core
+	 * 0xYYYY.7000 - Mux (if applicable)
+	 * 0xYYYY.A000 - PHY indirect access
+	 * 0xYYYY.C000 - Clock
+	 * 0xYYYY.D000 - RAM shutdown removal
+	 * As we map the entire region as one, it overlaps with the PHY driver.
+	 */
+	ctx->csr_base = devm_ioremap(dev, res->start, resource_size(res));
+	if (!ctx->csr_base) {
+		dev_err(dev, "can't map %pR\n", res);
+		return -ENOMEM;
+	}
+
+	dev_dbg(dev, "VAddr 0x%p Mmio VAddr 0x%p\n", ctx->csr_base,
+		hpriv->mmio);
+
+	/* Select ATA */
+	if (of_device_is_compatible(pdev->dev.of_node,
+		XGENE_AHCI_SGMII_DTS)) {
+		if (xgene_ahci_mux_select(ctx)) {
+			dev_err(dev, "SATA mux selection failed\n");
+			return -ENODEV;
+		}
+	}
+
+	/* Due to errata, HW requires full toggle transition */
+	rc = ahci_platform_enable_clks(hpriv);
+	if (rc)
+		goto disable_resources;
+	ahci_platform_disable_clks(hpriv);
+
+	rc = ahci_platform_enable_resources(hpriv);
+	if (rc)
+		goto disable_resources;
+
+	/* Configure the host controller */
+	xgene_ahci_hw_init(hpriv);
+
+	/* Setup DMA mask - 32 for 32-bit system and 64 for 64-bit system */
+	rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(8*sizeof(void *)));
+	if (rc) {
+		dev_err(dev, "Unable to set dma mask\n");
+		goto disable_resources;
+	}
+
+	rc = ahci_platform_init_host(pdev, hpriv, &xgene_ahci_port_info, 0, 0);
+	if (rc)
+		goto disable_resources;
+
+	dev_dbg(dev, "X-Gene SATA host controller initialized\n");
+	return 0;
+
+disable_resources:
+	ahci_platform_disable_resources(hpriv);
+	return rc;
+}
+
+static const struct of_device_id xgene_ahci_of_match[] = {
+	{.compatible = XGENE_AHCI_SGMII_DTS,},
+	{.compatible = XGENE_AHCI_PCIE_DTS,},
+	{},
+};
+MODULE_DEVICE_TABLE(of, xgene_ahci_of_match);
+
+static struct platform_driver xgene_ahci_driver = {
+	.probe = xgene_ahci_probe,
+	.remove = ata_platform_remove_one,
+	.driver = {
+		.name = "xgene-ahci",
+		.owner = THIS_MODULE,
+		.of_match_table = xgene_ahci_of_match,
+	},
+};
+
+module_platform_driver(xgene_ahci_driver);
+
+MODULE_DESCRIPTION("APM X-Gene AHCI SATA driver");
+MODULE_AUTHOR("Loc Ho <lho@apm.com>");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("0.4");
-- 
1.5.5

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v17 4/4] arm64: Add APM X-Gene SoC AHCI SATA host controller DTS entries
  2014-03-12 20:19       ` Loc Ho
@ 2014-03-12 20:19         ` Loc Ho
  -1 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-12 20:19 UTC (permalink / raw)
  To: olof, tj, arnd
  Cc: linux-scsi, linux-ide, devicetree, linux-arm-kernel, ddutile,
	jcm, patches, Loc Ho, Tuan Phan, Suman Tripathi

This patch adds APM X-Gene SoC AHCI SATA host controller DTS entries.

Signed-off-by: Loc Ho <lho@apm.com>
Signed-off-by: Tuan Phan <tphan@apm.com>
Signed-off-by: Suman Tripathi <stripathi@apm.com>
---
 arch/arm64/boot/dts/apm-storm.dtsi |   75 ++++++++++++++++++++++++++++++++++++
 1 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index c78ddcf..2a03e96 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -221,6 +221,48 @@
 				enable-offset = <0x0>;
 				enable-mask = <0x06>;
 			};
+
+			sata01clk: sata01clk@1f21c000 {
+				compatible = "apm,xgene-device-clock";
+				#clock-cells = <1>;
+				clocks = <&socplldiv2 0>;
+				clock-names = "socplldiv2";
+				reg = <0x0 0x1f21c000 0x0 0x1000>;
+				reg-names = "csr-reg";
+				clock-output-names = "sata01clk";
+				csr-offset = <0x4>;
+				csr-mask = <0x05>;
+				enable-offset = <0x0>;
+				enable-mask = <0x39>;
+			};
+
+			sata23clk: sata23clk@1f22c000 {
+				compatible = "apm,xgene-device-clock";
+				#clock-cells = <1>;
+				clocks = <&socplldiv2 0>;
+				clock-names = "socplldiv2";
+				reg = <0x0 0x1f22c000 0x0 0x1000>;
+				reg-names = "csr-reg";
+				clock-output-names = "sata23clk";
+				csr-offset = <0x4>;
+				csr-mask = <0x05>;
+				enable-offset = <0x0>;
+				enable-mask = <0x39>;
+			};
+
+			sata45clk: sata45clk@1f23c000 {
+				compatible = "apm,xgene-device-clock";
+				#clock-cells = <1>;
+				clocks = <&socplldiv2 0>;
+				clock-names = "socplldiv2";
+				reg = <0x0 0x1f23c000 0x0 0x1000>;
+				reg-names = "csr-reg";
+				clock-output-names = "sata45clk";
+				csr-offset = <0x4>;
+				csr-mask = <0x05>;
+				enable-offset = <0x0>;
+				enable-mask = <0x39>;
+			};
 		};
 
 		serial0: serial@1c020000 {
@@ -262,5 +304,38 @@
 			apm,tx-boost-gain = <31 31 31 31 31 31>;
 			apm,tx-eye-tuning = <2 10 10 2 10 10>;
 		};
+
+		sata1: sata@1a000000 {
+			compatible = "apm,xgene-ahci-sgmii";
+			reg = <0x0 0x1a000000 0x0 0x1000>,
+			      <0x0 0x1f210000 0x0 0x10000>;
+			interrupts = <0x0 0x86 0x4>;
+			status = "disabled";
+			clocks = <&sata01clk 0>;
+			phys = <&phy1 0>;
+			phy-names = "sata-phy";
+		};
+
+		sata2: sata@1a400000 {
+			compatible = "apm,xgene-ahci-sgmii";
+			reg = <0x0 0x1a400000 0x0 0x1000>,
+			      <0x0 0x1f220000 0x0 0x10000>;
+			interrupts = <0x0 0x87 0x4>;
+			status = "ok";
+			clocks = <&sata23clk 0>;
+			phys = <&phy2 0>;
+			phy-names = "sata-phy";
+		};
+
+		sata3: sata@1a800000 {
+			compatible = "apm,xgene-ahci-pcie";
+			reg = <0x0 0x1a800000 0x0 0x1000>,
+			      <0x0 0x1f230000 0x0 0x10000>;
+			interrupts = <0x0 0x88 0x4>;
+			status = "ok";
+			clocks = <&sata45clk 0>;
+			phys = <&phy3 0>;
+			phy-names = "sata-phy";
+		};
 	};
 };
-- 
1.5.5


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v17 4/4] arm64: Add APM X-Gene SoC AHCI SATA host controller DTS entries
@ 2014-03-12 20:19         ` Loc Ho
  0 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-12 20:19 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds APM X-Gene SoC AHCI SATA host controller DTS entries.

Signed-off-by: Loc Ho <lho@apm.com>
Signed-off-by: Tuan Phan <tphan@apm.com>
Signed-off-by: Suman Tripathi <stripathi@apm.com>
---
 arch/arm64/boot/dts/apm-storm.dtsi |   75 ++++++++++++++++++++++++++++++++++++
 1 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index c78ddcf..2a03e96 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -221,6 +221,48 @@
 				enable-offset = <0x0>;
 				enable-mask = <0x06>;
 			};
+
+			sata01clk: sata01clk at 1f21c000 {
+				compatible = "apm,xgene-device-clock";
+				#clock-cells = <1>;
+				clocks = <&socplldiv2 0>;
+				clock-names = "socplldiv2";
+				reg = <0x0 0x1f21c000 0x0 0x1000>;
+				reg-names = "csr-reg";
+				clock-output-names = "sata01clk";
+				csr-offset = <0x4>;
+				csr-mask = <0x05>;
+				enable-offset = <0x0>;
+				enable-mask = <0x39>;
+			};
+
+			sata23clk: sata23clk at 1f22c000 {
+				compatible = "apm,xgene-device-clock";
+				#clock-cells = <1>;
+				clocks = <&socplldiv2 0>;
+				clock-names = "socplldiv2";
+				reg = <0x0 0x1f22c000 0x0 0x1000>;
+				reg-names = "csr-reg";
+				clock-output-names = "sata23clk";
+				csr-offset = <0x4>;
+				csr-mask = <0x05>;
+				enable-offset = <0x0>;
+				enable-mask = <0x39>;
+			};
+
+			sata45clk: sata45clk at 1f23c000 {
+				compatible = "apm,xgene-device-clock";
+				#clock-cells = <1>;
+				clocks = <&socplldiv2 0>;
+				clock-names = "socplldiv2";
+				reg = <0x0 0x1f23c000 0x0 0x1000>;
+				reg-names = "csr-reg";
+				clock-output-names = "sata45clk";
+				csr-offset = <0x4>;
+				csr-mask = <0x05>;
+				enable-offset = <0x0>;
+				enable-mask = <0x39>;
+			};
 		};
 
 		serial0: serial at 1c020000 {
@@ -262,5 +304,38 @@
 			apm,tx-boost-gain = <31 31 31 31 31 31>;
 			apm,tx-eye-tuning = <2 10 10 2 10 10>;
 		};
+
+		sata1: sata at 1a000000 {
+			compatible = "apm,xgene-ahci-sgmii";
+			reg = <0x0 0x1a000000 0x0 0x1000>,
+			      <0x0 0x1f210000 0x0 0x10000>;
+			interrupts = <0x0 0x86 0x4>;
+			status = "disabled";
+			clocks = <&sata01clk 0>;
+			phys = <&phy1 0>;
+			phy-names = "sata-phy";
+		};
+
+		sata2: sata at 1a400000 {
+			compatible = "apm,xgene-ahci-sgmii";
+			reg = <0x0 0x1a400000 0x0 0x1000>,
+			      <0x0 0x1f220000 0x0 0x10000>;
+			interrupts = <0x0 0x87 0x4>;
+			status = "ok";
+			clocks = <&sata23clk 0>;
+			phys = <&phy2 0>;
+			phy-names = "sata-phy";
+		};
+
+		sata3: sata at 1a800000 {
+			compatible = "apm,xgene-ahci-pcie";
+			reg = <0x0 0x1a800000 0x0 0x1000>,
+			      <0x0 0x1f230000 0x0 0x10000>;
+			interrupts = <0x0 0x88 0x4>;
+			status = "ok";
+			clocks = <&sata45clk 0>;
+			phys = <&phy3 0>;
+			phy-names = "sata-phy";
+		};
 	};
 };
-- 
1.5.5

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
  2014-03-12 20:19       ` Loc Ho
@ 2014-03-13 17:01         ` Bartlomiej Zolnierkiewicz
  -1 siblings, 0 replies; 32+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2014-03-13 17:01 UTC (permalink / raw)
  To: Loc Ho
  Cc: olof, tj, arnd, linux-scsi, linux-ide, devicetree,
	linux-arm-kernel, ddutile, jcm, patches, Tuan Phan,
	Suman Tripathi

On Wednesday, March 12, 2014 02:19:32 PM Loc Ho wrote:
> This patch adds support for the APM X-Gene SoC AHCI SATA host controller
> driver. It requires the corresponding APM X-Gene SoC PHY driver. This
> initial version only supports Gen3 speed.
> 
> Signed-off-by: Loc Ho <lho@apm.com>
> Signed-off-by: Tuan Phan <tphan@apm.com>
> Signed-off-by: Suman Tripathi <stripathi@apm.com>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Thanks for fixing all the remaining issues.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
@ 2014-03-13 17:01         ` Bartlomiej Zolnierkiewicz
  0 siblings, 0 replies; 32+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2014-03-13 17:01 UTC (permalink / raw)
  To: linux-arm-kernel

On Wednesday, March 12, 2014 02:19:32 PM Loc Ho wrote:
> This patch adds support for the APM X-Gene SoC AHCI SATA host controller
> driver. It requires the corresponding APM X-Gene SoC PHY driver. This
> initial version only supports Gen3 speed.
> 
> Signed-off-by: Loc Ho <lho@apm.com>
> Signed-off-by: Tuan Phan <tphan@apm.com>
> Signed-off-by: Suman Tripathi <stripathi@apm.com>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Thanks for fixing all the remaining issues.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v17 1/4] arm64: Add APM X-Gene SoC 15Gbps Multi-purpose PHY DTS entries
  2014-03-12 20:19   ` Loc Ho
@ 2014-03-14 11:08     ` Arnd Bergmann
  -1 siblings, 0 replies; 32+ messages in thread
From: Arnd Bergmann @ 2014-03-14 11:08 UTC (permalink / raw)
  To: Loc Ho
  Cc: devicetree, Suman Tripathi, linux-scsi, linux-ide, jcm, patches,
	tj, ddutile, olof, Tuan Phan, linux-arm-kernel

On Wednesday 12 March 2014, Loc Ho wrote:
> +                       sataphy1clk: sataphy1clk@1f21c000 {
> +                               compatible = "apm,xgene-device-clock";
> +                               #clock-cells = <1>;
> +                               clocks = <&socplldiv2 0>;
> +                               clock-names = "socplldiv2";

I think we still need to resolve the question regarding the
"clock-names" properties in your clock provider nodes. I've commented
on this in the past, since it shows up in all the x-gene drivers,
but it's very inconsistent.

The binding says

+- clock-names : shall be the name of the device clock. If missing, use the
+                device name.

which to me makes no sense. What will "use the device name", and
which device? Your clock driver doesn't actually use these strings,
so could you please fix the binding to remove this, and stop adding
the strings to any clock nodes?

Since nothing ever used these and they are marked "optional"
in the binding, I don't think there is a chance of breaking
anything by changing it now.

	Arnd

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v17 1/4] arm64: Add APM X-Gene SoC 15Gbps Multi-purpose PHY DTS entries
@ 2014-03-14 11:08     ` Arnd Bergmann
  0 siblings, 0 replies; 32+ messages in thread
From: Arnd Bergmann @ 2014-03-14 11:08 UTC (permalink / raw)
  To: linux-arm-kernel

On Wednesday 12 March 2014, Loc Ho wrote:
> +                       sataphy1clk: sataphy1clk at 1f21c000 {
> +                               compatible = "apm,xgene-device-clock";
> +                               #clock-cells = <1>;
> +                               clocks = <&socplldiv2 0>;
> +                               clock-names = "socplldiv2";

I think we still need to resolve the question regarding the
"clock-names" properties in your clock provider nodes. I've commented
on this in the past, since it shows up in all the x-gene drivers,
but it's very inconsistent.

The binding says

+- clock-names : shall be the name of the device clock. If missing, use the
+                device name.

which to me makes no sense. What will "use the device name", and
which device? Your clock driver doesn't actually use these strings,
so could you please fix the binding to remove this, and stop adding
the strings to any clock nodes?

Since nothing ever used these and they are marked "optional"
in the binding, I don't think there is a chance of breaking
anything by changing it now.

	Arnd

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v17 2/4] Documentation: Add documentation for the APM X-Gene SoC SATA host controller DTS binding
  2014-03-12 20:19     ` Loc Ho
@ 2014-03-14 11:11       ` Arnd Bergmann
  -1 siblings, 0 replies; 32+ messages in thread
From: Arnd Bergmann @ 2014-03-14 11:11 UTC (permalink / raw)
  To: Loc Ho
  Cc: olof, tj, linux-scsi, linux-ide, devicetree, linux-arm-kernel,
	ddutile, jcm, patches, Tuan Phan, Suman Tripathi

On Wednesday 12 March 2014, Loc Ho wrote:
> +- clocks               : Reference to the clock entry.
> +- phys                 : A list of phandles + phy-specifiers, one for each
> +                         entry in phy-names.
> +- phy-names            : Should contain:
> +  * "sata-6g" for the SATA 6.0Gbps PHY
> +

The generic ahci binding has convered on using the string "sata-phy" for
ahci nodes. I think you should use the same for consistency. I realize
I agreed to using "sata-6g" earlier on, but the discussion about the
generic binding has moved on in the meantime and that is already merged.

	Arnd

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v17 2/4] Documentation: Add documentation for the APM X-Gene SoC SATA host controller DTS binding
@ 2014-03-14 11:11       ` Arnd Bergmann
  0 siblings, 0 replies; 32+ messages in thread
From: Arnd Bergmann @ 2014-03-14 11:11 UTC (permalink / raw)
  To: linux-arm-kernel

On Wednesday 12 March 2014, Loc Ho wrote:
> +- clocks               : Reference to the clock entry.
> +- phys                 : A list of phandles + phy-specifiers, one for each
> +                         entry in phy-names.
> +- phy-names            : Should contain:
> +  * "sata-6g" for the SATA 6.0Gbps PHY
> +

The generic ahci binding has convered on using the string "sata-phy" for
ahci nodes. I think you should use the same for consistency. I realize
I agreed to using "sata-6g" earlier on, but the discussion about the
generic binding has moved on in the meantime and that is already merged.

	Arnd

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
  2014-03-12 20:19       ` Loc Ho
@ 2014-03-14 11:41         ` Arnd Bergmann
  -1 siblings, 0 replies; 32+ messages in thread
From: Arnd Bergmann @ 2014-03-14 11:41 UTC (permalink / raw)
  To: Loc Ho
  Cc: olof, tj, linux-scsi, linux-ide, devicetree, linux-arm-kernel,
	ddutile, jcm, patches, Tuan Phan, Suman Tripathi

On Wednesday 12 March 2014, Loc Ho wrote:
> This patch adds support for the APM X-Gene SoC AHCI SATA host controller
> driver. It requires the corresponding APM X-Gene SoC PHY driver. This
> initial version only supports Gen3 speed.
> 
> Signed-off-by: Loc Ho <lho@apm.com>
> Signed-off-by: Tuan Phan <tphan@apm.com>
> Signed-off-by: Suman Tripathi <stripathi@apm.com>

Sorry I've skipped the last ten review rounds. I'm glad to see the
code has improved so much in the meantime!

I hope the points I'm making here were not already covered in the
many previous review rounds.

> +/* Controller who PHY shared with SGMII Ethernet PHY */
> +#define XGENE_AHCI_SGMII_DTS		"apm,xgene-ahci-sgmii"
> +
> +/* Controller who PHY (internal reference clock macro) shared with PCIe */
> +#define XGENE_AHCI_PCIE_DTS		"apm,xgene-ahci-pcie"

Please remove these macros, they don't help anything at all
but make it harder to follow what's going on.

Regarding the actual strings, reflecting them now I think listing 'pcie'
and 'sgmii' is not really helpful to the reader. The difference to
the AHCI driver should not be which device it's sharing its PHY with,
but rather how that looks from software side.

The only difference in your current driver is whether this function

+/* MUX CSR */
+#define SATA_ENET_CONFIG_REG           0x00000000
+#define  CFG_SATA_ENET_SELECT_MASK     0x00000001
+static int xgene_ahci_mux_select(struct xgene_ahci_context *ctx)
+{
+       void *mux_csr = ctx->csr_base + SATA_ENET_MUX_OFFSET;
+       u32 val;
+
+       val = readl(mux_csr + SATA_ENET_CONFIG_REG);
+       val &= ~CFG_SATA_ENET_SELECT_MASK;
+       writel(val, mux_csr + SATA_ENET_CONFIG_REG);
+       val = readl(mux_csr + SATA_ENET_CONFIG_REG);
+       return val & CFG_SATA_ENET_SELECT_MASK ? -1 : 0;
+}

gets called. Can you clarify what this register access does?
If it's just setting a index into a mux output, would it make
sense to have an optional DT property containing an integer with
the mux setting you want to set? That way you wouldn't even
have to have two compatible strings but just do

	ret = of_property_read_u32(node, "apm,ahci-mux", &mux);
	if (!ret)
		xgene_ahci_mux_select(ctx, mux);

> +	/*
> +	 * Can't use devm_ioremap_resource due to overlapping region.
> +	 * 0xYYYY.0000 - host core
> +	 * 0xYYYY.7000 - Mux (if applicable)
> +	 * 0xYYYY.A000 - PHY indirect access
> +	 * 0xYYYY.C000 - Clock
> +	 * 0xYYYY.D000 - RAM shutdown removal
> +	 * As we map the entire region as one, it overlaps with the PHY driver.
> +	 */
> +	ctx->csr_base = devm_ioremap(dev, res->start, resource_size(res));
> +	if (!ctx->csr_base) {
> +		dev_err(dev, "can't map %pR\n", res);
> +		return -ENOMEM;
> +	}

I still think we should try not to have overlapping memory areas here.
Could you split up the registers into another range in the reg property
to leave the PHY registers out?

> +	/* Setup DMA mask - 32 for 32-bit system and 64 for 64-bit system */
> +	rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(8*sizeof(void *)));
> +	if (rc) {
> +		dev_err(dev, "Unable to set dma mask\n");
> +		goto disable_resources;
> +	}

This is something we definitely have to fix properly. We are hitting
the problem of dma masks for internal devices on arm32 now, and there
is ongoing discussion about whether a device driver should touch these
at all, or rather rely on the DT core to set up the masks in a generic
way. This device is probably the first DMA master capable device we
have on arm64, other than PCI devices, and we must be careful to get it
right.

In either way, the mask of the device must *not* depend on sizeof(void*):
If the device is capable of doing 64-bit DMA, it should also be able
to do that when running a 32-bit kernel.

Please change this to DMA_BIT_MASK(64) for now, and add a comment
explaining that it is preliminary.

Also, please read the thread named "[PATCH 0/7] of: setup dma
parameters using dma-ranges and dma-coherent" and comment there
about what you think we should do here. I assume we will have to
add "dma-ranges" properties in a number of places to get this
all to work fine any 64-bit SoC. Do you know if you have any
DMA master devices in x-gene that are not 64-bit capable?

	Arnd

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
@ 2014-03-14 11:41         ` Arnd Bergmann
  0 siblings, 0 replies; 32+ messages in thread
From: Arnd Bergmann @ 2014-03-14 11:41 UTC (permalink / raw)
  To: linux-arm-kernel

On Wednesday 12 March 2014, Loc Ho wrote:
> This patch adds support for the APM X-Gene SoC AHCI SATA host controller
> driver. It requires the corresponding APM X-Gene SoC PHY driver. This
> initial version only supports Gen3 speed.
> 
> Signed-off-by: Loc Ho <lho@apm.com>
> Signed-off-by: Tuan Phan <tphan@apm.com>
> Signed-off-by: Suman Tripathi <stripathi@apm.com>

Sorry I've skipped the last ten review rounds. I'm glad to see the
code has improved so much in the meantime!

I hope the points I'm making here were not already covered in the
many previous review rounds.

> +/* Controller who PHY shared with SGMII Ethernet PHY */
> +#define XGENE_AHCI_SGMII_DTS		"apm,xgene-ahci-sgmii"
> +
> +/* Controller who PHY (internal reference clock macro) shared with PCIe */
> +#define XGENE_AHCI_PCIE_DTS		"apm,xgene-ahci-pcie"

Please remove these macros, they don't help anything at all
but make it harder to follow what's going on.

Regarding the actual strings, reflecting them now I think listing 'pcie'
and 'sgmii' is not really helpful to the reader. The difference to
the AHCI driver should not be which device it's sharing its PHY with,
but rather how that looks from software side.

The only difference in your current driver is whether this function

+/* MUX CSR */
+#define SATA_ENET_CONFIG_REG           0x00000000
+#define  CFG_SATA_ENET_SELECT_MASK     0x00000001
+static int xgene_ahci_mux_select(struct xgene_ahci_context *ctx)
+{
+       void *mux_csr = ctx->csr_base + SATA_ENET_MUX_OFFSET;
+       u32 val;
+
+       val = readl(mux_csr + SATA_ENET_CONFIG_REG);
+       val &= ~CFG_SATA_ENET_SELECT_MASK;
+       writel(val, mux_csr + SATA_ENET_CONFIG_REG);
+       val = readl(mux_csr + SATA_ENET_CONFIG_REG);
+       return val & CFG_SATA_ENET_SELECT_MASK ? -1 : 0;
+}

gets called. Can you clarify what this register access does?
If it's just setting a index into a mux output, would it make
sense to have an optional DT property containing an integer with
the mux setting you want to set? That way you wouldn't even
have to have two compatible strings but just do

	ret = of_property_read_u32(node, "apm,ahci-mux", &mux);
	if (!ret)
		xgene_ahci_mux_select(ctx, mux);

> +	/*
> +	 * Can't use devm_ioremap_resource due to overlapping region.
> +	 * 0xYYYY.0000 - host core
> +	 * 0xYYYY.7000 - Mux (if applicable)
> +	 * 0xYYYY.A000 - PHY indirect access
> +	 * 0xYYYY.C000 - Clock
> +	 * 0xYYYY.D000 - RAM shutdown removal
> +	 * As we map the entire region as one, it overlaps with the PHY driver.
> +	 */
> +	ctx->csr_base = devm_ioremap(dev, res->start, resource_size(res));
> +	if (!ctx->csr_base) {
> +		dev_err(dev, "can't map %pR\n", res);
> +		return -ENOMEM;
> +	}

I still think we should try not to have overlapping memory areas here.
Could you split up the registers into another range in the reg property
to leave the PHY registers out?

> +	/* Setup DMA mask - 32 for 32-bit system and 64 for 64-bit system */
> +	rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(8*sizeof(void *)));
> +	if (rc) {
> +		dev_err(dev, "Unable to set dma mask\n");
> +		goto disable_resources;
> +	}

This is something we definitely have to fix properly. We are hitting
the problem of dma masks for internal devices on arm32 now, and there
is ongoing discussion about whether a device driver should touch these
at all, or rather rely on the DT core to set up the masks in a generic
way. This device is probably the first DMA master capable device we
have on arm64, other than PCI devices, and we must be careful to get it
right.

In either way, the mask of the device must *not* depend on sizeof(void*):
If the device is capable of doing 64-bit DMA, it should also be able
to do that when running a 32-bit kernel.

Please change this to DMA_BIT_MASK(64) for now, and add a comment
explaining that it is preliminary.

Also, please read the thread named "[PATCH 0/7] of: setup dma
parameters using dma-ranges and dma-coherent" and comment there
about what you think we should do here. I assume we will have to
add "dma-ranges" properties in a number of places to get this
all to work fine any 64-bit SoC. Do you know if you have any
DMA master devices in x-gene that are not 64-bit capable?

	Arnd

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v17 4/4] arm64: Add APM X-Gene SoC AHCI SATA host controller DTS entries
  2014-03-12 20:19         ` Loc Ho
@ 2014-03-14 11:42           ` Arnd Bergmann
  -1 siblings, 0 replies; 32+ messages in thread
From: Arnd Bergmann @ 2014-03-14 11:42 UTC (permalink / raw)
  To: Loc Ho
  Cc: devicetree, Suman Tripathi, linux-scsi, linux-ide, jcm, patches,
	tj, ddutile, olof, Tuan Phan, linux-arm-kernel

On Wednesday 12 March 2014, Loc Ho wrote:
> +                       sata01clk: sata01clk@1f21c000 {
> +                               compatible = "apm,xgene-device-clock";
> +                               #clock-cells = <1>;
> +                               clocks = <&socplldiv2 0>;
> +                               clock-names = "socplldiv2";
> +                               reg = <0x0 0x1f21c000 0x0 0x1000>;
> +                               reg-names = "csr-reg";
> +                               clock-output-names = "sata01clk";
> +                               csr-offset = <0x4>;
> +                               csr-mask = <0x05>;
> +                               enable-offset = <0x0>;
> +                               enable-mask = <0x39>;

Same comment about the "clock-names" as in the first patch here.

	Arnd

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v17 4/4] arm64: Add APM X-Gene SoC AHCI SATA host controller DTS entries
@ 2014-03-14 11:42           ` Arnd Bergmann
  0 siblings, 0 replies; 32+ messages in thread
From: Arnd Bergmann @ 2014-03-14 11:42 UTC (permalink / raw)
  To: linux-arm-kernel

On Wednesday 12 March 2014, Loc Ho wrote:
> +                       sata01clk: sata01clk at 1f21c000 {
> +                               compatible = "apm,xgene-device-clock";
> +                               #clock-cells = <1>;
> +                               clocks = <&socplldiv2 0>;
> +                               clock-names = "socplldiv2";
> +                               reg = <0x0 0x1f21c000 0x0 0x1000>;
> +                               reg-names = "csr-reg";
> +                               clock-output-names = "sata01clk";
> +                               csr-offset = <0x4>;
> +                               csr-mask = <0x05>;
> +                               enable-offset = <0x0>;
> +                               enable-mask = <0x39>;

Same comment about the "clock-names" as in the first patch here.

	Arnd

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
  2014-03-14 11:41         ` Arnd Bergmann
@ 2014-03-14 18:49           ` Loc Ho
  -1 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-14 18:49 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Olof Johansson, Tejun Heo, Linux SCSI List, linux-ide,
	devicetree, linux-arm-kernel, Don Dutile, Jon Masters, patches,
	Tuan Phan, Suman Tripathi

Hi,

> On Wednesday 12 March 2014, Loc Ho wrote:
> > This patch adds support for the APM X-Gene SoC AHCI SATA host controller
> > driver. It requires the corresponding APM X-Gene SoC PHY driver. This
> > initial version only supports Gen3 speed.
> >
> > Signed-off-by: Loc Ho <lho@apm.com>
> > Signed-off-by: Tuan Phan <tphan@apm.com>
> > Signed-off-by: Suman Tripathi <stripathi@apm.com>
>
> Sorry I've skipped the last ten review rounds. I'm glad to see the
> code has improved so much in the meantime!
>
> I hope the points I'm making here were not already covered in the
> many previous review rounds.
>
> > +/* Controller who PHY shared with SGMII Ethernet PHY */
> > +#define XGENE_AHCI_SGMII_DTS         "apm,xgene-ahci-sgmii"
> > +
> > +/* Controller who PHY (internal reference clock macro) shared with PCIe */
> > +#define XGENE_AHCI_PCIE_DTS          "apm,xgene-ahci-pcie"
>
> Please remove these macros, they don't help anything at all
> but make it harder to follow what's going on.
>
> Regarding the actual strings, reflecting them now I think listing 'pcie'
> and 'sgmii' is not really helpful to the reader. The difference to
> the AHCI driver should not be which device it's sharing its PHY with,
> but rather how that looks from software side.
>
> The only difference in your current driver is whether this function
>
> +/* MUX CSR */
> +#define SATA_ENET_CONFIG_REG           0x00000000
> +#define  CFG_SATA_ENET_SELECT_MASK     0x00000001
> +static int xgene_ahci_mux_select(struct xgene_ahci_context *ctx)
> +{
> +       void *mux_csr = ctx->csr_base + SATA_ENET_MUX_OFFSET;
> +       u32 val;
> +
> +       val = readl(mux_csr + SATA_ENET_CONFIG_REG);
> +       val &= ~CFG_SATA_ENET_SELECT_MASK;
> +       writel(val, mux_csr + SATA_ENET_CONFIG_REG);
> +       val = readl(mux_csr + SATA_ENET_CONFIG_REG);
> +       return val & CFG_SATA_ENET_SELECT_MASK ? -1 : 0;
> +}
>
> gets called. Can you clarify what this register access does?
> If it's just setting a index into a mux output, would it make
> sense to have an optional DT property containing an integer with
> the mux setting you want to set? That way you wouldn't even
> have to have two compatible strings but just do
>
>         ret = of_property_read_u32(node, "apm,ahci-mux", &mux);
>         if (!ret)
>                 xgene_ahci_mux_select(ctx, mux);
>

Given that fact that I will break up the resource. Let's just use the
resource for the MUX to handle this. For the IP that doesn't existed,
I will just not list it.


> > +     /*
> > +      * Can't use devm_ioremap_resource due to overlapping region.
> > +      * 0xYYYY.0000 - host core
> > +      * 0xYYYY.7000 - Mux (if applicable)
> > +      * 0xYYYY.A000 - PHY indirect access
> > +      * 0xYYYY.C000 - Clock
> > +      * 0xYYYY.D000 - RAM shutdown removal
> > +      * As we map the entire region as one, it overlaps with the PHY driver.
> > +      */
> > +     ctx->csr_base = devm_ioremap(dev, res->start, resource_size(res));
> > +     if (!ctx->csr_base) {
> > +             dev_err(dev, "can't map %pR\n", res);
> > +             return -ENOMEM;
> > +     }
>
> I still think we should try not to have overlapping memory areas here.
> Could you split up the registers into another range in the reg property
> to leave the PHY registers out?

Let me do this:

1. One resource for the RAM shutdown
2. One resource for the host controller
3. One optional resource for the MUX if needed.

With #3, this also solved the MUX detection and avoid having another
node "apm,ahci-mux".

>
> > +     /* Setup DMA mask - 32 for 32-bit system and 64 for 64-bit system */
> > +     rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(8*sizeof(void *)));
> > +     if (rc) {
> > +             dev_err(dev, "Unable to set dma mask\n");
> > +             goto disable_resources;
> > +     }
>
> This is something we definitely have to fix properly. We are hitting
> the problem of dma masks for internal devices on arm32 now, and there
> is ongoing discussion about whether a device driver should touch these
> at all, or rather rely on the DT core to set up the masks in a generic
> way. This device is probably the first DMA master capable device we
> have on arm64, other than PCI devices, and we must be careful to get it
> right.
>
> In either way, the mask of the device must *not* depend on sizeof(void*):
> If the device is capable of doing 64-bit DMA, it should also be able
> to do that when running a 32-bit kernel.
>
> Please change this to DMA_BIT_MASK(64) for now, and add a comment
> explaining that it is preliminary.

okay.

>
> Also, please read the thread named "[PATCH 0/7] of: setup dma
> parameters using dma-ranges and dma-coherent" and comment there
> about what you think we should do here. I assume we will have to
> add "dma-ranges" properties in a number of places to get this
> all to work fine any 64-bit SoC. Do you know if you have any
> DMA master devices in x-gene that are not 64-bit capable?
>

I will read and follow later. We do have non-64-bit devices such as
SDIO and SPI. But these IP's have translation in the bus similar to
PCIe PIM.

-Loc

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
@ 2014-03-14 18:49           ` Loc Ho
  0 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-14 18:49 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

> On Wednesday 12 March 2014, Loc Ho wrote:
> > This patch adds support for the APM X-Gene SoC AHCI SATA host controller
> > driver. It requires the corresponding APM X-Gene SoC PHY driver. This
> > initial version only supports Gen3 speed.
> >
> > Signed-off-by: Loc Ho <lho@apm.com>
> > Signed-off-by: Tuan Phan <tphan@apm.com>
> > Signed-off-by: Suman Tripathi <stripathi@apm.com>
>
> Sorry I've skipped the last ten review rounds. I'm glad to see the
> code has improved so much in the meantime!
>
> I hope the points I'm making here were not already covered in the
> many previous review rounds.
>
> > +/* Controller who PHY shared with SGMII Ethernet PHY */
> > +#define XGENE_AHCI_SGMII_DTS         "apm,xgene-ahci-sgmii"
> > +
> > +/* Controller who PHY (internal reference clock macro) shared with PCIe */
> > +#define XGENE_AHCI_PCIE_DTS          "apm,xgene-ahci-pcie"
>
> Please remove these macros, they don't help anything at all
> but make it harder to follow what's going on.
>
> Regarding the actual strings, reflecting them now I think listing 'pcie'
> and 'sgmii' is not really helpful to the reader. The difference to
> the AHCI driver should not be which device it's sharing its PHY with,
> but rather how that looks from software side.
>
> The only difference in your current driver is whether this function
>
> +/* MUX CSR */
> +#define SATA_ENET_CONFIG_REG           0x00000000
> +#define  CFG_SATA_ENET_SELECT_MASK     0x00000001
> +static int xgene_ahci_mux_select(struct xgene_ahci_context *ctx)
> +{
> +       void *mux_csr = ctx->csr_base + SATA_ENET_MUX_OFFSET;
> +       u32 val;
> +
> +       val = readl(mux_csr + SATA_ENET_CONFIG_REG);
> +       val &= ~CFG_SATA_ENET_SELECT_MASK;
> +       writel(val, mux_csr + SATA_ENET_CONFIG_REG);
> +       val = readl(mux_csr + SATA_ENET_CONFIG_REG);
> +       return val & CFG_SATA_ENET_SELECT_MASK ? -1 : 0;
> +}
>
> gets called. Can you clarify what this register access does?
> If it's just setting a index into a mux output, would it make
> sense to have an optional DT property containing an integer with
> the mux setting you want to set? That way you wouldn't even
> have to have two compatible strings but just do
>
>         ret = of_property_read_u32(node, "apm,ahci-mux", &mux);
>         if (!ret)
>                 xgene_ahci_mux_select(ctx, mux);
>

Given that fact that I will break up the resource. Let's just use the
resource for the MUX to handle this. For the IP that doesn't existed,
I will just not list it.


> > +     /*
> > +      * Can't use devm_ioremap_resource due to overlapping region.
> > +      * 0xYYYY.0000 - host core
> > +      * 0xYYYY.7000 - Mux (if applicable)
> > +      * 0xYYYY.A000 - PHY indirect access
> > +      * 0xYYYY.C000 - Clock
> > +      * 0xYYYY.D000 - RAM shutdown removal
> > +      * As we map the entire region as one, it overlaps with the PHY driver.
> > +      */
> > +     ctx->csr_base = devm_ioremap(dev, res->start, resource_size(res));
> > +     if (!ctx->csr_base) {
> > +             dev_err(dev, "can't map %pR\n", res);
> > +             return -ENOMEM;
> > +     }
>
> I still think we should try not to have overlapping memory areas here.
> Could you split up the registers into another range in the reg property
> to leave the PHY registers out?

Let me do this:

1. One resource for the RAM shutdown
2. One resource for the host controller
3. One optional resource for the MUX if needed.

With #3, this also solved the MUX detection and avoid having another
node "apm,ahci-mux".

>
> > +     /* Setup DMA mask - 32 for 32-bit system and 64 for 64-bit system */
> > +     rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(8*sizeof(void *)));
> > +     if (rc) {
> > +             dev_err(dev, "Unable to set dma mask\n");
> > +             goto disable_resources;
> > +     }
>
> This is something we definitely have to fix properly. We are hitting
> the problem of dma masks for internal devices on arm32 now, and there
> is ongoing discussion about whether a device driver should touch these
> at all, or rather rely on the DT core to set up the masks in a generic
> way. This device is probably the first DMA master capable device we
> have on arm64, other than PCI devices, and we must be careful to get it
> right.
>
> In either way, the mask of the device must *not* depend on sizeof(void*):
> If the device is capable of doing 64-bit DMA, it should also be able
> to do that when running a 32-bit kernel.
>
> Please change this to DMA_BIT_MASK(64) for now, and add a comment
> explaining that it is preliminary.

okay.

>
> Also, please read the thread named "[PATCH 0/7] of: setup dma
> parameters using dma-ranges and dma-coherent" and comment there
> about what you think we should do here. I assume we will have to
> add "dma-ranges" properties in a number of places to get this
> all to work fine any 64-bit SoC. Do you know if you have any
> DMA master devices in x-gene that are not 64-bit capable?
>

I will read and follow later. We do have non-64-bit devices such as
SDIO and SPI. But these IP's have translation in the bus similar to
PCIe PIM.

-Loc

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
  2014-03-14 18:49           ` Loc Ho
@ 2014-03-14 19:05             ` Arnd Bergmann
  -1 siblings, 0 replies; 32+ messages in thread
From: Arnd Bergmann @ 2014-03-14 19:05 UTC (permalink / raw)
  To: Loc Ho
  Cc: Olof Johansson, Tejun Heo, Linux SCSI List, linux-ide,
	devicetree, linux-arm-kernel, Don Dutile, Jon Masters, patches,
	Tuan Phan, Suman Tripathi

On Friday 14 March 2014, Loc Ho wrote:
> > On Wednesday 12 March 2014, Loc Ho wrote:

> > gets called. Can you clarify what this register access does?
> > If it's just setting a index into a mux output, would it make
> > sense to have an optional DT property containing an integer with
> > the mux setting you want to set? That way you wouldn't even
> > have to have two compatible strings but just do
> >
> >         ret = of_property_read_u32(node, "apm,ahci-mux", &mux);
> >         if (!ret)
> >                 xgene_ahci_mux_select(ctx, mux);
> >
> 
> Given that fact that I will break up the resource. Let's just use the
> resource for the MUX to handle this. For the IP that doesn't existed,
> I will just not list it.

Ah, that sounds good. It also means that if the firmware has set up
the mux already, you don't have to touch it again. That would probably
be the preferred case.

I'm undecided about the value that you write in there. That could either
be passed through DT as I suggested above for extra flexibility, or you
can keep it hardcoded if you are absolutely sure that there will never
need to be a case where you have to set it to something other than '1',

> > > +     /*
> > > +      * Can't use devm_ioremap_resource due to overlapping region.
> > > +      * 0xYYYY.0000 - host core
> > > +      * 0xYYYY.7000 - Mux (if applicable)
> > > +      * 0xYYYY.A000 - PHY indirect access
> > > +      * 0xYYYY.C000 - Clock
> > > +      * 0xYYYY.D000 - RAM shutdown removal
> > > +      * As we map the entire region as one, it overlaps with the PHY driver.
> > > +      */
> > > +     ctx->csr_base = devm_ioremap(dev, res->start, resource_size(res));
> > > +     if (!ctx->csr_base) {
> > > +             dev_err(dev, "can't map %pR\n", res);
> > > +             return -ENOMEM;
> > > +     }
> >
> > I still think we should try not to have overlapping memory areas here.
> > Could you split up the registers into another range in the reg property
> > to leave the PHY registers out?
> 
> Let me do this:
> 
> 1. One resource for the RAM shutdown
> 2. One resource for the host controller
> 3. One optional resource for the MUX if needed.
> 
> With #3, this also solved the MUX detection and avoid having another
> node "apm,ahci-mux".

My suggestion was not to have another device node, just a property that
contains a value to be written into the mux register if present.

> > Also, please read the thread named "[PATCH 0/7] of: setup dma
> > parameters using dma-ranges and dma-coherent" and comment there
> > about what you think we should do here. I assume we will have to
> > add "dma-ranges" properties in a number of places to get this
> > all to work fine any 64-bit SoC. Do you know if you have any
> > DMA master devices in x-gene that are not 64-bit capable?
> >
> 
> I will read and follow later. We do have non-64-bit devices such as
> SDIO and SPI. But these IP's have translation in the bus similar to
> PCIe PIM.

Is there also an IOMMU, or just the PIM? If there is no IOMMU,
we will actually have to use the swiotlb code here, otherwise we
can rely on the IOMMU to do the translation so we can pretend that
there is always 64-bit DMA capability.

In theory, the PIM could be dynamically reprogrammed for each
DMA, as an extremely primitive IOMMU implementation, but I don't
know what the impact on performance would be from doing that.
It would also prevent you from starting multiple concurrent DMAs,
although I suspect that is not an important case for SPI and SDIO.

	Arnd

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
@ 2014-03-14 19:05             ` Arnd Bergmann
  0 siblings, 0 replies; 32+ messages in thread
From: Arnd Bergmann @ 2014-03-14 19:05 UTC (permalink / raw)
  To: linux-arm-kernel

On Friday 14 March 2014, Loc Ho wrote:
> > On Wednesday 12 March 2014, Loc Ho wrote:

> > gets called. Can you clarify what this register access does?
> > If it's just setting a index into a mux output, would it make
> > sense to have an optional DT property containing an integer with
> > the mux setting you want to set? That way you wouldn't even
> > have to have two compatible strings but just do
> >
> >         ret = of_property_read_u32(node, "apm,ahci-mux", &mux);
> >         if (!ret)
> >                 xgene_ahci_mux_select(ctx, mux);
> >
> 
> Given that fact that I will break up the resource. Let's just use the
> resource for the MUX to handle this. For the IP that doesn't existed,
> I will just not list it.

Ah, that sounds good. It also means that if the firmware has set up
the mux already, you don't have to touch it again. That would probably
be the preferred case.

I'm undecided about the value that you write in there. That could either
be passed through DT as I suggested above for extra flexibility, or you
can keep it hardcoded if you are absolutely sure that there will never
need to be a case where you have to set it to something other than '1',

> > > +     /*
> > > +      * Can't use devm_ioremap_resource due to overlapping region.
> > > +      * 0xYYYY.0000 - host core
> > > +      * 0xYYYY.7000 - Mux (if applicable)
> > > +      * 0xYYYY.A000 - PHY indirect access
> > > +      * 0xYYYY.C000 - Clock
> > > +      * 0xYYYY.D000 - RAM shutdown removal
> > > +      * As we map the entire region as one, it overlaps with the PHY driver.
> > > +      */
> > > +     ctx->csr_base = devm_ioremap(dev, res->start, resource_size(res));
> > > +     if (!ctx->csr_base) {
> > > +             dev_err(dev, "can't map %pR\n", res);
> > > +             return -ENOMEM;
> > > +     }
> >
> > I still think we should try not to have overlapping memory areas here.
> > Could you split up the registers into another range in the reg property
> > to leave the PHY registers out?
> 
> Let me do this:
> 
> 1. One resource for the RAM shutdown
> 2. One resource for the host controller
> 3. One optional resource for the MUX if needed.
> 
> With #3, this also solved the MUX detection and avoid having another
> node "apm,ahci-mux".

My suggestion was not to have another device node, just a property that
contains a value to be written into the mux register if present.

> > Also, please read the thread named "[PATCH 0/7] of: setup dma
> > parameters using dma-ranges and dma-coherent" and comment there
> > about what you think we should do here. I assume we will have to
> > add "dma-ranges" properties in a number of places to get this
> > all to work fine any 64-bit SoC. Do you know if you have any
> > DMA master devices in x-gene that are not 64-bit capable?
> >
> 
> I will read and follow later. We do have non-64-bit devices such as
> SDIO and SPI. But these IP's have translation in the bus similar to
> PCIe PIM.

Is there also an IOMMU, or just the PIM? If there is no IOMMU,
we will actually have to use the swiotlb code here, otherwise we
can rely on the IOMMU to do the translation so we can pretend that
there is always 64-bit DMA capability.

In theory, the PIM could be dynamically reprogrammed for each
DMA, as an extremely primitive IOMMU implementation, but I don't
know what the impact on performance would be from doing that.
It would also prevent you from starting multiple concurrent DMAs,
although I suspect that is not an important case for SPI and SDIO.

	Arnd

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
  2014-03-14 19:05             ` Arnd Bergmann
@ 2014-03-14 20:27               ` Loc Ho
  -1 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-14 20:27 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Olof Johansson, Tejun Heo, Linux SCSI List, linux-ide,
	devicetree, linux-arm-kernel, Don Dutile, Jon Masters, patches,
	Tuan Phan, Suman Tripathi

Hi,

>> > gets called. Can you clarify what this register access does?
>> > If it's just setting a index into a mux output, would it make
>> > sense to have an optional DT property containing an integer with
>> > the mux setting you want to set? That way you wouldn't even
>> > have to have two compatible strings but just do
>> >
>> >         ret = of_property_read_u32(node, "apm,ahci-mux", &mux);
>> >         if (!ret)
>> >                 xgene_ahci_mux_select(ctx, mux);
>> >
>>
>> Given that fact that I will break up the resource. Let's just use the
>> resource for the MUX to handle this. For the IP that doesn't existed,
>> I will just not list it.
>
> Ah, that sounds good. It also means that if the firmware has set up
> the mux already, you don't have to touch it again. That would probably
> be the preferred case.
>
> I'm undecided about the value that you write in there. That could either
> be passed through DT as I suggested above for extra flexibility, or you
> can keep it hardcoded if you are absolutely sure that there will never
> need to be a case where you have to set it to something other than '1',

It is only a mux between two IP's. Writing 1 is fine. For ACPI, it can
be missing and handled by the firmware or can be handled just like the
DTS way.

>
>> > > +     /*
>> > > +      * Can't use devm_ioremap_resource due to overlapping region.
>> > > +      * 0xYYYY.0000 - host core
>> > > +      * 0xYYYY.7000 - Mux (if applicable)
>> > > +      * 0xYYYY.A000 - PHY indirect access
>> > > +      * 0xYYYY.C000 - Clock
>> > > +      * 0xYYYY.D000 - RAM shutdown removal
>> > > +      * As we map the entire region as one, it overlaps with the PHY driver.
>> > > +      */
>> > > +     ctx->csr_base = devm_ioremap(dev, res->start, resource_size(res));
>> > > +     if (!ctx->csr_base) {
>> > > +             dev_err(dev, "can't map %pR\n", res);
>> > > +             return -ENOMEM;
>> > > +     }
>> >
>> > I still think we should try not to have overlapping memory areas here.
>> > Could you split up the registers into another range in the reg property
>> > to leave the PHY registers out?
>>
>> Let me do this:
>>
>> 1. One resource for the RAM shutdown
>> 2. One resource for the host controller
>> 3. One optional resource for the MUX if needed.
>>
>> With #3, this also solved the MUX detection and avoid having another
>> node "apm,ahci-mux".
>
> My suggestion was not to have another device node, just a property that
> contains a value to be written into the mux register if present.
>
>> > Also, please read the thread named "[PATCH 0/7] of: setup dma
>> > parameters using dma-ranges and dma-coherent" and comment there
>> > about what you think we should do here. I assume we will have to
>> > add "dma-ranges" properties in a number of places to get this
>> > all to work fine any 64-bit SoC. Do you know if you have any
>> > DMA master devices in x-gene that are not 64-bit capable?
>> >
>>
>> I will read and follow later. We do have non-64-bit devices such as
>> SDIO and SPI. But these IP's have translation in the bus similar to
>> PCIe PIM.
>
> Is there also an IOMMU, or just the PIM? If there is no IOMMU,
> we will actually have to use the swiotlb code here, otherwise we
> can rely on the IOMMU to do the translation so we can pretend that
> there is always 64-bit DMA capability.
>
> In theory, the PIM could be dynamically reprogrammed for each
> DMA, as an extremely primitive IOMMU implementation, but I don't
> know what the impact on performance would be from doing that.
> It would also prevent you from starting multiple concurrent DMAs,
> although I suspect that is not an important case for SPI and SDIO.

This first generation don't support IOMMU and rely on swiotlb. In any
case, for SDIO, the solution is to reprogram the PIM and limit to SDMA
- single buffer - for each operation.

-Loc

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
@ 2014-03-14 20:27               ` Loc Ho
  0 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-14 20:27 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

>> > gets called. Can you clarify what this register access does?
>> > If it's just setting a index into a mux output, would it make
>> > sense to have an optional DT property containing an integer with
>> > the mux setting you want to set? That way you wouldn't even
>> > have to have two compatible strings but just do
>> >
>> >         ret = of_property_read_u32(node, "apm,ahci-mux", &mux);
>> >         if (!ret)
>> >                 xgene_ahci_mux_select(ctx, mux);
>> >
>>
>> Given that fact that I will break up the resource. Let's just use the
>> resource for the MUX to handle this. For the IP that doesn't existed,
>> I will just not list it.
>
> Ah, that sounds good. It also means that if the firmware has set up
> the mux already, you don't have to touch it again. That would probably
> be the preferred case.
>
> I'm undecided about the value that you write in there. That could either
> be passed through DT as I suggested above for extra flexibility, or you
> can keep it hardcoded if you are absolutely sure that there will never
> need to be a case where you have to set it to something other than '1',

It is only a mux between two IP's. Writing 1 is fine. For ACPI, it can
be missing and handled by the firmware or can be handled just like the
DTS way.

>
>> > > +     /*
>> > > +      * Can't use devm_ioremap_resource due to overlapping region.
>> > > +      * 0xYYYY.0000 - host core
>> > > +      * 0xYYYY.7000 - Mux (if applicable)
>> > > +      * 0xYYYY.A000 - PHY indirect access
>> > > +      * 0xYYYY.C000 - Clock
>> > > +      * 0xYYYY.D000 - RAM shutdown removal
>> > > +      * As we map the entire region as one, it overlaps with the PHY driver.
>> > > +      */
>> > > +     ctx->csr_base = devm_ioremap(dev, res->start, resource_size(res));
>> > > +     if (!ctx->csr_base) {
>> > > +             dev_err(dev, "can't map %pR\n", res);
>> > > +             return -ENOMEM;
>> > > +     }
>> >
>> > I still think we should try not to have overlapping memory areas here.
>> > Could you split up the registers into another range in the reg property
>> > to leave the PHY registers out?
>>
>> Let me do this:
>>
>> 1. One resource for the RAM shutdown
>> 2. One resource for the host controller
>> 3. One optional resource for the MUX if needed.
>>
>> With #3, this also solved the MUX detection and avoid having another
>> node "apm,ahci-mux".
>
> My suggestion was not to have another device node, just a property that
> contains a value to be written into the mux register if present.
>
>> > Also, please read the thread named "[PATCH 0/7] of: setup dma
>> > parameters using dma-ranges and dma-coherent" and comment there
>> > about what you think we should do here. I assume we will have to
>> > add "dma-ranges" properties in a number of places to get this
>> > all to work fine any 64-bit SoC. Do you know if you have any
>> > DMA master devices in x-gene that are not 64-bit capable?
>> >
>>
>> I will read and follow later. We do have non-64-bit devices such as
>> SDIO and SPI. But these IP's have translation in the bus similar to
>> PCIe PIM.
>
> Is there also an IOMMU, or just the PIM? If there is no IOMMU,
> we will actually have to use the swiotlb code here, otherwise we
> can rely on the IOMMU to do the translation so we can pretend that
> there is always 64-bit DMA capability.
>
> In theory, the PIM could be dynamically reprogrammed for each
> DMA, as an extremely primitive IOMMU implementation, but I don't
> know what the impact on performance would be from doing that.
> It would also prevent you from starting multiple concurrent DMAs,
> although I suspect that is not an important case for SPI and SDIO.

This first generation don't support IOMMU and rely on swiotlb. In any
case, for SDIO, the solution is to reprogram the PIM and limit to SDMA
- single buffer - for each operation.

-Loc

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
  2014-03-14 20:27               ` Loc Ho
@ 2014-03-14 21:06                   ` Arnd Bergmann
  -1 siblings, 0 replies; 32+ messages in thread
From: Arnd Bergmann @ 2014-03-14 21:06 UTC (permalink / raw)
  To: Loc Ho
  Cc: Olof Johansson, Tejun Heo, Linux SCSI List,
	linux-ide-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Don Dutile,
	Jon Masters, patches-qTEPVZfXA3Y, Tuan Phan, Suman Tripathi

On Friday 14 March 2014, Loc Ho wrote:
> Hi,
> 
> >> > gets called. Can you clarify what this register access does?
> >> > If it's just setting a index into a mux output, would it make
> >> > sense to have an optional DT property containing an integer with
> >> > the mux setting you want to set? That way you wouldn't even
> >> > have to have two compatible strings but just do
> >> >
> >> >         ret = of_property_read_u32(node, "apm,ahci-mux", &mux);
> >> >         if (!ret)
> >> >                 xgene_ahci_mux_select(ctx, mux);
> >> >
> >>
> >> Given that fact that I will break up the resource. Let's just use the
> >> resource for the MUX to handle this. For the IP that doesn't existed,
> >> I will just not list it.
> >
> > Ah, that sounds good. It also means that if the firmware has set up
> > the mux already, you don't have to touch it again. That would probably
> > be the preferred case.
> >
> > I'm undecided about the value that you write in there. That could either
> > be passed through DT as I suggested above for extra flexibility, or you
> > can keep it hardcoded if you are absolutely sure that there will never
> > need to be a case where you have to set it to something other than '1',
> 
> It is only a mux between two IP's. Writing 1 is fine. For ACPI, it can
> be missing and handled by the firmware or can be handled just like the
> DTS way.

Why can't you have it handled by the firmware in the DTS case?

I still think it's rather unlikely that we will actually see ACPI support
on your platform, btw.

I'm willing to look at the patches you need for it, but I'm not very
optimistic, in particular because of the kind of hacks you need
for random bits of hardware.

> > Is there also an IOMMU, or just the PIM? If there is no IOMMU,
> > we will actually have to use the swiotlb code here, otherwise we
> > can rely on the IOMMU to do the translation so we can pretend that
> > there is always 64-bit DMA capability.
> >
> > In theory, the PIM could be dynamically reprogrammed for each
> > DMA, as an extremely primitive IOMMU implementation, but I don't
> > know what the impact on performance would be from doing that.
> > It would also prevent you from starting multiple concurrent DMAs,
> > although I suspect that is not an important case for SPI and SDIO.
> 
> This first generation don't support IOMMU and rely on swiotlb. In any
> case, for SDIO, the solution is to reprogram the PIM and limit to SDMA
> - single buffer - for each operation.

Ok, interesting. In that case, I suppose the DMA mask is actually 64-bit.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
@ 2014-03-14 21:06                   ` Arnd Bergmann
  0 siblings, 0 replies; 32+ messages in thread
From: Arnd Bergmann @ 2014-03-14 21:06 UTC (permalink / raw)
  To: linux-arm-kernel

On Friday 14 March 2014, Loc Ho wrote:
> Hi,
> 
> >> > gets called. Can you clarify what this register access does?
> >> > If it's just setting a index into a mux output, would it make
> >> > sense to have an optional DT property containing an integer with
> >> > the mux setting you want to set? That way you wouldn't even
> >> > have to have two compatible strings but just do
> >> >
> >> >         ret = of_property_read_u32(node, "apm,ahci-mux", &mux);
> >> >         if (!ret)
> >> >                 xgene_ahci_mux_select(ctx, mux);
> >> >
> >>
> >> Given that fact that I will break up the resource. Let's just use the
> >> resource for the MUX to handle this. For the IP that doesn't existed,
> >> I will just not list it.
> >
> > Ah, that sounds good. It also means that if the firmware has set up
> > the mux already, you don't have to touch it again. That would probably
> > be the preferred case.
> >
> > I'm undecided about the value that you write in there. That could either
> > be passed through DT as I suggested above for extra flexibility, or you
> > can keep it hardcoded if you are absolutely sure that there will never
> > need to be a case where you have to set it to something other than '1',
> 
> It is only a mux between two IP's. Writing 1 is fine. For ACPI, it can
> be missing and handled by the firmware or can be handled just like the
> DTS way.

Why can't you have it handled by the firmware in the DTS case?

I still think it's rather unlikely that we will actually see ACPI support
on your platform, btw.

I'm willing to look at the patches you need for it, but I'm not very
optimistic, in particular because of the kind of hacks you need
for random bits of hardware.

> > Is there also an IOMMU, or just the PIM? If there is no IOMMU,
> > we will actually have to use the swiotlb code here, otherwise we
> > can rely on the IOMMU to do the translation so we can pretend that
> > there is always 64-bit DMA capability.
> >
> > In theory, the PIM could be dynamically reprogrammed for each
> > DMA, as an extremely primitive IOMMU implementation, but I don't
> > know what the impact on performance would be from doing that.
> > It would also prevent you from starting multiple concurrent DMAs,
> > although I suspect that is not an important case for SPI and SDIO.
> 
> This first generation don't support IOMMU and rely on swiotlb. In any
> case, for SDIO, the solution is to reprogram the PIM and limit to SDMA
> - single buffer - for each operation.

Ok, interesting. In that case, I suppose the DMA mask is actually 64-bit.

	Arnd

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
  2014-03-14 21:06                   ` Arnd Bergmann
@ 2014-03-14 21:38                     ` Loc Ho
  -1 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-14 21:38 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Olof Johansson, Tejun Heo, Linux SCSI List, linux-ide,
	devicetree, linux-arm-kernel, Don Dutile, Jon Masters, patches,
	Tuan Phan, Suman Tripathi

Hi

>> >> > gets called. Can you clarify what this register access does?
>> >> > If it's just setting a index into a mux output, would it make
>> >> > sense to have an optional DT property containing an integer with
>> >> > the mux setting you want to set? That way you wouldn't even
>> >> > have to have two compatible strings but just do
>> >> >
>> >> >         ret = of_property_read_u32(node, "apm,ahci-mux", &mux);
>> >> >         if (!ret)
>> >> >                 xgene_ahci_mux_select(ctx, mux);
>> >> >
>> >>
>> >> Given that fact that I will break up the resource. Let's just use the
>> >> resource for the MUX to handle this. For the IP that doesn't existed,
>> >> I will just not list it.
>> >
>> > Ah, that sounds good. It also means that if the firmware has set up
>> > the mux already, you don't have to touch it again. That would probably
>> > be the preferred case.
>> >
>> > I'm undecided about the value that you write in there. That could either
>> > be passed through DT as I suggested above for extra flexibility, or you
>> > can keep it hardcoded if you are absolutely sure that there will never
>> > need to be a case where you have to set it to something other than '1',
>>
>> It is only a mux between two IP's. Writing 1 is fine. For ACPI, it can
>> be missing and handled by the firmware or can be handled just like the
>> DTS way.
>
> Why can't you have it handled by the firmware in the DTS case?
>
> I still think it's rather unlikely that we will actually see ACPI support
> on your platform, btw.
>
> I'm willing to look at the patches you need for it, but I'm not very
> optimistic, in particular because of the kind of hacks you need
> for random bits of hardware.
>

I am about to post another version. Can you look at the next version?
If you still think it needs to be moved to the FW (either U-Boot or
Tianocore UEFI boot loader), I will then move out of the Linux driver
code.

-Loc

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
@ 2014-03-14 21:38                     ` Loc Ho
  0 siblings, 0 replies; 32+ messages in thread
From: Loc Ho @ 2014-03-14 21:38 UTC (permalink / raw)
  To: linux-arm-kernel

Hi

>> >> > gets called. Can you clarify what this register access does?
>> >> > If it's just setting a index into a mux output, would it make
>> >> > sense to have an optional DT property containing an integer with
>> >> > the mux setting you want to set? That way you wouldn't even
>> >> > have to have two compatible strings but just do
>> >> >
>> >> >         ret = of_property_read_u32(node, "apm,ahci-mux", &mux);
>> >> >         if (!ret)
>> >> >                 xgene_ahci_mux_select(ctx, mux);
>> >> >
>> >>
>> >> Given that fact that I will break up the resource. Let's just use the
>> >> resource for the MUX to handle this. For the IP that doesn't existed,
>> >> I will just not list it.
>> >
>> > Ah, that sounds good. It also means that if the firmware has set up
>> > the mux already, you don't have to touch it again. That would probably
>> > be the preferred case.
>> >
>> > I'm undecided about the value that you write in there. That could either
>> > be passed through DT as I suggested above for extra flexibility, or you
>> > can keep it hardcoded if you are absolutely sure that there will never
>> > need to be a case where you have to set it to something other than '1',
>>
>> It is only a mux between two IP's. Writing 1 is fine. For ACPI, it can
>> be missing and handled by the firmware or can be handled just like the
>> DTS way.
>
> Why can't you have it handled by the firmware in the DTS case?
>
> I still think it's rather unlikely that we will actually see ACPI support
> on your platform, btw.
>
> I'm willing to look at the patches you need for it, but I'm not very
> optimistic, in particular because of the kind of hacks you need
> for random bits of hardware.
>

I am about to post another version. Can you look at the next version?
If you still think it needs to be moved to the FW (either U-Boot or
Tianocore UEFI boot loader), I will then move out of the Linux driver
code.

-Loc

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
  2014-03-14 21:38                     ` Loc Ho
@ 2014-03-15  9:01                       ` Arnd Bergmann
  -1 siblings, 0 replies; 32+ messages in thread
From: Arnd Bergmann @ 2014-03-15  9:01 UTC (permalink / raw)
  To: Loc Ho
  Cc: Olof Johansson, Tejun Heo, Linux SCSI List, linux-ide,
	devicetree, linux-arm-kernel, Don Dutile, Jon Masters, patches,
	Tuan Phan, Suman Tripathi

On Friday 14 March 2014, Loc Ho wrote:

> > I still think it's rather unlikely that we will actually see ACPI support
> > on your platform, btw.
> >
> > I'm willing to look at the patches you need for it, but I'm not very
> > optimistic, in particular because of the kind of hacks you need
> > for random bits of hardware.
> >
> 
> I am about to post another version. Can you look at the next version?
> If you still think it needs to be moved to the FW (either U-Boot or
> Tianocore UEFI boot loader), I will then move out of the Linux driver
> code.

I would certainly be happier if it could just be moved into the
firmware or boot loader all the time. The only times we actually
want to do this stuff in the kernel is when we can't trust the
boot loader to get it right, or when the kernel has to support
alternative configurations that the boot loader is not aware of,
e.g. when you can plug in different kinds of extension boards but
the boot loader doesn't know which one you have.

	Arnd

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver
@ 2014-03-15  9:01                       ` Arnd Bergmann
  0 siblings, 0 replies; 32+ messages in thread
From: Arnd Bergmann @ 2014-03-15  9:01 UTC (permalink / raw)
  To: linux-arm-kernel

On Friday 14 March 2014, Loc Ho wrote:

> > I still think it's rather unlikely that we will actually see ACPI support
> > on your platform, btw.
> >
> > I'm willing to look at the patches you need for it, but I'm not very
> > optimistic, in particular because of the kind of hacks you need
> > for random bits of hardware.
> >
> 
> I am about to post another version. Can you look at the next version?
> If you still think it needs to be moved to the FW (either U-Boot or
> Tianocore UEFI boot loader), I will then move out of the Linux driver
> code.

I would certainly be happier if it could just be moved into the
firmware or boot loader all the time. The only times we actually
want to do this stuff in the kernel is when we can't trust the
boot loader to get it right, or when the kernel has to support
alternative configurations that the boot loader is not aware of,
e.g. when you can plug in different kinds of extension boards but
the boot loader doesn't know which one you have.

	Arnd

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2014-03-15  9:02 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-12 20:19 [PATCH v17 0/4] ata: Add APM X-Gene SoC AHCI SATA host controller support Loc Ho
2014-03-12 20:19 ` Loc Ho
2014-03-12 20:19 ` [PATCH v17 1/4] arm64: Add APM X-Gene SoC 15Gbps Multi-purpose PHY DTS entries Loc Ho
2014-03-12 20:19   ` Loc Ho
2014-03-12 20:19   ` [PATCH v17 2/4] Documentation: Add documentation for the APM X-Gene SoC SATA host controller DTS binding Loc Ho
2014-03-12 20:19     ` Loc Ho
2014-03-12 20:19     ` [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver Loc Ho
2014-03-12 20:19       ` Loc Ho
2014-03-12 20:19       ` [PATCH v17 4/4] arm64: Add APM X-Gene SoC AHCI SATA host controller DTS entries Loc Ho
2014-03-12 20:19         ` Loc Ho
2014-03-14 11:42         ` Arnd Bergmann
2014-03-14 11:42           ` Arnd Bergmann
2014-03-13 17:01       ` [PATCH v17 3/4] ata: Add APM X-Gene SoC AHCI SATA host controller driver Bartlomiej Zolnierkiewicz
2014-03-13 17:01         ` Bartlomiej Zolnierkiewicz
2014-03-14 11:41       ` Arnd Bergmann
2014-03-14 11:41         ` Arnd Bergmann
2014-03-14 18:49         ` Loc Ho
2014-03-14 18:49           ` Loc Ho
2014-03-14 19:05           ` Arnd Bergmann
2014-03-14 19:05             ` Arnd Bergmann
2014-03-14 20:27             ` Loc Ho
2014-03-14 20:27               ` Loc Ho
     [not found]               ` <CAPw-ZT=1KWEowD_i=qoWwg372XdzC4p227rpNo7ov+NwOYFUDA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-03-14 21:06                 ` Arnd Bergmann
2014-03-14 21:06                   ` Arnd Bergmann
2014-03-14 21:38                   ` Loc Ho
2014-03-14 21:38                     ` Loc Ho
2014-03-15  9:01                     ` Arnd Bergmann
2014-03-15  9:01                       ` Arnd Bergmann
2014-03-14 11:11     ` [PATCH v17 2/4] Documentation: Add documentation for the APM X-Gene SoC SATA host controller DTS binding Arnd Bergmann
2014-03-14 11:11       ` Arnd Bergmann
2014-03-14 11:08   ` [PATCH v17 1/4] arm64: Add APM X-Gene SoC 15Gbps Multi-purpose PHY DTS entries Arnd Bergmann
2014-03-14 11:08     ` Arnd Bergmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.