| Message ID | 1394655573-32645-3-git-send-email-lho@apm.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wednesday, March 12, 2014 02:19:32 PM 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. > > Signed-off-by: Loc Ho <lho@apm.com> > Signed-off-by: Tuan Phan <tphan@apm.com> > Signed-off-by: Suman Tripathi <stripathi@apm.com> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Thanks for fixing all the remaining issues. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics -- 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 Wednesday 12 March 2014, Loc Ho wrote: > +- 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-6g" for the SATA 6.0Gbps PHY > + The generic ahci binding has convered on using the string "sata-phy" for ahci nodes. I think you should use the same for consistency. I realize I agreed to using "sata-6g" earlier on, but the discussion about the generic binding has moved on in the meantime and that is already merged. 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
On Wednesday 12 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. > > Signed-off-by: Loc Ho <lho@apm.com> > Signed-off-by: Tuan Phan <tphan@apm.com> > Signed-off-by: Suman Tripathi <stripathi@apm.com> Sorry I've skipped the last ten review rounds. I'm glad to see the code has improved so much in the meantime! I hope the points I'm making here were not already covered in the many previous review rounds. > +/* Controller who PHY shared with SGMII Ethernet PHY */ > +#define XGENE_AHCI_SGMII_DTS "apm,xgene-ahci-sgmii" > + > +/* Controller who PHY (internal reference clock macro) shared with PCIe */ > +#define XGENE_AHCI_PCIE_DTS "apm,xgene-ahci-pcie" Please remove these macros, they don't help anything at all but make it harder to follow what's going on. Regarding the actual strings, reflecting them now I think listing 'pcie' and 'sgmii' is not really helpful to the reader. The difference to the AHCI driver should not be which device it's sharing its PHY with, but rather how that looks from software side. The only difference in your current driver is whether this function +/* MUX CSR */ +#define SATA_ENET_CONFIG_REG 0x00000000 +#define CFG_SATA_ENET_SELECT_MASK 0x00000001 +static int xgene_ahci_mux_select(struct xgene_ahci_context *ctx) +{ + void *mux_csr = ctx->csr_base + SATA_ENET_MUX_OFFSET; + u32 val; + + val = readl(mux_csr + SATA_ENET_CONFIG_REG); + val &= ~CFG_SATA_ENET_SELECT_MASK; + writel(val, mux_csr + SATA_ENET_CONFIG_REG); + val = readl(mux_csr + SATA_ENET_CONFIG_REG); + return val & CFG_SATA_ENET_SELECT_MASK ? -1 : 0; +} gets called. Can you clarify what this register access does? If it's just setting a index into a mux output, would it make sense to have an optional DT property containing an integer with the mux setting you want to set? That way you wouldn't even have to have two compatible strings but just do ret = of_property_read_u32(node, "apm,ahci-mux", &mux); if (!ret) xgene_ahci_mux_select(ctx, mux); > + /* > + * Can't use devm_ioremap_resource due to overlapping region. > + * 0xYYYY.0000 - host core > + * 0xYYYY.7000 - Mux (if applicable) > + * 0xYYYY.A000 - PHY indirect access > + * 0xYYYY.C000 - Clock > + * 0xYYYY.D000 - RAM shutdown removal > + * As we map the entire region as one, it overlaps with the PHY driver. > + */ > + ctx->csr_base = devm_ioremap(dev, res->start, resource_size(res)); > + if (!ctx->csr_base) { > + dev_err(dev, "can't map %pR\n", res); > + return -ENOMEM; > + } I still think we should try not to have overlapping memory areas here. Could you split up the registers into another range in the reg property to leave the PHY registers out? > + /* Setup DMA mask - 32 for 32-bit system and 64 for 64-bit system */ > + rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(8*sizeof(void *))); > + if (rc) { > + dev_err(dev, "Unable to set dma mask\n"); > + goto disable_resources; > + } This is something we definitely have to fix properly. We are hitting the problem of dma masks for internal devices on arm32 now, and there is ongoing discussion about whether a device driver should touch these at all, or rather rely on the DT core to set up the masks in a generic way. This device is probably the first DMA master capable device we have on arm64, other than PCI devices, and we must be careful to get it right. In either way, the mask of the device must *not* depend on sizeof(void*): If the device is capable of doing 64-bit DMA, it should also be able to do that when running a 32-bit kernel. Please change this to DMA_BIT_MASK(64) for now, and add a comment explaining that it is preliminary. Also, please read the thread named "[PATCH 0/7] of: setup dma parameters using dma-ranges and dma-coherent" and comment there about what you think we should do here. I assume we will have to add "dma-ranges" properties in a number of places to get this all to work fine any 64-bit SoC. Do you know if you have any DMA master devices in x-gene that are not 64-bit capable? 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, > On Wednesday 12 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. > > > > Signed-off-by: Loc Ho <lho@apm.com> > > Signed-off-by: Tuan Phan <tphan@apm.com> > > Signed-off-by: Suman Tripathi <stripathi@apm.com> > > Sorry I've skipped the last ten review rounds. I'm glad to see the > code has improved so much in the meantime! > > I hope the points I'm making here were not already covered in the > many previous review rounds. > > > +/* Controller who PHY shared with SGMII Ethernet PHY */ > > +#define XGENE_AHCI_SGMII_DTS "apm,xgene-ahci-sgmii" > > + > > +/* Controller who PHY (internal reference clock macro) shared with PCIe */ > > +#define XGENE_AHCI_PCIE_DTS "apm,xgene-ahci-pcie" > > Please remove these macros, they don't help anything at all > but make it harder to follow what's going on. > > Regarding the actual strings, reflecting them now I think listing 'pcie' > and 'sgmii' is not really helpful to the reader. The difference to > the AHCI driver should not be which device it's sharing its PHY with, > but rather how that looks from software side. > > The only difference in your current driver is whether this function > > +/* MUX CSR */ > +#define SATA_ENET_CONFIG_REG 0x00000000 > +#define CFG_SATA_ENET_SELECT_MASK 0x00000001 > +static int xgene_ahci_mux_select(struct xgene_ahci_context *ctx) > +{ > + void *mux_csr = ctx->csr_base + SATA_ENET_MUX_OFFSET; > + u32 val; > + > + val = readl(mux_csr + SATA_ENET_CONFIG_REG); > + val &= ~CFG_SATA_ENET_SELECT_MASK; > + writel(val, mux_csr + SATA_ENET_CONFIG_REG); > + val = readl(mux_csr + SATA_ENET_CONFIG_REG); > + return val & CFG_SATA_ENET_SELECT_MASK ? -1 : 0; > +} > > gets called. Can you clarify what this register access does? > If it's just setting a index into a mux output, would it make > sense to have an optional DT property containing an integer with > the mux setting you want to set? That way you wouldn't even > have to have two compatible strings but just do > > ret = of_property_read_u32(node, "apm,ahci-mux", &mux); > if (!ret) > xgene_ahci_mux_select(ctx, mux); > Given that fact that I will break up the resource. Let's just use the resource for the MUX to handle this. For the IP that doesn't existed, I will just not list it. > > + /* > > + * Can't use devm_ioremap_resource due to overlapping region. > > + * 0xYYYY.0000 - host core > > + * 0xYYYY.7000 - Mux (if applicable) > > + * 0xYYYY.A000 - PHY indirect access > > + * 0xYYYY.C000 - Clock > > + * 0xYYYY.D000 - RAM shutdown removal > > + * As we map the entire region as one, it overlaps with the PHY driver. > > + */ > > + ctx->csr_base = devm_ioremap(dev, res->start, resource_size(res)); > > + if (!ctx->csr_base) { > > + dev_err(dev, "can't map %pR\n", res); > > + return -ENOMEM; > > + } > > I still think we should try not to have overlapping memory areas here. > Could you split up the registers into another range in the reg property > to leave the PHY registers out? Let me do this: 1. One resource for the RAM shutdown 2. One resource for the host controller 3. One optional resource for the MUX if needed. With #3, this also solved the MUX detection and avoid having another node "apm,ahci-mux". > > > + /* Setup DMA mask - 32 for 32-bit system and 64 for 64-bit system */ > > + rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(8*sizeof(void *))); > > + if (rc) { > > + dev_err(dev, "Unable to set dma mask\n"); > > + goto disable_resources; > > + } > > This is something we definitely have to fix properly. We are hitting > the problem of dma masks for internal devices on arm32 now, and there > is ongoing discussion about whether a device driver should touch these > at all, or rather rely on the DT core to set up the masks in a generic > way. This device is probably the first DMA master capable device we > have on arm64, other than PCI devices, and we must be careful to get it > right. > > In either way, the mask of the device must *not* depend on sizeof(void*): > If the device is capable of doing 64-bit DMA, it should also be able > to do that when running a 32-bit kernel. > > Please change this to DMA_BIT_MASK(64) for now, and add a comment > explaining that it is preliminary. okay. > > Also, please read the thread named "[PATCH 0/7] of: setup dma > parameters using dma-ranges and dma-coherent" and comment there > about what you think we should do here. I assume we will have to > add "dma-ranges" properties in a number of places to get this > all to work fine any 64-bit SoC. Do you know if you have any > DMA master devices in x-gene that are not 64-bit capable? > I will read and follow later. We do have non-64-bit devices such as SDIO and SPI. But these IP's have translation in the bus similar to PCIe PIM. -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 Friday 14 March 2014, Loc Ho wrote: > > On Wednesday 12 March 2014, Loc Ho wrote: > > gets called. Can you clarify what this register access does? > > If it's just setting a index into a mux output, would it make > > sense to have an optional DT property containing an integer with > > the mux setting you want to set? That way you wouldn't even > > have to have two compatible strings but just do > > > > ret = of_property_read_u32(node, "apm,ahci-mux", &mux); > > if (!ret) > > xgene_ahci_mux_select(ctx, mux); > > > > Given that fact that I will break up the resource. Let's just use the > resource for the MUX to handle this. For the IP that doesn't existed, > I will just not list it. Ah, that sounds good. It also means that if the firmware has set up the mux already, you don't have to touch it again. That would probably be the preferred case. I'm undecided about the value that you write in there. That could either be passed through DT as I suggested above for extra flexibility, or you can keep it hardcoded if you are absolutely sure that there will never need to be a case where you have to set it to something other than '1', > > > + /* > > > + * Can't use devm_ioremap_resource due to overlapping region. > > > + * 0xYYYY.0000 - host core > > > + * 0xYYYY.7000 - Mux (if applicable) > > > + * 0xYYYY.A000 - PHY indirect access > > > + * 0xYYYY.C000 - Clock > > > + * 0xYYYY.D000 - RAM shutdown removal > > > + * As we map the entire region as one, it overlaps with the PHY driver. > > > + */ > > > + ctx->csr_base = devm_ioremap(dev, res->start, resource_size(res)); > > > + if (!ctx->csr_base) { > > > + dev_err(dev, "can't map %pR\n", res); > > > + return -ENOMEM; > > > + } > > > > I still think we should try not to have overlapping memory areas here. > > Could you split up the registers into another range in the reg property > > to leave the PHY registers out? > > Let me do this: > > 1. One resource for the RAM shutdown > 2. One resource for the host controller > 3. One optional resource for the MUX if needed. > > With #3, this also solved the MUX detection and avoid having another > node "apm,ahci-mux". My suggestion was not to have another device node, just a property that contains a value to be written into the mux register if present. > > Also, please read the thread named "[PATCH 0/7] of: setup dma > > parameters using dma-ranges and dma-coherent" and comment there > > about what you think we should do here. I assume we will have to > > add "dma-ranges" properties in a number of places to get this > > all to work fine any 64-bit SoC. Do you know if you have any > > DMA master devices in x-gene that are not 64-bit capable? > > > > I will read and follow later. We do have non-64-bit devices such as > SDIO and SPI. But these IP's have translation in the bus similar to > PCIe PIM. Is there also an IOMMU, or just the PIM? If there is no IOMMU, we will actually have to use the swiotlb code here, otherwise we can rely on the IOMMU to do the translation so we can pretend that there is always 64-bit DMA capability. In theory, the PIM could be dynamically reprogrammed for each DMA, as an extremely primitive IOMMU implementation, but I don't know what the impact on performance would be from doing that. It would also prevent you from starting multiple concurrent DMAs, although I suspect that is not an important case for SPI and SDIO. 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, >> > gets called. Can you clarify what this register access does? >> > If it's just setting a index into a mux output, would it make >> > sense to have an optional DT property containing an integer with >> > the mux setting you want to set? That way you wouldn't even >> > have to have two compatible strings but just do >> > >> > ret = of_property_read_u32(node, "apm,ahci-mux", &mux); >> > if (!ret) >> > xgene_ahci_mux_select(ctx, mux); >> > >> >> Given that fact that I will break up the resource. Let's just use the >> resource for the MUX to handle this. For the IP that doesn't existed, >> I will just not list it. > > Ah, that sounds good. It also means that if the firmware has set up > the mux already, you don't have to touch it again. That would probably > be the preferred case. > > I'm undecided about the value that you write in there. That could either > be passed through DT as I suggested above for extra flexibility, or you > can keep it hardcoded if you are absolutely sure that there will never > need to be a case where you have to set it to something other than '1', It is only a mux between two IP's. Writing 1 is fine. For ACPI, it can be missing and handled by the firmware or can be handled just like the DTS way. > >> > > + /* >> > > + * Can't use devm_ioremap_resource due to overlapping region. >> > > + * 0xYYYY.0000 - host core >> > > + * 0xYYYY.7000 - Mux (if applicable) >> > > + * 0xYYYY.A000 - PHY indirect access >> > > + * 0xYYYY.C000 - Clock >> > > + * 0xYYYY.D000 - RAM shutdown removal >> > > + * As we map the entire region as one, it overlaps with the PHY driver. >> > > + */ >> > > + ctx->csr_base = devm_ioremap(dev, res->start, resource_size(res)); >> > > + if (!ctx->csr_base) { >> > > + dev_err(dev, "can't map %pR\n", res); >> > > + return -ENOMEM; >> > > + } >> > >> > I still think we should try not to have overlapping memory areas here. >> > Could you split up the registers into another range in the reg property >> > to leave the PHY registers out? >> >> Let me do this: >> >> 1. One resource for the RAM shutdown >> 2. One resource for the host controller >> 3. One optional resource for the MUX if needed. >> >> With #3, this also solved the MUX detection and avoid having another >> node "apm,ahci-mux". > > My suggestion was not to have another device node, just a property that > contains a value to be written into the mux register if present. > >> > Also, please read the thread named "[PATCH 0/7] of: setup dma >> > parameters using dma-ranges and dma-coherent" and comment there >> > about what you think we should do here. I assume we will have to >> > add "dma-ranges" properties in a number of places to get this >> > all to work fine any 64-bit SoC. Do you know if you have any >> > DMA master devices in x-gene that are not 64-bit capable? >> > >> >> I will read and follow later. We do have non-64-bit devices such as >> SDIO and SPI. But these IP's have translation in the bus similar to >> PCIe PIM. > > Is there also an IOMMU, or just the PIM? If there is no IOMMU, > we will actually have to use the swiotlb code here, otherwise we > can rely on the IOMMU to do the translation so we can pretend that > there is always 64-bit DMA capability. > > In theory, the PIM could be dynamically reprogrammed for each > DMA, as an extremely primitive IOMMU implementation, but I don't > know what the impact on performance would be from doing that. > It would also prevent you from starting multiple concurrent DMAs, > although I suspect that is not an important case for SPI and SDIO. This first generation don't support IOMMU and rely on swiotlb. In any case, for SDIO, the solution is to reprogram the PIM and limit to SDMA - single buffer - for each operation. -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 Friday 14 March 2014, Loc Ho wrote: > Hi, > > >> > gets called. Can you clarify what this register access does? > >> > If it's just setting a index into a mux output, would it make > >> > sense to have an optional DT property containing an integer with > >> > the mux setting you want to set? That way you wouldn't even > >> > have to have two compatible strings but just do > >> > > >> > ret = of_property_read_u32(node, "apm,ahci-mux", &mux); > >> > if (!ret) > >> > xgene_ahci_mux_select(ctx, mux); > >> > > >> > >> Given that fact that I will break up the resource. Let's just use the > >> resource for the MUX to handle this. For the IP that doesn't existed, > >> I will just not list it. > > > > Ah, that sounds good. It also means that if the firmware has set up > > the mux already, you don't have to touch it again. That would probably > > be the preferred case. > > > > I'm undecided about the value that you write in there. That could either > > be passed through DT as I suggested above for extra flexibility, or you > > can keep it hardcoded if you are absolutely sure that there will never > > need to be a case where you have to set it to something other than '1', > > It is only a mux between two IP's. Writing 1 is fine. For ACPI, it can > be missing and handled by the firmware or can be handled just like the > DTS way. Why can't you have it handled by the firmware in the DTS case? I still think it's rather unlikely that we will actually see ACPI support on your platform, btw. I'm willing to look at the patches you need for it, but I'm not very optimistic, in particular because of the kind of hacks you need for random bits of hardware. > > Is there also an IOMMU, or just the PIM? If there is no IOMMU, > > we will actually have to use the swiotlb code here, otherwise we > > can rely on the IOMMU to do the translation so we can pretend that > > there is always 64-bit DMA capability. > > > > In theory, the PIM could be dynamically reprogrammed for each > > DMA, as an extremely primitive IOMMU implementation, but I don't > > know what the impact on performance would be from doing that. > > It would also prevent you from starting multiple concurrent DMAs, > > although I suspect that is not an important case for SPI and SDIO. > > This first generation don't support IOMMU and rely on swiotlb. In any > case, for SDIO, the solution is to reprogram the PIM and limit to SDMA > - single buffer - for each operation. Ok, interesting. In that case, I suppose the DMA mask is actually 64-bit. 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 >> >> > gets called. Can you clarify what this register access does? >> >> > If it's just setting a index into a mux output, would it make >> >> > sense to have an optional DT property containing an integer with >> >> > the mux setting you want to set? That way you wouldn't even >> >> > have to have two compatible strings but just do >> >> > >> >> > ret = of_property_read_u32(node, "apm,ahci-mux", &mux); >> >> > if (!ret) >> >> > xgene_ahci_mux_select(ctx, mux); >> >> > >> >> >> >> Given that fact that I will break up the resource. Let's just use the >> >> resource for the MUX to handle this. For the IP that doesn't existed, >> >> I will just not list it. >> > >> > Ah, that sounds good. It also means that if the firmware has set up >> > the mux already, you don't have to touch it again. That would probably >> > be the preferred case. >> > >> > I'm undecided about the value that you write in there. That could either >> > be passed through DT as I suggested above for extra flexibility, or you >> > can keep it hardcoded if you are absolutely sure that there will never >> > need to be a case where you have to set it to something other than '1', >> >> It is only a mux between two IP's. Writing 1 is fine. For ACPI, it can >> be missing and handled by the firmware or can be handled just like the >> DTS way. > > Why can't you have it handled by the firmware in the DTS case? > > I still think it's rather unlikely that we will actually see ACPI support > on your platform, btw. > > I'm willing to look at the patches you need for it, but I'm not very > optimistic, in particular because of the kind of hacks you need > for random bits of hardware. > I am about to post another version. Can you look at the next version? If you still think it needs to be moved to the FW (either U-Boot or Tianocore UEFI boot loader), I will then move out of the Linux driver code. -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 Friday 14 March 2014, Loc Ho wrote: > > I still think it's rather unlikely that we will actually see ACPI support > > on your platform, btw. > > > > I'm willing to look at the patches you need for it, but I'm not very > > optimistic, in particular because of the kind of hacks you need > > for random bits of hardware. > > > > I am about to post another version. Can you look at the next version? > If you still think it needs to be moved to the FW (either U-Boot or > Tianocore UEFI boot loader), I will then move out of the Linux driver > code. I would certainly be happier if it could just be moved into the firmware or boot loader all the time. The only times we actually want to do this stuff in the kernel is when we can't trust the boot loader to get it right, or when the kernel has to support alternative configurations that the boot loader is not aware of, e.g. when you can plug in different kinds of extension boards but the boot loader doesn't know which one you have. 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
diff --git a/Documentation/devicetree/bindings/ata/apm-xgene.txt b/Documentation/devicetree/bindings/ata/apm-xgene.txt new file mode 100644 index 0000000..633eb3b --- /dev/null +++ b/Documentation/devicetree/bindings/ata/apm-xgene.txt @@ -0,0 +1,70 @@ +* 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-sgmii" if mux'ed with SGMII + * "apm,xgene-ahci-pcie" if mux'ed with PCIe +- reg : First memory resource shall be the AHCI memory + resource. + Second memory resource shall be the host controller + memory resource. +- 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-6g" for the SATA 6.0Gbps PHY + +Optional properties: +- status : Shall be "ok" if enabled or "disabled" if disabled. + Default is "ok". +- interrupt-parent : Interrupt controller. + +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>, + <0x0 0x1f22c000 0x0 0x100>; + #phy-cells = <1>; + }; + + phy3: phy@1f23a000 { + compatible = "apm,xgene-phy"; + reg = <0x0 0x1f23a000 0x0 0x100>, + <0x0 0x1f23c000 0x0 0x100>; + #phy-cells = <1>; + }; + + sata2: sata@1a400000 { + compatible = "apm,xgene-ahci-sgmii"; + reg = <0x0 0x1a400000 0x0 0x1000>, + <0x0 0x1f220000 0x0 0x10000>; + interrupt-parent = <&gic>; + interrupts = <0x0 0x87 0x4>; + status = "ok"; + clocks = <&sataclk 0>; + phys = <&phy2 0>; + phy-names = "sata-6g"; + }; + + sata3: sata@1a800000 { + compatible = "apm,xgene-ahci-pcie"; + reg = <0x0 0x1a800000 0x0 0x1000>, + <0x0 0x1f230000 0x0 0x10000>; + interrupt-parent = <&gic>; + interrupts = <0x0 0x88 0x4>; + status = "ok"; + clocks = <&sataclk 0>; + phys = <&phy3 0>; + phy-names = "sata-6g"; + };