diff mbox

[2/3] ARM: sunxi: Add an ahci-platform compatible AHCI driver for the Allwinner SUNXi series of SoCs

Message ID 1386159055-10264-3-git-send-email-oliver@schinagl.nl
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Olliver Schinagl Dec. 4, 2013, 12:10 p.m. UTC
From: Oliver Schinagl <oliver@schinagl.nl>

This patch adds support for the sunxi series of SoC's by allwinner. It
plugs into the ahci-platform framework.

Note: Currently it uses a somewhat hackish approach that probably needs
a lot more work, but does the same as the IMX SoC's.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 .../devicetree/bindings/ata/ahci-sunxi.txt         |  24 ++
 drivers/ata/Kconfig                                |   9 +
 drivers/ata/Makefile                               |   1 +
 drivers/ata/ahci_platform.c                        |  12 +
 drivers/ata/ahci_sunxi.c                           | 305 +++++++++++++++++++++
 5 files changed, 351 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ata/ahci-sunxi.txt
 create mode 100644 drivers/ata/ahci_sunxi.c

Comments

Mark Rutland Dec. 4, 2013, 12:26 p.m. UTC | #1
On Wed, Dec 04, 2013 at 12:10:54PM +0000, oliver@schinagl.nl wrote:
> From: Oliver Schinagl <oliver@schinagl.nl>
> 
> This patch adds support for the sunxi series of SoC's by allwinner. It
> plugs into the ahci-platform framework.
> 
> Note: Currently it uses a somewhat hackish approach that probably needs
> a lot more work, but does the same as the IMX SoC's.
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  .../devicetree/bindings/ata/ahci-sunxi.txt         |  24 ++
>  drivers/ata/Kconfig                                |   9 +
>  drivers/ata/Makefile                               |   1 +
>  drivers/ata/ahci_platform.c                        |  12 +
>  drivers/ata/ahci_sunxi.c                           | 305 +++++++++++++++++++++
>  5 files changed, 351 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ata/ahci-sunxi.txt
>  create mode 100644 drivers/ata/ahci_sunxi.c
> 
> diff --git a/Documentation/devicetree/bindings/ata/ahci-sunxi.txt b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
> new file mode 100644
> index 0000000..0792fa5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
> @@ -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"

- compatible: Should contain "allwinner,sun4i-a10-ahci"

> +- reg          : <registers mapping>

- reg: The offset and length of the MMIO registers.

> +- interrupts   : <interrupt mapping for AHCI IRQ>

- interrupts: An interrupt-specifier for the ACHI interrupt

> +- clocks       : clocks for ACHI
> +- clock-names  : clock names for AHCI

Please _define_ the set of clock-names you expect. This binding is
meaningless without it. If you require clock-names, define the clocks
property in terms of it.

Thanks,
Mark.
--
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
Tejun Heo Dec. 4, 2013, 12:37 p.m. UTC | #2
On Wed, Dec 04, 2013 at 01:10:54PM +0100, oliver@schinagl.nl wrote:
> From: Oliver Schinagl <oliver@schinagl.nl>
> 
> This patch adds support for the sunxi series of SoC's by allwinner. It
> plugs into the ahci-platform framework.
> 
> Note: Currently it uses a somewhat hackish approach that probably needs
> a lot more work, but does the same as the IMX SoC's.
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  .../devicetree/bindings/ata/ahci-sunxi.txt         |  24 ++
>  drivers/ata/Kconfig                                |   9 +
>  drivers/ata/Makefile                               |   1 +
>  drivers/ata/ahci_platform.c                        |  12 +
>  drivers/ata/ahci_sunxi.c                           | 305 +++++++++++++++++++++

I'm not really liking the way things are going.  Do we really need
separate drivers for each platform ahci implementation.  Are they
really that different?  Would it be impossible to make ahci_platform
generic enough so that we don't eventually end up with a gazillion
ahci_XXX drivers?
Olliver Schinagl Dec. 4, 2013, 12:49 p.m. UTC | #3
On 04-12-13 13:26, Mark Rutland wrote:
> On Wed, Dec 04, 2013 at 12:10:54PM +0000, oliver@schinagl.nl wrote:
>> From: Oliver Schinagl <oliver@schinagl.nl>
>>
>> This patch adds support for the sunxi series of SoC's by allwinner. It
>> plugs into the ahci-platform framework.
>>
>> Note: Currently it uses a somewhat hackish approach that probably needs
>> a lot more work, but does the same as the IMX SoC's.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   .../devicetree/bindings/ata/ahci-sunxi.txt         |  24 ++
>>   drivers/ata/Kconfig                                |   9 +
>>   drivers/ata/Makefile                               |   1 +
>>   drivers/ata/ahci_platform.c                        |  12 +
>>   drivers/ata/ahci_sunxi.c                           | 305 +++++++++++++++++++++
>>   5 files changed, 351 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/ata/ahci-sunxi.txt
>>   create mode 100644 drivers/ata/ahci_sunxi.c
>>
>> diff --git a/Documentation/devicetree/bindings/ata/ahci-sunxi.txt b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
>> new file mode 100644
>> index 0000000..0792fa5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
>> @@ -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"
> - compatible: Should contain "allwinner,sun4i-a10-ahci"
>
>> +- reg          : <registers mapping>
> - reg: The offset and length of the MMIO registers.
>
>> +- interrupts   : <interrupt mapping for AHCI IRQ>
> - interrupts: An interrupt-specifier for the ACHI interrupt
>
>> +- clocks       : clocks for ACHI
>> +- clock-names  : clock names for AHCI
> Please _define_ the set of clock-names you expect. This binding is
> meaningless without it. If you require clock-names, define the clocks
> property in terms of it.
I copied ahci_platform.txt and filled in the missing bits, I will 
improve all the above. Appologies!
>
> Thanks,
> Mark.

--
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
Olliver Schinagl Dec. 4, 2013, 12:56 p.m. UTC | #4
On 04-12-13 13:37, Tejun Heo wrote:
> On Wed, Dec 04, 2013 at 01:10:54PM +0100, oliver@schinagl.nl wrote:
>> From: Oliver Schinagl <oliver@schinagl.nl>
>>
>> This patch adds support for the sunxi series of SoC's by allwinner. It
>> plugs into the ahci-platform framework.
>>
>> Note: Currently it uses a somewhat hackish approach that probably needs
>> a lot more work, but does the same as the IMX SoC's.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   .../devicetree/bindings/ata/ahci-sunxi.txt         |  24 ++
>>   drivers/ata/Kconfig                                |   9 +
>>   drivers/ata/Makefile                               |   1 +
>>   drivers/ata/ahci_platform.c                        |  12 +
>>   drivers/ata/ahci_sunxi.c                           | 305 +++++++++++++++++++++
> I'm not really liking the way things are going.  Do we really need
> separate drivers for each platform ahci implementation.  Are they
> really that different?  Would it be impossible to make ahci_platform
> generic enough so that we don't eventually end up with a gazillion
> ahci_XXX drivers?
I took the imx driver as example, as I wasn't sure on where to start. 
But I don't think it's possible yet without improving ahci_platform as I 
suggested in the cover letter. So if ahci_platform needs to be improved, 
I guess a separate patch series would be more appropriate?

So would it be acceptable to have this as the 2nd (and last?) 
ahci_platform driver and go from there? Or do you want to block new 
ahci_XXX drivers until ahci_platform has been improved?

Oliver
>

--
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
Tejun Heo Dec. 4, 2013, 1:14 p.m. UTC | #5
Hello,

On Wed, Dec 04, 2013 at 01:56:23PM +0100, Oliver Schinagl wrote:
> I took the imx driver as example, as I wasn't sure on where to
> start. But I don't think it's possible yet without improving
> ahci_platform as I suggested in the cover letter. So if
> ahci_platform needs to be improved, I guess a separate patch series
> would be more appropriate?
> 
> So would it be acceptable to have this as the 2nd (and last?)
> ahci_platform driver and go from there? Or do you want to block new
> ahci_XXX drivers until ahci_platform has been improved?

I don't want to block new drivers unconditionally but at least I want
to know which direction we're headed in the longer term.  Right now it
feels like we could be at the beginning of an uncoordinated explosion
of these drivers which will take a hell lot mpore effort to clean up
after the fact.  I could be wrong and these could actually be
different enough to justify separate drivers and there isn't gonna be
an avalanche of these but again I at least want to know the general
direction things are headed before making any decisions.

Thanks.
Olliver Schinagl Dec. 4, 2013, 1:16 p.m. UTC | #6
On 04-12-13 14:14, Tejun Heo wrote:
> Hello,
>
> On Wed, Dec 04, 2013 at 01:56:23PM +0100, Oliver Schinagl wrote:
>> I took the imx driver as example, as I wasn't sure on where to
>> start. But I don't think it's possible yet without improving
>> ahci_platform as I suggested in the cover letter. So if
>> ahci_platform needs to be improved, I guess a separate patch series
>> would be more appropriate?
>>
>> So would it be acceptable to have this as the 2nd (and last?)
>> ahci_platform driver and go from there? Or do you want to block new
>> ahci_XXX drivers until ahci_platform has been improved?
>
> I don't want to block new drivers unconditionally but at least I want
> to know which direction we're headed in the longer term.  Right now it
> feels like we could be at the beginning of an uncoordinated explosion
> of these drivers which will take a hell lot mpore effort to clean up
> after the fact.  I could be wrong and these could actually be
> different enough to justify separate drivers and there isn't gonna be
> an avalanche of these but again I at least want to know the general
> direction things are headed before making any decisions.
I'd be happy to pour it in any form that's needed. I even do the 
modification/rewrite of ahci_platform if I get enough help as it might 
be a little over my head initially ;)

That said, I don't think it's much different at all and I do think it 
could be much simpler. In my mind, the sunxi_ahci driver wouldn't need 
to be much bigger then a few lines that are specific to the SoC 
(hardware init) and registerd to the ahci_platform framework via 
platform_ahci_register() instead of platform_device_register().

But again, point me (for dummies ;) in the right direction and I'll work 
on it with some help.

Oliver
>
> Thanks.
>

--
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
Tejun Heo Dec. 4, 2013, 1:23 p.m. UTC | #7
Hello,

(cc'ing Richard and Shawn, hi!)

On Wed, Dec 04, 2013 at 02:16:49PM +0100, Olliver Schinagl wrote:
> On 04-12-13 14:14, Tejun Heo wrote:
> >Hello,
> >
> >On Wed, Dec 04, 2013 at 01:56:23PM +0100, Oliver Schinagl wrote:
> >>I took the imx driver as example, as I wasn't sure on where to
> >>start. But I don't think it's possible yet without improving
> >>ahci_platform as I suggested in the cover letter. So if
> >>ahci_platform needs to be improved, I guess a separate patch series
> >>would be more appropriate?
> >>
> >>So would it be acceptable to have this as the 2nd (and last?)
> >>ahci_platform driver and go from there? Or do you want to block new
> >>ahci_XXX drivers until ahci_platform has been improved?
> >
> >I don't want to block new drivers unconditionally but at least I want
> >to know which direction we're headed in the longer term.  Right now it
> >feels like we could be at the beginning of an uncoordinated explosion
> >of these drivers which will take a hell lot mpore effort to clean up
> >after the fact.  I could be wrong and these could actually be
> >different enough to justify separate drivers and there isn't gonna be
> >an avalanche of these but again I at least want to know the general
> >direction things are headed before making any decisions.
> I'd be happy to pour it in any form that's needed. I even do the
> modification/rewrite of ahci_platform if I get enough help as it
> might be a little over my head initially ;)
> 
> That said, I don't think it's much different at all and I do think
> it could be much simpler. In my mind, the sunxi_ahci driver wouldn't
> need to be much bigger then a few lines that are specific to the SoC
> (hardware init) and registerd to the ahci_platform framework via
> platform_ahci_register() instead of platform_device_register().
> 
> But again, point me (for dummies ;) in the right direction and I'll
> work on it with some help.

Richard and Shawn recently worked on ahci_imx.  Can you guys please
talk with each other and figure out what can be done to share as much
as possible among these new platform-specific drivers?  I'd really
like to see the common things factored out as much as possible with
only the actual hardware differences described for each device.

Thanks a lot!
Thomas Petazzoni Dec. 6, 2013, 9:01 a.m. UTC | #8
Dear Tejun Heo,

On Wed, 4 Dec 2013 08:23:12 -0500, Tejun Heo wrote:

> > But again, point me (for dummies ;) in the right direction and I'll
> > work on it with some help.
> 
> Richard and Shawn recently worked on ahci_imx.  Can you guys please
> talk with each other and figure out what can be done to share as much
> as possible among these new platform-specific drivers?  I'd really
> like to see the common things factored out as much as possible with
> only the actual hardware differences described for each device.

Also, please Cc me on such discussions. I have a pending AHCI platform
driver for another ARM SoC family. It is very similar to ahci_platform,
but needs to do a few more things that are SoC specific (map an
additional register area, and do some SoC-specific stuff with them).

For the moment, we're left with two approaches:

 * Do what Oliver did, where the ahci_<foo> driver will do its own
   SoC-specific stuff, and then will register an additional
   platform_device to trigger the ->probe() of the generic
   ahci_platform driver. I must say I don't really like this solution,
   since it involves having two platform_device registered for the same
   piece of hardware (one platform_device to trigger the ->probe of
   ahci_<foo>, and another one to trigger the ->probe of ahci_platform).

 * Duplicate in ahci_<foo> the (relatively small) amount of code that
   is present in ahci_platform.

From my point of view, ahci_platform should be turned into a small
"library", that provides an API for ahci_<foo> drivers to 1/ do their
own custom stuff and 2/ do the common ahci_platform stuff.

This way we avoid the registration of two platform_device for the same
piece of hardware, and we avoid the duplication of code.

Want me to propose a RFC for this idea?

Best regards,

Thomas
Olliver Schinagl Dec. 6, 2013, 9:12 a.m. UTC | #9
On 06-12-13 10:01, Thomas Petazzoni wrote:
> Dear Tejun Heo,
>
> On Wed, 4 Dec 2013 08:23:12 -0500, Tejun Heo wrote:
>
>>> But again, point me (for dummies ;) in the right direction and I'll
>>> work on it with some help.
>> Richard and Shawn recently worked on ahci_imx.  Can you guys please
>> talk with each other and figure out what can be done to share as much
>> as possible among these new platform-specific drivers?  I'd really
>> like to see the common things factored out as much as possible with
>> only the actual hardware differences described for each device.
> Also, please Cc me on such discussions. I have a pending AHCI platform
> driver for another ARM SoC family. It is very similar to ahci_platform,
> but needs to do a few more things that are SoC specific (map an
> additional register area, and do some SoC-specific stuff with them).
>
> For the moment, we're left with two approaches:
>
>   * Do what Oliver did, where the ahci_<foo> driver will do its own
>     SoC-specific stuff, and then will register an additional
>     platform_device to trigger the ->probe() of the generic
>     ahci_platform driver. I must say I don't really like this solution,
>     since it involves having two platform_device registered for the same
>     piece of hardware (one platform_device to trigger the ->probe of
>     ahci_<foo>, and another one to trigger the ->probe of ahci_platform).
>
>   * Duplicate in ahci_<foo> the (relatively small) amount of code that
>     is present in ahci_platform.
>
>  From my point of view, ahci_platform should be turned into a small
> "library", that provides an API for ahci_<foo> drivers to 1/ do their
> own custom stuff and 2/ do the common ahci_platform stuff.
>
> This way we avoid the registration of two platform_device for the same
> piece of hardware, and we avoid the duplication of code.
>
> Want me to propose a RFC for this idea?
I've started to do what sdhci does with their pltfrm driver, assuming 
that's the right approach. Since i'm only dabbling and not always 100% 
sure what should or shouldn't be done, it may take a little while, but 
looks promising from my end ;)

So is the sdhci-pltfrm approach the correct one? We still have ahci_* 
drivers, but ahci_platform.c won't be a driver in the sense that it is 
now anymore.

Oliver
>
> Best regards,
>
> Thomas

--
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
Thomas Petazzoni Dec. 6, 2013, 9:18 a.m. UTC | #10
Dear Oliver Schinagl,

On Fri, 06 Dec 2013 10:12:34 +0100, Oliver Schinagl wrote:

> >  From my point of view, ahci_platform should be turned into a small
> > "library", that provides an API for ahci_<foo> drivers to 1/ do their
> > own custom stuff and 2/ do the common ahci_platform stuff.
> >
> > This way we avoid the registration of two platform_device for the same
> > piece of hardware, and we avoid the duplication of code.
> >
> > Want me to propose a RFC for this idea?
> I've started to do what sdhci does with their pltfrm driver, assuming 
> that's the right approach. Since i'm only dabbling and not always 100% 
> sure what should or shouldn't be done, it may take a little while, but 
> looks promising from my end ;)

Yes, the approach of shdci_pltfrm is exactly the one I was proposing
here, so it definitely looks like the right direction to me (though my
opinion is not authoritative at all in this area, obviously).

> So is the sdhci-pltfrm approach the correct one? We still have ahci_* 
> drivers, but ahci_platform.c won't be a driver in the sense that it is 
> now anymore.

Yes, seems good. We will still need ahci_<foo> for the various SoC
families, because depending on the SoC, you have different things to do
(take various clocks, or do other SoC-specific configuration). As an
example, the main thing I have to do on the SoC I'm working with is
configuring some memory windows that allow the SATA controller to do
DMA to the DRAM. This configuration is inherently completely specific
to this SoC family, and it wouldn't make sense to have that in an
ahci_platform driver shared by all platforms.

Oliver, can you Cc me on your future patches about this topic, so that
I can test them in the context of the SoC I'm working on?

Thanks!

Thomas
Hans de Goede Dec. 6, 2013, 11:06 a.m. UTC | #11
Hi,

On 12/06/2013 10:12 AM, Oliver Schinagl wrote:
>
> On 06-12-13 10:01, Thomas Petazzoni wrote:
>> Dear Tejun Heo,
>>
>> On Wed, 4 Dec 2013 08:23:12 -0500, Tejun Heo wrote:
>>
>>>> But again, point me (for dummies ;) in the right direction and I'll
>>>> work on it with some help.
>>> Richard and Shawn recently worked on ahci_imx.  Can you guys please
>>> talk with each other and figure out what can be done to share as much
>>> as possible among these new platform-specific drivers?  I'd really
>>> like to see the common things factored out as much as possible with
>>> only the actual hardware differences described for each device.
>> Also, please Cc me on such discussions. I have a pending AHCI platform
>> driver for another ARM SoC family. It is very similar to ahci_platform,
>> but needs to do a few more things that are SoC specific (map an
>> additional register area, and do some SoC-specific stuff with them).
>>
>> For the moment, we're left with two approaches:
>>
>>   * Do what Oliver did, where the ahci_<foo> driver will do its own
>>     SoC-specific stuff, and then will register an additional
>>     platform_device to trigger the ->probe() of the generic
>>     ahci_platform driver. I must say I don't really like this solution,
>>     since it involves having two platform_device registered for the same
>>     piece of hardware (one platform_device to trigger the ->probe of
>>     ahci_<foo>, and another one to trigger the ->probe of ahci_platform).
>>
>>   * Duplicate in ahci_<foo> the (relatively small) amount of code that
>>     is present in ahci_platform.
>>
>>  From my point of view, ahci_platform should be turned into a small
>> "library", that provides an API for ahci_<foo> drivers to 1/ do their
>> own custom stuff and 2/ do the common ahci_platform stuff.
>>
>> This way we avoid the registration of two platform_device for the same
>> piece of hardware, and we avoid the duplication of code.
>>
>> Want me to propose a RFC for this idea?
> I've started to do what sdhci does with their pltfrm driver, assuming that's the right approach. Since i'm only dabbling and not always 100% sure what should or shouldn't be done, it may take a little while, but looks promising from my end ;)
>
> So is the sdhci-pltfrm approach the correct one? We still have ahci_* drivers, but ahci_platform.c won't be a driver in the sense that it is now anymore.

Sounds good to me. May I suggest simply adding a new ahci_pltfrm driver for
this and leaving the existing ahci_platform alone? Of course in the end
we want the old ahci_platform to go away, but it is probably best to
introduce the new one in parallel and then port things over 1 by 1
by people who can actually test the port :)

Regards,

Hans
--
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
Olliver Schinagl Dec. 11, 2013, 2:51 p.m. UTC | #12
Hey all,

On 04-12-13 14:23, Tejun Heo wrote:
> Hello,
>
> (cc'ing Richard and Shawn, hi!)
>
> On Wed, Dec 04, 2013 at 02:16:49PM +0100, Olliver Schinagl wrote:
>> On 04-12-13 14:14, Tejun Heo wrote:
>>> Hello,
>>>
>>> On Wed, Dec 04, 2013 at 01:56:23PM +0100, Oliver Schinagl wrote:
>>>> I took the imx driver as example, as I wasn't sure on where to
>>>> start. But I don't think it's possible yet without improving
>>>> ahci_platform as I suggested in the cover letter. So if
>>>> ahci_platform needs to be improved, I guess a separate patch series
>>>> would be more appropriate?
>>>>
>>>> So would it be acceptable to have this as the 2nd (and last?)
>>>> ahci_platform driver and go from there? Or do you want to block new
>>>> ahci_XXX drivers until ahci_platform has been improved?
>>> I don't want to block new drivers unconditionally but at least I want
>>> to know which direction we're headed in the longer term.  Right now it
>>> feels like we could be at the beginning of an uncoordinated explosion
>>> of these drivers which will take a hell lot mpore effort to clean up
>>> after the fact.  I could be wrong and these could actually be
>>> different enough to justify separate drivers and there isn't gonna be
>>> an avalanche of these but again I at least want to know the general
>>> direction things are headed before making any decisions.
>> I'd be happy to pour it in any form that's needed. I even do the
>> modification/rewrite of ahci_platform if I get enough help as it
>> might be a little over my head initially ;)
>>
>> That said, I don't think it's much different at all and I do think
>> it could be much simpler. In my mind, the sunxi_ahci driver wouldn't
>> need to be much bigger then a few lines that are specific to the SoC
>> (hardware init) and registerd to the ahci_platform framework via
>> platform_ahci_register() instead of platform_device_register().
>>
>> But again, point me (for dummies ;) in the right direction and I'll
>> work on it with some help.
> Richard and Shawn recently worked on ahci_imx.  Can you guys please
> talk with each other and figure out what can be done to share as much
> as possible among these new platform-specific drivers?  I'd really
> like to see the common things factored out as much as possible with
> only the actual hardware differences described for each device.
Working on this and studying the existing ahci_platform/shci_platform 
drivers the last few days and was figuring out why ahci_platform only 
supports 1 clock. IMX handles this by having 3 clocks defined in the DT, 
the first one gets enabled by default via ahci_platform, the other 2 get 
enabled in IMX's probe function.

Is it an idea to extend this to support all clocks that would be 
required (via a callback)? Or do we prefer having the clocks separated 
for other technical reasons? Or do we want to handle the clocks via the 
ahci_platform framework and extend hpriv->clk to an array of clocks?

Oliver

>
> Thanks a lot!
>

--
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
Shawn Guo Dec. 12, 2013, 6:40 a.m. UTC | #13
On Wed, Dec 11, 2013 at 03:51:51PM +0100, Olliver Schinagl wrote:
> Working on this and studying the existing
> ahci_platform/shci_platform drivers the last few days and was
> figuring out why ahci_platform only supports 1 clock. IMX handles
> this by having 3 clocks defined in the DT, the first one gets
> enabled by default via ahci_platform, the other 2 get enabled in
> IMX's probe function.
> 
> Is it an idea to extend this to support all clocks that would be
> required (via a callback)?

Not really.  We did this for ahci_imx driver only because we do not want
to churn generic ahci_platform driver with those imx specific setup
code.  Note, beside the additional two clocks, we have some PHY
parameters to set up in IMX IOMUXC general purpose registers, and vendor
specific register HOST_TIMER1MS to be set up as well.

> Or do we prefer having the clocks
> separated for other technical reasons? Or do we want to handle the
> clocks via the ahci_platform framework and extend hpriv->clk to an
> array of clocks?

The direction of the generic ahci platform driver will be having it be
a library providing helper functions, as discussed as below.

https://lkml.org/lkml/2013/12/6/153

We can ask the helper function to handle the common clocks and leave the
platform specific ones to platform driver.

Shawn

--
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
Olliver Schinagl Dec. 12, 2013, 8:47 a.m. UTC | #14
On 12-12-13 07:40, Shawn Guo wrote:
> On Wed, Dec 11, 2013 at 03:51:51PM +0100, Olliver Schinagl wrote:
>> Working on this and studying the existing
>> ahci_platform/shci_platform drivers the last few days and was
>> figuring out why ahci_platform only supports 1 clock. IMX handles
>> this by having 3 clocks defined in the DT, the first one gets
>> enabled by default via ahci_platform, the other 2 get enabled in
>> IMX's probe function.
>>
>> Is it an idea to extend this to support all clocks that would be
>> required (via a callback)?
> Not really.  We did this for ahci_imx driver only because we do not want
> to churn generic ahci_platform driver with those imx specific setup
> code.  Note, beside the additional two clocks, we have some PHY
> parameters to set up in IMX IOMUXC general purpose registers, and vendor
> specific register HOST_TIMER1MS to be set up as well.
Well many SoC's have these specific things so we need a place to provide 
for this.

The the question then becomes, what do we define as being the common 
clocks? Only the clock for the SATA IP? Or also the clock to which it 
connects to the bus? Where is the line drawn etc.

E.g. a rewritten new ahci-pltfrm.c will support all clocks defined in 
the DT for the ahci node.
>
>> Or do we prefer having the clocks
>> separated for other technical reasons? Or do we want to handle the
>> clocks via the ahci_platform framework and extend hpriv->clk to an
>> array of clocks?
> The direction of the generic ahci platform driver will be having it be
> a library providing helper functions, as discussed as below.
>
> https://lkml.org/lkml/2013/12/6/153
I know, I started this entire thread :p hence my question :)

Oliver
>
> We can ask the helper function to handle the common clocks and leave the
> platform specific ones to platform driver.
>
> Shawn
>

--
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
Hans de Goede Dec. 15, 2013, 7 p.m. UTC | #15
Hi Tejun,

I think it would be a good idea to merge ahci upstream using the
ahci_imx.c method for now. You already indicated that you were not against
doing that for now.

Oliver is working on getting a cleaner solution for this, but doing this
properly takes tinme, and we would like to move forward with getting
sunxi ahci support upstream in the mean time.

So if it is ok with you we would like to move forward with the version
initially posted. Therefor I would like to request you to review it
(glossing over the platform device instantiating a platform device
approach for now), and provide us with feedback so that we can do a v2
and start moving this towards the mainline kernel.

Regards,

Hans
--
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
Tejun Heo Dec. 15, 2013, 7:04 p.m. UTC | #16
Hello, Hans.

On Sun, Dec 15, 2013 at 08:00:20PM +0100, Hans de Goede wrote:
> I think it would be a good idea to merge ahci upstream using the
> ahci_imx.c method for now. You already indicated that you were not against
> doing that for now.

Well, the thing is nothing actually happened since ahci_imx got
merged, so I'm wondering maybe there isn't enough pressure.

> Oliver is working on getting a cleaner solution for this, but doing this
> properly takes tinme, and we would like to move forward with getting
> sunxi ahci support upstream in the mean time.
> 
> So if it is ok with you we would like to move forward with the version
> initially posted. Therefor I would like to request you to review it
> (glossing over the platform device instantiating a platform device
> approach for now), and provide us with feedback so that we can do a v2
> and start moving this towards the mainline kernel.

We still have some time left before the next merge window and if it's
really necessary, I can submit drivers post -rc1 too, so I'd still
*much* prefer doing it properly rather than creating more drivers
which would need to be cleaned up later.

Thanks.
Olliver Schinagl Dec. 16, 2013, 6:21 a.m. UTC | #17
On 15-12-13 20:04, Tejun Heo wrote:
> Hello, Hans.
>
> On Sun, Dec 15, 2013 at 08:00:20PM +0100, Hans de Goede wrote:
>> I think it would be a good idea to merge ahci upstream using the
>> ahci_imx.c method for now. You already indicated that you were not against
>> doing that for now.
> Well, the thing is nothing actually happened since ahci_imx got
> merged, so I'm wondering maybe there isn't enough pressure.
I a slowly progressing on the improved platform driver, but still 
waiting on an answer about my clock question that I asked a few days ago.
>
>> Oliver is working on getting a cleaner solution for this, but doing this
>> properly takes tinme, and we would like to move forward with getting
>> sunxi ahci support upstream in the mean time.
>>
>> So if it is ok with you we would like to move forward with the version
>> initially posted. Therefor I would like to request you to review it
>> (glossing over the platform device instantiating a platform device
>> approach for now), and provide us with feedback so that we can do a v2
>> and start moving this towards the mainline kernel.
> We still have some time left before the next merge window and if it's
> really necessary, I can submit drivers post -rc1 too, so I'd still
> *much* prefer doing it properly rather than creating more drivers
> which would need to be cleaned up later.
Since I do this in my spare time and my technical skill isn't as 
advanced as some, I don't think I have something reviewed and merge-able 
before that time frame. I do enjoy the challenge and do like working on 
the new ahci_platform framework. and do wish to continue to work on it 
however.

Oliver
>
> Thanks.
>

--
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
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/ata/ahci-sunxi.txt b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
new file mode 100644
index 0000000..0792fa5
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
@@ -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@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 = <&reg_ahci_5v>;
+	};
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 4e73772..b87e2ba 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -106,6 +106,15 @@  config AHCI_IMX
 
 	  If unsure, say N.
 
+config AHCI_SUNXI
+	tristate "Allwinner sunxi AHCI SATA support"
+	depends on SATA_AHCI_PLATFORM && 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
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 46518c6..246050b 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -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
 
 # SFF w/ custom DMA
 obj-$(CONFIG_PDC_ADMA)		+= pdc_adma.o
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index f955431..28ff1eb 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -31,6 +31,7 @@  enum ahci_type {
 	AHCI,		/* standard platform ahci */
 	IMX53_AHCI,	/* ahci on i.mx53 */
 	STRICT_AHCI,	/* delayed DMA engine start */
+	SUNXI_AHCI,	/* ahci on sunxi */
 };
 
 static struct platform_device_id ahci_devtype[] = {
@@ -44,6 +45,9 @@  static struct platform_device_id ahci_devtype[] = {
 		.name = "strict-ahci",
 		.driver_data = STRICT_AHCI,
 	}, {
+		.name = "sunxi-ahci",
+		.driver_data = SUNXI_AHCI,
+	}, {
 		/* sentinel */
 	}
 };
@@ -81,6 +85,14 @@  static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_platform_ops,
 	},
+	[SUNXI_AHCI] = {
+		AHCI_HFLAGS	(AHCI_HFLAG_32BIT_ONLY | AHCI_HFLAG_NO_MSI |
+				 AHCI_HFLAG_NO_PMP | AHCI_HFLAG_YES_NCQ),
+		.flags		= AHCI_FLAG_COMMON,
+		.pio_mask	= ATA_PIO4,
+		.udma_mask	= ATA_UDMA6,
+		.port_ops	= &ahci_platform_ops,
+	},
 };
 
 static struct scsi_host_template ahci_platform_sht = {
diff --git a/drivers/ata/ahci_sunxi.c b/drivers/ata/ahci_sunxi.c
new file mode 100644
index 0000000..982641f
--- /dev/null
+++ b/drivers/ata/ahci_sunxi.c
@@ -0,0 +1,305 @@ 
+/*
+ * Allwinner sunxi AHCI SATA platform driver
+ * Copyright 2013 Olliver Schinagl <oliver@schinagl.nl>
+ *
+ * Based on the AHCI SATA platform driver by Freescale and Allwinner
+ * Based on code from
+ * Allwinner Technology Co., Ltd. <www.allwinnertech.com>
+ * Daniel Wang <danielwang@allwinnertech.com>
+ *
+ * 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/kernel.h>
+#include <linux/regulator/consumer.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of_device.h>
+#include <linux/ioport.h>
+#include <linux/device.h>
+#include <linux/gfp.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/errno.h>
+#include <linux/ahci_platform.h>
+#include "ahci.h"
+
+#define DRV_NAME "sunxi-sata"
+
+#define AHCI_BISTAFR 0x00a0
+#define AHCI_BISTCR 0x00a4
+#define AHCI_BISTFCTR 0x00a8
+#define AHCI_BISTSR 0x00ac
+#define AHCI_BISTDECR 0x00b0
+#define AHCI_DIAGNR0 0x00b4
+#define AHCI_DIAGNR1 0x00b8
+#define AHCI_OOBR 0x00bc
+#define AHCI_PHYCS0R 0x00c0
+#define AHCI_PHYCS1R 0x00c4
+#define AHCI_PHYCS2R 0x00c8
+#define AHCI_TIMER1MS 0x00e0
+#define AHCI_GPARAM1R 0x00e8
+#define AHCI_GPARAM2R 0x00ec
+#define AHCI_PPARAMR 0x00f0
+#define AHCI_TESTR 0x00f4
+#define AHCI_VERSIONR 0x00f8
+#define AHCI_IDR 0x00fc
+#define AHCI_RWCR 0x00fc
+#define AHCI_P0DMACR 0x0170
+#define AHCI_P0PHYCR 0x0178
+#define AHCI_P0PHYSR 0x017c
+
+struct sunxi_ahci_data {
+	struct platform_device *ahci_pdev;
+	struct regulator *regulator;
+	struct clk *sata_clk;
+	struct clk *ahb_clk;
+};
+
+static void sunxi_clrbits(void __iomem *reg, u32 clr_val)
+{
+	u32 reg_val;
+
+	reg_val = readl(reg);
+	reg_val &= ~(clr_val);
+	writel(reg_val, reg);
+}
+
+static void sunxi_setbits(void __iomem *reg, u32 set_val)
+{
+	u32 reg_val;
+
+	reg_val = readl(reg);
+	reg_val |= set_val;
+	writel(reg_val, reg);
+}
+
+static void sunxi_clrsetbits(void __iomem *reg, u32 clr_val, u32 set_val)
+{
+	u32 reg_val;
+
+	reg_val = readl(reg);
+	reg_val &= ~(clr_val);
+	reg_val |= set_val;
+	writel(reg_val, reg);
+}
+
+static u32 sunxi_getbits(void __iomem *reg, u8 mask, u8 shift)
+{
+	return (readl(reg) >> shift) & mask;
+}
+
+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);
+
+	sunxi_setbits(reg_base + AHCI_PHYCS1R, BIT(19));
+	sunxi_clrsetbits(reg_base + AHCI_PHYCS0R,
+			 (0x7 << 24),
+			 (0x5 << 24) | BIT(23) | BIT(18));
+	sunxi_clrsetbits(reg_base + AHCI_PHYCS1R,
+			 (0x3 << 16) | (0x1f << 8) | (0x3 << 6),
+			 (0x2 << 16) | (0x6 << 8) | (0x2 << 6));
+	sunxi_setbits(reg_base + AHCI_PHYCS1R, BIT(28) | BIT(15));
+	sunxi_clrbits(reg_base + AHCI_PHYCS1R, BIT(19));
+	sunxi_clrsetbits(reg_base + AHCI_PHYCS0R,
+			 (0x7 << 20), (0x3 << 20));
+	sunxi_clrsetbits(reg_base + AHCI_PHYCS2R,
+			 (0x1f << 5), (0x19 << 5));
+	mdelay(5);
+
+	sunxi_setbits(reg_base + AHCI_PHYCS0R, (0x1 << 19));
+
+	timeout = 0x100000;
+	do {
+		reg_val = sunxi_getbits(reg_base + AHCI_PHYCS0R, 0x7, 28);
+	} while (--timeout && (reg_val != 0x2));
+	if (!timeout)
+		dev_err(dev, "PHY power up failed.\n");
+
+	sunxi_setbits(reg_base + AHCI_PHYCS2R, (0x1 << 24));
+
+	timeout = 0x100000;
+	do {
+		reg_val = sunxi_getbits(reg_base + AHCI_PHYCS2R, 0x1, 24);
+	} while (--timeout && reg_val);
+	if (!timeout)
+		dev_err(dev, "PHY calibration failed.\n");
+	mdelay(15);
+
+	writel(0x7, reg_base + AHCI_RWCR);
+
+	return 0;
+}
+
+static int sunxi_ahci_init(struct device *dev, void __iomem *reg_base)
+{
+	struct sunxi_ahci_data *ahci_data;
+	int ret;
+
+	ahci_data = dev_get_drvdata(dev->parent);
+
+	ret = clk_prepare_enable(ahci_data->sata_clk);
+	if (ret < 0)
+		return ret;
+
+	ret = clk_prepare_enable(ahci_data->ahb_clk);
+	if (ret < 0)
+		return ret;
+
+	ret = regulator_enable(ahci_data->regulator);
+	if (ret)
+		return ret;
+
+	return sunxi_ahci_phy_init(dev, reg_base);
+}
+
+static void sunxi_ahci_exit(struct device *dev)
+{
+	struct sunxi_ahci_data *ahci_data;
+
+	ahci_data = dev_get_drvdata(dev->parent);
+
+	regulator_disable(ahci_data->regulator);
+
+	clk_disable_unprepare(ahci_data->ahb_clk);
+	clk_disable_unprepare(ahci_data->sata_clk);
+}
+
+static struct ahci_platform_data sunxi_ahci_pdata = {
+	.init = sunxi_ahci_init,
+	.exit = sunxi_ahci_exit,
+};
+
+static int sunxi_ahci_remove(struct platform_device *pdev)
+{
+	struct sunxi_ahci_data *ahci_data;
+
+	ahci_data = platform_get_drvdata(pdev);
+	platform_device_unregister(ahci_data->ahci_pdev);
+
+	dev_dbg(&pdev->dev, "driver unloaded\n");
+
+	return 0;
+}
+
+static const struct of_device_id sunxi_ahci_of_match[] = {
+	{ .compatible = "allwinner,sun4i-a10-ahci", .data = &sunxi_ahci_pdata},
+	{/* sentinel */},
+};
+MODULE_DEVICE_TABLE(of, sunxi_ahci_of_match);
+
+static int sunxi_ahci_probe(struct platform_device *pdev)
+{
+	const struct ahci_platform_data *pdata;
+	const struct of_device_id *of_dev_id;
+	struct resource *mem, *irq, res[2];
+	struct platform_device *ahci_pdev;
+	struct sunxi_ahci_data *ahci_data;
+	struct regulator *regulator;
+	int ret;
+
+	regulator = devm_regulator_get(&pdev->dev, "pwr");
+	if (IS_ERR(regulator)) {
+		ret = PTR_ERR(regulator);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "no regulator found (%d)\n", ret);
+		return ret;
+	}
+
+	ahci_data = devm_kzalloc(&pdev->dev, sizeof(*ahci_data), GFP_KERNEL);
+	if (!ahci_data)
+		return -ENOMEM;
+
+	ahci_pdev = platform_device_alloc("sunxi-ahci", -1);
+	if (!ahci_pdev)
+		return -ENODEV;
+
+	ahci_pdev->dev.parent = &pdev->dev;
+
+	ahci_data->regulator = regulator;
+	ahci_data->ahb_clk = devm_clk_get(&pdev->dev, "ahb_sata");
+	if (IS_ERR(ahci_data->ahb_clk)) {
+		ret = PTR_ERR(ahci_data->ahb_clk);
+		goto err_out;
+	}
+
+	ahci_data->sata_clk = devm_clk_get(&pdev->dev, "pll6_sata");
+	if (IS_ERR(ahci_data->sata_clk)) {
+		ret = PTR_ERR(ahci_data->sata_clk);
+		goto err_out;
+	}
+
+	ahci_data->ahci_pdev = ahci_pdev;
+	platform_set_drvdata(pdev, ahci_data);
+
+	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;
+
+	of_dev_id = of_match_device(sunxi_ahci_of_match, &pdev->dev);
+	if (of_dev_id) {
+		pdata = of_dev_id->data;
+	} else {
+		ret = -EINVAL;
+		goto err_out;
+	}
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!mem || !irq) {
+		ret = -ENOMEM;
+		goto err_out;
+	}
+	res[0] = *mem;
+	res[1] = *irq;
+	ret = platform_device_add_resources(ahci_pdev, res, 2);
+	if (ret)
+		goto err_out;
+
+	ret = platform_device_add_data(ahci_pdev, pdata, sizeof(*pdata));
+	if (ret)
+		goto err_out;
+
+	ret = platform_device_add(ahci_pdev);
+	if (ret)
+		goto err_out;
+
+	return 0;
+
+err_out:
+	platform_device_put(ahci_pdev);
+	return ret;
+}
+
+static struct platform_driver sunxi_ahci_driver = {
+	.probe = sunxi_ahci_probe,
+	.remove = sunxi_ahci_remove,
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table = sunxi_ahci_of_match,
+	},
+};
+module_platform_driver(sunxi_ahci_driver);
+
+MODULE_DESCRIPTION("Allwinner sunxi AHCI SATA platform driver");
+MODULE_AUTHOR("Olliver Schinagl <oliver@schinagl.nl>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("ahci:sunxi");