Hi,
> -----Original Message-----
> From: Hans de Goede [mailto:hdego...@redhat.com]
> Sent: Wednesday, September 02, 2015 4:32 PM
> To: Tang Yuantian-B29983 <yuantian.t...@freescale.com>
> Cc: t...@kernel.org; linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] ahci: added a new driver for supporting Freescale AHCI
> sata
>
> Hi,
>
> On 02-09-15 04:25, yuantian.t...@freescale.com wrote:
> > From: Tang Yuantian <yuantian.t...@freescale.com>
> >
> > Currently Freescale QorIQ series SATA is supported by ahci_platform
> > driver. Some SoC specific settings have been put in uboot. So whether
> > SATA works or not heavily depends on uboot.
> > This patch will add a new driver to support QorIQ sata which removes
> > the dependency on any other boot loader.
> > Freescale QorIQ series sata, like ls1021a ls2085a ls1043a, is
> > compatible with serial ATA 3.0 and AHCI 1.3 specification.
> >
> > Signed-off-by: Yuantian Tang <yuantian.t...@freescale.com>
>
> Thanks for the patch looks good overall.
>
> You need to add a Documentation/devicetree/bindings/ata/ahci-fsl-qoriq.txt
> (or a similar named file) documenting the compatible strings and what the
> devicetree nodes should contain wrt reg, interrupts, etc.
> properties. See Documentation/devicetree/bindings/ata/ahci-platform.txt
> as an example.
>
> Further comments inline.
>
I was about to use ahci_platform driver, so I added the bindings stuff to
Documentation/devicetree/bindings/ata/ahci-platform.txt
So I need to revert the old bingings first and then add a new one.
> > ---
> > drivers/ata/Kconfig | 9 ++
> > drivers/ata/Makefile | 1 +
> > drivers/ata/ahci_platform.c | 1 -
> > drivers/ata/ahci_qoriq.c | 308
> ++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 318 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/ata/ahci_qoriq.c
> >
> > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig index
> > 15e40ee..6aaa3f8 100644
> > --- a/drivers/ata/Kconfig
> > +++ b/drivers/ata/Kconfig
> > @@ -175,6 +175,15 @@ config AHCI_XGENE
> > help
> > This option enables support for APM X-Gene SoC SATA host
> controller.
> >
> > +config AHCI_QORIQ
> > + tristate "Freescale QorIQ AHCI SATA support"
> > + depends on OF
> > + help
> > + This option enables support for the Freescale QorIQ AHCI 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
> > af70919..af45eff 100644
> > --- a/drivers/ata/Makefile
> > +++ b/drivers/ata/Makefile
> > @@ -19,6 +19,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_AHCI_QORIQ) += ahci_qoriq.o libahci.o
> libahci_platform.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 1befb11..04975b8 100644
> > --- a/drivers/ata/ahci_platform.c
> > +++ b/drivers/ata/ahci_platform.c
> > @@ -76,7 +76,6 @@ static const struct of_device_id ahci_of_match[] = {
> > { .compatible = "ibm,476gtr-ahci", },
> > { .compatible = "snps,dwc-ahci", },
> > { .compatible = "hisilicon,hisi-ahci", },
> > - { .compatible = "fsl,qoriq-ahci", },
> > {},
> > };
> > MODULE_DEVICE_TABLE(of, ahci_of_match);
>
> This will break booting new kernels with old dtb files, something which in
> general is considered a big non-no, I suggest adding a comment that this has
> been superseded by the new ahci_qoriq.c code, and maybe add a date to
> retire the compatible in say a year from now.
>
There is no old dtb because LS* platforms are not been upstreamed yet.
So I think we can safely replace it without breaking anything.
Regards,
Yuantian
> > diff --git a/drivers/ata/ahci_qoriq.c b/drivers/ata/ahci_qoriq.c new
> > file mode 100644 index 0000000..943b783
> > --- /dev/null
> > +++ b/drivers/ata/ahci_qoriq.c
> > @@ -0,0 +1,308 @@
> > +/*
> > + * Freescale QorIQ AHCI SATA platform driver
> > + *
> > + * Copyright 2015 Freescale, Inc.
> > + * Tang Yuantian <yuantian.t...@freescale.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License as published
> > +by
> > + * the Free Software Foundation; either version 2, or (at your
> > +option)
> > + * any later version.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pm.h>
> > +#include <linux/ahci_platform.h>
> > +#include <linux/device.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/libata.h>
> > +#include "ahci.h"
> > +
> > +#define DRV_NAME "ahci-qoriq"
> > +
> > +#define AHCI_PORT_PHY_1_CFG 0xa003fffe
> > +#define AHCI_PORT_PHY_2_CFG 0x28183411
> > +#define AHCI_PORT_PHY_3_CFG 0x0e081004
> > +#define AHCI_PORT_PHY_4_CFG 0x00480811
> > +#define AHCI_PORT_PHY_5_CFG 0x192c96a4
> > +#define AHCI_PORT_TRANS_CFG 0x08000025
> > +
> > +#define SATA_ECC_REG_ADDR 0x20220520
> > +#define SATA_ECC_DISABLE 0x00020000
> > +
> > +enum ahci_qoriq_type {
> > + AHCI_LS1021A,
> > + AHCI_LS1043A,
> > + AHCI_LS2085A,
> > +};
> > +
> > +struct ahci_qoriq_priv {
> > + struct ccsr_ahci *reg_base;
> > + enum ahci_qoriq_type type;
> > + void __iomem *ecc_addr;
> > +};
> > +
> > +/* AHCI (sata) register map */
> > +struct ccsr_ahci {
> > + u32 res1[0xa4/4]; /* 0x0 - 0xa4 */
> > + u32 pcfg; /* port config */
> > + u32 ppcfg; /* port phy1 config */
> > + u32 pp2c; /* port phy2 config */
> > + u32 pp3c; /* port phy3 config */
> > + u32 pp4c; /* port phy4 config */
> > + u32 pp5c; /* port phy5 config */
> > + u32 paxic; /* port AXI config */
> > + u32 axicc; /* AXI cache control */
> > + u32 axipc; /* AXI PROT control */
> > + u32 ptc; /* port Trans Config */
> > + u32 pts; /* port Trans Status */
> > + u32 plc; /* port link config */
> > + u32 plc1; /* port link config1 */
> > + u32 plc2; /* port link config2 */
> > + u32 pls; /* port link status */
> > + u32 pls1; /* port link status1 */
> > + u32 pcmdc; /* port CMD config */
> > + u32 ppcs; /* port phy control status */
> > + u32 pberr; /* port 0/1 BIST error */
> > + u32 cmds; /* port 0/1 CMD status error */
> > +};
> > +
> > +static const struct of_device_id ahci_qoriq_of_match[] = {
> > + { .compatible = "fsl,ls1021a-ahci", .data = (void *)AHCI_LS1021A},
> > + { .compatible = "fsl,ls1043a-ahci", .data = (void *)AHCI_LS1043A},
> > + { .compatible = "fsl,ls2085a-ahci", .data = (void *)AHCI_LS2085A},
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, ahci_qoriq_of_match);
> > +
> > +static int ahci_qoriq_hardreset(struct ata_link *link, unsigned int *class,
> > + unsigned long deadline)
> > +{
> > + const unsigned long *timing = sata_ehc_deb_timing(&link-
> >eh_context);
> > + void __iomem *port_mmio = ahci_port_base(link->ap);
> > + u32 px_cmd, px_is, px_val;
> > + struct ata_port *ap = link->ap;
> > + struct ahci_port_priv *pp = ap->private_data;
> > + struct ahci_host_priv *hpriv = ap->host->private_data;
> > + struct ahci_qoriq_priv *qoriq_priv = hpriv->plat_data;
> > + u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
> > + struct ata_taskfile tf;
> > + bool online;
> > + int rc;
> > +
> > + DPRINTK("ENTER\n");
> > +
> > + ahci_stop_engine(ap);
> > +
> > + /*
> > + * There is a errata on ls1021a Rev1.0 and Rev2.0 which is:
> > + * A-009042: The device detection initialization sequence
> > + * mistakenly resets some registers.
> > + *
> > + * Workaround for this is:
> > + * The software should read and store PxCMD and PxIS values
> > + * before issuing the device detection initialization sequence.
> > + * After the sequence is complete, software should restore the
> > + * PxCMD and PxIS with the stored values.
> > + */
> > + if (qoriq_priv->type == AHCI_LS1021A) {
> > + px_cmd = readl(port_mmio + PORT_CMD);
> > + px_is = readl(port_mmio + PORT_IRQ_STAT);
> > + }
> > +
> > + /* clear D2H reception area to properly wait for D2H FIS */
> > + ata_tf_init(link->device, &tf);
> > + tf.command = ATA_BUSY;
> > + ata_tf_to_fis(&tf, 0, 0, d2h_fis);
> > +
> > + rc = sata_link_hardreset(link, timing, deadline, &online,
> > + ahci_check_ready);
> > +
> > + /* restore the PxCMD and PxIS on ls1021 */
> > + if (qoriq_priv->type == AHCI_LS1021A) {
> > + px_val = readl(port_mmio + PORT_CMD);
> > + if (px_val != px_cmd)
> > + writel(px_cmd, port_mmio + PORT_CMD);
> > +
> > + px_val = readl(port_mmio + PORT_IRQ_STAT);
> > + if (px_val != px_is)
> > + writel(px_is, port_mmio + PORT_IRQ_STAT);
> > + }
> > +
> > + hpriv->start_engine(ap);
> > +
> > + if (online)
> > + *class = ahci_dev_classify(ap);
> > +
> > + DPRINTK("EXIT, rc=%d, class=%u\n", rc, *class);
> > + return rc;
> > +}
> > +
> > +static struct ata_port_operations ahci_qoriq_ops = {
> > + .inherits = &ahci_ops,
> > + .hardreset = ahci_qoriq_hardreset,
> > +};
> > +
> > +static const struct ata_port_info ahci_qoriq_port_info = {
> > + .flags = AHCI_FLAG_COMMON | ATA_FLAG_NCQ,
> > + .pio_mask = ATA_PIO4,
> > + .udma_mask = ATA_UDMA6,
> > + .port_ops = &ahci_qoriq_ops,
> > +};
> > +
> > +static struct scsi_host_template ahci_qoriq_sht = {
> > + AHCI_SHT(DRV_NAME),
> > +};
> > +
> > +static int ahci_qoriq_phy_init(struct ahci_qoriq_priv *qpriv) {
> > + struct ccsr_ahci *reg_base = qpriv->reg_base;
> > +
> > + switch (qpriv->type) {
> > + case AHCI_LS1021A:
> > + writel(SATA_ECC_DISABLE, qpriv->ecc_addr);
> > + writel(AHCI_PORT_PHY_1_CFG, ®_base->ppcfg);
> > + writel(AHCI_PORT_PHY_2_CFG, ®_base->pp2c);
> > + writel(AHCI_PORT_PHY_3_CFG, ®_base->pp3c);
> > + writel(AHCI_PORT_PHY_4_CFG, ®_base->pp4c);
> > + writel(AHCI_PORT_PHY_5_CFG, ®_base->pp5c);
> > + writel(AHCI_PORT_TRANS_CFG, ®_base->ptc);
> > + break;
> > +
> > + case AHCI_LS1043A:
> > + case AHCI_LS2085A:
> > + writel(AHCI_PORT_PHY_1_CFG, ®_base->ppcfg);
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ahci_qoriq_probe(struct platform_device *pdev) {
> > + struct device_node *np = pdev->dev.of_node;
> > + struct device *dev = &pdev->dev;
> > + struct ahci_host_priv *hpriv;
> > + struct ahci_qoriq_priv *qoriq_priv;
> > + const struct of_device_id *of_id;
> > + int rc;
> > +
> > + hpriv = ahci_platform_get_resources(pdev);
> > + if (IS_ERR(hpriv))
> > + return PTR_ERR(hpriv);
> > +
> > + rc = ahci_platform_enable_resources(hpriv);
> > + if (rc)
> > + return rc;
>
> You may want to move this down in the function to remove unnecessary
> goto disable_foo error handling, you can do this directly before the phy_init.
>
> > +
> > + of_id = of_match_node(ahci_qoriq_of_match, np);
> > + if (!of_id) {
> > + rc = -ENODEV;
> > + goto disable_resources;
> > + }
> > +
> > + qoriq_priv = devm_kzalloc(dev, sizeof(*qoriq_priv), GFP_KERNEL);
> > + if (!qoriq_priv) {
> > + rc = -ENOMEM;
> > + goto disable_resources;
> > + }
> > +
> > + qoriq_priv->reg_base = of_iomap(np, 0);
> > + if (!qoriq_priv->reg_base) {
> > + rc = -ENOMEM;
> > + goto free_qpriv;
> > + }
>
> You should use devm_ioremap_resource() to map registers so that
> resources get released automatically.
>
> In this case hower you already have access to there registers through
> hpriv->mmio, so there is no need to map them a second time.
>
> > +
> > + qoriq_priv->type = (enum ahci_qoriq_type)of_id->data;
> > +
> > + if (qoriq_priv->type == AHCI_LS1021A) {
> > + qoriq_priv->ecc_addr =
> > + ioremap((phys_addr_t)SATA_ECC_REG_ADDR, 4);
>
> In a devicetree enabled driver you should never hardcode register addresses
> like this, instead they should be specified in the reg property of the driver.
>
> You should put something like this in the dts:
>
> reg = <0x01c13400 0x10>, <0x01c14800 0x4>;
> reg-names = "ahci", "sata_ecc";
>
> And then in the code do:
>
> struct resource *res = platform_get_resource_byname(pdev,
> IORESOURCE_MEM, "sata_ecc");
> qoriq_priv->ecc_addr = devm_ioremap_resource(dev, res);
> if (IS_ERR(qoriq_priv->ecc_addr))
> return PTR_ERR(qoriq_priv->ecc_addr);
>
> Note the direct return. This is ok to do if you move enable_resources down
> as discussed below.
>
> > + if (!qoriq_priv->ecc_addr) {
> > + rc = -ENOMEM;
> > + goto err_iomap;
> > + }
> > + }
> > + hpriv->plat_data = qoriq_priv;
> > + rc = ahci_qoriq_phy_init(qoriq_priv);
> > + if (rc) {
> > + rc = -ENOMEM;
> > + goto err_iomap2;
> > + }
> > +
> > + rc = ahci_platform_init_host(pdev, hpriv, &ahci_qoriq_port_info,
> > + &ahci_qoriq_sht);
> > + if (rc)
> > + goto err_iomap2;
> > +
> > + return 0;
> > +
> > +err_iomap2:
> > + iounmap(qoriq_priv->ecc_addr);
> > +
> > +err_iomap:
> > + iounmap(qoriq_priv->reg_base);
> > +
> > +free_qpriv:
> > + devm_kfree(dev, qoriq_priv);
>
> All these 3 labels + code can go away since devm will do this automatically
> when you fail the probe method (assuming you use
> devm_ioremap_resource as discussed above).
>
> > +
> > +disable_resources:
> > + ahci_platform_disable_resources(hpriv);
> > +
> > + return rc;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int ahci_qoriq_resume(struct device *dev) {
> > + struct ata_host *host = dev_get_drvdata(dev);
> > + struct ahci_host_priv *hpriv = host->private_data;
> > + int rc;
> > +
> > + rc = ahci_platform_enable_resources(hpriv);
> > + if (rc)
> > + return rc;
> > +
> > + rc = ahci_qoriq_phy_init(hpriv->plat_data);
> > + if (rc)
> > + goto disable_resources;
> > +
> > + rc = ahci_platform_resume_host(dev);
> > + if (rc)
> > + goto disable_resources;
> > +
> > + /* We resumed so update PM runtime state */
> > + pm_runtime_disable(dev);
> > + pm_runtime_set_active(dev);
> > + pm_runtime_enable(dev);
> > +
> > + return 0;
> > +
> > +disable_resources:
> > + ahci_platform_disable_resources(hpriv);
> > +
> > + return rc;
> > +}
> > +#endif
> > +
> > +static SIMPLE_DEV_PM_OPS(ahci_qoriq_pm_ops,
> ahci_platform_suspend,
> > + ahci_qoriq_resume);
> > +
> > +static struct platform_driver ahci_qoriq_driver = {
> > + .probe = ahci_qoriq_probe,
> > + .remove = ata_platform_remove_one,
> > + .driver = {
> > + .name = DRV_NAME,
> > + .of_match_table = ahci_qoriq_of_match,
> > + .pm = &ahci_qoriq_pm_ops,
> > + },
> > +};
> > +module_platform_driver(ahci_qoriq_driver);
> > +
> > +MODULE_DESCRIPTION("Freescale QorIQ AHCI SATA platform driver");
> > +MODULE_AUTHOR("Tang Yuantian <yuantian.t...@freescale.com>");
> > +MODULE_LICENSE("GPL");
> >
>
>
> Regards,
>
> Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
