Message ID | 1386159055-10264-3-git-send-email-oliver@schinagl.nl |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
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
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?
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
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
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.
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
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!
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
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
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
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
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
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
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
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
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.
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 --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 = <®_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");