ARM: sunxi: Add ahci-sunxi driver for Allwinner SoCs sata
Subsystems (3)
v2 cover ARM: sunxi: Add ahci-sunxi driver for Allwinner SoCs sata
Hi Tejun et al,
Here is the second version of the ahci-sunxi driver. Since Oliver is a bit low on time atm, I've taken over the efforts to upstream this for now.
v1 was using the approach of having a platform device which probe method creates a new child platform device which gets driven by ahci_platform.c, as done by ahci_imx.c . This was rightfully frowned upon, so we've gone looking for a better / cleaner solution.
The option of turning ahci_platform.c into a generic re-usable library to be shared between different ahci platform drivers was considered and rejected.
Almost all functionality is already shared through libahci / ata-core, ahci_platform.c really is just a few lines of glue-code generalized to be shared for the simply ahci platform device case.
Modifying ahci_platform.c to be generic enough to cleanly handle more complex platform ahci devices, results in much more complex code, which likely will still need to be modified each time a new exotic platform device shows up. There simply is not enough common code to share there, and making it generic enough would grow the code to be larger then simply having stand-alone platform drivers using libahci directly.
IOW I believe that the "last mile" of glue code simply cannot be efficiently generalized.
So I've decided avoiding the ahci_imx.c ugliness is best done by refactoring ahci_sunxi.c into a stand-alone platform driver, like ie sata_highbank.c.
To put this in numbers, ahci_platform.c is 353 lines, ahci_sunxi.c using the ahci_imx.c approach (which was using ahci_platform.c as a lib) was 305 lines + 12 new lines in ahci_platform.c. ahci_sunxi.c as a standalone driver is 351 lines. And the old ahci_sunxi.c was missing proper suspend / resume support.
Please review and if there are no remark add patch 1 + 2 to your tree for Linus. The dts bits will be merged through Maxime Ripard's tree.
Thanks & Regards,
Hans
PATCH v2 1/4 libahci: Add a pre ahci_start_engine hook
From: Oliver Schinagl <[email protected]>
Allwinner A10 and A20 ARM SoCs have an AHCI sata controller which need a special register to be poked before starting the DMA engine.
This register gets reset on an ahci_stop_engine call, so there is no other place then ahci_start_engine where this poking can be done.
This commit adds a pre ahci_start_engine hook for use by the Allwinner AHCI driver (and potentially other drivers in the future).
--- drivers/ata/ahci.h | 2 ++ drivers/ata/libahci.c | 4 ++++ 2 files changed, 6 insertions(+)
drivers/ata/ahci.h +2 -0
@@ -323,6 +323,8 @@ struct ahci_host_priv {u32 em_msg_type;/* EM message type */ struct clk *clk;/* Only for platforms supporting clk */ void *plat_data;/* Other platform data */ +/* Optional pre ahci_start_engine hook */ +void (*pre_start_engine)(struct ata_port *ap); };extern int ahci_ignore_sss;
drivers/ata/libahci.c +4 -0
@@ -568,8 +568,12 @@ static int ahci_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val)void ahci_start_engine (struct ata_port *ap) {void __iomem *port_mmio =ahci_port_base (ap); +struct ahci_host_priv *hpriv = ap->host->private_data;u32 tmp; +if (hpriv->pre_start_engine) + hpriv->pre_start_engine (ap); +/* start DMA */ tmp =readl (port_mmio +PORT_CMD ); tmp |=PORT_CMD_START ;
Signature · 1 line
1.8.4.2
PATCH v2 1/4 libahci: Add a pre ahci_start_engine hook
Hello,
drivers/ata/ahci.h struct ahci_host_priv { · line 327
Quoted 6 lines · Hans de Goede
drivers/ata/ahci.h +2
-0@@ -323,6 +323,8 @@ struct ahci_host_priv {u32 em_msg_type;/* EM message type */ struct clk *clk;/* Only for platforms supporting clk */ void *plat_data;/* Other platform data */ +/* Optional pre ahci_start_engine hook */ +void (*pre_start_engine)(struct ata_port *ap);
I'd much prefer if the callback overrides start_engine itself rather than adding a hook inside start_engine. This is a bit too specific.
Thanks.
Signature · 1 line
tejun
PATCH v2 2/4 ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata
In reply to Hans de GoedeFrom: Oliver Schinagl <[email protected]>
This patch adds support for the ahci sata controler found on Allwinner A10 and A20 SoCs.
Orignally written by Olliver Schinagl using the approach of having a platform device which probe method creates a new child platform device which gets driven by ahci_platform.c, as done by ahci_imx.c .
Given that almost all functionality already is shared through libahci / ata-core, and that ahci_platform.c cannot cleanly handle somewhat more complex platform specific ahci cases, such as the sunxi case, it was refactored into a stand-alone platform driver by Hans de Goede.
--- .../devicetree/bindings/ata/ahci-sunxi.txt | 24 ++ drivers/ata/Kconfig | 9 + drivers/ata/Makefile | 1 + drivers/ata/ahci_sunxi.c | 349 +++++++++++++++++++++ 4 files changed, 383 insertions(+) create mode 100644 Documentation/devicetree/bindings/ata/ahci-sunxi.txt create mode 100644 drivers/ata/ahci_sunxi.c
Documentation/devicetree/bindings/ata/ahci-sunxi.txt new file +24 -0
@@ -0,0 +1,24 @@ +Allwinner SUNXI AHCI SATA Controller + +SATA nodes are defined to describe on-chip Serial ATA controllers. +Each SATA controller should have its own node. + +Required properties: +- compatible : compatible list, contains "allwinner,sun4i-a10-ahci" +- reg : <registers mapping> +- interrupts : <interrupt mapping for AHCI IRQ> +- clocks : clocks for ACHI +- clock-names : clock names for AHCI + +Optional properties: +- pwr-supply : regulator to control the power supply GPIO + +Example: + ahci at 01c18000 { + compatible = "allwinner,sun4i-a10-ahci"; + reg = <0x01c18000 0x1000>; + interrupts = <0 56 1>; + clocks = <&ahb_gates 25>, <&pll6 0>; + clock-names = "ahb_sata", "pll6_sata"; + pwr-supply = <®_ahci_5v>; + };
drivers/ata/Kconfig +9 -0
@@ -106,6 +106,15 @@ config AHCI_IMX If unsure, say N. +config AHCI_SUNXI + tristate "Allwinner sunxi AHCI SATA support" + depends on ARCH_SUNXI + help + This option enables support for the Allwinner sunxi SoC's + onboard AHCI SATA. + + If unsure, say N. + config SATA_FSL tristate "Freescale 3.0Gbps SATA support" depends on FSL_SOC
drivers/ata/Makefile +1 -0
@@ -11,6 +11,7 @@ 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_AHCI_IMX) += ahci_imx.o +obj-$(CONFIG_AHCI_SUNXI) += ahci_sunxi.o libahci.o # SFF w/ custom DMA obj-$(CONFIG_PDC_ADMA) += pdc_adma.o
drivers/ata/ahci_sunxi.c new file +349 -0
349 lines not on this page · raw message
Signature · 1 line
1.8.4.2
PATCH v2 2/4 ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata
Quoted 7 lines · On Saturday 04 January 2014 10:14:36 Hans de Goede wrote:
Documentation/devicetree/bindings/ata/ahci-sunxi.txt new file +1
-0@@ -0,0 +1,24 @@ +Allwinner SUNXI AHCI SATA Controller
Documentation/devicetree/bindings/ata/ahci-sunxi.txt line 11
Quoted 10 lines
Documentation/devicetree/bindings/ata/ahci-sunxi.txt new file +10
-0+ +SATA nodes are defined to describe on-chip Serial ATA controllers. +Each SATA controller should have its own node. + +Required properties: +- compatible : compatible list, contains "allwinner,sun4i-a10-ahci" +- reg : <registers mapping> +- interrupts : <interrupt mapping for AHCI IRQ> +- clocks : clocks for ACHI +- clock-names : clock names for AHCI
The binding needs to specify the required names for the clocks.
Documentation/devicetree/bindings/ata/ahci-sunxi.txt line 22
Quoted 10 lines
Documentation/devicetree/bindings/ata/ahci-sunxi.txt new file +10
-0+Optional properties: +- pwr-supply : regulator to control the power supply GPIO + +Example: + ahci at 01c18000 { + compatible = "allwinner,sun4i-a10-ahci"; + reg = <0x01c18000 0x1000>; + interrupts = <0 56 1>; + clocks = <&ahb_gates 25>, <&pll6 0>; + clock-names = "ahb_sata", "pll6_sata";
"pll6_sata" doesn't sound particularly generic. The name should reflect what the clock is used for, not what drives it.
Also, please send the binding as a separate patch with Cc to the devicetree-discuss mailing list.
Documentation/devicetree/bindings/ata/ahci-sunxi.txt
Quoted 9 lines
Documentation/devicetree/bindings/ata/ahci-sunxi.txt new file +9
-0+ +static int sunxi_ahci_phy_init(struct device *dev, void __iomem *reg_base) +{ + u32 reg_val; + int timeout; + + /* This magic is from the original code */ + writel(0, reg_base + AHCI_RWCR); + mdelay(5);
This function should probably be in a separate phy driver. I would very much hope that we can minimize the required code in an AHCI driver and move code from this new file into the ahci-platform driver. The clock, regulator and phy setup can all be optional properties of the generic driver, and then there shouldn't be much left that is sunxi specific.
Quoted 12 lines
Documentation/devicetree/bindings/ata/ahci-sunxi.txt new file +12
-0+static int sunxi_ahci_susp(struct device *dev) +{ + struct ata_host *host = dev_get_drvdata(dev); + struct ahci_host_priv *hpriv = host->private_data; + struct sunxi_ahci *ahci = hpriv->plat_data; + int ret; + + /* + * AHCI spec rev1.1 section 8.3.3: + * Software must disable interrupts prior to requesting a + * transition of the HBA to D3 state. + */
Documentation/devicetree/bindings/ata/ahci-sunxi.txt
Quoted 10 lines
Documentation/devicetree/bindings/ata/ahci-sunxi.txt new file +10
-0+ sunxi_clrbits(hpriv->mmio + HOST_CTL, HOST_IRQ_EN); + + ret = ata_host_suspend(host, PMSG_SUSPEND); + if (ret) + return ret; + + sunxi_ahci_disable_clks(ahci); + + return 0; +}
The only thing in here that seems sunxi-specific is the irq disabling part. Can't you do this instead by calling disable_irq() and make the function completely generic?
Arnd
PATCH v2 2/4 ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata
Quoted 8 lines · On Saturday 04 January 2014 22:39:50 Arnd Bergmann wrote:
Quoted 6 lines
+Required properties:
+- compatible : compatible list, contains "allwinner,sun4i-a10-ahci" +- reg : <registers mapping> +- interrupts : <interrupt mapping for AHCI IRQ> +- clocks : clocks for ACHI +- clock-names : clock names for AHCIThe binding needs to specify the required names for the clocks.
fwiw, the imx driver uses "ahb" and "sata_ref" as the clock names. I would strongly suggest using the same names here, and documenting them in the ahci binding as optional.
Arnd
PATCH v2 2/4 ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata
Quoted 11 lines · Arnd Bergmann
Quoted 7 lines · On Saturday 04 January 2014 22:39:50 Arnd Bergmann wrote:
Quoted 6 lines
+Required properties:
+- compatible : compatible list, contains "allwinner,sun4i-a10-ahci" +- reg : <registers mapping> +- interrupts : <interrupt mapping for AHCI IRQ> +- clocks : clocks for ACHI +- clock-names : clock names for AHCIThe binding needs to specify the required names for the clocks.
fwiw, the imx driver uses "ahb" and "
sata_ref" as the clock names. I would strongly suggest using the same names here, and documenting them in the ahci binding as optional.
This is my fault, and you just reminded me I should have fixed that from the previous comments. It just slipped my mind and I'm sorry for that!
Hans, I'll go over the original commit thread and pick up all changes and send them as a patch to you Monday.
Oliver
Quoted 2 lines
Arnd
PATCH v2 2/4 ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata
Hi,
Quoted 8 lines · Olliver Schinagl
Quoted 7 lines · Arnd Bergmann
Quoted 6 lines · On Saturday 04 January 2014 22:39:50 Arnd Bergmann wrote:
Quoted 6 lines
+Required properties:
+- compatible : compatible list, contains "allwinner,sun4i-a10-ahci" +- reg : <registers mapping> +- interrupts : <interrupt mapping for AHCI IRQ> +- clocks : clocks for ACHI +- clock-names : clock names for AHCI
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
The binding needs to specify the required names for the clocks.
fwiw, the imx driver uses "ahb" and "
sata_ref" as the clock names. I would strongly suggest using the same names here, and documenting them in the ahci binding as optional.This is my fault, and you just reminded me I should have fixed that from the previous comments. It just slipped my mind and I'm sorry for that!
Hans, I'll go over the original commit thread and pick up all changes and send them as a patch to you Monday.
Thanks, sounds good. Then I won't touch the code till I get your patch (way too busy with usb now anyways).
Regards,
Hans
PATCH v2 2/4 ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata
In reply to Olliver SchinaglHi,
Quoted 8 lines · Olliver Schinagl
Quoted 7 lines · Arnd Bergmann
Quoted 6 lines · On Saturday 04 January 2014 22:39:50 Arnd Bergmann wrote:
Quoted 6 lines
+Required properties:
+- compatible : compatible list, contains "allwinner,sun4i-a10-ahci" +- reg : <registers mapping> +- interrupts : <interrupt mapping for AHCI IRQ> +- clocks : clocks for ACHI +- clock-names : clock names for AHCI
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
The binding needs to specify the required names for the clocks.
fwiw, the imx driver uses "ahb" and "
sata_ref" as the clock names. I would strongly suggest using the same names here, and documenting them in the ahci binding as optional.This is my fault, and you just reminded me I should have fixed that from the previous comments. It just slipped my mind and I'm sorry for that!
Hans, I'll go over the original commit thread and pick up all changes and send them as a patch to you Monday.
As I just mentioned to Arnd I've a better idea to be able to re-use most of the ahci_platform.c code without copy-pasting it. So I'm going to do a v3 soonish. Can you please just make a mail with summary of the requested changes and send that to me, or do this after I've send v3 ?
Thanks,
Hans
PATCH v2 2/4 ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata
Quoted 15 lines · Hans de Goede
Hi,
Quoted 12 lines · Olliver Schinagl
Quoted 11 lines · Arnd Bergmann
Quoted 7 lines · On Saturday 04 January 2014 22:39:50 Arnd Bergmann wrote:
Quoted 6 lines
+Required properties:
+- compatible : compatible list, contains "allwinner,sun4i-a10-ahci" +- reg : <registers mapping> +- interrupts : <interrupt mapping for AHCI IRQ> +- clocks : clocks for ACHI +- clock-names : clock names for AHCIThe binding needs to specify the required names for the clocks.
fwiw, the imx driver uses "ahb" and "
sata_ref" as the clock names. I would strongly suggest using the same names here, and documenting them in the ahci binding as optional.
Quoted 7 lines
Quoted 3 lines
This is my fault, and you just reminded me I should have fixed that from the previous comments. It just slipped my mind and I'm sorry for that!
Hans, I'll go over the original commit thread and pick up all changes and send them as a patch to you Monday.
As I just mentioned to Arnd I've a better idea to be able to re-use most of the
ahci_platform.ccode without copy-pasting it. So I'm going to do a v3 soonish. Can you please just make a mail with summary of the requested changes and send that to me, or do this after I've send v3 ?
if memory serves, it was mostly about the dt documentation and binding names, i'll work on it first thing tomorrow morning ;)
Quoted 4 lines
Thanks,
Hans
PATCH v2 2/4 ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata
In reply to Arnd BergmannHi,
Quoted 12 lines · Arnd Bergmann
Quoted 11 lines · On Saturday 04 January 2014 10:14:36 Hans de Goede wrote:
Documentation/devicetree/bindings/ata/ahci-sunxi.txt new file +5
-0@@ -0,0 +1,24 @@ +Allwinner SUNXI AHCI SATA Controller + +SATA nodes are defined to describe on-chip Serial ATA controllers. +Each SATA controller should have its own node. +
Quoted 8 lines
Documentation/devicetree/bindings/ata/ahci-sunxi.txt line 11
Quoted 6 lines
Documentation/devicetree/bindings/ata/ahci-sunxi.txt new file +6
-0+Required properties: +- compatible : compatible list, contains "allwinner,sun4i-a10-ahci" +- reg : <registers mapping> +- interrupts : <interrupt mapping for AHCI IRQ> +- clocks : clocks for ACHI +- clock-names : clock names for AHCIThe binding needs to specify the required names for the clocks.
Will fix.
Quoted 14 lines
Documentation/devicetree/bindings/ata/ahci-sunxi.txt line 22
Quoted 11 lines
Documentation/devicetree/bindings/ata/ahci-sunxi.txt new file +10
-0+Optional properties: +- pwr-supply : regulator to control the power supply GPIO + +Example: + ahci at 01c18000 { + compatible = "allwinner,sun4i-a10-ahci"; + reg = <0x01c18000 0x1000>; + interrupts = <0 56 1>; + clocks = <&ahb_gates 25>, <&pll6 0>; + clock-names = "ahb_sata", "pll6_sata";"
pll6_sata" doesn't sound particularly generic. The name should reflect what the clock is used for, not what drives it.
Will change to ahb + sata_ref.
Quoted 3 lines
Also, please send the binding as a separate patch with Cc to the devicetree-discuss mailing list.
Hmm, this contradicts what others are saying who have requested for the binding docs to be part of the same commit as the driver (with various other drivers). Note that I've cc-ed devicetree already.
Quoted 17 lines
Documentation/devicetree/bindings/ata/ahci-sunxi.txt
Quoted 10 lines
Documentation/devicetree/bindings/ata/ahci-sunxi.txt new file +9
-0+ +static int sunxi_ahci_phy_init(struct device *dev, void __iomem *reg_base) +{ + u32 reg_val; + int timeout; + + /* This magic is from the original code */ + writel(0, reg_base + AHCI_RWCR); + mdelay(5);This function should probably be in a separate phy driver. I would very much hope that we can minimize the required code in an AHCI driver and move code from this new file into the ahci-platform driver. The clock, regulator and phy setup can all be optional properties of the generic driver, and then there shouldn't be much left that is sunxi specific.
Writing a phy driver, and extending ahci-platform to use that was my original plan. But the phy really is part of the ahci ip-block here, and not a separate ip-block. Its registers are smack in the middle of the io-range for the ahci function.
Also note that sunxi_ahci_pre_start_engine is rather sunxi specific. Needing to put that in a generic ahci_platform driver and only activating it for sunxi socs would only serve to prove my point that at some point it is simply easier and better to write a non generic platform glue driver when dealing with exotic ahci ip blocks.
If we end up putting all sort of if soca do foo else if socb do bar, else do nothing magic in ahci_platform.c I think we're over generalizing. If something nicely fits as a generic platform dev, by all means we should use a generic platform driver, but that just won't work cleanly here.
Actually I've just completed writing a phy driver for the usbphy in the same soc, and a patch to extend ehci-platform to take an optional clk and phy. So I completely agree with the basic idea, it just does not seem doable cleanly in this case, hence my choice to write a separate platform driver. For the usb stuff (wip) see:
https://github.com/jwrdegoede/linux-sunxi/commit/4655dd01936f42d8a75da08a00af439e0a34eaf7
https://github.com/jwrdegoede/linux-sunxi/commit/bcb674859d015ff9e082829dbd5cf239b8b53d4a
As said this is exactly what your asking me to do for sunxi-ahci, but the difference is that for sunxi-ehci it works nicely, and for sunxi-ahci it just doesn't work cleanly.
Quoted 19 lines
Quoted 19 lines
Documentation/devicetree/bindings/ata/ahci-sunxi.txt new file +18
-0+static int sunxi_ahci_susp(struct device *dev) +{ + struct ata_host *host = dev_get_drvdata(dev); + struct ahci_host_priv *hpriv = host->private_data; + struct sunxi_ahci *ahci = hpriv->plat_data; + int ret; + + /* + * AHCI spec rev1.1 section 8.3.3: + * Software must disable interrupts prior to requesting a + * transition of the HBA to D3 state. + */ + sunxi_clrbits(hpriv->mmio + HOST_CTL, HOST_IRQ_EN); + + ret = ata_host_suspend(host, PMSG_SUSPEND); + if (ret) + return ret; +
Quoted 8 lines
Documentation/devicetree/bindings/ata/ahci-sunxi.txt
Quoted 4 lines
Documentation/devicetree/bindings/ata/ahci-sunxi.txt new file +4
-0+ sunxi_ahci_disable_clks(ahci); + + return 0; +}The only thing in here that seems sunxi-specific is the irq disabling part. Can't you do this instead by calling
disable_irq()and make the function completely generic?
The irq disabling part actually is not sunxi specific, ahci_platform.c has it too.
Regards,
Hans
PATCH v2 2/4 ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata
Quoted 7 lines · Hans de Goede
Quoted 3 lines
Also, please send the binding as a separate patch with Cc to the devicetree-discuss mailing list.
Hmm, this contradicts what others are saying who have requested for the binding docs to be part of the same commit as the driver (with various other drivers). Note that I've cc-ed devicetree already.
Probably my fault then, I've been away for some time and may misremember what we discussed. Maybe someone else can clarify.
Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
drivers/ata/ahci_sunxi.c new file +7
-0+static int sunxi_ahci_phy_init (struct device *dev,void __iomem *reg_base) +{ +u32 reg_val; +int timeout; + +/* This magic is from the original code */ +writel (0 , reg_base +AHCI_RWCR );Quoted 8 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ mdelay(5);This function should probably be in a separate phy driver. I would very much hope that we can minimize the required code in an AHCI driver and move code from this new file into the ahci-platform driver. The clock, regulator and phy setup can all be optional properties of the generic driver, and then there shouldn't be much left that is sunxi specific.
Quoted 4 lines
Writing a phy driver, and extending ahci-platform to use that was my original plan. But the phy really is part of the ahci ip-block here, and not a separate ip-block. Its registers are smack in the middle of the io-range for the ahci function.
I see. I wonder if the register layout is common with some other implementation then. If it's part of the AHCI block, it's probably not an Allwinner invention but comes from whoever implemented the AHCI.
Quoted 7 lines
Also note that
sunxi_ahci_pre_start_engineis rather sunxi specific. Needing to put that in a genericahci_platformdriver and only activating it for sunxi socs would only serve to prove my point that at some point it is simply easier and better to write a non generic platform glue driver when dealing with exotic ahci ip blocks.
Quoted 5 lines
If we end up putting all sort of if soca do foo else if socb do bar, else do nothing magic in
ahci_platform.cI think we're over generalizing. If something nicely fits as a generic platform dev, by all means we should use a generic platform driver, but that just won't work cleanly here.
Yes, but there may be some middle ground. I still think it would be worthwhile to make the clock handling part of the common ahci (or ahci-platform) driver and reuse that, since it seems to be needed on a number of implementations. IIRC there is already some inheritence model in libata that can be used to define variations of drivers and have common parts done only once.
Quoted 8 lines
Actually I've just completed writing a phy driver for the usbphy in the same soc, and a patch to extend ehci-platform to take an optional clk and phy. So I completely agree with the basic idea, it just does not seem doable cleanly in this case, hence my choice to write a separate platform driver. For the usb stuff (wip) see:
https://github.com/jwrdegoede/linux-sunxi/commit/4655dd01936f42d8a75da08a00af439e0a34eaf7
https://github.com/jwrdegoede/linux-sunxi/commit/bcb674859d015ff9e082829dbd5cf239b8b53d4a
Ah, very nice!
Quoted 27 lines
Quoted 18 lines
Quoted 18 lines
drivers/ata/ahci_sunxi.c new file +18
-0+static int sunxi_ahci_susp (struct device *dev) +{ +struct ata_host *host =dev_get_drvdata (dev); +struct ahci_host_priv *hpriv = host->private_data; +struct sunxi_ahci *ahci = hpriv->plat_data; +int ret; + +/* +* AHCI spec rev1.1 section 8.3.3: +* Software must disable interrupts prior to requesting a +* transition of the HBA to D3 state. +*/ +sunxi_clrbits (hpriv->mmio +HOST_CTL ,HOST_IRQ_EN ); + + ret =ata_host_suspend (host,PMSG_SUSPEND ); +if (ret) +return ret; +Quoted 8 lines
drivers/ata/ahci_sunxi.c line 300
Quoted 4 lines
drivers/ata/ahci_sunxi.c new file +4
-0+sunxi_ahci_disable_clks (ahci); + +return 0 ; +}The only thing in here that seems sunxi-specific is the irq disabling part. Can't you do this instead by calling
disable_irq()and make the function completely generic?
Quoted 2 lines
The irq disabling part actually is not sunxi specific,
ahci_platform.chas it too.
Ok. Can this function be shared in some way then, e.g. by exporting the ahci-platform implementation?
Arnd
PATCH v2 2/4 ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata
Hi,
Quoted 1 line · Arnd Bergmann
On Sunday 05 January 2014, Hans de Goede wrote:
<snip>
Quoted 21 lines
Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
drivers/ata/ahci_sunxi.c new file +7
-0+static int sunxi_ahci_phy_init (struct device *dev,void __iomem *reg_base) +{ +u32 reg_val; +int timeout; + +/* This magic is from the original code */ +writel (0 , reg_base +AHCI_RWCR );Quoted 8 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ mdelay(5);This function should probably be in a separate phy driver. I would very much hope that we can minimize the required code in an AHCI driver and move code from this new file into the ahci-platform driver. The clock, regulator and phy setup can all be optional properties of the generic driver, and then there shouldn't be much left that is sunxi specific.
Quoted 4 lines
Writing a phy driver, and extending ahci-platform to use that was my original plan. But the phy really is part of the ahci ip-block here, and not a separate ip-block. Its registers are smack in the middle of the io-range for the ahci function.
Quoted 4 lines
I see. I wonder if the register layout is common with some other implementation then. If it's part of the AHCI block, it's probably not an Allwinner invention but comes from whoever implemented the AHCI.
Right, but so far we've been unable to find anything quite like it.
Quoted 13 lines
Quoted 7 lines
Also note that
sunxi_ahci_pre_start_engineis rather sunxi specific. Needing to put that in a genericahci_platformdriver and only activating it for sunxi socs would only serve to prove my point that at some point it is simply easier and better to write a non generic platform glue driver when dealing with exotic ahci ip blocks.Quoted 5 lines
If we end up putting all sort of if soca do foo else if socb do bar, else do nothing magic in
ahci_platform.cI think we're over generalizing. If something nicely fits as a generic platform dev, by all means we should use a generic platform driver, but that just won't work cleanly here.
Quoted 6 lines
Yes, but there may be some middle ground. I still think it would be worthwhile to make the clock handling part of the common ahci (or ahci-platform) driver and reuse that, since it seems to be needed on a number of implementations. IIRC there is already some inheritence model in libata that can be used to define variations of drivers and have common parts done only once.
Ok, thinking more about this I think I have a solution which should be acceptable. I'll do a v3 the coming days.
Regards,
Hans
PATCH v2 3/4 ARM: sun4i: dts: Add ahci / sata support
In reply to Hans de GoedeFrom: Oliver Schinagl <[email protected]>
This patch adds sunxi sata support to A10 boards that have such a connector. Some boards also feature a regulator via a GPIO and support for this is also added.
--- arch/arm/boot/dts/sun4i-a10-a1000.dts | 4 ++++ arch/arm/boot/dts/sun4i-a10-cubieboard.dts | 27 +++++++++++++++++++++++++++ arch/arm/boot/dts/sun4i-a10.dtsi | 9 +++++++++ 3 files changed, 40 insertions(+)
arch/arm/boot/dts/sun4i-a10-a1000.dts +4 -0
@@ -48,6 +48,10 @@ status = "okay"; }; + sata: ahci at 01c18000 { + status = "okay"; + }; + pinctrl at 01c20800 { mmc0_cd_pin_a1000: mmc0_cd_pin at 0 { allwinner,pins = "PH1";
arch/arm/boot/dts/sun4i-a10-cubieboard.dts +27 -0
@@ -51,7 +51,19 @@ status = "okay"; }; + sata: ahci at 01c18000 { + pwr-supply = <®_ahci_5v>; + status = "okay"; + }; + pinctrl at 01c20800 { + ahci_pwr_pin_cubieboard: ahci_pwr_pin at 0 { + allwinner,pins = "PB8"; + allwinner,function = "gpio_out"; + allwinner,drive = <0>; + allwinner,pull = <0>; + }; + mmc0_cd_pin_cubieboard: mmc0_cd_pin at 0 { allwinner,pins = "PH1"; allwinner,function = "gpio_in";
@@ -102,4 +114,19 @@ linux,default-trigger = "heartbeat"; }; }; + + regulators { + compatible = "simple-bus"; + pinctrl-names = "default"; + + reg_ahci_5v: ahci-5v { + compatible = "regulator-fixed"; + regulator-name = "ahci-5v"; + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <5000000>; + pinctrl-0 = <&ahci_pwr_pin_cubieboard>; + gpio = <&pio 1 8 0>; + enable-active-high; + }; + }; };
arch/arm/boot/dts/sun4i-a10.dtsi +9 -0
@@ -331,6 +331,15 @@ status = "disabled"; }; + sata: ahci at 01c18000 { + compatible = "allwinner,sun4i-a10-ahci"; + reg = <0x01c18000 0x1000>; + interrupts = <56>; + clocks = <&ahb_gates 25>, <&pll6 0>; + clock-names = "ahb_sata", "pll6_sata"; + status = "disabled"; + }; + intc: interrupt-controller at 01c20400 { compatible = "allwinner,sun4i-ic"; reg = <0x01c20400 0x400>;
Signature · 1 line
1.8.4.2
PATCH v2 4/4 ARM: sun7i: dts: Add ahci / sata support
In reply to Hans de GoedeThis patch adds sunxi sata support to A20 boards that have such a connector. Some boards also feature a regulator via a GPIO and support for this is also added.
--- arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 27 +++++++++++++++++++++++++ arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 27 +++++++++++++++++++++++++ arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 26 ++++++++++++++++++++++++ arch/arm/boot/dts/sun7i-a20.dtsi | 9 +++++++++ 4 files changed, 89 insertions(+)
arch/arm/boot/dts/sun7i-a20-cubieboard2.dts +27 -0
@@ -28,7 +28,19 @@ status = "okay"; }; + sata: ahci at 01c18000 { + pwr-supply = <®_ahci_5v>; + status = "okay"; + }; + pinctrl at 01c20800 { + ahci_pwr_pin_cubieboard2: ahci_pwr_pin at 0 { + allwinner,pins = "PB8"; + allwinner,function = "gpio_out"; + allwinner,drive = <0>; + allwinner,pull = <0>; + }; + mmc0_cd_pin_cubieboard2: mmc0_cd_pin at 0 { allwinner,pins = "PH1"; allwinner,function = "gpio_in";
@@ -86,4 +98,19 @@ gpios = <&pio 7 20 0>; }; }; + + regulators { + compatible = "simple-bus"; + pinctrl-names = "default"; + + reg_ahci_5v: ahci-5v { + compatible = "regulator-fixed"; + regulator-name = "ahci-5v"; + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <5000000>; + pinctrl-0 = <&ahci_pwr_pin_cubieboard2>; + gpio = <&pio 1 8 0>; + enable-active-high; + }; + }; };
arch/arm/boot/dts/sun7i-a20-cubietruck.dts +27 -0
@@ -28,6 +28,11 @@ status = "okay"; }; + sata: ahci at 01c18000 { + pwr-supply = <®_ahci_5v>; + status = "okay"; + }; + pinctrl at 01c20800 { mmc0_cd_pin_cubietruck: mmc0_cd_pin at 0 { allwinner,pins = "PH1";
@@ -36,6 +41,13 @@ allwinner,pull = <0>; }; + ahci_pwr_pin_cubietruck: ahci_pwr_pin at 0 { + allwinner,pins = "PH12"; + allwinner,function = "gpio_out"; + allwinner,drive = <0>; + allwinner,pull = <0>; + }; + led_pins_cubietruck: led_pins at 0 { allwinner,pins = "PH7", "PH11", "PH20", "PH21"; allwinner,function = "gpio_out";
@@ -84,4 +96,19 @@ gpios = <&pio 7 7 0>; }; }; + + regulators { + compatible = "simple-bus"; + pinctrl-names = "default"; + + reg_ahci_5v: ahci-5v { + compatible = "regulator-fixed"; + regulator-name = "ahci-5v"; + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <5000000>; + pinctrl-0 = <&ahci_pwr_pin_cubietruck>; + gpio = <&pio 7 12 0>; + enable-active-high; + }; + }; };
arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts +26 -0
@@ -37,7 +37,19 @@ status = "okay"; }; + sata: ahci at 01c18000 { + pwr-supply = <®_ahci_5v>; + status = "okay"; + }; + pinctrl at 01c20800 { + ahci_pwr_pin_olinuxinom: ahci_pwr_pin at 0 { + allwinner,pins = "PB8"; + allwinner,function = "gpio_out"; + allwinner,drive = <0>; + allwinner,pull = <0>; + }; + mmc0_cd_pin_olinuxinom: mmc0_cd_pin at 0 { allwinner,pins = "PH1"; allwinner,function = "gpio_in";
@@ -116,4 +128,18 @@ default-state = "on"; }; }; + + regulators { + compatible = "simple-bus"; + + reg_ahci_5v: ahci-5v { + compatible = "regulator-fixed"; + regulator-name = "ahci-5v"; + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <5000000>; + pinctrl-0 = <&ahci_pwr_pin_olinuxinom>; + gpio = <&pio 1 8 0>; + enable-active-high; + }; + }; };
arch/arm/boot/dts/sun7i-a20.dtsi +9 -0
@@ -448,6 +448,15 @@ }; }; + sata: ahci at 01c18000 { + compatible = "allwinner,sun4i-a10-ahci"; + reg = <0x01c18000 0x1000>; + interrupts = <0 56 1>; + clocks = <&ahb_gates 25>, <&pll6 0>; + clock-names = "ahb_sata", "pll6_sata"; + status = "disabled"; + }; + timer at 01c20c00 { compatible = "allwinner,sun4i-timer"; reg = <0x01c20c00 0x90>;
Signature · 1 line
1.8.4.2