* [PATCH v3 0/3] ahci: enable ahci sata support on imx6q
@ 2013-07-05 9:52 ` Richard Zhu
0 siblings, 0 replies; 24+ messages in thread
From: Richard Zhu @ 2013-07-05 9:52 UTC (permalink / raw)
To: shawn.guo
Cc: linux-arm-kernel, jgarzik, avorontsov, rob.herring, s.hauer, linux-ide
v3: add imx6q specific ahci sata support
- Keep arch/arm and ahci_platform driver clean.
- Add the sata_imx standalone driver contained all the
specific setup
- Add the release function, support the loadable module
driver.
- Tested on imx6q sd board.
v2: http://www.spinics.net/lists/linux-ide/msg45666.html
- Setup standalone imx ahci sata driver, because of
the misalignments of the bits definition of the HBA register.
- Replace the node by the label in the board dts.
-
v1: http://www.spinics.net/lists/linux-ide/msg45581.html
- add imx6q specific ahci sata support to arch/arm/mach-imx/mach-imx6q.c
These patches is based on imx/dt branch of
"http://git.linaro.org/git-ro/people/shawnguo/linux-2.6.git"
[v3 1/3] ARM: dtsi: enable ahci sata on imx6q platforms
[v3 2/3] ARM: imx6q: update the sata bits definitions of gpr13
[v3 3/3] sata: imx: add ahci sata support on imx platforms
arch/arm/boot/dts/imx6q-sabreauto.dts | 4 +
arch/arm/boot/dts/imx6q-sabrelite.dts | 4 +
arch/arm/boot/dts/imx6q-sabresd.dts | 4 +
arch/arm/boot/dts/imx6q.dtsi | 9 +
drivers/ata/Kconfig | 9 +
drivers/ata/Makefile | 1 +
drivers/ata/sata_imx.c | 221 +++++++++++++++++++++++++++
include/linux/mfd/syscon/imx6q-iomuxc-gpr.h | 121 ++++++++++-----
8 files changed, 336 insertions(+), 37 deletions(-)
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 0/3] ahci: enable ahci sata support on imx6q
@ 2013-07-05 9:52 ` Richard Zhu
0 siblings, 0 replies; 24+ messages in thread
From: Richard Zhu @ 2013-07-05 9:52 UTC (permalink / raw)
To: linux-arm-kernel
v3: add imx6q specific ahci sata support
- Keep arch/arm and ahci_platform driver clean.
- Add the sata_imx standalone driver contained all the
specific setup
- Add the release function, support the loadable module
driver.
- Tested on imx6q sd board.
v2: http://www.spinics.net/lists/linux-ide/msg45666.html
- Setup standalone imx ahci sata driver, because of
the misalignments of the bits definition of the HBA register.
- Replace the node by the label in the board dts.
-
v1: http://www.spinics.net/lists/linux-ide/msg45581.html
- add imx6q specific ahci sata support to arch/arm/mach-imx/mach-imx6q.c
These patches is based on imx/dt branch of
"http://git.linaro.org/git-ro/people/shawnguo/linux-2.6.git"
[v3 1/3] ARM: dtsi: enable ahci sata on imx6q platforms
[v3 2/3] ARM: imx6q: update the sata bits definitions of gpr13
[v3 3/3] sata: imx: add ahci sata support on imx platforms
arch/arm/boot/dts/imx6q-sabreauto.dts | 4 +
arch/arm/boot/dts/imx6q-sabrelite.dts | 4 +
arch/arm/boot/dts/imx6q-sabresd.dts | 4 +
arch/arm/boot/dts/imx6q.dtsi | 9 +
drivers/ata/Kconfig | 9 +
drivers/ata/Makefile | 1 +
drivers/ata/sata_imx.c | 221 +++++++++++++++++++++++++++
include/linux/mfd/syscon/imx6q-iomuxc-gpr.h | 121 ++++++++++-----
8 files changed, 336 insertions(+), 37 deletions(-)
^ permalink raw reply [flat|nested] 24+ messages in thread
* [v3 1/3] ARM: dtsi: enable ahci sata on imx6q platforms
2013-07-05 9:52 ` Richard Zhu
(?)
@ 2013-07-05 9:52 ` Richard Zhu
-1 siblings, 0 replies; 24+ messages in thread
From: Richard Zhu @ 2013-07-05 9:52 UTC (permalink / raw)
To: shawn.guo
Cc: linux-arm-kernel, jgarzik, avorontsov, rob.herring, s.hauer,
linux-ide, Richard Zhu
From: Richard Zhu <r65037@freescale.com>
Only imx6q has the ahci sata controller, enable
it on imx6q platforms.
Signed-off-by: Richard Zhu <r65037@freescale.com>
---
arch/arm/boot/dts/imx6q-sabreauto.dts | 4 ++++
arch/arm/boot/dts/imx6q-sabrelite.dts | 4 ++++
arch/arm/boot/dts/imx6q-sabresd.dts | 4 ++++
arch/arm/boot/dts/imx6q.dtsi | 9 +++++++++
4 files changed, 21 insertions(+), 0 deletions(-)
diff --git a/arch/arm/boot/dts/imx6q-sabreauto.dts b/arch/arm/boot/dts/imx6q-sabreauto.dts
index 09a7580..5cd5209 100644
--- a/arch/arm/boot/dts/imx6q-sabreauto.dts
+++ b/arch/arm/boot/dts/imx6q-sabreauto.dts
@@ -20,6 +20,10 @@
compatible = "fsl,imx6q-sabreauto", "fsl,imx6q";
};
+&ahci {
+ status = "okay";
+};
+
&iomuxc {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_hog>;
diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
index 6a00066..6bf6fec 100644
--- a/arch/arm/boot/dts/imx6q-sabrelite.dts
+++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
@@ -65,6 +65,10 @@
};
};
+&ahci {
+ status = "okay";
+};
+
&ecspi1 {
fsl,spi-num-chipselects = <1>;
cs-gpios = <&gpio3 19 0>;
diff --git a/arch/arm/boot/dts/imx6q-sabresd.dts b/arch/arm/boot/dts/imx6q-sabresd.dts
index 0038228..d0579b8 100644
--- a/arch/arm/boot/dts/imx6q-sabresd.dts
+++ b/arch/arm/boot/dts/imx6q-sabresd.dts
@@ -20,6 +20,10 @@
compatible = "fsl,imx6q-sabresd", "fsl,imx6q";
};
+&ahci {
+ status = "okay";
+};
+
&iomuxc {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_hog>;
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index e7dd2c4..98f071e 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -424,6 +424,15 @@
};
};
+ ahci: ahci@02200000 {
+ compatible = "fsl,imx6q-ahci";
+ reg = <0x02200000 0x4000>;
+ interrupts = <0 39 0x04>;
+ clocks = <&clks 154>, <&clks 187>;
+ clock-names = "sata", "sata_ref_100m";
+ status = "disabled";
+ };
+
ipu2: ipu@02800000 {
#crtc-cells = <1>;
compatible = "fsl,imx6q-ipu";
--
1.7.5.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [v3 2/3] ARM: imx6q: update the sata bits definitions of gpr13
2013-07-05 9:52 ` Richard Zhu
(?)
(?)
@ 2013-07-05 9:52 ` Richard Zhu
-1 siblings, 0 replies; 24+ messages in thread
From: Richard Zhu @ 2013-07-05 9:52 UTC (permalink / raw)
To: shawn.guo
Cc: linux-arm-kernel, jgarzik, avorontsov, rob.herring, s.hauer,
linux-ide, Richard Zhu
From: Richard Zhu <r65037@freescale.com>
Replace the SATA_PHY_# by the more readable definitons.
Signed-off-by: Richard Zhu <r65037@freescale.com>
---
include/linux/mfd/syscon/imx6q-iomuxc-gpr.h | 121 ++++++++++++++++++--------
1 files changed, 84 insertions(+), 37 deletions(-)
diff --git a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
index dab34a1..e235251 100644
--- a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
+++ b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
@@ -279,41 +279,88 @@
#define IMX6Q_GPR13_CAN2_STOP_REQ BIT(29)
#define IMX6Q_GPR13_CAN1_STOP_REQ BIT(28)
#define IMX6Q_GPR13_ENET_STOP_REQ BIT(27)
-#define IMX6Q_GPR13_SATA_PHY_8_MASK (0x7 << 24)
-#define IMX6Q_GPR13_SATA_PHY_8_0_5_DB (0x0 << 24)
-#define IMX6Q_GPR13_SATA_PHY_8_1_0_DB (0x1 << 24)
-#define IMX6Q_GPR13_SATA_PHY_8_1_5_DB (0x2 << 24)
-#define IMX6Q_GPR13_SATA_PHY_8_2_0_DB (0x3 << 24)
-#define IMX6Q_GPR13_SATA_PHY_8_2_5_DB (0x4 << 24)
-#define IMX6Q_GPR13_SATA_PHY_8_3_0_DB (0x5 << 24)
-#define IMX6Q_GPR13_SATA_PHY_8_3_5_DB (0x6 << 24)
-#define IMX6Q_GPR13_SATA_PHY_8_4_0_DB (0x7 << 24)
-#define IMX6Q_GPR13_SATA_PHY_7_MASK (0x1f << 19)
-#define IMX6Q_GPR13_SATA_PHY_7_SATA1I (0x10 << 19)
-#define IMX6Q_GPR13_SATA_PHY_7_SATA1M (0x10 << 19)
-#define IMX6Q_GPR13_SATA_PHY_7_SATA1X (0x1a << 19)
-#define IMX6Q_GPR13_SATA_PHY_7_SATA2I (0x12 << 19)
-#define IMX6Q_GPR13_SATA_PHY_7_SATA2M (0x12 << 19)
-#define IMX6Q_GPR13_SATA_PHY_7_SATA2X (0x1a << 19)
-#define IMX6Q_GPR13_SATA_PHY_6_MASK (0x7 << 16)
-#define IMX6Q_GPR13_SATA_SPEED_MASK BIT(15)
-#define IMX6Q_GPR13_SATA_SPEED_1P5G 0x0
-#define IMX6Q_GPR13_SATA_SPEED_3P0G BIT(15)
-#define IMX6Q_GPR13_SATA_PHY_5 BIT(14)
-#define IMX6Q_GPR13_SATA_PHY_4_MASK (0x7 << 11)
-#define IMX6Q_GPR13_SATA_PHY_4_16_16 (0x0 << 11)
-#define IMX6Q_GPR13_SATA_PHY_4_14_16 (0x1 << 11)
-#define IMX6Q_GPR13_SATA_PHY_4_12_16 (0x2 << 11)
-#define IMX6Q_GPR13_SATA_PHY_4_10_16 (0x3 << 11)
-#define IMX6Q_GPR13_SATA_PHY_4_9_16 (0x4 << 11)
-#define IMX6Q_GPR13_SATA_PHY_4_8_16 (0x5 << 11)
-#define IMX6Q_GPR13_SATA_PHY_3_MASK (0xf << 7)
-#define IMX6Q_GPR13_SATA_PHY_3_OFF 0x7
-#define IMX6Q_GPR13_SATA_PHY_2_MASK (0x1f << 2)
-#define IMX6Q_GPR13_SATA_PHY_2_OFF 0x2
-#define IMX6Q_GPR13_SATA_PHY_1_MASK (0x3 << 0)
-#define IMX6Q_GPR13_SATA_PHY_1_FAST (0x0 << 0)
-#define IMX6Q_GPR13_SATA_PHY_1_MED (0x1 << 0)
-#define IMX6Q_GPR13_SATA_PHY_1_SLOW (0x2 << 0)
-
+#define IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK (0x7 << 24)
+#define IMX6Q_GPR13_SATA_RX_EQ_VAL_0_5_DB (0x0 << 24)
+#define IMX6Q_GPR13_SATA_RX_EQ_VAL_1_0_DB (0x1 << 24)
+#define IMX6Q_GPR13_SATA_RX_EQ_VAL_1_5_DB (0x2 << 24)
+#define IMX6Q_GPR13_SATA_RX_EQ_VAL_2_0_DB (0x3 << 24)
+#define IMX6Q_GPR13_SATA_RX_EQ_VAL_2_5_DB (0x4 << 24)
+#define IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB (0x5 << 24)
+#define IMX6Q_GPR13_SATA_RX_EQ_VAL_3_5_DB (0x6 << 24)
+#define IMX6Q_GPR13_SATA_RX_EQ_VAL_4_0_DB (0x7 << 24)
+#define IMX6Q_GPR13_SATA_RX_LOS_LVL_MASK (0x1f << 19)
+#define IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA1I (0x10 << 19)
+#define IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA1M (0x10 << 19)
+#define IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA1X (0x1a << 19)
+#define IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2I (0x12 << 19)
+#define IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M (0x12 << 19)
+#define IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2X (0x1a << 19)
+#define IMX6Q_GPR13_SATA_RX_DPLL_MODE_MASK (0x7 << 16)
+#define IMX6Q_GPR13_SATA_RX_DPLL_MODE_1P_1F (0x0 << 16)
+#define IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_2F (0x1 << 16)
+#define IMX6Q_GPR13_SATA_RX_DPLL_MODE_1P_4F (0x2 << 16)
+#define IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F (0x3 << 16)
+#define IMX6Q_GPR13_SATA_SPD_MODE_MASK BIT(15)
+#define IMX6Q_GPR13_SATA_SPD_MODE_1P5G 0x0
+#define IMX6Q_GPR13_SATA_SPD_MODE_3P0G BIT(15)
+#define IMX6Q_GPR13_SATA_MPLL_SS_EN BIT(14)
+#define IMX6Q_GPR13_SATA_TX_ATTEN_MASK (0x7 << 11)
+#define IMX6Q_GPR13_SATA_TX_ATTEN_16_16 (0x0 << 11)
+#define IMX6Q_GPR13_SATA_TX_ATTEN_14_16 (0x1 << 11)
+#define IMX6Q_GPR13_SATA_TX_ATTEN_12_16 (0x2 << 11)
+#define IMX6Q_GPR13_SATA_TX_ATTEN_10_16 (0x3 << 11)
+#define IMX6Q_GPR13_SATA_TX_ATTEN_9_16 (0x4 << 11)
+#define IMX6Q_GPR13_SATA_TX_ATTEN_8_16 (0x5 << 11)
+#define IMX6Q_GPR13_SATA_TX_BOOST_MASK (0xf << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_0_00_DB (0x0 << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_0_37_DB (0x1 << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_0_74_DB (0x2 << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_1_11_DB (0x3 << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_1_48_DB (0x4 << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_1_85_DB (0x5 << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_2_22_DB (0x6 << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_2_59_DB (0x7 << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_2_96_DB (0x8 << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB (0x9 << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_3_70_DB (0xa << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_4_07_DB (0xb << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_4_44_DB (0xc << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_4_81_DB (0xd << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_5_28_DB (0xe << 7)
+#define IMX6Q_GPR13_SATA_TX_BOOST_5_75_DB (0xf << 7)
+#define IMX6Q_GPR13_SATA_TX_LVL_MASK (0x1f << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_0_937_V (0x00 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_0_947_V (0x01 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_0_957_V (0x02 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_0_966_V (0x03 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_0_976_V (0x04 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_0_986_V (0x05 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_0_996_V (0x06 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_005_V (0x07 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_015_V (0x08 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_025_V (0x09 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_035_V (0x0a << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_045_V (0x0b << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_054_V (0x0c << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_064_V (0x0d << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_074_V (0x0e << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_084_V (0x0f << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_094_V (0x10 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_104_V (0x11 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_113_V (0x12 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_123_V (0x13 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_133_V (0x14 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_143_V (0x15 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_152_V (0x16 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_162_V (0x17 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_172_V (0x18 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_182_V (0x19 << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_191_V (0x1a << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_201_V (0x1b << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_211_V (0x1c << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_221_V (0x1d << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_230_V (0x1e << 2)
+#define IMX6Q_GPR13_SATA_TX_LVL_1_240_V (0x1f << 2)
+#define IMX6Q_GPR13_SATA_MPLL_CLK_EN BIT(1)
+#define IMX6Q_GPR13_SATA_TX_EDGE_RATE BIT(0)
#endif /* __LINUX_IMX6Q_IOMUXC_GPR_H */
--
1.7.5.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [v3 3/3] sata: imx: add ahci sata support on imx platforms
2013-07-05 9:52 ` Richard Zhu
` (2 preceding siblings ...)
(?)
@ 2013-07-05 9:52 ` Richard Zhu
2013-07-05 15:59 ` Sascha Hauer
2013-07-05 17:43 ` Tejun Heo
-1 siblings, 2 replies; 24+ messages in thread
From: Richard Zhu @ 2013-07-05 9:52 UTC (permalink / raw)
To: shawn.guo
Cc: linux-arm-kernel, jgarzik, avorontsov, rob.herring, s.hauer,
linux-ide, Richard Zhu
From: Richard Zhu <r65037@freescale.com>
imx6q contains one Synopsys AHCI SATA controller,
But it can't shares ahci_platform driver with other
controllers.
Because there are some misalignments of the bits
definitions of the HBA registers and the Vendor
Specific registers
- CAP_SSS(bit20) of the HOST_CAP is writable, default
value is '0', should be configured to be '1'
- bit0 (only one AHCI SATA port on imx6q) of the
HOST_PORTS_IMPL should be set to be '1'.(default 0)
- One Vendor Specific register HOST_TIMER1MS(offset:0xe0)
should be configured regarding to the frequency of AHB
bus clock.
- Configurations of the AHCI PHY clock, and the signal
parameters of the GPR13
Setup its own ahci sata driver, enable the imx6q ahci
sata support.
Signed-off-by: Richard Zhu <r65037@freescale.com>
---
drivers/ata/Kconfig | 9 ++
drivers/ata/Makefile | 1 +
drivers/ata/sata_imx.c | 230 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 240 insertions(+), 0 deletions(-)
create mode 100644 drivers/ata/sata_imx.c
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index a5a3ebc..45ed2f0 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -97,6 +97,15 @@ config SATA_AHCI_PLATFORM
If unsure, say N.
+config SATA_IMX
+ tristate "Freescale iMX AHCI SATA support"
+ depends on SATA_AHCI_PLATFORM
+ help
+ This option enables support for the Freescale iMX SoC's
+ onboard AHCI SATA.
+
+ If unsure, say N.
+
config SATA_FSL
tristate "Freescale 3.0Gbps SATA support"
depends on FSL_SOC
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index c04d0fd..04b1c6c 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_SATA_INIC162X) += sata_inic162x.o
obj-$(CONFIG_SATA_SIL24) += sata_sil24.o
obj-$(CONFIG_SATA_DWC) += sata_dwc_460ex.o
obj-$(CONFIG_SATA_HIGHBANK) += sata_highbank.o libahci.o
+obj-$(CONFIG_SATA_IMX) += sata_imx.o
# SFF w/ custom DMA
obj-$(CONFIG_PDC_ADMA) += pdc_adma.o
diff --git a/drivers/ata/sata_imx.c b/drivers/ata/sata_imx.c
new file mode 100644
index 0000000..a129efb
--- /dev/null
+++ b/drivers/ata/sata_imx.c
@@ -0,0 +1,230 @@
+/*
+ * Freescale IMX AHCI SATA platform driver
+ * Copyright 2013 Freescale Semiconductor, Inc.
+ *
+ * based on the AHCI SATA platform driver by Jeff Garzik and Anton Vorontsov
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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/>.
+ */
+
+#include <linux/clk.h>
+#include <linux/kernel.h>
+#include <linux/gfp.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/libata.h>
+#include <linux/ahci_platform.h>
+#include <linux/of_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
+#include "ahci.h"
+#include <linux/clk-private.h>
+
+enum {
+ HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
+};
+
+/* imx6q ahci module initialization. */
+static int imx6q_sata_phy_clk(struct device *dev, int enable)
+{
+ int ret = 0;
+ struct clk *sata_ref_clk;
+
+ sata_ref_clk = devm_clk_get(dev, "sata_ref_100m");
+ if (IS_ERR(sata_ref_clk)) {
+ dev_err(dev, "can't get sata_ref clock.\n");
+ return PTR_ERR(sata_ref_clk);
+ }
+ if (enable) {
+ /* Enable PHY clock */
+ ret = clk_prepare_enable(sata_ref_clk);
+ if (ret < 0)
+ dev_err(dev, "can't prepare-enable sata_ref clock\n");
+ } else {
+ /* Disable PHY clock */
+ clk_disable_unprepare(sata_ref_clk);
+ }
+
+ return ret;
+}
+
+static int imx6q_sata_init(struct device *dev, void __iomem *mmio)
+{
+ int ret = 0;
+ struct regmap *gpr;
+ struct clk *ahb_clk;
+
+ ret = imx6q_sata_phy_clk(dev, true);
+ if (ret < 0)
+ return ret;
+
+ gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
+ if (IS_ERR(gpr)) {
+ dev_err(dev, "failed to find fsl,imx6q-iomux-gpr regmap\n");
+ return PTR_ERR(gpr);
+ }
+
+ /*
+ * set PHY Paremeters, two steps to configure the GPR13,
+ * one write for rest of parameters, mask of first write
+ * is 0x07fffffd, and the other one write for setting
+ * the mpll_clk_en.
+ */
+ regmap_update_bits(gpr, 0x34, IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK
+ | IMX6Q_GPR13_SATA_RX_LOS_LVL_MASK
+ | IMX6Q_GPR13_SATA_RX_DPLL_MODE_MASK
+ | IMX6Q_GPR13_SATA_SPD_MODE_MASK
+ | IMX6Q_GPR13_SATA_MPLL_SS_EN
+ | IMX6Q_GPR13_SATA_TX_ATTEN_MASK
+ | IMX6Q_GPR13_SATA_TX_BOOST_MASK
+ | IMX6Q_GPR13_SATA_TX_LVL_MASK
+ | IMX6Q_GPR13_SATA_TX_EDGE_RATE
+ , IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB
+ | IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M
+ | IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F
+ | IMX6Q_GPR13_SATA_SPD_MODE_3P0G
+ | IMX6Q_GPR13_SATA_MPLL_SS_EN
+ | IMX6Q_GPR13_SATA_TX_ATTEN_9_16
+ | IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB
+ | IMX6Q_GPR13_SATA_TX_LVL_1_025_V);
+ regmap_update_bits(gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN,
+ IMX6Q_GPR13_SATA_MPLL_CLK_EN);
+ usleep_range(100, 200);
+
+ /*
+ * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL,
+ * and IP vendor specific register HOST_TIMER1MS.
+ *
+ * Configure CAP_SSS (support stagered spin up).
+ * Implement the port0.
+ * Get the ahb clock rate, and configure the TIMER1MS register.
+ */
+ ret = readl(mmio + HOST_CAP);
+ if (!(ret & HOST_CAP_SSS))
+ writel(ret |= HOST_CAP_SSS, mmio + HOST_CAP);
+ ret = readl(mmio + HOST_PORTS_IMPL);
+ if (!(ret & 0x1))
+ writel((ret | 0x1), mmio + HOST_PORTS_IMPL);
+
+ ahb_clk = devm_clk_get(dev, "ahb");
+ if (IS_ERR(ahb_clk)) {
+ dev_err(dev, "no ahb clock.\n");
+ return PTR_ERR(ahb_clk);
+ }
+ ret = clk_get_rate(ahb_clk) / 1000;
+ writel(ret, mmio + HOST_TIMER1MS);
+ devm_clk_put(dev, ahb_clk);
+
+ return 0;
+}
+
+static void imx6q_sata_exit(struct device *dev)
+{
+ struct regmap *gpr;
+
+ gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
+ if (IS_ERR(gpr))
+ dev_err(dev, "failed to find fsl,imx6q-iomux-gpr regmap\n");
+
+ regmap_update_bits(gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN,
+ !IMX6Q_GPR13_SATA_MPLL_CLK_EN);
+
+ imx6q_sata_phy_clk(dev, false);
+}
+
+static struct ahci_platform_data imx6q_sata_pdata = {
+ .init = imx6q_sata_init,
+ .exit = imx6q_sata_exit,
+};
+static const struct of_device_id imx_ahci_of_match[] = {
+ { .compatible = "fsl,imx6q-ahci", .data = &imx6q_sata_pdata},
+ {},
+};
+MODULE_DEVICE_TABLE(of, imx_ahci_of_match);
+
+static void imx_ahci_platform_release(struct device *dev)
+{
+ return;
+}
+
+static struct platform_device ahci_pdev = {
+ .name = "ahci",
+ .id = -1,
+ .dev = {
+ .release = imx_ahci_platform_release,
+ }
+};
+
+static int imx_ahci_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct resource *mem, *irq, **res;
+ const struct of_device_id *of_id;
+ const struct ahci_platform_data *pdata = NULL;
+ int ret;
+
+ of_id = of_match_device(imx_ahci_of_match, &pdev->dev);
+ if (of_id)
+ pdata = of_id->data;
+ else
+ return -EINVAL;
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ if (!mem || !irq) {
+ dev_err(dev, "no mmio/irq resource\n");
+ return -EINVAL;
+ }
+
+ ahci_pdev.dev.coherent_dma_mask = DMA_BIT_MASK(32);
+ ahci_pdev.dev.dma_mask = &ahci_pdev.dev.coherent_dma_mask;
+ ahci_pdev.dev.of_node = pdev->dev.of_node;
+
+ res[0] = mem;
+ res[1] = irq;
+ platform_device_add_resources(&ahci_pdev, *res, 2);
+ platform_device_add_data(&ahci_pdev, pdata, sizeof(*pdata));
+
+ ret = platform_device_register(&ahci_pdev);
+ if (!ret)
+ return ret;
+
+ return 0;
+}
+
+static int imx_ahci_remove(struct platform_device *pdev)
+{
+ platform_device_unregister(&ahci_pdev);
+ return 0;
+}
+
+static struct platform_driver imx_ahci_driver = {
+ .probe = imx_ahci_probe,
+ .remove = imx_ahci_remove,
+ .driver = {
+ .name = "sata-imx",
+ .owner = THIS_MODULE,
+ .of_match_table = imx_ahci_of_match,
+ },
+};
+module_platform_driver(imx_ahci_driver);
+
+MODULE_DESCRIPTION("Freescale iMX AHCI SATA platform driver");
+MODULE_AUTHOR("Richard Zhu <Hong-Xing.Zhu@freescale.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("sata:imx");
--
1.7.5.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [v3 3/3] sata: imx: add ahci sata support on imx platforms
2013-07-05 9:52 ` [v3 3/3] sata: imx: add ahci sata support on imx platforms Richard Zhu
@ 2013-07-05 15:59 ` Sascha Hauer
2013-07-05 17:43 ` Tejun Heo
1 sibling, 0 replies; 24+ messages in thread
From: Sascha Hauer @ 2013-07-05 15:59 UTC (permalink / raw)
To: Richard Zhu
Cc: shawn.guo, linux-arm-kernel, jgarzik, avorontsov, rob.herring,
linux-ide, Richard Zhu
On Fri, Jul 05, 2013 at 05:52:22PM +0800, Richard Zhu wrote:
> From: Richard Zhu <r65037@freescale.com>
>
> imx6q contains one Synopsys AHCI SATA controller,
> But it can't shares ahci_platform driver with other
> controllers.
> Because there are some misalignments of the bits
> definitions of the HBA registers and the Vendor
> Specific registers
> - CAP_SSS(bit20) of the HOST_CAP is writable, default
> value is '0', should be configured to be '1'
> - bit0 (only one AHCI SATA port on imx6q) of the
> HOST_PORTS_IMPL should be set to be '1'.(default 0)
> - One Vendor Specific register HOST_TIMER1MS(offset:0xe0)
> should be configured regarding to the frequency of AHB
> bus clock.
> - Configurations of the AHCI PHY clock, and the signal
> parameters of the GPR13
>
> Setup its own ahci sata driver, enable the imx6q ahci
> sata support.
>
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
> drivers/ata/Kconfig | 9 ++
> drivers/ata/Makefile | 1 +
> drivers/ata/sata_imx.c | 230 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 240 insertions(+), 0 deletions(-)
> create mode 100644 drivers/ata/sata_imx.c
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index a5a3ebc..45ed2f0 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -97,6 +97,15 @@ config SATA_AHCI_PLATFORM
>
> If unsure, say N.
>
> +config SATA_IMX
> + tristate "Freescale iMX AHCI SATA support"
It's rather untypical to write iMX. Please write i.MX.
> + depends on SATA_AHCI_PLATFORM
> + help
> + This option enables support for the Freescale iMX SoC's
> + onboard AHCI SATA.
> +
> + If unsure, say N.
> +
> config SATA_FSL
> tristate "Freescale 3.0Gbps SATA support"
> depends on FSL_SOC
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index c04d0fd..04b1c6c 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_SATA_INIC162X) += sata_inic162x.o
> obj-$(CONFIG_SATA_SIL24) += sata_sil24.o
> obj-$(CONFIG_SATA_DWC) += sata_dwc_460ex.o
> obj-$(CONFIG_SATA_HIGHBANK) += sata_highbank.o libahci.o
> +obj-$(CONFIG_SATA_IMX) += sata_imx.o
>
> # SFF w/ custom DMA
> obj-$(CONFIG_PDC_ADMA) += pdc_adma.o
> diff --git a/drivers/ata/sata_imx.c b/drivers/ata/sata_imx.c
> new file mode 100644
> index 0000000..a129efb
> --- /dev/null
> +++ b/drivers/ata/sata_imx.c
> @@ -0,0 +1,230 @@
> +/*
> + * Freescale IMX AHCI SATA platform driver
> + * Copyright 2013 Freescale Semiconductor, Inc.
> + *
> + * based on the AHCI SATA platform driver by Jeff Garzik and Anton Vorontsov
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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/>.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/kernel.h>
> +#include <linux/gfp.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/libata.h>
> +#include <linux/ahci_platform.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
> +#include "ahci.h"
> +#include <linux/clk-private.h>
Do you need anything from clk-private.h? You shouldn't.
> +
> +enum {
> + HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
> +};
> +
> +/* imx6q ahci module initialization. */
> +static int imx6q_sata_phy_clk(struct device *dev, int enable)
> +{
> + int ret = 0;
> + struct clk *sata_ref_clk;
> +
> + sata_ref_clk = devm_clk_get(dev, "sata_ref_100m");
> + if (IS_ERR(sata_ref_clk)) {
> + dev_err(dev, "can't get sata_ref clock.\n");
> + return PTR_ERR(sata_ref_clk);
> + }
clk_get must be balanced with clk_put. You need to have some private
data and keep a pointer to the clock.
> + if (enable) {
> + /* Enable PHY clock */
> + ret = clk_prepare_enable(sata_ref_clk);
> + if (ret < 0)
> + dev_err(dev, "can't prepare-enable sata_ref clock\n");
When printing error messages it's always helpful to print the error code
aswell.
> + } else {
> + /* Disable PHY clock */
This comment (also the one above enable) doesn't contain any useful
information.
> + clk_disable_unprepare(sata_ref_clk);
> + }
> +
> + return ret;
> +}
> +
> +static int imx6q_sata_init(struct device *dev, void __iomem *mmio)
> +{
> + int ret = 0;
> + struct regmap *gpr;
> + struct clk *ahb_clk;
> +
> + ret = imx6q_sata_phy_clk(dev, true);
> + if (ret < 0)
> + return ret;
> +
> + gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> + if (IS_ERR(gpr)) {
> + dev_err(dev, "failed to find fsl,imx6q-iomux-gpr regmap\n");
> + return PTR_ERR(gpr);
> + }
> +
> + /*
> + * set PHY Paremeters, two steps to configure the GPR13,
> + * one write for rest of parameters, mask of first write
> + * is 0x07fffffd, and the other one write for setting
> + * the mpll_clk_en.
> + */
> + regmap_update_bits(gpr, 0x34, IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK
> + | IMX6Q_GPR13_SATA_RX_LOS_LVL_MASK
> + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_MASK
> + | IMX6Q_GPR13_SATA_SPD_MODE_MASK
> + | IMX6Q_GPR13_SATA_MPLL_SS_EN
> + | IMX6Q_GPR13_SATA_TX_ATTEN_MASK
> + | IMX6Q_GPR13_SATA_TX_BOOST_MASK
> + | IMX6Q_GPR13_SATA_TX_LVL_MASK
> + | IMX6Q_GPR13_SATA_TX_EDGE_RATE
> + , IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB
> + | IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M
> + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F
> + | IMX6Q_GPR13_SATA_SPD_MODE_3P0G
> + | IMX6Q_GPR13_SATA_MPLL_SS_EN
> + | IMX6Q_GPR13_SATA_TX_ATTEN_9_16
> + | IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB
> + | IMX6Q_GPR13_SATA_TX_LVL_1_025_V);
> + regmap_update_bits(gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN,
> + IMX6Q_GPR13_SATA_MPLL_CLK_EN);
> + usleep_range(100, 200);
> +
> + /*
> + * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL,
> + * and IP vendor specific register HOST_TIMER1MS.
> + *
> + * Configure CAP_SSS (support stagered spin up).
> + * Implement the port0.
> + * Get the ahb clock rate, and configure the TIMER1MS register.
> + */
> + ret = readl(mmio + HOST_CAP);
You should use a u32 type for storing readl results.
> + if (!(ret & HOST_CAP_SSS))
> + writel(ret |= HOST_CAP_SSS, mmio + HOST_CAP);
Why do you write this conditionally? Just write it unconditionally.
> + ret = readl(mmio + HOST_PORTS_IMPL);
> + if (!(ret & 0x1))
> + writel((ret | 0x1), mmio + HOST_PORTS_IMPL);
ditto.
> +
> + ahb_clk = devm_clk_get(dev, "ahb");
> + if (IS_ERR(ahb_clk)) {
> + dev_err(dev, "no ahb clock.\n");
> + return PTR_ERR(ahb_clk);
> + }
This leaves the phy clock enabled in the error case.
> + ret = clk_get_rate(ahb_clk) / 1000;
> + writel(ret, mmio + HOST_TIMER1MS);
> + devm_clk_put(dev, ahb_clk);
> +
> + return 0;
> +}
> +
> +static void imx6q_sata_exit(struct device *dev)
> +{
> + struct regmap *gpr;
> +
> + gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> + if (IS_ERR(gpr))
> + dev_err(dev, "failed to find fsl,imx6q-iomux-gpr regmap\n");
> +
> + regmap_update_bits(gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN,
> + !IMX6Q_GPR13_SATA_MPLL_CLK_EN);
> +
> + imx6q_sata_phy_clk(dev, false);
> +}
> +
> +static struct ahci_platform_data imx6q_sata_pdata = {
> + .init = imx6q_sata_init,
> + .exit = imx6q_sata_exit,
> +};
> +static const struct of_device_id imx_ahci_of_match[] = {
> + { .compatible = "fsl,imx6q-ahci", .data = &imx6q_sata_pdata},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, imx_ahci_of_match);
> +
> +static void imx_ahci_platform_release(struct device *dev)
> +{
> + return;
> +}
> +
> +static struct platform_device ahci_pdev = {
> + .name = "ahci",
> + .id = -1,
> + .dev = {
> + .release = imx_ahci_platform_release,
> + }
> +};
> +
> +static int imx_ahci_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct resource *mem, *irq, **res;
> + const struct of_device_id *of_id;
> + const struct ahci_platform_data *pdata = NULL;
> + int ret;
> +
> + of_id = of_match_device(imx_ahci_of_match, &pdev->dev);
> + if (of_id)
> + pdata = of_id->data;
> + else
> + return -EINVAL;
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (!mem || !irq) {
> + dev_err(dev, "no mmio/irq resource\n");
> + return -EINVAL;
> + }
> +
> + ahci_pdev.dev.coherent_dma_mask = DMA_BIT_MASK(32);
> + ahci_pdev.dev.dma_mask = &ahci_pdev.dev.coherent_dma_mask;
> + ahci_pdev.dev.of_node = pdev->dev.of_node;
No need to make ahci_pdev global. platform_device_add_data makes a copy
anyway and making it global makes this driver broken for multiple
instances.
> +
> + res[0] = mem;
> + res[1] = irq;
You have an uninitalized pointer to a pointer to struct resource which
you treat as an array here. Doesn't the compiler warn you?
> + platform_device_add_resources(&ahci_pdev, *res, 2);
> + platform_device_add_data(&ahci_pdev, pdata, sizeof(*pdata));
> +
> + ret = platform_device_register(&ahci_pdev);
> + if (!ret)
> + return ret;
s/!ret/ret/
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 24+ messages in thread
* [v3 3/3] sata: imx: add ahci sata support on imx platforms
@ 2013-07-05 15:59 ` Sascha Hauer
0 siblings, 0 replies; 24+ messages in thread
From: Sascha Hauer @ 2013-07-05 15:59 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jul 05, 2013 at 05:52:22PM +0800, Richard Zhu wrote:
> From: Richard Zhu <r65037@freescale.com>
>
> imx6q contains one Synopsys AHCI SATA controller,
> But it can't shares ahci_platform driver with other
> controllers.
> Because there are some misalignments of the bits
> definitions of the HBA registers and the Vendor
> Specific registers
> - CAP_SSS(bit20) of the HOST_CAP is writable, default
> value is '0', should be configured to be '1'
> - bit0 (only one AHCI SATA port on imx6q) of the
> HOST_PORTS_IMPL should be set to be '1'.(default 0)
> - One Vendor Specific register HOST_TIMER1MS(offset:0xe0)
> should be configured regarding to the frequency of AHB
> bus clock.
> - Configurations of the AHCI PHY clock, and the signal
> parameters of the GPR13
>
> Setup its own ahci sata driver, enable the imx6q ahci
> sata support.
>
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
> drivers/ata/Kconfig | 9 ++
> drivers/ata/Makefile | 1 +
> drivers/ata/sata_imx.c | 230 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 240 insertions(+), 0 deletions(-)
> create mode 100644 drivers/ata/sata_imx.c
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index a5a3ebc..45ed2f0 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -97,6 +97,15 @@ config SATA_AHCI_PLATFORM
>
> If unsure, say N.
>
> +config SATA_IMX
> + tristate "Freescale iMX AHCI SATA support"
It's rather untypical to write iMX. Please write i.MX.
> + depends on SATA_AHCI_PLATFORM
> + help
> + This option enables support for the Freescale iMX SoC's
> + onboard AHCI SATA.
> +
> + If unsure, say N.
> +
> config SATA_FSL
> tristate "Freescale 3.0Gbps SATA support"
> depends on FSL_SOC
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index c04d0fd..04b1c6c 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_SATA_INIC162X) += sata_inic162x.o
> obj-$(CONFIG_SATA_SIL24) += sata_sil24.o
> obj-$(CONFIG_SATA_DWC) += sata_dwc_460ex.o
> obj-$(CONFIG_SATA_HIGHBANK) += sata_highbank.o libahci.o
> +obj-$(CONFIG_SATA_IMX) += sata_imx.o
>
> # SFF w/ custom DMA
> obj-$(CONFIG_PDC_ADMA) += pdc_adma.o
> diff --git a/drivers/ata/sata_imx.c b/drivers/ata/sata_imx.c
> new file mode 100644
> index 0000000..a129efb
> --- /dev/null
> +++ b/drivers/ata/sata_imx.c
> @@ -0,0 +1,230 @@
> +/*
> + * Freescale IMX AHCI SATA platform driver
> + * Copyright 2013 Freescale Semiconductor, Inc.
> + *
> + * based on the AHCI SATA platform driver by Jeff Garzik and Anton Vorontsov
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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/>.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/kernel.h>
> +#include <linux/gfp.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/libata.h>
> +#include <linux/ahci_platform.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
> +#include "ahci.h"
> +#include <linux/clk-private.h>
Do you need anything from clk-private.h? You shouldn't.
> +
> +enum {
> + HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
> +};
> +
> +/* imx6q ahci module initialization. */
> +static int imx6q_sata_phy_clk(struct device *dev, int enable)
> +{
> + int ret = 0;
> + struct clk *sata_ref_clk;
> +
> + sata_ref_clk = devm_clk_get(dev, "sata_ref_100m");
> + if (IS_ERR(sata_ref_clk)) {
> + dev_err(dev, "can't get sata_ref clock.\n");
> + return PTR_ERR(sata_ref_clk);
> + }
clk_get must be balanced with clk_put. You need to have some private
data and keep a pointer to the clock.
> + if (enable) {
> + /* Enable PHY clock */
> + ret = clk_prepare_enable(sata_ref_clk);
> + if (ret < 0)
> + dev_err(dev, "can't prepare-enable sata_ref clock\n");
When printing error messages it's always helpful to print the error code
aswell.
> + } else {
> + /* Disable PHY clock */
This comment (also the one above enable) doesn't contain any useful
information.
> + clk_disable_unprepare(sata_ref_clk);
> + }
> +
> + return ret;
> +}
> +
> +static int imx6q_sata_init(struct device *dev, void __iomem *mmio)
> +{
> + int ret = 0;
> + struct regmap *gpr;
> + struct clk *ahb_clk;
> +
> + ret = imx6q_sata_phy_clk(dev, true);
> + if (ret < 0)
> + return ret;
> +
> + gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> + if (IS_ERR(gpr)) {
> + dev_err(dev, "failed to find fsl,imx6q-iomux-gpr regmap\n");
> + return PTR_ERR(gpr);
> + }
> +
> + /*
> + * set PHY Paremeters, two steps to configure the GPR13,
> + * one write for rest of parameters, mask of first write
> + * is 0x07fffffd, and the other one write for setting
> + * the mpll_clk_en.
> + */
> + regmap_update_bits(gpr, 0x34, IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK
> + | IMX6Q_GPR13_SATA_RX_LOS_LVL_MASK
> + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_MASK
> + | IMX6Q_GPR13_SATA_SPD_MODE_MASK
> + | IMX6Q_GPR13_SATA_MPLL_SS_EN
> + | IMX6Q_GPR13_SATA_TX_ATTEN_MASK
> + | IMX6Q_GPR13_SATA_TX_BOOST_MASK
> + | IMX6Q_GPR13_SATA_TX_LVL_MASK
> + | IMX6Q_GPR13_SATA_TX_EDGE_RATE
> + , IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB
> + | IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M
> + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F
> + | IMX6Q_GPR13_SATA_SPD_MODE_3P0G
> + | IMX6Q_GPR13_SATA_MPLL_SS_EN
> + | IMX6Q_GPR13_SATA_TX_ATTEN_9_16
> + | IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB
> + | IMX6Q_GPR13_SATA_TX_LVL_1_025_V);
> + regmap_update_bits(gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN,
> + IMX6Q_GPR13_SATA_MPLL_CLK_EN);
> + usleep_range(100, 200);
> +
> + /*
> + * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL,
> + * and IP vendor specific register HOST_TIMER1MS.
> + *
> + * Configure CAP_SSS (support stagered spin up).
> + * Implement the port0.
> + * Get the ahb clock rate, and configure the TIMER1MS register.
> + */
> + ret = readl(mmio + HOST_CAP);
You should use a u32 type for storing readl results.
> + if (!(ret & HOST_CAP_SSS))
> + writel(ret |= HOST_CAP_SSS, mmio + HOST_CAP);
Why do you write this conditionally? Just write it unconditionally.
> + ret = readl(mmio + HOST_PORTS_IMPL);
> + if (!(ret & 0x1))
> + writel((ret | 0x1), mmio + HOST_PORTS_IMPL);
ditto.
> +
> + ahb_clk = devm_clk_get(dev, "ahb");
> + if (IS_ERR(ahb_clk)) {
> + dev_err(dev, "no ahb clock.\n");
> + return PTR_ERR(ahb_clk);
> + }
This leaves the phy clock enabled in the error case.
> + ret = clk_get_rate(ahb_clk) / 1000;
> + writel(ret, mmio + HOST_TIMER1MS);
> + devm_clk_put(dev, ahb_clk);
> +
> + return 0;
> +}
> +
> +static void imx6q_sata_exit(struct device *dev)
> +{
> + struct regmap *gpr;
> +
> + gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> + if (IS_ERR(gpr))
> + dev_err(dev, "failed to find fsl,imx6q-iomux-gpr regmap\n");
> +
> + regmap_update_bits(gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN,
> + !IMX6Q_GPR13_SATA_MPLL_CLK_EN);
> +
> + imx6q_sata_phy_clk(dev, false);
> +}
> +
> +static struct ahci_platform_data imx6q_sata_pdata = {
> + .init = imx6q_sata_init,
> + .exit = imx6q_sata_exit,
> +};
> +static const struct of_device_id imx_ahci_of_match[] = {
> + { .compatible = "fsl,imx6q-ahci", .data = &imx6q_sata_pdata},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, imx_ahci_of_match);
> +
> +static void imx_ahci_platform_release(struct device *dev)
> +{
> + return;
> +}
> +
> +static struct platform_device ahci_pdev = {
> + .name = "ahci",
> + .id = -1,
> + .dev = {
> + .release = imx_ahci_platform_release,
> + }
> +};
> +
> +static int imx_ahci_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct resource *mem, *irq, **res;
> + const struct of_device_id *of_id;
> + const struct ahci_platform_data *pdata = NULL;
> + int ret;
> +
> + of_id = of_match_device(imx_ahci_of_match, &pdev->dev);
> + if (of_id)
> + pdata = of_id->data;
> + else
> + return -EINVAL;
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (!mem || !irq) {
> + dev_err(dev, "no mmio/irq resource\n");
> + return -EINVAL;
> + }
> +
> + ahci_pdev.dev.coherent_dma_mask = DMA_BIT_MASK(32);
> + ahci_pdev.dev.dma_mask = &ahci_pdev.dev.coherent_dma_mask;
> + ahci_pdev.dev.of_node = pdev->dev.of_node;
No need to make ahci_pdev global. platform_device_add_data makes a copy
anyway and making it global makes this driver broken for multiple
instances.
> +
> + res[0] = mem;
> + res[1] = irq;
You have an uninitalized pointer to a pointer to struct resource which
you treat as an array here. Doesn't the compiler warn you?
> + platform_device_add_resources(&ahci_pdev, *res, 2);
> + platform_device_add_data(&ahci_pdev, pdata, sizeof(*pdata));
> +
> + ret = platform_device_register(&ahci_pdev);
> + if (!ret)
> + return ret;
s/!ret/ret/
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re[2]: [v3 3/3] sata: imx: add ahci sata support on imx platforms
2013-07-05 15:59 ` Sascha Hauer
(?)
@ 2013-07-05 16:06 ` Alexander Shiyan
2013-07-06 1:55 ` Zhu Richard-R65037
-1 siblings, 1 reply; 24+ messages in thread
From: Alexander Shiyan @ 2013-07-05 16:06 UTC (permalink / raw)
To: Sascha Hauer
Cc: Richard Zhu, Richard Zhu, shawn.guo, rob.herring, linux-ide,
avorontsov, jgarzik, linux-arm-kernel
> On Fri, Jul 05, 2013 at 05:52:22PM +0800, Richard Zhu wrote:
> > From: Richard Zhu <r65037@freescale.com>
...
> > + if (!(ret & HOST_CAP_SSS))
> > + writel(ret |= HOST_CAP_SSS, mmio + HOST_CAP);
>
> Why do you write this conditionally? Just write it unconditionally.
"writel" with assignment is incorrect in any case.
---
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v3 3/3] sata: imx: add ahci sata support on imx platforms
2013-07-05 9:52 ` [v3 3/3] sata: imx: add ahci sata support on imx platforms Richard Zhu
@ 2013-07-05 17:43 ` Tejun Heo
2013-07-05 17:43 ` Tejun Heo
1 sibling, 0 replies; 24+ messages in thread
From: Tejun Heo @ 2013-07-05 17:43 UTC (permalink / raw)
To: Richard Zhu
Cc: shawn.guo, linux-arm-kernel, jgarzik, avorontsov, rob.herring,
s.hauer, linux-ide, Richard Zhu
Hello,
On Fri, Jul 05, 2013 at 05:52:22PM +0800, Richard Zhu wrote:
> - CAP_SSS(bit20) of the HOST_CAP is writable, default
> value is '0', should be configured to be '1'
> - bit0 (only one AHCI SATA port on imx6q) of the
> HOST_PORTS_IMPL should be set to be '1'.(default 0)
> - One Vendor Specific register HOST_TIMER1MS(offset:0xe0)
> should be configured regarding to the frequency of AHB
> bus clock.
> - Configurations of the AHCI PHY clock, and the signal
> parameters of the GPR13
Can't you just add a platform specific init callback to ahci_platform?
I don't see how these small differences in init sequence justify a
separate driver.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 24+ messages in thread
* [v3 3/3] sata: imx: add ahci sata support on imx platforms
@ 2013-07-05 17:43 ` Tejun Heo
0 siblings, 0 replies; 24+ messages in thread
From: Tejun Heo @ 2013-07-05 17:43 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On Fri, Jul 05, 2013 at 05:52:22PM +0800, Richard Zhu wrote:
> - CAP_SSS(bit20) of the HOST_CAP is writable, default
> value is '0', should be configured to be '1'
> - bit0 (only one AHCI SATA port on imx6q) of the
> HOST_PORTS_IMPL should be set to be '1'.(default 0)
> - One Vendor Specific register HOST_TIMER1MS(offset:0xe0)
> should be configured regarding to the frequency of AHB
> bus clock.
> - Configurations of the AHCI PHY clock, and the signal
> parameters of the GPR13
Can't you just add a platform specific init callback to ahci_platform?
I don't see how these small differences in init sequence justify a
separate driver.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [v3 3/3] sata: imx: add ahci sata support on imx platforms
2013-07-05 15:59 ` Sascha Hauer
@ 2013-07-06 1:54 ` Zhu Richard-R65037
-1 siblings, 0 replies; 24+ messages in thread
From: Zhu Richard-R65037 @ 2013-07-06 1:54 UTC (permalink / raw)
To: Sascha Hauer, Richard Zhu
Cc: shawn.guo, linux-arm-kernel, jgarzik, avorontsov, rob.herring, linux-ide
Hi Sascha:
Thanks for your comments.
Best Regards
Richard Zhu
________________________________________
From: Sascha Hauer [s.hauer@pengutronix.de]
Sent: Friday, July 05, 2013 11:59 PM
To: Richard Zhu
Cc: shawn.guo@linaro.org; linux-arm-kernel@lists.infradead.org; jgarzik@pobox.com; avorontsov@ru.mvista.com; rob.herring@calxeda.com; linux-ide@vger.kernel.org; Zhu Richard-R65037
Subject: Re: [v3 3/3] sata: imx: add ahci sata support on imx platforms
On Fri, Jul 05, 2013 at 05:52:22PM +0800, Richard Zhu wrote:
> From: Richard Zhu <r65037@freescale.com>
>
> imx6q contains one Synopsys AHCI SATA controller,
> But it can't shares ahci_platform driver with other
> controllers.
> Because there are some misalignments of the bits
> definitions of the HBA registers and the Vendor
> Specific registers
> - CAP_SSS(bit20) of the HOST_CAP is writable, default
> value is '0', should be configured to be '1'
> - bit0 (only one AHCI SATA port on imx6q) of the
> HOST_PORTS_IMPL should be set to be '1'.(default 0)
> - One Vendor Specific register HOST_TIMER1MS(offset:0xe0)
> should be configured regarding to the frequency of AHB
> bus clock.
> - Configurations of the AHCI PHY clock, and the signal
> parameters of the GPR13
>
> Setup its own ahci sata driver, enable the imx6q ahci
> sata support.
>
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
> drivers/ata/Kconfig | 9 ++
> drivers/ata/Makefile | 1 +
> drivers/ata/sata_imx.c | 230 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 240 insertions(+), 0 deletions(-)
> create mode 100644 drivers/ata/sata_imx.c
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index a5a3ebc..45ed2f0 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -97,6 +97,15 @@ config SATA_AHCI_PLATFORM
>
> If unsure, say N.
>
> +config SATA_IMX
> + tristate "Freescale iMX AHCI SATA support"
It's rather untypical to write iMX. Please write i.MX.
[Richard] Accepted.
> + depends on SATA_AHCI_PLATFORM
> + help
> + This option enables support for the Freescale iMX SoC's
> + onboard AHCI SATA.
> +
> + If unsure, say N.
> +
> config SATA_FSL
> tristate "Freescale 3.0Gbps SATA support"
> depends on FSL_SOC
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index c04d0fd..04b1c6c 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_SATA_INIC162X) += sata_inic162x.o
> obj-$(CONFIG_SATA_SIL24) += sata_sil24.o
> obj-$(CONFIG_SATA_DWC) += sata_dwc_460ex.o
> obj-$(CONFIG_SATA_HIGHBANK) += sata_highbank.o libahci.o
> +obj-$(CONFIG_SATA_IMX) += sata_imx.o
>
> # SFF w/ custom DMA
> obj-$(CONFIG_PDC_ADMA) += pdc_adma.o
> diff --git a/drivers/ata/sata_imx.c b/drivers/ata/sata_imx.c
> new file mode 100644
> index 0000000..a129efb
> --- /dev/null
> +++ b/drivers/ata/sata_imx.c
> @@ -0,0 +1,230 @@
> +/*
> + * Freescale IMX AHCI SATA platform driver
> + * Copyright 2013 Freescale Semiconductor, Inc.
> + *
> + * based on the AHCI SATA platform driver by Jeff Garzik and Anton Vorontsov
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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/>.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/kernel.h>
> +#include <linux/gfp.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/libata.h>
> +#include <linux/ahci_platform.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
> +#include "ahci.h"
> +#include <linux/clk-private.h>
Do you need anything from clk-private.h? You shouldn't.
[Richard]it would be removed.
> +
> +enum {
> + HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
> +};
> +
> +/* imx6q ahci module initialization. */
> +static int imx6q_sata_phy_clk(struct device *dev, int enable)
> +{
> + int ret = 0;
> + struct clk *sata_ref_clk;
> +
> + sata_ref_clk = devm_clk_get(dev, "sata_ref_100m");
> + if (IS_ERR(sata_ref_clk)) {
> + dev_err(dev, "can't get sata_ref clock.\n");
> + return PTR_ERR(sata_ref_clk);
> + }
clk_get must be balanced with clk_put. You need to have some private
data and keep a pointer to the clock.
[Richard] Ok, I can create the private data, and keep a pointer to this clock.
> + if (enable) {
> + /* Enable PHY clock */
> + ret = clk_prepare_enable(sata_ref_clk);
> + if (ret < 0)
> + dev_err(dev, "can't prepare-enable sata_ref clock\n");
When printing error messages it's always helpful to print the error code
aswell.
[Richard] Ok, the return-err code would be added.
> + } else {
> + /* Disable PHY clock */
This comment (also the one above enable) doesn't contain any useful
information.
[Richard] ok, they would be removed.
> + clk_disable_unprepare(sata_ref_clk);
> + }
> +
> + return ret;
> +}
> +
> +static int imx6q_sata_init(struct device *dev, void __iomem *mmio)
> +{
> + int ret = 0;
> + struct regmap *gpr;
> + struct clk *ahb_clk;
> +
> + ret = imx6q_sata_phy_clk(dev, true);
> + if (ret < 0)
> + return ret;
> +
> + gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> + if (IS_ERR(gpr)) {
> + dev_err(dev, "failed to find fsl,imx6q-iomux-gpr regmap\n");
> + return PTR_ERR(gpr);
> + }
> +
> + /*
> + * set PHY Paremeters, two steps to configure the GPR13,
> + * one write for rest of parameters, mask of first write
> + * is 0x07fffffd, and the other one write for setting
> + * the mpll_clk_en.
> + */
> + regmap_update_bits(gpr, 0x34, IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK
> + | IMX6Q_GPR13_SATA_RX_LOS_LVL_MASK
> + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_MASK
> + | IMX6Q_GPR13_SATA_SPD_MODE_MASK
> + | IMX6Q_GPR13_SATA_MPLL_SS_EN
> + | IMX6Q_GPR13_SATA_TX_ATTEN_MASK
> + | IMX6Q_GPR13_SATA_TX_BOOST_MASK
> + | IMX6Q_GPR13_SATA_TX_LVL_MASK
> + | IMX6Q_GPR13_SATA_TX_EDGE_RATE
> + , IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB
> + | IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M
> + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F
> + | IMX6Q_GPR13_SATA_SPD_MODE_3P0G
> + | IMX6Q_GPR13_SATA_MPLL_SS_EN
> + | IMX6Q_GPR13_SATA_TX_ATTEN_9_16
> + | IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB
> + | IMX6Q_GPR13_SATA_TX_LVL_1_025_V);
> + regmap_update_bits(gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN,
> + IMX6Q_GPR13_SATA_MPLL_CLK_EN);
> + usleep_range(100, 200);
> +
> + /*
> + * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL,
> + * and IP vendor specific register HOST_TIMER1MS.
> + *
> + * Configure CAP_SSS (support stagered spin up).
> + * Implement the port0.
> + * Get the ahb clock rate, and configure the TIMER1MS register.
> + */
> + ret = readl(mmio + HOST_CAP);
You should use a u32 type for storing readl results.
[Richard] Accepted.
> + if (!(ret & HOST_CAP_SSS))
> + writel(ret |= HOST_CAP_SSS, mmio + HOST_CAP);
Why do you write this conditionally? Just write it unconditionally.
[Richard]Ok, no problem. Would write it uncontitionally.
> + ret = readl(mmio + HOST_PORTS_IMPL);
> + if (!(ret & 0x1))
> + writel((ret | 0x1), mmio + HOST_PORTS_IMPL);
ditto.
> +
> + ahb_clk = devm_clk_get(dev, "ahb");
> + if (IS_ERR(ahb_clk)) {
> + dev_err(dev, "no ahb clock.\n");
> + return PTR_ERR(ahb_clk);
> + }
This leaves the phy clock enabled in the error case.
[Richard] Yes, it is. the phy clock should be disabled.
> + ret = clk_get_rate(ahb_clk) / 1000;
> + writel(ret, mmio + HOST_TIMER1MS);
> + devm_clk_put(dev, ahb_clk);
> +
> + return 0;
> +}
> +
> +static void imx6q_sata_exit(struct device *dev)
> +{
> + struct regmap *gpr;
> +
> + gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> + if (IS_ERR(gpr))
> + dev_err(dev, "failed to find fsl,imx6q-iomux-gpr regmap\n");
> +
> + regmap_update_bits(gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN,
> + !IMX6Q_GPR13_SATA_MPLL_CLK_EN);
> +
> + imx6q_sata_phy_clk(dev, false);
> +}
> +
> +static struct ahci_platform_data imx6q_sata_pdata = {
> + .init = imx6q_sata_init,
> + .exit = imx6q_sata_exit,
> +};
> +static const struct of_device_id imx_ahci_of_match[] = {
> + { .compatible = "fsl,imx6q-ahci", .data = &imx6q_sata_pdata},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, imx_ahci_of_match);
> +
> +static void imx_ahci_platform_release(struct device *dev)
> +{
> + return;
> +}
> +
> +static struct platform_device ahci_pdev = {
> + .name = "ahci",
> + .id = -1,
> + .dev = {
> + .release = imx_ahci_platform_release,
> + }
> +};
> +
> +static int imx_ahci_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct resource *mem, *irq, **res;
> + const struct of_device_id *of_id;
> + const struct ahci_platform_data *pdata = NULL;
> + int ret;
> +
> + of_id = of_match_device(imx_ahci_of_match, &pdev->dev);
> + if (of_id)
> + pdata = of_id->data;
> + else
> + return -EINVAL;
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (!mem || !irq) {
> + dev_err(dev, "no mmio/irq resource\n");
> + return -EINVAL;
> + }
> +
> + ahci_pdev.dev.coherent_dma_mask = DMA_BIT_MASK(32);
> + ahci_pdev.dev.dma_mask = &ahci_pdev.dev.coherent_dma_mask;
> + ahci_pdev.dev.of_node = pdev->dev.of_node;
No need to make ahci_pdev global. platform_device_add_data makes a copy
anyway and making it global makes this driver broken for multiple
instances.
[Richard]accepted.
> +
> + res[0] = mem;
> + res[1] = irq;
You have an uninitalized pointer to a pointer to struct resource which
you treat as an array here. Doesn't the compiler warn you?
[Richard]Compiler doesn't report the warning message, anyway, I would fix it later.
> + platform_device_add_resources(&ahci_pdev, *res, 2);
> + platform_device_add_data(&ahci_pdev, pdata, sizeof(*pdata));
> +
> + ret = platform_device_register(&ahci_pdev);
> + if (!ret)
> + return ret;
s/!ret/ret/
[Richard] Would be fixed.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 24+ messages in thread
* [v3 3/3] sata: imx: add ahci sata support on imx platforms
@ 2013-07-06 1:54 ` Zhu Richard-R65037
0 siblings, 0 replies; 24+ messages in thread
From: Zhu Richard-R65037 @ 2013-07-06 1:54 UTC (permalink / raw)
To: linux-arm-kernel
Hi Sascha:
Thanks for your comments.
Best Regards
Richard Zhu
________________________________________
From: Sascha Hauer [s.hauer at pengutronix.de]
Sent: Friday, July 05, 2013 11:59 PM
To: Richard Zhu
Cc: shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org; jgarzik at pobox.com; avorontsov at ru.mvista.com; rob.herring at calxeda.com; linux-ide at vger.kernel.org; Zhu Richard-R65037
Subject: Re: [v3 3/3] sata: imx: add ahci sata support on imx platforms
On Fri, Jul 05, 2013 at 05:52:22PM +0800, Richard Zhu wrote:
> From: Richard Zhu <r65037@freescale.com>
>
> imx6q contains one Synopsys AHCI SATA controller,
> But it can't shares ahci_platform driver with other
> controllers.
> Because there are some misalignments of the bits
> definitions of the HBA registers and the Vendor
> Specific registers
> - CAP_SSS(bit20) of the HOST_CAP is writable, default
> value is '0', should be configured to be '1'
> - bit0 (only one AHCI SATA port on imx6q) of the
> HOST_PORTS_IMPL should be set to be '1'.(default 0)
> - One Vendor Specific register HOST_TIMER1MS(offset:0xe0)
> should be configured regarding to the frequency of AHB
> bus clock.
> - Configurations of the AHCI PHY clock, and the signal
> parameters of the GPR13
>
> Setup its own ahci sata driver, enable the imx6q ahci
> sata support.
>
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
> drivers/ata/Kconfig | 9 ++
> drivers/ata/Makefile | 1 +
> drivers/ata/sata_imx.c | 230 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 240 insertions(+), 0 deletions(-)
> create mode 100644 drivers/ata/sata_imx.c
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index a5a3ebc..45ed2f0 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -97,6 +97,15 @@ config SATA_AHCI_PLATFORM
>
> If unsure, say N.
>
> +config SATA_IMX
> + tristate "Freescale iMX AHCI SATA support"
It's rather untypical to write iMX. Please write i.MX.
[Richard] Accepted.
> + depends on SATA_AHCI_PLATFORM
> + help
> + This option enables support for the Freescale iMX SoC's
> + onboard AHCI SATA.
> +
> + If unsure, say N.
> +
> config SATA_FSL
> tristate "Freescale 3.0Gbps SATA support"
> depends on FSL_SOC
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index c04d0fd..04b1c6c 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_SATA_INIC162X) += sata_inic162x.o
> obj-$(CONFIG_SATA_SIL24) += sata_sil24.o
> obj-$(CONFIG_SATA_DWC) += sata_dwc_460ex.o
> obj-$(CONFIG_SATA_HIGHBANK) += sata_highbank.o libahci.o
> +obj-$(CONFIG_SATA_IMX) += sata_imx.o
>
> # SFF w/ custom DMA
> obj-$(CONFIG_PDC_ADMA) += pdc_adma.o
> diff --git a/drivers/ata/sata_imx.c b/drivers/ata/sata_imx.c
> new file mode 100644
> index 0000000..a129efb
> --- /dev/null
> +++ b/drivers/ata/sata_imx.c
> @@ -0,0 +1,230 @@
> +/*
> + * Freescale IMX AHCI SATA platform driver
> + * Copyright 2013 Freescale Semiconductor, Inc.
> + *
> + * based on the AHCI SATA platform driver by Jeff Garzik and Anton Vorontsov
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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/>.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/kernel.h>
> +#include <linux/gfp.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/libata.h>
> +#include <linux/ahci_platform.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
> +#include "ahci.h"
> +#include <linux/clk-private.h>
Do you need anything from clk-private.h? You shouldn't.
[Richard]it would be removed.
> +
> +enum {
> + HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
> +};
> +
> +/* imx6q ahci module initialization. */
> +static int imx6q_sata_phy_clk(struct device *dev, int enable)
> +{
> + int ret = 0;
> + struct clk *sata_ref_clk;
> +
> + sata_ref_clk = devm_clk_get(dev, "sata_ref_100m");
> + if (IS_ERR(sata_ref_clk)) {
> + dev_err(dev, "can't get sata_ref clock.\n");
> + return PTR_ERR(sata_ref_clk);
> + }
clk_get must be balanced with clk_put. You need to have some private
data and keep a pointer to the clock.
[Richard] Ok, I can create the private data, and keep a pointer to this clock.
> + if (enable) {
> + /* Enable PHY clock */
> + ret = clk_prepare_enable(sata_ref_clk);
> + if (ret < 0)
> + dev_err(dev, "can't prepare-enable sata_ref clock\n");
When printing error messages it's always helpful to print the error code
aswell.
[Richard] Ok, the return-err code would be added.
> + } else {
> + /* Disable PHY clock */
This comment (also the one above enable) doesn't contain any useful
information.
[Richard] ok, they would be removed.
> + clk_disable_unprepare(sata_ref_clk);
> + }
> +
> + return ret;
> +}
> +
> +static int imx6q_sata_init(struct device *dev, void __iomem *mmio)
> +{
> + int ret = 0;
> + struct regmap *gpr;
> + struct clk *ahb_clk;
> +
> + ret = imx6q_sata_phy_clk(dev, true);
> + if (ret < 0)
> + return ret;
> +
> + gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> + if (IS_ERR(gpr)) {
> + dev_err(dev, "failed to find fsl,imx6q-iomux-gpr regmap\n");
> + return PTR_ERR(gpr);
> + }
> +
> + /*
> + * set PHY Paremeters, two steps to configure the GPR13,
> + * one write for rest of parameters, mask of first write
> + * is 0x07fffffd, and the other one write for setting
> + * the mpll_clk_en.
> + */
> + regmap_update_bits(gpr, 0x34, IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK
> + | IMX6Q_GPR13_SATA_RX_LOS_LVL_MASK
> + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_MASK
> + | IMX6Q_GPR13_SATA_SPD_MODE_MASK
> + | IMX6Q_GPR13_SATA_MPLL_SS_EN
> + | IMX6Q_GPR13_SATA_TX_ATTEN_MASK
> + | IMX6Q_GPR13_SATA_TX_BOOST_MASK
> + | IMX6Q_GPR13_SATA_TX_LVL_MASK
> + | IMX6Q_GPR13_SATA_TX_EDGE_RATE
> + , IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB
> + | IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M
> + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F
> + | IMX6Q_GPR13_SATA_SPD_MODE_3P0G
> + | IMX6Q_GPR13_SATA_MPLL_SS_EN
> + | IMX6Q_GPR13_SATA_TX_ATTEN_9_16
> + | IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB
> + | IMX6Q_GPR13_SATA_TX_LVL_1_025_V);
> + regmap_update_bits(gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN,
> + IMX6Q_GPR13_SATA_MPLL_CLK_EN);
> + usleep_range(100, 200);
> +
> + /*
> + * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL,
> + * and IP vendor specific register HOST_TIMER1MS.
> + *
> + * Configure CAP_SSS (support stagered spin up).
> + * Implement the port0.
> + * Get the ahb clock rate, and configure the TIMER1MS register.
> + */
> + ret = readl(mmio + HOST_CAP);
You should use a u32 type for storing readl results.
[Richard] Accepted.
> + if (!(ret & HOST_CAP_SSS))
> + writel(ret |= HOST_CAP_SSS, mmio + HOST_CAP);
Why do you write this conditionally? Just write it unconditionally.
[Richard]Ok, no problem. Would write it uncontitionally.
> + ret = readl(mmio + HOST_PORTS_IMPL);
> + if (!(ret & 0x1))
> + writel((ret | 0x1), mmio + HOST_PORTS_IMPL);
ditto.
> +
> + ahb_clk = devm_clk_get(dev, "ahb");
> + if (IS_ERR(ahb_clk)) {
> + dev_err(dev, "no ahb clock.\n");
> + return PTR_ERR(ahb_clk);
> + }
This leaves the phy clock enabled in the error case.
[Richard] Yes, it is. the phy clock should be disabled.
> + ret = clk_get_rate(ahb_clk) / 1000;
> + writel(ret, mmio + HOST_TIMER1MS);
> + devm_clk_put(dev, ahb_clk);
> +
> + return 0;
> +}
> +
> +static void imx6q_sata_exit(struct device *dev)
> +{
> + struct regmap *gpr;
> +
> + gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> + if (IS_ERR(gpr))
> + dev_err(dev, "failed to find fsl,imx6q-iomux-gpr regmap\n");
> +
> + regmap_update_bits(gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN,
> + !IMX6Q_GPR13_SATA_MPLL_CLK_EN);
> +
> + imx6q_sata_phy_clk(dev, false);
> +}
> +
> +static struct ahci_platform_data imx6q_sata_pdata = {
> + .init = imx6q_sata_init,
> + .exit = imx6q_sata_exit,
> +};
> +static const struct of_device_id imx_ahci_of_match[] = {
> + { .compatible = "fsl,imx6q-ahci", .data = &imx6q_sata_pdata},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, imx_ahci_of_match);
> +
> +static void imx_ahci_platform_release(struct device *dev)
> +{
> + return;
> +}
> +
> +static struct platform_device ahci_pdev = {
> + .name = "ahci",
> + .id = -1,
> + .dev = {
> + .release = imx_ahci_platform_release,
> + }
> +};
> +
> +static int imx_ahci_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct resource *mem, *irq, **res;
> + const struct of_device_id *of_id;
> + const struct ahci_platform_data *pdata = NULL;
> + int ret;
> +
> + of_id = of_match_device(imx_ahci_of_match, &pdev->dev);
> + if (of_id)
> + pdata = of_id->data;
> + else
> + return -EINVAL;
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (!mem || !irq) {
> + dev_err(dev, "no mmio/irq resource\n");
> + return -EINVAL;
> + }
> +
> + ahci_pdev.dev.coherent_dma_mask = DMA_BIT_MASK(32);
> + ahci_pdev.dev.dma_mask = &ahci_pdev.dev.coherent_dma_mask;
> + ahci_pdev.dev.of_node = pdev->dev.of_node;
No need to make ahci_pdev global. platform_device_add_data makes a copy
anyway and making it global makes this driver broken for multiple
instances.
[Richard]accepted.
> +
> + res[0] = mem;
> + res[1] = irq;
You have an uninitalized pointer to a pointer to struct resource which
you treat as an array here. Doesn't the compiler warn you?
[Richard]Compiler doesn't report the warning message, anyway, I would fix it later.
> + platform_device_add_resources(&ahci_pdev, *res, 2);
> + platform_device_add_data(&ahci_pdev, pdata, sizeof(*pdata));
> +
> + ret = platform_device_register(&ahci_pdev);
> + if (!ret)
> + return ret;
s/!ret/ret/
[Richard] Would be fixed.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: Re[2]: [v3 3/3] sata: imx: add ahci sata support on imx platforms
2013-07-05 16:06 ` Re[2]: " Alexander Shiyan
@ 2013-07-06 1:55 ` Zhu Richard-R65037
0 siblings, 0 replies; 24+ messages in thread
From: Zhu Richard-R65037 @ 2013-07-06 1:55 UTC (permalink / raw)
To: Alexander Shiyan, Sascha Hauer
Cc: Richard Zhu, shawn.guo, rob.herring, linux-ide, avorontsov,
jgarzik, linux-arm-kernel
Hi Alexander:
Thanks for your comments.
Best Regards
Richard Zhu
________________________________________
From: Alexander Shiyan [shc_work@mail.ru]
Sent: Saturday, July 06, 2013 12:06 AM
To: Sascha Hauer
Cc: Richard Zhu; Zhu Richard-R65037; shawn.guo@linaro.org; rob.herring@calxeda.com; linux-ide@vger.kernel.org; avorontsov@ru.mvista.com; jgarzik@pobox.com; linux-arm-kernel@lists.infradead.org
Subject: Re[2]: [v3 3/3] sata: imx: add ahci sata support on imx platforms
> On Fri, Jul 05, 2013 at 05:52:22PM +0800, Richard Zhu wrote:
> > From: Richard Zhu <r65037@freescale.com>
...
> > + if (!(ret & HOST_CAP_SSS))
> > + writel(ret |= HOST_CAP_SSS, mmio + HOST_CAP);
>
> Why do you write this conditionally? Just write it unconditionally.
"writel" with assignment is incorrect in any case.
[Richard] would be fixed later.
---
^ permalink raw reply [flat|nested] 24+ messages in thread
* [v3 3/3] sata: imx: add ahci sata support on imx platforms
@ 2013-07-06 1:55 ` Zhu Richard-R65037
0 siblings, 0 replies; 24+ messages in thread
From: Zhu Richard-R65037 @ 2013-07-06 1:55 UTC (permalink / raw)
To: linux-arm-kernel
Hi Alexander:
Thanks for your comments.
Best Regards
Richard Zhu
________________________________________
From: Alexander Shiyan [shc_work at mail.ru]
Sent: Saturday, July 06, 2013 12:06 AM
To: Sascha Hauer
Cc: Richard Zhu; Zhu Richard-R65037; shawn.guo at linaro.org; rob.herring at calxeda.com; linux-ide at vger.kernel.org; avorontsov at ru.mvista.com; jgarzik at pobox.com; linux-arm-kernel at lists.infradead.org
Subject: Re[2]: [v3 3/3] sata: imx: add ahci sata support on imx platforms
> On Fri, Jul 05, 2013 at 05:52:22PM +0800, Richard Zhu wrote:
> > From: Richard Zhu <r65037@freescale.com>
...
> > + if (!(ret & HOST_CAP_SSS))
> > + writel(ret |= HOST_CAP_SSS, mmio + HOST_CAP);
>
> Why do you write this conditionally? Just write it unconditionally.
"writel" with assignment is incorrect in any case.
[Richard] would be fixed later.
---
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [v3 3/3] sata: imx: add ahci sata support on imx platforms
2013-07-05 17:43 ` Tejun Heo
@ 2013-07-06 2:03 ` Zhu Richard-R65037
-1 siblings, 0 replies; 24+ messages in thread
From: Zhu Richard-R65037 @ 2013-07-06 2:03 UTC (permalink / raw)
To: Tejun Heo, Richard Zhu
Cc: shawn.guo, linux-arm-kernel, jgarzik, avorontsov, rob.herring,
s.hauer, linux-ide
Hi Tejun:
Thanks for your comments.
Best Regards
Richard Zhu
________________________________________
From: linux-ide-owner@vger.kernel.org [linux-ide-owner@vger.kernel.org] on behalf of Tejun Heo [tj@kernel.org]
Sent: Saturday, July 06, 2013 1:43 AM
To: Richard Zhu
Cc: shawn.guo@linaro.org; linux-arm-kernel@lists.infradead.org; jgarzik@pobox.com; avorontsov@ru.mvista.com; rob.herring@calxeda.com; s.hauer@pengutronix.de; linux-ide@vger.kernel.org; Zhu Richard-R65037
Subject: Re: [v3 3/3] sata: imx: add ahci sata support on imx platforms
Hello,
On Fri, Jul 05, 2013 at 05:52:22PM +0800, Richard Zhu wrote:
> - CAP_SSS(bit20) of the HOST_CAP is writable, default
> value is '0', should be configured to be '1'
> - bit0 (only one AHCI SATA port on imx6q) of the
> HOST_PORTS_IMPL should be set to be '1'.(default 0)
> - One Vendor Specific register HOST_TIMER1MS(offset:0xe0)
> should be configured regarding to the frequency of AHB
> bus clock.
> - Configurations of the AHCI PHY clock, and the signal
> parameters of the GPR13
Can't you just add a platform specific init callback to ahci_platform?
I don't see how these small differences in init sequence justify a
separate driver.
[Richard] Just like what we dicussed in the previous v1/v2 patch-set.
Shawn has the concerns that the IP speicific codes shouldn't be put in the platform level.
So he suggested that setup a stand-alone driver, contained the imx6q ahci sata specific
codes, and re-use the generic ahci_platform driver as much as possible.
This imx6q standalone ahci sata driver just registers the platform data, and the others would be handled
by ahci_platform driver.
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* [v3 3/3] sata: imx: add ahci sata support on imx platforms
@ 2013-07-06 2:03 ` Zhu Richard-R65037
0 siblings, 0 replies; 24+ messages in thread
From: Zhu Richard-R65037 @ 2013-07-06 2:03 UTC (permalink / raw)
To: linux-arm-kernel
Hi Tejun:
Thanks for your comments.
Best Regards
Richard Zhu
________________________________________
From: linux-ide-owner@vger.kernel.org [linux-ide-owner at vger.kernel.org] on behalf of Tejun Heo [tj at kernel.org]
Sent: Saturday, July 06, 2013 1:43 AM
To: Richard Zhu
Cc: shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org; jgarzik at pobox.com; avorontsov at ru.mvista.com; rob.herring at calxeda.com; s.hauer at pengutronix.de; linux-ide at vger.kernel.org; Zhu Richard-R65037
Subject: Re: [v3 3/3] sata: imx: add ahci sata support on imx platforms
Hello,
On Fri, Jul 05, 2013 at 05:52:22PM +0800, Richard Zhu wrote:
> - CAP_SSS(bit20) of the HOST_CAP is writable, default
> value is '0', should be configured to be '1'
> - bit0 (only one AHCI SATA port on imx6q) of the
> HOST_PORTS_IMPL should be set to be '1'.(default 0)
> - One Vendor Specific register HOST_TIMER1MS(offset:0xe0)
> should be configured regarding to the frequency of AHB
> bus clock.
> - Configurations of the AHCI PHY clock, and the signal
> parameters of the GPR13
Can't you just add a platform specific init callback to ahci_platform?
I don't see how these small differences in init sequence justify a
separate driver.
[Richard] Just like what we dicussed in the previous v1/v2 patch-set.
Shawn has the concerns that the IP speicific codes shouldn't be put in the platform level.
So he suggested that setup a stand-alone driver, contained the imx6q ahci sata specific
codes, and re-use the generic ahci_platform driver as much as possible.
This imx6q standalone ahci sata driver just registers the platform data, and the others would be handled
by ahci_platform driver.
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v3 3/3] sata: imx: add ahci sata support on imx platforms
2013-07-06 2:03 ` Zhu Richard-R65037
@ 2013-07-06 2:07 ` Tejun Heo
-1 siblings, 0 replies; 24+ messages in thread
From: Tejun Heo @ 2013-07-06 2:07 UTC (permalink / raw)
To: Zhu Richard-R65037
Cc: Richard Zhu, shawn.guo, linux-arm-kernel, jgarzik, avorontsov,
rob.herring, s.hauer, linux-ide
Hello,
On Sat, Jul 06, 2013 at 02:03:30AM +0000, Zhu Richard-R65037 wrote:
> [Richard] Just like what we dicussed in the previous v1/v2
> patch-set. Shawn has the concerns that the IP speicific codes
> shouldn't be put in the platform level. So he suggested that setup
> a stand-alone driver, contained the imx6q ahci sata specific codes,
> and re-use the generic ahci_platform driver as much as possible.
> This imx6q standalone ahci sata driver just registers the platform
> data, and the others would be handled by ahci_platform driver.
Oh, I'm not objecting to the ahci specific part not being in platform
code. I'm wondering whether specific handling for imx6q can be
included into ahci_platform rather than being in its own driver. It
just seems that there aren't too many differences. I could be
misreading it so if there are enough differences to warrant a separate
driver, please enlighten me.
Thanks!
--
tejun
^ permalink raw reply [flat|nested] 24+ messages in thread
* [v3 3/3] sata: imx: add ahci sata support on imx platforms
@ 2013-07-06 2:07 ` Tejun Heo
0 siblings, 0 replies; 24+ messages in thread
From: Tejun Heo @ 2013-07-06 2:07 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On Sat, Jul 06, 2013 at 02:03:30AM +0000, Zhu Richard-R65037 wrote:
> [Richard] Just like what we dicussed in the previous v1/v2
> patch-set. Shawn has the concerns that the IP speicific codes
> shouldn't be put in the platform level. So he suggested that setup
> a stand-alone driver, contained the imx6q ahci sata specific codes,
> and re-use the generic ahci_platform driver as much as possible.
> This imx6q standalone ahci sata driver just registers the platform
> data, and the others would be handled by ahci_platform driver.
Oh, I'm not objecting to the ahci specific part not being in platform
code. I'm wondering whether specific handling for imx6q can be
included into ahci_platform rather than being in its own driver. It
just seems that there aren't too many differences. I could be
misreading it so if there are enough differences to warrant a separate
driver, please enlighten me.
Thanks!
--
tejun
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [v3 3/3] sata: imx: add ahci sata support on imx platforms
2013-07-06 2:07 ` Tejun Heo
@ 2013-07-22 7:02 ` Zhu Richard-R65037
-1 siblings, 0 replies; 24+ messages in thread
From: Zhu Richard-R65037 @ 2013-07-22 7:02 UTC (permalink / raw)
To: Tejun Heo
Cc: Richard Zhu, shawn.guo, linux-arm-kernel, jgarzik, avorontsov,
rob.herring, s.hauer, linux-ide
Hi Tejun:
Re-send this email to you and community.
I'm afraid that you may be missed or don't receive it.
BTW, can you help to pick up the #2 and #3(reviewed and acked by Shawn) of the v7 patch-set of this serial?
Thanks in advance.
2013/7/10 HongXing Zhu <richard.zhuhongxing@gmail.com>
Hi Tejun:
There are some differences between generic AHCI controller and imx AHCI controller.
The bits definitions of the HBA registers, the Vendor
Specific registers, and the AHCI PHY clock, and the PHY signal adjustment window.
- CAP_SSS(bit20) of the HOST_CAP is writable, default
value is '0', should be configured to be '1'
- bit0 (only one AHCI SATA port on imx6q) of the
HOST_PORTS_IMPL should be set to be '1'.(default 0)
- One Vendor Specific register HOST_TIMER1MS(offset:0xe0)
should be configured regarding to the frequency of AHB
bus clock.
- Configurations of the AHCI PHY clock, and the signal
parameters of the GPR13
I'm afraid that the generic ahci_platform driver is not a good place to contain
imx6q ahci specific differences.
As I know that, the AHCI sata on imx53 doesn't have the PHY signal parameters
adjustment window, but the AHCI sata on imx6q does has one.
On imx6q, the standard .port_ops = &ahci_platform_ops, is used, but
.port_ops = &ahci_platform_retry_srst_ops, would be required by imx53 AHCI controller.
We can put all the imx AHCI(imx6q, imx53) specific difference into the sata_imx driver,
and keep ahci_platform as clean as possible, if the separate sata_imx driver can be created.
How do you think about that?
BTW, the version4 patch-set would be sent out a moment later, any suggests and comments are appreciated.
Best Regards.
2013/7/6 Tejun Heo <tj@kernel.org>
Hello,
On Sat, Jul 06, 2013 at 02:03:30AM +0000, Zhu Richard-R65037 wrote:
> [Richard] Just like what we dicussed in the previous v1/v2
> patch-set. Shawn has the concerns that the IP speicific codes
> shouldn't be put in the platform level. So he suggested that setup
> a stand-alone driver, contained the imx6q ahci sata specific codes,
> and re-use the generic ahci_platform driver as much as possible.
> This imx6q standalone ahci sata driver just registers the platform
> data, and the others would be handled by ahci_platform driver.
Oh, I'm not objecting to the ahci specific part not being in platform
code. I'm wondering whether specific handling for imx6q can be
included into ahci_platform rather than being in its own driver. It
just seems that there aren't too many differences. I could be
misreading it so if there are enough differences to warrant a separate
driver, please enlighten me.
Thanks!
--
tejun
Best Regards
Richard Zhu
-----Original Message-----
From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of Tejun Heo
Sent: Saturday, July 06, 2013 10:08 AM
To: Zhu Richard-R65037
Cc: Richard Zhu; shawn.guo@linaro.org; linux-arm-kernel@lists.infradead.org; jgarzik@pobox.com; avorontsov@ru.mvista.com; rob.herring@calxeda.com; s.hauer@pengutronix.de; linux-ide@vger.kernel.org
Subject: Re: [v3 3/3] sata: imx: add ahci sata support on imx platforms
Hello,
On Sat, Jul 06, 2013 at 02:03:30AM +0000, Zhu Richard-R65037 wrote:
> [Richard] Just like what we dicussed in the previous v1/v2 patch-set.
> Shawn has the concerns that the IP speicific codes shouldn't be put in
> the platform level. So he suggested that setup a stand-alone driver,
> contained the imx6q ahci sata specific codes, and re-use the generic
> ahci_platform driver as much as possible.
> This imx6q standalone ahci sata driver just registers the platform
> data, and the others would be handled by ahci_platform driver.
Oh, I'm not objecting to the ahci specific part not being in platform code. I'm wondering whether specific handling for imx6q can be included into ahci_platform rather than being in its own driver. It just seems that there aren't too many differences. I could be misreading it so if there are enough differences to warrant a separate driver, please enlighten me.
Thanks!
--
tejun
^ permalink raw reply [flat|nested] 24+ messages in thread
* [v3 3/3] sata: imx: add ahci sata support on imx platforms
@ 2013-07-22 7:02 ` Zhu Richard-R65037
0 siblings, 0 replies; 24+ messages in thread
From: Zhu Richard-R65037 @ 2013-07-22 7:02 UTC (permalink / raw)
To: linux-arm-kernel
Hi Tejun:
Re-send this email to you and community.
I'm afraid that you may be missed or don't receive it.
BTW, can you help to pick up the #2 and #3(reviewed and acked by Shawn) of the v7 patch-set of this serial?
Thanks in advance.
2013/7/10 HongXing Zhu <richard.zhuhongxing@gmail.com>
Hi Tejun:
There are some differences between generic AHCI controller and imx AHCI controller.
The bits definitions of the HBA registers, the Vendor
Specific registers, and the AHCI PHY clock, and the PHY signal adjustment window.
- CAP_SSS(bit20) of the HOST_CAP is writable, default
value is '0', should be configured to be '1'
- bit0 (only one AHCI SATA port on imx6q) of the
HOST_PORTS_IMPL should be set to be '1'.(default 0)
- One Vendor Specific register HOST_TIMER1MS(offset:0xe0)
should be configured regarding to the frequency of AHB
bus clock.
- Configurations of the AHCI PHY clock, and the signal
parameters of the GPR13
I'm afraid that the generic ahci_platform driver is not a good place to contain
imx6q ahci specific differences.
As I know that, the AHCI sata on imx53 doesn't have the PHY signal parameters
adjustment window, but the AHCI sata on imx6q does has one.
On imx6q, the standard .port_ops = &ahci_platform_ops, is used, but
.port_ops = &ahci_platform_retry_srst_ops, would be required by imx53 AHCI controller.
We can put all the imx AHCI(imx6q, imx53) specific difference into the sata_imx driver,
and keep ahci_platform as clean as possible, if the separate sata_imx driver can be created.
How do you think about that?
BTW, the version4 patch-set would be sent out a moment later, any suggests and comments are appreciated.
Best Regards.
2013/7/6 Tejun Heo <tj@kernel.org>
Hello,
On Sat, Jul 06, 2013 at 02:03:30AM +0000, Zhu Richard-R65037 wrote:
> [Richard] Just like what we dicussed in the previous v1/v2
> patch-set. Shawn has the concerns that the IP speicific codes
> shouldn't be put in the platform level. So he suggested that setup
> a stand-alone driver, contained the imx6q ahci sata specific codes,
> and re-use the generic ahci_platform driver as much as possible.
> This imx6q standalone ahci sata driver just registers the platform
> data, and the others would be handled by ahci_platform driver.
Oh, I'm not objecting to the ahci specific part not being in platform
code. I'm wondering whether specific handling for imx6q can be
included into ahci_platform rather than being in its own driver. It
just seems that there aren't too many differences. I could be
misreading it so if there are enough differences to warrant a separate
driver, please enlighten me.
Thanks!
--
tejun
Best Regards
Richard Zhu
-----Original Message-----
From: Tejun Heo [mailto:htejun at gmail.com] On Behalf Of Tejun Heo
Sent: Saturday, July 06, 2013 10:08 AM
To: Zhu Richard-R65037
Cc: Richard Zhu; shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org; jgarzik at pobox.com; avorontsov at ru.mvista.com; rob.herring at calxeda.com; s.hauer at pengutronix.de; linux-ide at vger.kernel.org
Subject: Re: [v3 3/3] sata: imx: add ahci sata support on imx platforms
Hello,
On Sat, Jul 06, 2013 at 02:03:30AM +0000, Zhu Richard-R65037 wrote:
> [Richard] Just like what we dicussed in the previous v1/v2 patch-set.
> Shawn has the concerns that the IP speicific codes shouldn't be put in
> the platform level. So he suggested that setup a stand-alone driver,
> contained the imx6q ahci sata specific codes, and re-use the generic
> ahci_platform driver as much as possible.
> This imx6q standalone ahci sata driver just registers the platform
> data, and the others would be handled by ahci_platform driver.
Oh, I'm not objecting to the ahci specific part not being in platform code. I'm wondering whether specific handling for imx6q can be included into ahci_platform rather than being in its own driver. It just seems that there aren't too many differences. I could be misreading it so if there are enough differences to warrant a separate driver, please enlighten me.
Thanks!
--
tejun
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [v3 3/3] sata: imx: add ahci sata support on imx platforms
2013-07-22 7:02 ` Zhu Richard-R65037
@ 2013-07-23 16:26 ` Tejun Heo
-1 siblings, 0 replies; 24+ messages in thread
From: Tejun Heo @ 2013-07-23 16:26 UTC (permalink / raw)
To: Zhu Richard-R65037
Cc: Richard Zhu, shawn.guo, linux-arm-kernel, jgarzik, avorontsov,
rob.herring, s.hauer, linux-ide
Hello,
On Mon, Jul 22, 2013 at 07:02:56AM +0000, Zhu Richard-R65037 wrote:
> Re-send this email to you and community.
> I'm afraid that you may be missed or don't receive it.
Sorry about the delay. I was on the road.
> BTW, can you help to pick up the #2 and #3(reviewed and acked by Shawn) of the v7 patch-set of this serial?
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Just one thing, can you please rename the driver to ahci_imx? I'd
like to keep all ahci drivers with ahci prefix (yeah, there's already
an exception, I know).
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 24+ messages in thread
* [v3 3/3] sata: imx: add ahci sata support on imx platforms
@ 2013-07-23 16:26 ` Tejun Heo
0 siblings, 0 replies; 24+ messages in thread
From: Tejun Heo @ 2013-07-23 16:26 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On Mon, Jul 22, 2013 at 07:02:56AM +0000, Zhu Richard-R65037 wrote:
> Re-send this email to you and community.
> I'm afraid that you may be missed or don't receive it.
Sorry about the delay. I was on the road.
> BTW, can you help to pick up the #2 and #3(reviewed and acked by Shawn) of the v7 patch-set of this serial?
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Just one thing, can you please rename the driver to ahci_imx? I'd
like to keep all ahci drivers with ahci prefix (yeah, there's already
an exception, I know).
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [v3 3/3] sata: imx: add ahci sata support on imx platforms
2013-07-23 16:26 ` Tejun Heo
@ 2013-07-24 2:21 ` Zhu Richard-R65037
-1 siblings, 0 replies; 24+ messages in thread
From: Zhu Richard-R65037 @ 2013-07-24 2:21 UTC (permalink / raw)
To: Tejun Heo
Cc: Richard Zhu, shawn.guo, linux-arm-kernel, jgarzik, avorontsov,
rob.herring, s.hauer, linux-ide
Hi Tejun:
Thanks a lot.
No problem, I can rename the driver from sata_imx to ahci_imx.
Best Regards
Richard Zhu
-----Original Message-----
From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of Tejun Heo
Sent: Wednesday, July 24, 2013 12:26 AM
To: Zhu Richard-R65037
Cc: Richard Zhu; shawn.guo@linaro.org; linux-arm-kernel@lists.infradead.org; jgarzik@pobox.com; avorontsov@ru.mvista.com; rob.herring@calxeda.com; s.hauer@pengutronix.de; linux-ide@vger.kernel.org
Subject: Re: [v3 3/3] sata: imx: add ahci sata support on imx platforms
Hello,
On Mon, Jul 22, 2013 at 07:02:56AM +0000, Zhu Richard-R65037 wrote:
> Re-send this email to you and community.
> I'm afraid that you may be missed or don't receive it.
Sorry about the delay. I was on the road.
> BTW, can you help to pick up the #2 and #3(reviewed and acked by Shawn) of the v7 patch-set of this serial?
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Just one thing, can you please rename the driver to ahci_imx? I'd like to keep all ahci drivers with ahci prefix (yeah, there's already an exception, I know).
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 24+ messages in thread
* [v3 3/3] sata: imx: add ahci sata support on imx platforms
@ 2013-07-24 2:21 ` Zhu Richard-R65037
0 siblings, 0 replies; 24+ messages in thread
From: Zhu Richard-R65037 @ 2013-07-24 2:21 UTC (permalink / raw)
To: linux-arm-kernel
Hi Tejun:
Thanks a lot.
No problem, I can rename the driver from sata_imx to ahci_imx.
Best Regards
Richard Zhu
-----Original Message-----
From: Tejun Heo [mailto:htejun at gmail.com] On Behalf Of Tejun Heo
Sent: Wednesday, July 24, 2013 12:26 AM
To: Zhu Richard-R65037
Cc: Richard Zhu; shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org; jgarzik at pobox.com; avorontsov at ru.mvista.com; rob.herring at calxeda.com; s.hauer at pengutronix.de; linux-ide at vger.kernel.org
Subject: Re: [v3 3/3] sata: imx: add ahci sata support on imx platforms
Hello,
On Mon, Jul 22, 2013 at 07:02:56AM +0000, Zhu Richard-R65037 wrote:
> Re-send this email to you and community.
> I'm afraid that you may be missed or don't receive it.
Sorry about the delay. I was on the road.
> BTW, can you help to pick up the #2 and #3(reviewed and acked by Shawn) of the v7 patch-set of this serial?
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Just one thing, can you please rename the driver to ahci_imx? I'd like to keep all ahci drivers with ahci prefix (yeah, there's already an exception, I know).
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2013-07-24 2:21 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-05 9:52 [PATCH v3 0/3] ahci: enable ahci sata support on imx6q Richard Zhu
2013-07-05 9:52 ` Richard Zhu
2013-07-05 9:52 ` [v3 1/3] ARM: dtsi: enable ahci sata on imx6q platforms Richard Zhu
2013-07-05 9:52 ` [v3 2/3] ARM: imx6q: update the sata bits definitions of gpr13 Richard Zhu
2013-07-05 9:52 ` [v3 3/3] sata: imx: add ahci sata support on imx platforms Richard Zhu
2013-07-05 15:59 ` Sascha Hauer
2013-07-05 15:59 ` Sascha Hauer
2013-07-05 16:06 ` Re[2]: " Alexander Shiyan
2013-07-06 1:55 ` Zhu Richard-R65037
2013-07-06 1:55 ` Zhu Richard-R65037
2013-07-06 1:54 ` Zhu Richard-R65037
2013-07-06 1:54 ` Zhu Richard-R65037
2013-07-05 17:43 ` Tejun Heo
2013-07-05 17:43 ` Tejun Heo
2013-07-06 2:03 ` Zhu Richard-R65037
2013-07-06 2:03 ` Zhu Richard-R65037
2013-07-06 2:07 ` Tejun Heo
2013-07-06 2:07 ` Tejun Heo
2013-07-22 7:02 ` Zhu Richard-R65037
2013-07-22 7:02 ` Zhu Richard-R65037
2013-07-23 16:26 ` Tejun Heo
2013-07-23 16:26 ` Tejun Heo
2013-07-24 2:21 ` Zhu Richard-R65037
2013-07-24 2:21 ` Zhu Richard-R65037
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.