Message ID | 1394841201-29495-3-git-send-email-lho@apm.com |
---|---|
State | Accepted, archived |
Commit | 1ccaead5f5a66162fafd1ce1e6282df04e181d0a |
Headers | show |
On Saturday 15 March 2014, Loc Ho wrote: > This patch adds documentation for the APM X-Gene SoC SATA host controller DTS > binding. > > Signed-off-by: Loc Ho <lho@apm.com> > Signed-off-by: Tuan Phan <tphan@apm.com> > Signed-off-by: Suman Tripathi <stripathi@apm.com> Acked-by: Arnd Bergmann <arnd@arndb.de> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 15 March 2014, Loc Ho wrote: > This patch adds support for the APM X-Gene SoC AHCI SATA host controller > driver. It requires the corresponding APM X-Gene SoC PHY driver. This > initial version only supports Gen3 speed. This version seems workable, thanks for the quick follow-up. The comment about Gen3 speed above reminds me that you took some shortcuts to get here and you removed support for some features as well as some bug workarounds in the process. I'm guessing some of them won't be necessary because they are only for prototype hardware or for early boot loader versions that don't yet set up the hardware right, but others actually need to come back. That is usually a good approach, but I'd also like to make sure we can deal with them nicely when you have to add them back later, and don't have to add ugly extensions to the DT binding to support the old dtb files. Can you list (also in the changelog) the parts of the driver that you have taken out for now and that you expect to add back at later stage? I think that would be helpful for perspective. Regarding the support for multiple link speeds, how do you think it will be done? Can you have a driver-side link speed autoconfiguration, or do you have to add DT properties and let the driver know about the attached device? Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, >> This patch adds support for the APM X-Gene SoC AHCI SATA host controller >> driver. It requires the corresponding APM X-Gene SoC PHY driver. This >> initial version only supports Gen3 speed. > > This version seems workable, thanks for the quick follow-up. > > The comment about Gen3 speed above reminds me that you took some > shortcuts to get here and you removed support for some features > as well as some bug workarounds in the process. I'm guessing some > of them won't be necessary because they are only for prototype > hardware or for early boot loader versions that don't yet set up > the hardware right, but others actually need to come back. > > That is usually a good approach, but I'd also like to make sure we can > deal with them nicely when you have to add them back later, and don't > have to add ugly extensions to the DT binding to support the old dtb > files. > > Can you list (also in the changelog) the parts of the driver that you > have taken out for now and that you expect to add back at later > stage? I think that would be helpful for perspective. > Here is an list of patches that we will be preparing for once the basic driver is completed. Do you want me to re-generate the patch change log with this info? 1. Support for Gen1/Gen2/Gen3 Solution: The simplest solution is to have the PHY framework support setting the desire speed. I had argued with the TI folks but they are reluctant to add this function to the framework. They argued that this is still not generic enough. I will start the discussion again later on. The other possibility (but not sure if this is doable) is to have the PHY init function to be smarter and do the necessary operation assuming the underlying PHY is capable in detecting the link up speed. I will need to check the spec of this. 2. Retry COMRESET (hardreset) if failed first time Solution: This code is purely in the host driver hardreset function. It isn't for sure that we will need this. Given our current initial board design, this seems to required 1 out of many, many power cycle boot. 3. Cleaning the receiver line when SER_DISPARITY/SER_10B_8B error during COMRESET (hardreset function call) Solution: It is observed that during COMRESET, the receiver line can report these errors and an clean up is required. For this, the code will be with in the host driver and an call into the PHY layer. The solution is the similar to #1 above. 4. IO flush before servicing completed operations (mostly read operations) Solution: The read IO operations require an flush from the host controller hardware side. For this, we will need to change the libahci to call an function that is provided by the host driver if non-NULL. 5. PM (power management) support Solution: Due to an errata in the host controller, stopping the controller requires an slightly modified version in the way it detects the host port completed the requested operation. For this we will have to introduce an AHCI_HFLAG_XXX to detect the completion of FIS_RX stopped by the CI register instead the PORT_CMD register or just delay for 500ms. > Regarding the support for multiple link speeds, how do you think > it will be done? > Can you have a driver-side link speed autoconfiguration, > or do you have to add DT properties and let the driver know about > the attached device? > See above #1. -Loc -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Kishon/Felipe, > >> This patch adds support for the APM X-Gene SoC AHCI SATA host controller > >> driver. It requires the corresponding APM X-Gene SoC PHY driver. This > >> initial version only supports Gen3 speed. > > > > This version seems workable, thanks for the quick follow-up. > > > > The comment about Gen3 speed above reminds me that you took some > > shortcuts to get here and you removed support for some features > > as well as some bug workarounds in the process. I'm guessing some > > of them won't be necessary because they are only for prototype > > hardware or for early boot loader versions that don't yet set up > > the hardware right, but others actually need to come back. > > > > That is usually a good approach, but I'd also like to make sure we can > > deal with them nicely when you have to add them back later, and don't > > have to add ugly extensions to the DT binding to support the old dtb > > files. > > > > Can you list (also in the changelog) the parts of the driver that you > > have taken out for now and that you expect to add back at later > > stage? I think that would be helpful for perspective. > > > > Here is an list of patches that we will be preparing for once the > basic driver is completed. Do you want me to re-generate the patch > change log with this info? > > 1. Support for Gen1/Gen2/Gen3 > Solution: The simplest solution is to have the PHY framework support > setting the desire speed. I had argued with the TI folks but they are > reluctant to add this function to the framework. They argued that this > is still not generic enough. I will start the discussion again later > on. The other possibility (but not sure if this is doable) is to have > the PHY init function to be smarter and do the necessary operation > assuming the underlying PHY is capable in detecting the link up speed. > I will need to check the spec of this. In order for the X-Gene SATA PHY to support SATA Gen1 and Gen2 speed, we need an method to instruct the underlying PHY driver to switch to an specified setting after link up. For this errata, Suman Tripathi had submitted the patch [1]. It is not possible to hide this within the PHY driver. Each instance of the PHY driver controls two ports. By calling the exit function and then init function, it will affect the other ports - which is not the correct behavior. At this point, we don't see any solution besides introduce an PHY framework function set_rate. Can you let us know if this solution is acceptable? [1] https://lkml.org/lkml/2014/4/18/491 -Loc -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 28, 2014 at 04:12:02PM -0700, Loc Ho wrote: > Hi Kishon/Felipe, > > > >> This patch adds support for the APM X-Gene SoC AHCI SATA host controller > > >> driver. It requires the corresponding APM X-Gene SoC PHY driver. This > > >> initial version only supports Gen3 speed. > > > > > > This version seems workable, thanks for the quick follow-up. > > > > > > The comment about Gen3 speed above reminds me that you took some > > > shortcuts to get here and you removed support for some features > > > as well as some bug workarounds in the process. I'm guessing some > > > of them won't be necessary because they are only for prototype > > > hardware or for early boot loader versions that don't yet set up > > > the hardware right, but others actually need to come back. > > > > > > That is usually a good approach, but I'd also like to make sure we can > > > deal with them nicely when you have to add them back later, and don't > > > have to add ugly extensions to the DT binding to support the old dtb > > > files. > > > > > > Can you list (also in the changelog) the parts of the driver that you > > > have taken out for now and that you expect to add back at later > > > stage? I think that would be helpful for perspective. > > > > > > > Here is an list of patches that we will be preparing for once the > > basic driver is completed. Do you want me to re-generate the patch > > change log with this info? > > > > 1. Support for Gen1/Gen2/Gen3 > > Solution: The simplest solution is to have the PHY framework support > > setting the desire speed. I had argued with the TI folks but they are > > reluctant to add this function to the framework. They argued that this > > is still not generic enough. I will start the discussion again later > > on. The other possibility (but not sure if this is doable) is to have > > the PHY init function to be smarter and do the necessary operation > > assuming the underlying PHY is capable in detecting the link up speed. > > I will need to check the spec of this. > > > In order for the X-Gene SATA PHY to support SATA Gen1 and Gen2 speed, > we need an method to instruct the underlying PHY driver to switch to > an specified setting after link up. For this errata, Suman Tripathi > had submitted the patch [1]. It is not possible to hide this within > the PHY driver. Each instance of the PHY driver controls two ports. By > calling the exit function and then init function, it will affect the > other ports - which is not the correct behavior. At this point, we > don't see any solution besides introduce an PHY framework function > set_rate. Can you let us know if this solution is acceptable? > > [1] https://lkml.org/lkml/2014/4/18/491 that 'lane' argument isn't acceptable. If one PHY talks to two Links, your PHY driver should register two providers, then the lane argument can be ignored. Rate can be reused for different things depending on the underlying Bus; in case of USB, it could be for switching among Superspeed10/Superspeed5Highspeed; or 1Gbit/100Mbit switch on Networking interfaces; or Gen1/2/3 selection for PCI controllers and so on. The only thing I can't find a way to abstract is that 'lane' argument which isn't even specific to SATA, it's particular to how you guys wrote your driver. Should you have one PHY per SATA link, you wouldn't have added that 'lane' argument at all. cheers
diff --git a/Documentation/devicetree/bindings/ata/apm-xgene.txt b/Documentation/devicetree/bindings/ata/apm-xgene.txt new file mode 100644 index 0000000..7bcfbf5 --- /dev/null +++ b/Documentation/devicetree/bindings/ata/apm-xgene.txt @@ -0,0 +1,76 @@ +* APM X-Gene 6.0 Gb/s SATA host controller nodes + +SATA host controller nodes are defined to describe on-chip Serial ATA +controllers. Each SATA controller (pair of ports) have its own node. + +Required properties: +- compatible : Shall contain: + * "apm,xgene-ahci" +- reg : First memory resource shall be the AHCI memory + resource. + Second memory resource shall be the host controller + core memory resource. + Third memory resource shall be the host controller + diagnostic memory resource. + 4th memory resource shall be the host controller + AXI memory resource. + 5th optional memory resource shall be the host + controller MUX memory resource if required. +- interrupts : Interrupt-specifier for SATA host controller IRQ. +- clocks : Reference to the clock entry. +- phys : A list of phandles + phy-specifiers, one for each + entry in phy-names. +- phy-names : Should contain: + * "sata-phy" for the SATA 6.0Gbps PHY + +Optional properties: +- status : Shall be "ok" if enabled or "disabled" if disabled. + Default is "ok". + +Example: + sataclk: sataclk { + compatible = "fixed-clock"; + #clock-cells = <1>; + clock-frequency = <100000000>; + clock-output-names = "sataclk"; + }; + + phy2: phy@1f22a000 { + compatible = "apm,xgene-phy"; + reg = <0x0 0x1f22a000 0x0 0x100>; + #phy-cells = <1>; + }; + + phy3: phy@1f23a000 { + compatible = "apm,xgene-phy"; + reg = <0x0 0x1f23a000 0x0 0x100>; + #phy-cells = <1>; + }; + + sata2: sata@1a400000 { + compatible = "apm,xgene-ahci"; + reg = <0x0 0x1a400000 0x0 0x1000>, + <0x0 0x1f220000 0x0 0x1000>, + <0x0 0x1f22d000 0x0 0x1000>, + <0x0 0x1f22e000 0x0 0x1000>, + <0x0 0x1f227000 0x0 0x1000>; + interrupts = <0x0 0x87 0x4>; + status = "ok"; + clocks = <&sataclk 0>; + phys = <&phy2 0>; + phy-names = "sata-phy"; + }; + + sata3: sata@1a800000 { + compatible = "apm,xgene-ahci-pcie"; + reg = <0x0 0x1a800000 0x0 0x1000>, + <0x0 0x1f230000 0x0 0x1000>, + <0x0 0x1f23d000 0x0 0x1000>, + <0x0 0x1f23e000 0x0 0x1000>, + <0x0 0x1f237000 0x0 0x1000>; + interrupts = <0x0 0x88 0x4>; + status = "ok"; + clocks = <&sataclk 0>; + phys = <&phy3 0>; + phy-names = "sata-phy"; + };