[v2,4/5] ahci: mediatek: add ahci driver

From: Frank Wunderlich <frank-w@public-files.de>

add AHCI driver ported from linux

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/ahci_mtk.c

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 drivers/ata/Kconfig    |   8 ++
 drivers/ata/Makefile   |   1 +
 drivers/ata/mtk_ahci.c | 196 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 205 insertions(+)
 create mode 100644 drivers/ata/mtk_ahci.c

On Mon, 2020-08-10 at 19:52 +0200, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> add AHCI driver ported from linux
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/ahci_mtk.c
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  drivers/ata/Kconfig    |   8 ++
>  drivers/ata/Makefile   |   1 +
>  drivers/ata/mtk_ahci.c | 196 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 205 insertions(+)
>  create mode 100644 drivers/ata/mtk_ahci.c
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index d8c9756c2a..f2f8275aec 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -130,4 +130,12 @@ config AHCI_MVEBU
>  	  onboard AHCI SATA.
>  
>  	  If unsure, say N.
> +
> +config MTK_AHCI
> +	bool "Enable Mediatek AHCI driver support"
> +	depends on AHCI
> +	help
> +	  Enable this driver to support Sata devices through
> +	  Mediatek AHCI controller (e.g. MT7622).
> +
>  endmenu
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index a69edb10f7..98fb480700 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -19,3 +19,4 @@ obj-$(CONFIG_SATA_SIL) += sata_sil.o
>  obj-$(CONFIG_SANDBOX) += sata_sandbox.o
>  obj-$(CONFIG_AHCI_MVEBU) += ahci_mvebu.o
>  obj-$(CONFIG_SUNXI_AHCI) += ahci_sunxi.o
> +obj-$(CONFIG_MTK_AHCI) += mtk_ahci.o
> diff --git a/drivers/ata/mtk_ahci.c b/drivers/ata/mtk_ahci.c
> new file mode 100644
> index 0000000000..51e9890e1c
> --- /dev/null
> +++ b/drivers/ata/mtk_ahci.c
> @@ -0,0 +1,196 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * MTK SATA platform driver
> + *
> + * (C) Copyright 2020
> + *     Mediatek
> + *
> + * Author: Frank Wunderlich <frank-w@public-files.de>
> + * based on https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/ahci_mtk.c
> + * Author: Ryder Lee <ryder.lee@mediatek.com>
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <ahci.h>
> +#include <scsi.h>
> +#include <sata.h>
> +#include <reset.h>
> +#include <asm/io.h>
> +#include <generic-phy.h>
> +#include <dm/of_access.h>
> +#include <syscon.h>
> +#include <linux/err.h>
> +#include <regmap.h>

alphabetic sort?

> +
> +#define SYS_CFG			0x14
> +#define SYS_CFG_SATA_MSK	GENMASK(31, 30)
> +#define SYS_CFG_SATA_EN		BIT(31)
> +
> +struct mtk_ahci_priv {
> +	void *base;
> +
> +	struct regmap *mode;
> +	struct reset_ctl axi_rst;
> +	struct reset_ctl sw_rst;
> +	struct reset_ctl reg_rst;
How about using reset_ctl_bulk?

> +};
> +
> +static int mtk_ahci_bind(struct udevice *dev)
> +{
> +	struct udevice *scsi_dev;
> +
> +	return ahci_bind_scsi(dev, &scsi_dev);
> +}
> +
> +static int mtk_ahci_ofdata_to_platdata(struct udevice *dev)
> +{
> +	struct mtk_ahci_priv *priv = dev_get_priv(dev);
> +
> +	priv->base = map_physmem(devfdt_get_addr(dev), sizeof(void *),
> +				 MAP_NOCACHE);
use devfdt_remap_addr_index/name instead?
> +
> +	return 0;
> +}
> +
> +static int mtk_ahci_platform_resets(struct udevice *dev)
> +{
> +	struct mtk_ahci_priv *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	ret = reset_get_by_name(dev, "axi", &priv->axi_rst);
> +	if (ret) {
> +		pr_err("unable to find reset controller 'axi'\n");
> +		return ret;
> +	}
> +
> +	ret = reset_get_by_name(dev, "sw", &priv->sw_rst);
> +	if (ret) {
> +		pr_err("unable to find reset controller 'sw'\n");
> +		return ret;
> +	}
> +
> +	ret = reset_get_by_name(dev, "reg", &priv->reg_rst);
> +	if (ret) {
> +		pr_err("unable to find reset controller 'reg'\n");
> +		return ret;
> +	}
> +
> +	ret = reset_assert(&priv->axi_rst);
> +	if (ret) {
> +		pr_err("unable to assert reset controller 'axi'\n");
> +		return ret;
> +	}
> +
> +	ret = reset_assert(&priv->sw_rst);
> +	if (ret) {
> +		pr_err("unable to assert reset controller 'sw'\n");
> +		return ret;
> +	}
> +
> +	ret = reset_assert(&priv->reg_rst);
> +	if (ret) {
> +		pr_err("unable to assert reset controller 'reg'\n");
> +		return ret;
> +	}
> +
> +	ret = reset_deassert(&priv->axi_rst);
> +	if (ret) {
> +		pr_err("unable to deassert reset controller 'axi'\n");
> +		return ret;
> +	}
> +
> +	ret = reset_deassert(&priv->sw_rst);
> +	if (ret) {
> +		pr_err("unable to deassert reset controller 'sw'\n");
> +		return ret;
> +	}
> +
> +	ret = reset_deassert(&priv->reg_rst);
> +	if (ret) {
> +		pr_err("unable to deassert reset controller 'reg'\n");
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +static int mtk_ahci_parse_property(struct ahci_uc_priv *hpriv,
> +				   struct udevice *dev)
> +{
> +	struct mtk_ahci_priv *plat = dev_get_priv(dev);
> +	const void *fdt = gd->fdt_blob;
> +
> +	/* enable SATA function if needed */
> +	if (fdt_get_property(fdt, dev_of_offset(dev),
> +			     "mediatek,phy-mode", NULL)) {
> +		plat->mode = syscon_regmap_lookup_by_phandle(dev,
> +						"mediatek,phy-mode");
> +		if (IS_ERR(plat->mode)) {
> +			dev_err(dev, "missing phy-mode phandle\n");
> +			return PTR_ERR(plat->mode);
> +		}
> +		regmap_update_bits(plat->mode, SYS_CFG,
> +				   SYS_CFG_SATA_MSK, SYS_CFG_SATA_EN);
> +	}
> +
> +	ofnode_read_u32(dev->node, "ports-implemented", &hpriv->port_map);
> +	return 0;
> +}
> +
> +static int mtk_ahci_probe(struct udevice *dev)
> +{
> +	struct mtk_ahci_priv *priv = dev_get_priv(dev);
> +	int ret;
> +	struct phy phy;
> +	struct ahci_uc_priv *hpriv;
> +
> +	hpriv = malloc(sizeof(struct ahci_uc_priv));
> +	if (!hpriv)
> +		return -ENOMEM;
How about put an ahci_uc_priv struct instance in mtk_ahci_priv struct,
then no need malloc it here?

> +
> +	memset(hpriv, 0, sizeof(struct ahci_uc_priv));
> +
> +	ret = mtk_ahci_parse_property(hpriv, dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = mtk_ahci_platform_resets(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = generic_phy_get_by_name(dev, "sata-phy", &phy);
> +	if (ret) {
> +		pr_err("can't get the phy from DT\n");
> +		return ret;
> +	}
> +
> +	ret = generic_phy_init(&phy);
> +	if (ret) {
> +		pr_err("unable to initialize the sata phy\n");
> +		return ret;
> +	}
> +
> +	ret = generic_phy_power_on(&phy);
> +	if (ret) {
> +		pr_err("unable to power on the sata phy\n");
> +		return ret;
> +	}
> +
> +	return ahci_probe_scsi(dev, (ulong)priv->base);
> +}
> +
> +static const struct udevice_id mtk_ahci_ids[] = {
> +	{ .compatible = "mediatek,mtk-ahci" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(mtk_ahci) = {
> +	.name	= "mtk_ahci",
> +	.id	= UCLASS_AHCI,
> +	.of_match = mtk_ahci_ids,
> +	.bind	= mtk_ahci_bind,
> +	.ofdata_to_platdata = mtk_ahci_ofdata_to_platdata,
> +	.ops	= &scsi_ops,
> +	.probe	= mtk_ahci_probe,
> +	.priv_auto_alloc_size = sizeof(struct mtk_ahci_priv),
> +};

Hi,

thanks for first review

> Gesendet: Donnerstag, 13. August 2020 um 07:52 Uhr
> Von: "Chunfeng Yun" <chunfeng.yun@mediatek.com>
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <ahci.h>
> > +#include <scsi.h>
> > +#include <sata.h>
> > +#include <reset.h>
> > +#include <asm/io.h>
> > +#include <generic-phy.h>
> > +#include <dm/of_access.h>
> > +#include <syscon.h>
> > +#include <linux/err.h>
> > +#include <regmap.h>
>
> alphabetic sort?

at least common.h neds to be first ;) but then it looks like this:

#include <common.h>
#include <ahci.h>
#include <asm/io.h>
#include <dm.h>
#include <dm/of_access.h>
#include <generic-phy.h>
#include <linux/err.h>
#include <regmap.h>
#include <reset.h>
#include <sata.h>
#include <scsi.h>
#include <syscon.h>

> > +struct mtk_ahci_priv {
> > +	void *base;
> > +
> > +	struct regmap *mode;
> > +	struct reset_ctl axi_rst;
> > +	struct reset_ctl sw_rst;
> > +	struct reset_ctl reg_rst;
> How about using reset_ctl_bulk?

this will drop mtk_ahci_platform_resets and add this in probe, right?

struct reset_ctl_bulk rst_bulk;

    ret = reset_get_bulk(dev, &rst_bulk);
    if (!ret) {
        reset_assert_bulk(&rst_bulk);
        reset_deassert_bulk(&rst_bulk);
    } else
        dev_err(dev, "Failed to get reset: %d\n", ret);


> > +	priv->base = map_physmem(devfdt_get_addr(dev), sizeof(void *),
> > +				 MAP_NOCACHE);
> use devfdt_remap_addr_index/name instead?

have to look how these work/other drivers use this...

> > +	hpriv = malloc(sizeof(struct ahci_uc_priv));
> > +	if (!hpriv)
> > +		return -ENOMEM;
> How about put an ahci_uc_priv struct instance in mtk_ahci_priv struct,
> then no need malloc it here?

ok, i test with this :)
can memset also dropped?

On Thu, 2020-08-13 at 09:33 +0200, Frank Wunderlich wrote:
> Hi,
> 
> thanks for first review
> 
> > Gesendet: Donnerstag, 13. August 2020 um 07:52 Uhr
> > Von: "Chunfeng Yun" <chunfeng.yun@mediatek.com>
> > > +#include <common.h>
> > > +#include <dm.h>
> > > +#include <ahci.h>
> > > +#include <scsi.h>
> > > +#include <sata.h>
> > > +#include <reset.h>
> > > +#include <asm/io.h>
> > > +#include <generic-phy.h>
> > > +#include <dm/of_access.h>
> > > +#include <syscon.h>
> > > +#include <linux/err.h>
> > > +#include <regmap.h>
> >
> > alphabetic sort?
> 
> at least common.h neds to be first ;) but then it looks like this:
> 
> #include <common.h>
> #include <ahci.h>
> #include <asm/io.h>
> #include <dm.h>
> #include <dm/of_access.h>
> #include <generic-phy.h>
> #include <linux/err.h>
> #include <regmap.h>
> #include <reset.h>
> #include <sata.h>
> #include <scsi.h>
> #include <syscon.h>
> 
> > > +struct mtk_ahci_priv {
> > > +	void *base;
> > > +
> > > +	struct regmap *mode;
> > > +	struct reset_ctl axi_rst;
> > > +	struct reset_ctl sw_rst;
> > > +	struct reset_ctl reg_rst;
> > How about using reset_ctl_bulk?
> 
> this will drop mtk_ahci_platform_resets and add this in probe, right?
Both are ok for me

> 
> struct reset_ctl_bulk rst_bulk;
> 
>     ret = reset_get_bulk(dev, &rst_bulk);
>     if (!ret) {
>         reset_assert_bulk(&rst_bulk);
>         reset_deassert_bulk(&rst_bulk);
>     } else
>         dev_err(dev, "Failed to get reset: %d\n", ret);
add {} for dev_err

> 
> 
> > > +	priv->base = map_physmem(devfdt_get_addr(dev), sizeof(void *),
> > > +				 MAP_NOCACHE);
> > use devfdt_remap_addr_index/name instead?
> 
> have to look how these work/other drivers use this...
> 
> > > +	hpriv = malloc(sizeof(struct ahci_uc_priv));
> > > +	if (!hpriv)
> > > +		return -ENOMEM;
> > How about put an ahci_uc_priv struct instance in mtk_ahci_priv struct,
> > then no need malloc it here?
> 
> ok, i test with this :)
> can memset also dropped?
Yes, device core will set it to zero in alloc_priv()