From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Rutland Subject: Re: [PATCH] SATA: OCTEON: support SATA on OCTEON platform Date: Wed, 21 Jan 2015 16:54:27 +0000 Message-ID: <20150121165427.GA8722@leverpostej> References: <1421681040-3392-1-git-send-email-aleksey.makarov@auriga.com> <20150119154357.GH21553@leverpostej> <54BD580C.6030701@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <54BD580C.6030701-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: David Daney Cc: Aleksey Makarov , "linux-ide-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-mips-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , David Daney , Anton Vorontsov , Vinita Gupta , Rob Herring , Pawel Moll , Ian Campbell , Kumar Gala , Ralf Baechle , Tejun Heo , Hans de Goede , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: linux-ide@vger.kernel.org On Mon, Jan 19, 2015 at 07:16:28PM +0000, David Daney wrote: > On 01/19/2015 07:43 AM, Mark Rutland wrote: > > 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-tx70/tkNEWbQT0dZR+AlfA@public.gmane.org: 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? > > Well, UCTL is the internal name of the hardware block. It functions to > connect a standard AHCI controller to the internal busses of the OCTEON SoC. > > > > >> + > >> +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? > > They can. It happens to be the case that it is always empty. Then why does the binding mandate those particular values? Whether or not it currently happens to be the case is independent from the contract that the binding defines. Why does it not say something like: #address-cells, #size-cells, and ranges must be present and hold suitable values to map all child nodes. [...] > >> 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. > > > > Good idea. > > We will attempt to factor this into a separate driver module for the > "cavium,octeon-7130-sata-uctl" block. If that turns out to be too ugly, > I would like to keep the code in this file, but with the change you suggest. > > >> 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? > > Because it is an "existing DTB", The device tree doesn't contain the > compatible property of "generic-ahci", only "cavium,octeon-7130-ahci". While the DTB may already exist, the string "cavium,octeon-7130-ahci" isn't in mainline, and as far as I can see has never been supported. We _maintain_ support for existing DTBs, we don't just copy what some forked kernel happens to do. Trying to push that under the "don't break existing DTBs" rule is bending the definition. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754692AbbAUQzG (ORCPT ); Wed, 21 Jan 2015 11:55:06 -0500 Received: from foss-mx-na.foss.arm.com ([217.140.108.86]:40287 "EHLO foss-mx-na.foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754639AbbAUQyw (ORCPT ); Wed, 21 Jan 2015 11:54:52 -0500 Date: Wed, 21 Jan 2015 16:54:27 +0000 From: Mark Rutland To: David Daney Cc: Aleksey Makarov , "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" Subject: Re: [PATCH] SATA: OCTEON: support SATA on OCTEON platform Message-ID: <20150121165427.GA8722@leverpostej> References: <1421681040-3392-1-git-send-email-aleksey.makarov@auriga.com> <20150119154357.GH21553@leverpostej> <54BD580C.6030701@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <54BD580C.6030701@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 19, 2015 at 07:16:28PM +0000, David Daney wrote: > On 01/19/2015 07:43 AM, Mark Rutland wrote: > > 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? > > Well, UCTL is the internal name of the hardware block. It functions to > connect a standard AHCI controller to the internal busses of the OCTEON SoC. > > > > >> + > >> +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? > > They can. It happens to be the case that it is always empty. Then why does the binding mandate those particular values? Whether or not it currently happens to be the case is independent from the contract that the binding defines. Why does it not say something like: #address-cells, #size-cells, and ranges must be present and hold suitable values to map all child nodes. [...] > >> 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. > > > > Good idea. > > We will attempt to factor this into a separate driver module for the > "cavium,octeon-7130-sata-uctl" block. If that turns out to be too ugly, > I would like to keep the code in this file, but with the change you suggest. > > >> 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? > > Because it is an "existing DTB", The device tree doesn't contain the > compatible property of "generic-ahci", only "cavium,octeon-7130-ahci". While the DTB may already exist, the string "cavium,octeon-7130-ahci" isn't in mainline, and as far as I can see has never been supported. We _maintain_ support for existing DTBs, we don't just copy what some forked kernel happens to do. Trying to push that under the "don't break existing DTBs" rule is bending the definition. Thanks, Mark.