| Message ID | 1452676288-62711-5-git-send-email-jamesjj.liao@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 13/01/16 10:11, James Liao wrote: > From: Shunli Wang <shunli.wang@mediatek.com> > > Add scpsys driver for MT2701. > > Signed-off-by: Shunli Wang <shunli.wang@mediatek.com> > Signed-off-by: James Liao <jamesjj.liao@mediatek.com> > --- NAK, same as in v1. Please don't split up the driver in several code files, but find a a way to share the common code elegantly. Comments below. > drivers/soc/mediatek/Makefile | 1 + > drivers/soc/mediatek/mtk-scpsys-mt2701.c | 170 +++++++++++++++++++++++++++++++ > 2 files changed, 171 insertions(+) > create mode 100644 drivers/soc/mediatek/mtk-scpsys-mt2701.c > > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile > index 13f7d21..d693f7d 100644 > --- a/drivers/soc/mediatek/Makefile > +++ b/drivers/soc/mediatek/Makefile > @@ -2,3 +2,4 @@ obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o > obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o > obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys-mt8173.o > +obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys-mt2701.o > diff --git a/drivers/soc/mediatek/mtk-scpsys-mt2701.c b/drivers/soc/mediatek/mtk-scpsys-mt2701.c > new file mode 100644 > index 0000000..82c00ed > --- /dev/null > +++ b/drivers/soc/mediatek/mtk-scpsys-mt2701.c > @@ -0,0 +1,170 @@ > +/* > + * Copyright (c) 2015 Mediatek, Shunli Wang <shunli.wang@mediatek.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/pm_domain.h> > +#include <linux/soc/mediatek/infracfg.h> > +#include <dt-bindings/power/mt2701-power.h> > + > +#include "mtk-scpsys.h" > + > +#define SPM_VDE_PWR_CON 0x0210 > +#define SPM_MFG_PWR_CON 0x0214 > +#define SPM_ISP_PWR_CON 0x0238 > +#define SPM_DIS_PWR_CON 0x023C > +#define SPM_CONN_PWR_CON 0x0280 > +#define SPM_BDP_PWR_CON 0x029C > +#define SPM_ETH_PWR_CON 0x02A0 > +#define SPM_HIF_PWR_CON 0x02A4 > +#define SPM_IFR_MSC_PWR_CON 0x02A8 > +#define SPM_PWR_STATUS 0x060c > +#define SPM_PWR_STATUS_2ND 0x0610 > + > +#define CONN_PWR_STA_MASK BIT(1) > +#define DIS_PWR_STA_MASK BIT(3) > +#define MFG_PWR_STA_MASK BIT(4) > +#define ISP_PWR_STA_MASK BIT(5) > +#define VDE_PWR_STA_MASK BIT(7) > +#define BDP_PWR_STA_MASK BIT(14) > +#define ETH_PWR_STA_MASK BIT(15) > +#define HIF_PWR_STA_MASK BIT(16) > +#define IFR_MSC_PWR_STA_MASK BIT(17) Apart from SPM_BDP_PWR_CON and BDP_PWR_STA_MASK this are compatible with mt8173. So please avoid code duplication. Apart you changed the define names from PWR_STATUS_* to *_STA_MASK, please keep a uniform naming across the driver. Regards, Matthias > + > +#define MT2701_TOP_AXI_PROT_EN_CONN 0x0104 > +#define MT2701_TOP_AXI_PROT_EN_DISP 0x0002 > + > +static const struct scp_domain_data scp_domain_data[] = { > + [MT2701_POWER_DOMAIN_CONN] = { > + .name = "conn", > + .sta_mask = CONN_PWR_STA_MASK, > + .ctl_offs = SPM_CONN_PWR_CON, > + .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_CONN, > + .active_wakeup = true, > + }, > + [MT2701_POWER_DOMAIN_DISP] = { > + .name = "disp", > + .sta_mask = DIS_PWR_STA_MASK, > + .ctl_offs = SPM_DIS_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .clk_id = {CLK_MM}, > + .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_DISP, > + .active_wakeup = true, > + }, > + [MT2701_POWER_DOMAIN_MFG] = { > + .name = "mfg", > + .sta_mask = MFG_PWR_STA_MASK, > + .ctl_offs = SPM_MFG_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(12, 12), > + .active_wakeup = true, > + }, > + [MT2701_POWER_DOMAIN_VDEC] = { > + .name = "vdec", > + .sta_mask = VDE_PWR_STA_MASK, > + .ctl_offs = SPM_VDE_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(12, 12), > + .clk_id = {CLK_MM}, > + .active_wakeup = true, > + }, > + [MT2701_POWER_DOMAIN_ISP] = { > + .name = "isp", > + .sta_mask = ISP_PWR_STA_MASK, > + .ctl_offs = SPM_ISP_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(13, 12), > + .active_wakeup = true, > + }, > + [MT2701_POWER_DOMAIN_BDP] = { > + .name = "bdp", > + .sta_mask = BDP_PWR_STA_MASK, > + .ctl_offs = SPM_BDP_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .active_wakeup = true, > + }, > + [MT2701_POWER_DOMAIN_ETH] = { > + .name = "eth", > + .sta_mask = ETH_PWR_STA_MASK, > + .ctl_offs = SPM_ETH_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(15, 12), > + .active_wakeup = true, > + }, > + [MT2701_POWER_DOMAIN_HIF] = { > + .name = "hif", > + .sta_mask = HIF_PWR_STA_MASK, > + .ctl_offs = SPM_HIF_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(15, 12), > + .active_wakeup = true, > + }, > + [MT2701_POWER_DOMAIN_IFR_MSC] = { > + .name = "ifr_msc", > + .sta_mask = IFR_MSC_PWR_STA_MASK, > + .ctl_offs = SPM_IFR_MSC_PWR_CON, > + .active_wakeup = true, > + }, > +}; > + > +#define NUM_DOMAINS ARRAY_SIZE(scp_domain_data) > + > +static int __init scpsys_probe(struct platform_device *pdev) > +{ > + struct scp *scp; > + > + scp = init_scp(pdev, scp_domain_data, NUM_DOMAINS); > + if (IS_ERR(scp)) > + return PTR_ERR(scp); > + > + mtk_register_power_domains(pdev, scp, NUM_DOMAINS); > + > + return 0; > +} > + > +static const struct of_device_id of_scpsys_match_tbl[] = { > + { > + .compatible = "mediatek,mt2701-scpsys", > + }, { > + /* sentinel */ > + } > +}; > + > +static struct platform_driver scpsys_drv = { > + .driver = { > + .name = "mtk-scpsys-mt2701", > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(of_scpsys_match_tbl), > + }, > +}; > + > +static int __init scpsys_drv_init(void) > +{ > + return platform_driver_probe(&scpsys_drv, scpsys_probe); > +} > + > +/* > + * There are some Mediatek drivers which depend on the power domain driver need > + * to probe in earlier initcall levels. So scpsys driver also need to probe > + * earlier. > + * > + * IOMMU(M4U) and SMI drivers for example. SMI is a bridge between IOMMU and > + * multimedia HW. IOMMU depends on SMI, and SMI is a power domain consumer, > + * so the proper probe sequence should be scpsys -> SMI -> IOMMU driver. > + * IOMMU drivers are initialized during subsys_init by default, so we need to > + * move SMI and scpsys drivers to subsys_init or earlier init levels. > + */ > +subsys_initcall(scpsys_drv_init); > + > +MODULE_DESCRIPTION("MediaTek MT2701 scpsys driver"); > +MODULE_LICENSE("GPL v2"); >
Hi Matthias, On Wed, 2016-01-13 at 16:46 +0100, Matthias Brugger wrote: > > On 13/01/16 10:11, James Liao wrote: > > From: Shunli Wang <shunli.wang@mediatek.com> > > > > Add scpsys driver for MT2701. > > > > Signed-off-by: Shunli Wang <shunli.wang@mediatek.com> > > Signed-off-by: James Liao <jamesjj.liao@mediatek.com> > > --- > > NAK, same as in v1. Please don't split up the driver in several code > files, but find a a way to share the common code elegantly. > Comments below. Do you mean we should support MT8173 and MT2701 scpsys in a single file (such as mtk-scpsys.c)? We have lots of SoC with different power domain support. It means we need to add a different power domain table for a SoC, and may need to organize power domains (such as subdomains) for each SoC. It may not easy to maintain if we integrate all SoC's power domain into the same file. > > drivers/soc/mediatek/Makefile | 1 + > > drivers/soc/mediatek/mtk-scpsys-mt2701.c | 170 +++++++++++++++++++++++++++++++ > > 2 files changed, 171 insertions(+) > > create mode 100644 drivers/soc/mediatek/mtk-scpsys-mt2701.c > > > > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile > > index 13f7d21..d693f7d 100644 > > --- a/drivers/soc/mediatek/Makefile > > +++ b/drivers/soc/mediatek/Makefile > > @@ -2,3 +2,4 @@ obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o > > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o > > obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o > > obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys-mt8173.o > > +obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys-mt2701.o > > diff --git a/drivers/soc/mediatek/mtk-scpsys-mt2701.c b/drivers/soc/mediatek/mtk-scpsys-mt2701.c > > new file mode 100644 > > index 0000000..82c00ed > > --- /dev/null > > +++ b/drivers/soc/mediatek/mtk-scpsys-mt2701.c > > @@ -0,0 +1,170 @@ > > +/* > > + * Copyright (c) 2015 Mediatek, Shunli Wang <shunli.wang@mediatek.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > +#include <linux/mfd/syscon.h> > > +#include <linux/module.h> > > +#include <linux/of_device.h> > > +#include <linux/pm_domain.h> > > +#include <linux/soc/mediatek/infracfg.h> > > +#include <dt-bindings/power/mt2701-power.h> > > + > > +#include "mtk-scpsys.h" > > + > > +#define SPM_VDE_PWR_CON 0x0210 > > +#define SPM_MFG_PWR_CON 0x0214 > > +#define SPM_ISP_PWR_CON 0x0238 > > +#define SPM_DIS_PWR_CON 0x023C > > +#define SPM_CONN_PWR_CON 0x0280 > > +#define SPM_BDP_PWR_CON 0x029C > > +#define SPM_ETH_PWR_CON 0x02A0 > > +#define SPM_HIF_PWR_CON 0x02A4 > > +#define SPM_IFR_MSC_PWR_CON 0x02A8 > > +#define SPM_PWR_STATUS 0x060c > > +#define SPM_PWR_STATUS_2ND 0x0610 > > + > > +#define CONN_PWR_STA_MASK BIT(1) > > +#define DIS_PWR_STA_MASK BIT(3) > > +#define MFG_PWR_STA_MASK BIT(4) > > +#define ISP_PWR_STA_MASK BIT(5) > > +#define VDE_PWR_STA_MASK BIT(7) > > +#define BDP_PWR_STA_MASK BIT(14) > > +#define ETH_PWR_STA_MASK BIT(15) > > +#define HIF_PWR_STA_MASK BIT(16) > > +#define IFR_MSC_PWR_STA_MASK BIT(17) > > Apart from SPM_BDP_PWR_CON and BDP_PWR_STA_MASK this are compatible with > mt8173. So please avoid code duplication. > Apart you changed the define names from PWR_STATUS_* to *_STA_MASK, > please keep a uniform naming across the driver. OK, I'll unify the register naming or remove some register names in next patch. Some duplicated code can be reduced by moving them into the internal head file (mtk-scpsys.h). But some init code, such as driver init functions, may not easy to eliminate because the probe functions need to customize subdomains. Best regards, James
diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile index 13f7d21..d693f7d 100644 --- a/drivers/soc/mediatek/Makefile +++ b/drivers/soc/mediatek/Makefile @@ -2,3 +2,4 @@ obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys-mt8173.o +obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys-mt2701.o diff --git a/drivers/soc/mediatek/mtk-scpsys-mt2701.c b/drivers/soc/mediatek/mtk-scpsys-mt2701.c new file mode 100644 index 0000000..82c00ed --- /dev/null +++ b/drivers/soc/mediatek/mtk-scpsys-mt2701.c @@ -0,0 +1,170 @@ +/* + * Copyright (c) 2015 Mediatek, Shunli Wang <shunli.wang@mediatek.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/pm_domain.h> +#include <linux/soc/mediatek/infracfg.h> +#include <dt-bindings/power/mt2701-power.h> + +#include "mtk-scpsys.h" + +#define SPM_VDE_PWR_CON 0x0210 +#define SPM_MFG_PWR_CON 0x0214 +#define SPM_ISP_PWR_CON 0x0238 +#define SPM_DIS_PWR_CON 0x023C +#define SPM_CONN_PWR_CON 0x0280 +#define SPM_BDP_PWR_CON 0x029C +#define SPM_ETH_PWR_CON 0x02A0 +#define SPM_HIF_PWR_CON 0x02A4 +#define SPM_IFR_MSC_PWR_CON 0x02A8 +#define SPM_PWR_STATUS 0x060c +#define SPM_PWR_STATUS_2ND 0x0610 + +#define CONN_PWR_STA_MASK BIT(1) +#define DIS_PWR_STA_MASK BIT(3) +#define MFG_PWR_STA_MASK BIT(4) +#define ISP_PWR_STA_MASK BIT(5) +#define VDE_PWR_STA_MASK BIT(7) +#define BDP_PWR_STA_MASK BIT(14) +#define ETH_PWR_STA_MASK BIT(15) +#define HIF_PWR_STA_MASK BIT(16) +#define IFR_MSC_PWR_STA_MASK BIT(17) + +#define MT2701_TOP_AXI_PROT_EN_CONN 0x0104 +#define MT2701_TOP_AXI_PROT_EN_DISP 0x0002 + +static const struct scp_domain_data scp_domain_data[] = { + [MT2701_POWER_DOMAIN_CONN] = { + .name = "conn", + .sta_mask = CONN_PWR_STA_MASK, + .ctl_offs = SPM_CONN_PWR_CON, + .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_CONN, + .active_wakeup = true, + }, + [MT2701_POWER_DOMAIN_DISP] = { + .name = "disp", + .sta_mask = DIS_PWR_STA_MASK, + .ctl_offs = SPM_DIS_PWR_CON, + .sram_pdn_bits = GENMASK(11, 8), + .clk_id = {CLK_MM}, + .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_DISP, + .active_wakeup = true, + }, + [MT2701_POWER_DOMAIN_MFG] = { + .name = "mfg", + .sta_mask = MFG_PWR_STA_MASK, + .ctl_offs = SPM_MFG_PWR_CON, + .sram_pdn_bits = GENMASK(11, 8), + .sram_pdn_ack_bits = GENMASK(12, 12), + .active_wakeup = true, + }, + [MT2701_POWER_DOMAIN_VDEC] = { + .name = "vdec", + .sta_mask = VDE_PWR_STA_MASK, + .ctl_offs = SPM_VDE_PWR_CON, + .sram_pdn_bits = GENMASK(11, 8), + .sram_pdn_ack_bits = GENMASK(12, 12), + .clk_id = {CLK_MM}, + .active_wakeup = true, + }, + [MT2701_POWER_DOMAIN_ISP] = { + .name = "isp", + .sta_mask = ISP_PWR_STA_MASK, + .ctl_offs = SPM_ISP_PWR_CON, + .sram_pdn_bits = GENMASK(11, 8), + .sram_pdn_ack_bits = GENMASK(13, 12), + .active_wakeup = true, + }, + [MT2701_POWER_DOMAIN_BDP] = { + .name = "bdp", + .sta_mask = BDP_PWR_STA_MASK, + .ctl_offs = SPM_BDP_PWR_CON, + .sram_pdn_bits = GENMASK(11, 8), + .active_wakeup = true, + }, + [MT2701_POWER_DOMAIN_ETH] = { + .name = "eth", + .sta_mask = ETH_PWR_STA_MASK, + .ctl_offs = SPM_ETH_PWR_CON, + .sram_pdn_bits = GENMASK(11, 8), + .sram_pdn_ack_bits = GENMASK(15, 12), + .active_wakeup = true, + }, + [MT2701_POWER_DOMAIN_HIF] = { + .name = "hif", + .sta_mask = HIF_PWR_STA_MASK, + .ctl_offs = SPM_HIF_PWR_CON, + .sram_pdn_bits = GENMASK(11, 8), + .sram_pdn_ack_bits = GENMASK(15, 12), + .active_wakeup = true, + }, + [MT2701_POWER_DOMAIN_IFR_MSC] = { + .name = "ifr_msc", + .sta_mask = IFR_MSC_PWR_STA_MASK, + .ctl_offs = SPM_IFR_MSC_PWR_CON, + .active_wakeup = true, + }, +}; + +#define NUM_DOMAINS ARRAY_SIZE(scp_domain_data) + +static int __init scpsys_probe(struct platform_device *pdev) +{ + struct scp *scp; + + scp = init_scp(pdev, scp_domain_data, NUM_DOMAINS); + if (IS_ERR(scp)) + return PTR_ERR(scp); + + mtk_register_power_domains(pdev, scp, NUM_DOMAINS); + + return 0; +} + +static const struct of_device_id of_scpsys_match_tbl[] = { + { + .compatible = "mediatek,mt2701-scpsys", + }, { + /* sentinel */ + } +}; + +static struct platform_driver scpsys_drv = { + .driver = { + .name = "mtk-scpsys-mt2701", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(of_scpsys_match_tbl), + }, +}; + +static int __init scpsys_drv_init(void) +{ + return platform_driver_probe(&scpsys_drv, scpsys_probe); +} + +/* + * There are some Mediatek drivers which depend on the power domain driver need + * to probe in earlier initcall levels. So scpsys driver also need to probe + * earlier. + * + * IOMMU(M4U) and SMI drivers for example. SMI is a bridge between IOMMU and + * multimedia HW. IOMMU depends on SMI, and SMI is a power domain consumer, + * so the proper probe sequence should be scpsys -> SMI -> IOMMU driver. + * IOMMU drivers are initialized during subsys_init by default, so we need to + * move SMI and scpsys drivers to subsys_init or earlier init levels. + */ +subsys_initcall(scpsys_drv_init); + +MODULE_DESCRIPTION("MediaTek MT2701 scpsys driver"); +MODULE_LICENSE("GPL v2");