From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Rutland Subject: Re: [PATCH] SATA: OCTEON: support SATA on OCTEON platform Date: Mon, 19 Jan 2015 15:43:57 +0000 Message-ID: <20150119154357.GH21553@leverpostej> References: <1421681040-3392-1-git-send-email-aleksey.makarov@auriga.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from foss-mx-na.foss.arm.com ([217.140.108.86]:38130 "EHLO foss-mx-na.foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750880AbbASPoV (ORCPT ); Mon, 19 Jan 2015 10:44:21 -0500 Content-Disposition: inline In-Reply-To: <1421681040-3392-1-git-send-email-aleksey.makarov@auriga.com> Content-Language: en-US Sender: linux-ide-owner@vger.kernel.org List-Id: linux-ide@vger.kernel.org To: Aleksey Makarov Cc: "linux-ide@vger.kernel.org" , "linux-mips@linux-mips.org" , "linux-kernel@vger.kernel.org" , David Daney , Anton Vorontsov , Vinita Gupta , Rob Herring , Pawel Moll , Ian Campbell , Kumar Gala , Ralf Baechle , Tejun Heo , Hans de Goede , "devicetree@vger.kernel.org" On Mon, Jan 19, 2015 at 03:23:58PM +0000, Aleksey Makarov wrote: > The OCTEON SATA controller is currently found on cn71XX devices. > > Signed-off-by: David Daney > Signed-off-by: Vinita Gupta > [aleksey.makarov@auriga.com: preparing for submission, > conflict resolution, fixes for the platform code] > Signed-off-by: Aleksey Makarov > --- > .../devicetree/bindings/ata/ahci-platform.txt | 1 + > .../devicetree/bindings/mips/cavium/sata-uctl.txt | 31 ++++++ > arch/mips/cavium-octeon/octeon-platform.c | 1 + > drivers/ata/Kconfig | 9 ++ > drivers/ata/Makefile | 1 + > drivers/ata/ahci_platform.c | 10 ++ > drivers/ata/sata_octeon.c | 107 +++++++++++++++++++++ > 7 files changed, 160 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mips/cavium/sata-uctl.txt > create mode 100644 drivers/ata/sata_octeon.c > > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt > index 4ab09f2..1a5d3be 100644 > --- a/Documentation/devicetree/bindings/ata/ahci-platform.txt > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt > @@ -11,6 +11,7 @@ Required properties: > - compatible : compatible string, one of: > - "allwinner,sun4i-a10-ahci" > - "hisilicon,hisi-ahci" > + - "cavium,octeon-7130-ahci" > - "ibm,476gtr-ahci" > - "marvell,armada-380-ahci" > - "snps,dwc-ahci" > diff --git a/Documentation/devicetree/bindings/mips/cavium/sata-uctl.txt b/Documentation/devicetree/bindings/mips/cavium/sata-uctl.txt > new file mode 100644 > index 0000000..222e66e > --- /dev/null > +++ b/Documentation/devicetree/bindings/mips/cavium/sata-uctl.txt > @@ -0,0 +1,31 @@ > +* UCTL SATA controller glue I'm not sure I follow. What does this mean? > + > +Properties: > +- compatible: "cavium,octeon-7130-sata-uctl" > + > + Compatibility with the cn7130 SOC. > + > +- reg: The base address of the UCTL register bank. > + > +- #address-cells: Must be <2>. > + > +- #size-cells: Must be <2>. > + > +- ranges: Empty to signify direct mapping of the children. Why can't these be any values which are sufficient to map children? > + > +Example: > + > + uctl@118006c000000 { > + compatible = "cavium,octeon-7130-sata-uctl"; > + reg = <0x00011800 0x6c000000 0x00000000 0x00000100>; > + ranges; > + #address-cells = <0x00000002>; > + #size-cells = <0x00000002>; No need for all the zero-padding on these, they aren't addresses. > + sata@16c00 { > + compatible = "cavium,octeon-7130-ahci"; > + reg = <0x00016c00 0x00000000 0x00000000 0x00000200>; > + interrupt-parent = <0x0000000d>; > + interrupts = <0x00000002 0x00000004>; > + cavium,qlm-trim-alias = "sata"; What's this property for? It wasn't documented above, and doesn't exist in mainline. [...] > diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile > index ae41107..4a0e5e3 100644 > --- a/drivers/ata/Makefile > +++ b/drivers/ata/Makefile > @@ -17,6 +17,7 @@ obj-$(CONFIG_AHCI_SUNXI) += ahci_sunxi.o libahci.o libahci_platform.o > obj-$(CONFIG_AHCI_ST) += ahci_st.o libahci.o libahci_platform.o > obj-$(CONFIG_AHCI_TEGRA) += ahci_tegra.o libahci.o libahci_platform.o > obj-$(CONFIG_AHCI_XGENE) += ahci_xgene.o libahci.o libahci_platform.o > +obj-$(CONFIG_SATA_OCTEON) += sata_octeon.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 18d5398..bb36396 100644 > --- a/drivers/ata/ahci_platform.c > +++ b/drivers/ata/ahci_platform.c > @@ -22,6 +22,12 @@ > #include > #include "ahci.h" > > +#if IS_ENABLED(CONFIG_SATA_OCTEON) > +void ahci_octeon_config(struct platform_device *pdev); > +#else > +static inline void ahci_octeon_config(struct platform_device *pdev) {} > +#endif > + > static const struct ata_port_info ahci_port_info = { > .flags = AHCI_FLAG_COMMON, > .pio_mask = ATA_PIO4, > @@ -46,6 +52,9 @@ static int ahci_probe(struct platform_device *pdev) > if (of_device_is_compatible(dev->of_node, "hisilicon,hisi-ahci")) > hpriv->flags |= AHCI_HFLAG_NO_FBS | AHCI_HFLAG_NO_NCQ; > > + if (of_device_is_compatible(dev->of_node, "cavium,octeon-7130-ahci")) > + ahci_octeon_config(pdev); > + If we really need this kind of thing, make a new struct and associate it with of_device_id::data in the table below. Then we make this path free from any device-specific code. > rc = ahci_platform_init_host(pdev, hpriv, &ahci_port_info); > if (rc) > goto disable_resources; > @@ -67,6 +76,7 @@ static const struct of_device_id ahci_of_match[] = { > { .compatible = "ibm,476gtr-ahci", }, > { .compatible = "snps,dwc-ahci", }, > { .compatible = "hisilicon,hisi-ahci", }, > + { .compatible = "cavium,octeon-7130-ahci", }, > {}, I was under the impression that the strings other than "generic-ahci" were only for compatibility with existing DTBs. Why do we need to add new platform-specific strings here? [...] > +void ahci_octeon_config(struct platform_device *pdev) > +{ > + union cvmx_sata_uctl_shim_cfg shim_cfg; > + > + /* set-up endian mode */ > + shim_cfg.u64 = cvmx_read_csr(CVMX_SATA_UCTL_SHIM_CFG); > +#ifdef __BIG_ENDIAN > + shim_cfg.s.dma_endian_mode = 1; > + shim_cfg.s.csr_endian_mode = 1; > +#else > + shim_cfg.s.dma_endian_mode = 0; > + shim_cfg.s.csr_endian_mode = 0; > +#endif > + shim_cfg.s.dma_read_cmd = 1; /* No allocate L2C */ > + cvmx_write_csr(CVMX_SATA_UCTL_SHIM_CFG, shim_cfg.u64); > + > + /* Set a good dma_mask */ > + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(64); > + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; I thought a dma-ranges property in the DT could be used to set up the DMA mask appropriately? Thanks, Mark.