Message ID | 1431372206-1237-1-git-send-email-s.hauer@pengutronix.de |
---|---|
State | New |
Headers | show |
On Mon, May 11, 2015 at 09:23:22PM +0200, Sascha Hauer wrote: > This adds support for some miscellaneous bits of the infracfg controller. > The mtk_infracfg_set/clear_bus_protection functions are necessary for > the scpsys power domain driver to handle the bus protection bits which > are contained in the infacfg register space. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/soc/mediatek/Kconfig | 9 +++++ > drivers/soc/mediatek/Makefile | 1 + > drivers/soc/mediatek/mtk-infracfg.c | 80 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 90 insertions(+) > create mode 100644 drivers/soc/mediatek/mtk-infracfg.c > > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig > index bcdb22d..6fae66f 100644 > --- a/drivers/soc/mediatek/Kconfig > +++ b/drivers/soc/mediatek/Kconfig > @@ -9,3 +9,12 @@ config MTK_PMIC_WRAP > Say yes here to add support for MediaTek PMIC Wrapper found > on different MediaTek SoCs. The PMIC wrapper is a proprietary > hardware to connect the PMIC. > + > +config MTK_INFRACFG > + tristate "MediaTek INFRACFG Support" > + depends on ARCH_MEDIATEK > + select REGMAP > + help > + Say yes here to add support for the MediaTek INFRACFG controller. The > + INFRACFG controller contains various infrastructure registers not > + directly associated to any device. > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile > index ecaf4de..ce39119 100644 > --- a/drivers/soc/mediatek/Makefile > +++ b/drivers/soc/mediatek/Makefile > @@ -1 +1,2 @@ > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o > +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o > diff --git a/drivers/soc/mediatek/mtk-infracfg.c b/drivers/soc/mediatek/mtk-infracfg.c > new file mode 100644 > index 0000000..b3ebfae > --- /dev/null > +++ b/drivers/soc/mediatek/mtk-infracfg.c > @@ -0,0 +1,80 @@ > +#include <linux/regmap.h> > +#include <linux/export.h> > +#include <linux/jiffies.h> > +#include <linux/soc/mediatek/infracfg.h> > +#include <asm/processor.h> > + > +#define INFRA_TOPAXI_PROTECTEN 0x0220 > +#define INFRA_TOPAXI_PROTECTSTA1 0x0228 > + > +/** > + * mtk_infracfg_set_bus_protection - enable bus protection > + * @regmap: The infracfg regmap > + * @mask: The mask containing the protection bits to be enabled. > + * > + * This function enables the bus protection bits for disabled power > + * domains so that the system does not hanf when some unit accesses the > + * bus while in power down. > + */ > +int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask) > +{ > + unsigned long expired; > + u32 val; > + int ret; > + > + regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, mask); > + > + expired = jiffies + HZ; > + > + while (1) { > + ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val); > + if (ret) > + return ret; > + > + if ((val & mask) == mask) > + break; > + > + cpu_relax(); > + if (time_after(jiffies, expired)) > + return -EIO; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(mtk_infracfg_set_bus_protection); > + > +/** > + * mtk_infracfg_clear_bus_protection - disable bus protection > + * @regmap: The infracfg regmap > + * @mask: The mask containing the protection bits to be disabled. > + * > + * This function disables the bus protection bits previously enabled with > + * mtk_infracfg_set_bus_protection. > + */ > +int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask) > +{ > + unsigned long expired; > + int ret; > + > + regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, 0); > + > + expired = jiffies + HZ; > + > + while (1) { > + u32 val; > + > + ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val); > + if (ret) > + return ret; > + > + if (!(val & mask)) > + break; > + > + cpu_relax(); > + if (time_after(jiffies, expired)) > + return -EIO; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(mtk_infracfg_clear_bus_protection); Forgot to git add include/linux/soc/mediatek/infracfg.h, this file contains: #ifndef __SOC_MEDIATEK_INFRACFG_H #define __SOC_MEDIATEK_INFRACFG_H #define MT8173_TOP_AXI_PROT_EN_MCI_M2 BIT(0) #define MT8173_TOP_AXI_PROT_EN_MM_M0 BIT(1) #define MT8173_TOP_AXI_PROT_EN_MM_M1 BIT(2) #define MT8173_TOP_AXI_PROT_EN_MMAPB_S BIT(6) #define MT8173_TOP_AXI_PROT_EN_L2C_M2 BIT(9) #define MT8173_TOP_AXI_PROT_EN_L2SS_SMI BIT(11) #define MT8173_TOP_AXI_PROT_EN_L2SS_ADD BIT(12) #define MT8173_TOP_AXI_PROT_EN_CCI_M2 BIT(13) #define MT8173_TOP_AXI_PROT_EN_MFG_S BIT(14) #define MT8173_TOP_AXI_PROT_EN_PERI_M0 BIT(15) #define MT8173_TOP_AXI_PROT_EN_PERI_M1 BIT(16) #define MT8173_TOP_AXI_PROT_EN_DEBUGSYS BIT(17) #define MT8173_TOP_AXI_PROT_EN_CQ_DMA BIT(18) #define MT8173_TOP_AXI_PROT_EN_GCPU BIT(19) #define MT8173_TOP_AXI_PROT_EN_IOMMU BIT(20) #define MT8173_TOP_AXI_PROT_EN_MFG_M0 BIT(21) #define MT8173_TOP_AXI_PROT_EN_MFG_M1 BIT(22) #define MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT BIT(23) int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask); int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask); #endif /* __SOC_MEDIATEK_INFRACFG_H */
On Mon, 2015-05-11 at 21:23 +0200, Sascha Hauer wrote: > --- a/drivers/soc/mediatek/Kconfig > +++ b/drivers/soc/mediatek/Kconfig > +config MTK_INFRACFG > + tristate "MediaTek INFRACFG Support" > + depends on ARCH_MEDIATEK > + select REGMAP > + help > + Say yes here to add support for the MediaTek INFRACFG controller. The > + INFRACFG controller contains various infrastructure registers not > + directly associated to any device. > --- a/drivers/soc/mediatek/Makefile > +++ b/drivers/soc/mediatek/Makefile > +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o > --- /dev/null > +++ b/drivers/soc/mediatek/mtk-infracfg.c > +EXPORT_SYMBOL_GPL(mtk_infracfg_set_bus_protection); > +EXPORT_SYMBOL_GPL(mtk_infracfg_clear_bus_protection); As I understand it, if both MTK_INFRACFG and MTK_SCPSYS (see 3/5) are m we will get mtk-infracfg.ko and mtk-scpsys.ko (see 3/5). And loading mtk-scpsys.ko will trigger loading mtk-infracfg.ko, right? But since this file has no MODULE_LICENSE() that should generate a warning and taint the kernel. (I haven't tested this. Please correct me if I'm overlooking something here.) Thanks, Paul Bolle
2015-05-11 21:23 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > This adds a power domain driver for the Mediatek SCPSYS unit. > > The System Control Processor System (SCPSYS) has several power > management related tasks in the system. The tasks include thermal > measurement, dynamic voltage frequency scaling (DVFS), interrupt > filter and lowlevel sleep control. The System Power Manager (SPM) > inside the SCPSYS is for the MTCMOS power domain control. > > For now this driver only adds power domain support, the more > advanced features are not yet supported. The driver implements > the generic PM domain device tree bindings, the first user will > most likely be the Mediatek AFE audio driver. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > .../devicetree/bindings/soc/mediatek/scpsys.txt | 2 + > drivers/soc/mediatek/Kconfig | 8 + > drivers/soc/mediatek/Makefile | 1 + > drivers/soc/mediatek/mtk-scpsys.c | 416 +++++++++++++++++++++ > include/dt-bindings/power/mt8173-power.h | 15 + > 5 files changed, 442 insertions(+) > create mode 100644 drivers/soc/mediatek/mtk-scpsys.c > create mode 100644 include/dt-bindings/power/mt8173-power.h > > diff --git a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt > index 4764a03..87f2091 100644 > --- a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt > +++ b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt > @@ -15,6 +15,7 @@ Required properties: > - compatible: Must be "mediatek,mt8173-scpsys" > - #power-domain-cells: Must be 1 > - reg: Address range of the SCPSYS unit > +- infracfg: must contain a phandle to the infracfg controller > > Example: > > @@ -22,6 +23,7 @@ Example: > #power-domain-cells = <1>; > compatible = "mediatek,mt8173-scpsys"; > reg = <0 0x10006000 0 0x1000>; > + infracfg = <&infracfg>; > }; > > Example consumer: > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig > index 6fae66f..1386c79 100644 > --- a/drivers/soc/mediatek/Kconfig > +++ b/drivers/soc/mediatek/Kconfig > @@ -18,3 +18,11 @@ config MTK_INFRACFG > Say yes here to add support for the MediaTek INFRACFG controller. The > INFRACFG controller contains various infrastructure registers not > directly associated to any device. > + > +config MTK_SCPSYS > + tristate "MediaTek SCPSYS Support" > + depends on MTK_INFRACFG > + select REGMAP > + help > + Say yes here to add support for the MediaTek SCPSYS power domain > + driver. > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile > index ce39119..f8eebab 100644 > --- a/drivers/soc/mediatek/Makefile > +++ b/drivers/soc/mediatek/Makefile > @@ -1,2 +1,3 @@ > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o > obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o > +obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o > diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c > new file mode 100644 > index 0000000..c42c7f1 > --- /dev/null > +++ b/drivers/soc/mediatek/mtk-scpsys.c > @@ -0,0 +1,416 @@ > +/* > + * Copyright (c) 2015 Pengutronix, Sascha Hauer <kernel@pengutronix.de> > + * > + * 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/clk.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/pm_domain.h> > +#include <linux/delay.h> > +#include <linux/soc/mediatek/infracfg.h> > +#include <dt-bindings/power/mt8173-power.h> > +#include <linux/mfd/syscon.h> > + > +#define SPM_VDE_PWR_CON 0x0210 > +#define SPM_MFG_PWR_CON 0x0214 > +#define SPM_VEN_PWR_CON 0x0230 > +#define SPM_ISP_PWR_CON 0x0238 > +#define SPM_DIS_PWR_CON 0x023c > +#define SPM_VEN2_PWR_CON 0x0298 > +#define SPM_AUDIO_PWR_CON 0x029c > +#define SPM_MFG_2D_PWR_CON 0x02c0 > +#define SPM_MFG_ASYNC_PWR_CON 0x02c4 > +#define SPM_USB_PWR_CON 0x02cc > +#define SPM_PWR_STATUS 0x060c > +#define SPM_PWR_STATUS_2ND 0x0610 > + > +#define PWR_RST_B_BIT BIT(0) > +#define PWR_ISO_BIT BIT(1) > +#define PWR_ON_BIT BIT(2) > +#define PWR_ON_2ND_BIT BIT(3) > +#define PWR_CLK_DIS_BIT BIT(4) > + > +#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 VEN2_PWR_STA_MASK BIT(20) > +#define VEN_PWR_STA_MASK BIT(21) > +#define MFG_2D_PWR_STA_MASK BIT(22) > +#define MFG_ASYNC_PWR_STA_MASK BIT(23) > +#define AUDIO_PWR_STA_MASK BIT(24) > +#define USB_PWR_STA_MASK BIT(25) > + > +struct scp_domain_data { > + const char *name; > + u32 sta_mask; > + int ctl_offs; > + u32 sram_pdn_bits; > + u32 sram_pdn_ack_bits; > + u32 bus_prot_mask; > + int id; > + const char *clk_name; > +}; > + > +static const struct scp_domain_data scp_domain_data[] = { > + { > + .id = MT8173_POWER_DOMAIN_VDE, > + .name = "vde", > + .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_name = "vdec", > + }, { > + .id = MT8173_POWER_DOMAIN_VEN, > + .name = "ven", > + .sta_mask = VEN_PWR_STA_MASK, > + .ctl_offs = SPM_VEN_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(15, 12), > + .clk_name = "venc", > + }, { > + .id = MT8173_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), > + }, { > + .id = MT8173_POWER_DOMAIN_DIS, > + .name = "disp", > + .sta_mask = DIS_PWR_STA_MASK, > + .ctl_offs = SPM_DIS_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(12, 12), > + .clk_name = "disp", > + .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MM_M0 | > + MT8173_TOP_AXI_PROT_EN_MM_M1, > + }, { > + .id = MT8173_POWER_DOMAIN_VEN2, > + .name = "ven2", > + .sta_mask = VEN2_PWR_STA_MASK, > + .ctl_offs = SPM_VEN2_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(15, 12), > + .clk_name = "ven2", > + }, { > + .id = MT8173_POWER_DOMAIN_AUDIO, > + .name = "audio", > + .sta_mask = AUDIO_PWR_STA_MASK, > + .ctl_offs = SPM_AUDIO_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(15, 12), > + }, { > + .id = MT8173_POWER_DOMAIN_MFG_ASYNC, > + .name = "mfg_async", > + .sta_mask = MFG_ASYNC_PWR_STA_MASK, > + .ctl_offs = SPM_MFG_ASYNC_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = 0, > + .clk_name = "mfg", > + }, { > + .id = MT8173_POWER_DOMAIN_MFG_2D, > + .name = "mfg_2d", > + .sta_mask = MFG_2D_PWR_STA_MASK, > + .ctl_offs = SPM_MFG_2D_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(13, 12), > + .clk_name = "mfg", > + }, { > + .id = MT8173_POWER_DOMAIN_MFG, > + .name = "mfg", > + .sta_mask = MFG_PWR_STA_MASK, > + .ctl_offs = SPM_MFG_PWR_CON, > + .sram_pdn_bits = GENMASK(13, 8), > + .sram_pdn_ack_bits = GENMASK(21, 16), > + .clk_name = "mfg", > + .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MFG_S | > + MT8173_TOP_AXI_PROT_EN_MFG_M0 | > + MT8173_TOP_AXI_PROT_EN_MFG_M1 | > + MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT, > + }, { > + .id = MT8173_POWER_DOMAIN_USB, > + .name = "usb", > + .sta_mask = USB_PWR_STA_MASK, > + .ctl_offs = SPM_USB_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(15, 12), > + }, > +}; > + > +#define NUM_DOMAINS ARRAY_SIZE(scp_domain_data) > + > +struct scp; > + > +struct scp_domain { > + struct generic_pm_domain pmd; > + const struct scp_domain_data *data; > + struct scp *scp; > + struct clk *clk; > +}; > + > +struct scp { > + struct scp_domain domains[NUM_DOMAINS]; > + struct genpd_onecell_data pd_data; > + struct device *dev; > + void __iomem *base; > + struct regmap *infracfg; > +}; > + > +static int scpsys_power_on(struct generic_pm_domain *genpd) > +{ > + struct scp_domain *scpd = container_of(genpd, struct scp_domain, pmd); > + struct scp *scp = scpd->scp; > + const struct scp_domain_data *data = scpd->data; > + unsigned long expired; > + void __iomem *ctl_addr = scpd->scp->base + data->ctl_offs; > + u32 sram_pdn_ack = data->sram_pdn_ack_bits; > + u32 val; > + int ret; > + > + if (scpd->clk) { > + ret = clk_prepare_enable(scpd->clk); > + if (ret) > + return ret; > + } > + > + val = readl(ctl_addr); > + val |= PWR_ON_BIT; > + writel(val, ctl_addr); > + val |= PWR_ON_2ND_BIT; > + writel(val, ctl_addr); > + > + /* wait until PWR_ACK = 1 */ > + expired = jiffies + HZ; > + while (!(readl(scp->base + SPM_PWR_STATUS) & data->sta_mask) || > + !(readl(scp->base + SPM_PWR_STATUS_2ND) & data->sta_mask)) { Nit pick, line over 80 characters. > + cpu_relax(); > + if (time_after(jiffies, expired)) { > + ret = -EIO; > + goto out; > + } > + } > + > + val &= ~PWR_CLK_DIS_BIT; > + writel(val, ctl_addr); > + > + val &= ~PWR_ISO_BIT; > + writel(val, ctl_addr); > + > + val |= PWR_RST_B_BIT; > + writel(val, ctl_addr); > + > + val &= ~data->sram_pdn_bits; > + writel(val, ctl_addr); > + > + /* wait until SRAM_PDN_ACK all 0 */ > + expired = jiffies + HZ; > + while (sram_pdn_ack && (readl(ctl_addr) & sram_pdn_ack)) { > + cpu_relax(); > + if (time_after(jiffies, expired)) { > + ret = -EIO; > + goto out; > + } > + } > + > + if (data->bus_prot_mask) { > + ret = mtk_infracfg_clear_bus_protection(scp->infracfg, > + data->bus_prot_mask); > + if (ret) > + return ret; > + } > + > + return 0; > +out: > + dev_err(scp->dev, "Failed to power on domain %s\n", scpd->data->name); > + > + return ret; > +} > + > +static int scpsys_power_off(struct generic_pm_domain *genpd) > +{ > + struct scp_domain *scpd = container_of(genpd, struct scp_domain, pmd); > + struct scp *scp = scpd->scp; > + const struct scp_domain_data *data = scpd->data; > + unsigned long expired; > + void __iomem *ctl_addr = scpd->scp->base + data->ctl_offs; > + u32 sram_pdn_ack = data->sram_pdn_ack_bits; > + u32 val; > + int ret; > + > + if (data->bus_prot_mask) { > + ret = mtk_infracfg_set_bus_protection(scp->infracfg, > + data->bus_prot_mask); > + if (ret) > + return ret; > + } > + > + val = readl(ctl_addr); > + val |= data->sram_pdn_bits; > + writel(val, ctl_addr); > + > + /* wait until SRAM_PDN_ACK all 1 */ > + expired = jiffies + HZ; > + while ((readl(ctl_addr) & sram_pdn_ack) != sram_pdn_ack) { > + cpu_relax(); > + if (time_after(jiffies, expired)) { > + ret = -EIO; > + goto out; > + } > + } > + > + val |= PWR_ISO_BIT; > + writel(val, ctl_addr); > + > + val &= ~PWR_RST_B_BIT; > + writel(val, ctl_addr); > + > + val |= PWR_CLK_DIS_BIT; > + writel(val, ctl_addr); > + > + val &= ~PWR_ON_BIT; > + writel(val, ctl_addr); > + > + val &= ~PWR_ON_2ND_BIT; > + writel(val, ctl_addr); > + > + /* wait until PWR_ACK = 0 */ > + expired = jiffies + HZ; > + while ((readl(scp->base + SPM_PWR_STATUS) & data->sta_mask) || > + (readl(scp->base + SPM_PWR_STATUS_2ND) & data->sta_mask)) { Same here. > + cpu_relax(); > + if (time_after(jiffies, expired)) { > + ret = -EIO; > + goto out; > + } > + } > + > + if (scpd->clk) > + clk_disable_unprepare(scpd->clk); > + > + return 0; > + > +out: > + dev_err(scp->dev, "Failed to power off domain %s\n", scpd->data->name); > + > + return ret; > +} > + > +static int scpsys_probe(struct platform_device *pdev) > +{ > + struct genpd_onecell_data *pd_data; > + struct resource *res; > + int i; > + struct scp *scp; > + > + scp = devm_kzalloc(&pdev->dev, sizeof(*scp), GFP_KERNEL); > + if (!scp) > + return -ENOMEM; > + > + scp->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + scp->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(scp->base)) > + return PTR_ERR(scp->base); > + > + pd_data = &scp->pd_data; > + > + pd_data->domains = devm_kzalloc(&pdev->dev, > + sizeof(*pd_data->domains) * NUM_DOMAINS, GFP_KERNEL); > + if (!pd_data->domains) > + return -ENOMEM; > + > + scp->infracfg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, > + "infracfg"); > + if (IS_ERR(scp->infracfg)) { > + dev_err(&pdev->dev, "Cannot find infracfg controller: %ld\n", > + PTR_ERR(scp->infracfg)); > + return PTR_ERR(scp->infracfg); > + } > + > + pd_data->num_domains = NUM_DOMAINS; > + > + for (i = 0; i < NUM_DOMAINS; i++) { > + struct scp_domain *scpd = &scp->domains[i]; > + > + if (scp_domain_data[i].clk_name) { > + const char *name = scp_domain_data[i].clk_name; > + > + scpd->clk = devm_clk_get(&pdev->dev, name); > + if (IS_ERR(scpd->clk)) { > + dev_err(&pdev->dev, "Failed to get %s clk: %ld\n", > + name, PTR_ERR(scpd->clk)); > + return PTR_ERR(scpd->clk); > + } > + } > + } > + > + for (i = 0; i < NUM_DOMAINS; i++) { Why two for loops? > + struct scp_domain *scpd = &scp->domains[i]; > + struct generic_pm_domain *pmd = &scpd->pmd; > + > + pd_data->domains[scp_domain_data[i].id] = pmd; > + scpd->data = &scp_domain_data[i]; > + scpd->scp = scp; > + > + pmd->name = scp_domain_data[i].name; > + pmd->power_off = scpsys_power_off; > + pmd->power_on = scpsys_power_on; > + pmd->power_off_latency_ns = 20000; > + pmd->power_on_latency_ns = 20000; > + > + pm_genpd_init(pmd, NULL, true); > + > + /* > + * If PM is disabled turn on all domains by default so that > + * consumers can work. > + */ > + if (!IS_ENABLED(CONFIG_PM)) > + pmd->power_on(pmd); > + } > + > + pm_genpd_add_subdomain(&scp->domains[MT8173_POWER_DOMAIN_MFG_ASYNC].pmd, > + &scp->domains[MT8173_POWER_DOMAIN_MFG_2D].pmd); > + pm_genpd_add_subdomain(&scp->domains[MT8173_POWER_DOMAIN_MFG_2D].pmd, > + &scp->domains[MT8173_POWER_DOMAIN_MFG].pmd); > + > + return of_genpd_add_provider_onecell(pdev->dev.of_node, pd_data); > +} > + > +static struct of_device_id of_scpsys_match_tbl[] = { static const?
Hi Paul, On Tue, May 12, 2015 at 11:24:31AM +0200, Paul Bolle wrote: > On Mon, 2015-05-11 at 21:23 +0200, Sascha Hauer wrote: > > --- a/drivers/soc/mediatek/Kconfig > > +++ b/drivers/soc/mediatek/Kconfig > > > +config MTK_INFRACFG > > + tristate "MediaTek INFRACFG Support" > > + depends on ARCH_MEDIATEK > > + select REGMAP > > + help > > + Say yes here to add support for the MediaTek INFRACFG controller. The > > + INFRACFG controller contains various infrastructure registers not > > + directly associated to any device. > > > --- a/drivers/soc/mediatek/Makefile > > +++ b/drivers/soc/mediatek/Makefile > > > +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o > > > --- /dev/null > > +++ b/drivers/soc/mediatek/mtk-infracfg.c > > > +EXPORT_SYMBOL_GPL(mtk_infracfg_set_bus_protection); > > > +EXPORT_SYMBOL_GPL(mtk_infracfg_clear_bus_protection); > > As I understand it, if both MTK_INFRACFG and MTK_SCPSYS (see 3/5) are m > we will get mtk-infracfg.ko and mtk-scpsys.ko (see 3/5). And loading > mtk-scpsys.ko will trigger loading mtk-infracfg.ko, right? > > But since this file has no MODULE_LICENSE() that should generate a > warning and taint the kernel. (I haven't tested this. Please correct me > if I'm overlooking something here.) No, you're absolutely right here. It turned out though that the power domain specific functions are not exported to modules and also there is no unregistration code for power domains, so compiling this code as modules doesn't work anyway. I'll change the tristate to bool. Sascha
On Tue, May 12, 2015 at 01:52:06PM +0200, Matthias Brugger wrote: > 2015-05-11 21:23 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > > This adds a power domain driver for the Mediatek SCPSYS unit. > > > > The System Control Processor System (SCPSYS) has several power > > management related tasks in the system. The tasks include thermal > > measurement, dynamic voltage frequency scaling (DVFS), interrupt > > filter and lowlevel sleep control. The System Power Manager (SPM) > > inside the SCPSYS is for the MTCMOS power domain control. > > > > For now this driver only adds power domain support, the more > > advanced features are not yet supported. The driver implements > > the generic PM domain device tree bindings, the first user will > > most likely be the Mediatek AFE audio driver. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > .../devicetree/bindings/soc/mediatek/scpsys.txt | 2 + > > drivers/soc/mediatek/Kconfig | 8 + > > drivers/soc/mediatek/Makefile | 1 + > > drivers/soc/mediatek/mtk-scpsys.c | 416 +++++++++++++++++++++ > > include/dt-bindings/power/mt8173-power.h | 15 + > > 5 files changed, 442 insertions(+) > > create mode 100644 drivers/soc/mediatek/mtk-scpsys.c > > create mode 100644 include/dt-bindings/power/mt8173-power.h > > > > diff --git a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt > > index 4764a03..87f2091 100644 > > --- a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt > > +++ b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt > > @@ -15,6 +15,7 @@ Required properties: > > - compatible: Must be "mediatek,mt8173-scpsys" > > - #power-domain-cells: Must be 1 > > - reg: Address range of the SCPSYS unit > > +- infracfg: must contain a phandle to the infracfg controller > > > > Example: > > > > @@ -22,6 +23,7 @@ Example: > > #power-domain-cells = <1>; > > compatible = "mediatek,mt8173-scpsys"; > > reg = <0 0x10006000 0 0x1000>; > > + infracfg = <&infracfg>; > > }; This hunk shouldn't be here btw, fixed. > > + pd_data->num_domains = NUM_DOMAINS; > > + > > + for (i = 0; i < NUM_DOMAINS; i++) { > > + struct scp_domain *scpd = &scp->domains[i]; > > + > > + if (scp_domain_data[i].clk_name) { > > + const char *name = scp_domain_data[i].clk_name; > > + > > + scpd->clk = devm_clk_get(&pdev->dev, name); > > + if (IS_ERR(scpd->clk)) { > > + dev_err(&pdev->dev, "Failed to get %s clk: %ld\n", > > + name, PTR_ERR(scpd->clk)); > > + return PTR_ERR(scpd->clk); > > + } > > + } > > + } > > + > > + for (i = 0; i < NUM_DOMAINS; i++) { > > Why two for loops? I'm sure there was a reason in earlier versions, but now it seems rather useless. I'll make this a single loop. > > static const? Yes. Thanks for reviewing. Sascha
On Tue, May 12, 2015 at 3:23 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > This adds a power domain driver for the Mediatek SCPSYS unit. > > The System Control Processor System (SCPSYS) has several power > management related tasks in the system. The tasks include thermal > measurement, dynamic voltage frequency scaling (DVFS), interrupt > filter and lowlevel sleep control. The System Power Manager (SPM) > inside the SCPSYS is for the MTCMOS power domain control. > > For now this driver only adds power domain support, the more > advanced features are not yet supported. The driver implements > the generic PM domain device tree bindings, the first user will > most likely be the Mediatek AFE audio driver. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > .../devicetree/bindings/soc/mediatek/scpsys.txt | 2 + > drivers/soc/mediatek/Kconfig | 8 + > drivers/soc/mediatek/Makefile | 1 + > drivers/soc/mediatek/mtk-scpsys.c | 416 +++++++++++++++++++++ > include/dt-bindings/power/mt8173-power.h | 15 + > 5 files changed, 442 insertions(+) > create mode 100644 drivers/soc/mediatek/mtk-scpsys.c > create mode 100644 include/dt-bindings/power/mt8173-power.h > > diff --git a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt > index 4764a03..87f2091 100644 > --- a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt > +++ b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt > @@ -15,6 +15,7 @@ Required properties: > - compatible: Must be "mediatek,mt8173-scpsys" > - #power-domain-cells: Must be 1 > - reg: Address range of the SCPSYS unit > +- infracfg: must contain a phandle to the infracfg controller > > Example: > > @@ -22,6 +23,7 @@ Example: > #power-domain-cells = <1>; > compatible = "mediatek,mt8173-scpsys"; > reg = <0 0x10006000 0 0x1000>; > + infracfg = <&infracfg>; > }; > > Example consumer: > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig > index 6fae66f..1386c79 100644 > --- a/drivers/soc/mediatek/Kconfig > +++ b/drivers/soc/mediatek/Kconfig > @@ -18,3 +18,11 @@ config MTK_INFRACFG > Say yes here to add support for the MediaTek INFRACFG controller. The > INFRACFG controller contains various infrastructure registers not > directly associated to any device. > + > +config MTK_SCPSYS > + tristate "MediaTek SCPSYS Support" > + depends on MTK_INFRACFG Should this be: depends on ARCH_MEDIATEK || COMPILE_TEST selects MTK_INFRACFG > + select REGMAP > + help > + Say yes here to add support for the MediaTek SCPSYS power domain > + driver. > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile > index ce39119..f8eebab 100644 > --- a/drivers/soc/mediatek/Makefile > +++ b/drivers/soc/mediatek/Makefile > @@ -1,2 +1,3 @@ > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o > obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o > +obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o > diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c > new file mode 100644 > index 0000000..c42c7f1 > --- /dev/null > +++ b/drivers/soc/mediatek/mtk-scpsys.c > @@ -0,0 +1,416 @@ > +/* > + * Copyright (c) 2015 Pengutronix, Sascha Hauer <kernel@pengutronix.de> > + * > + * 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/clk.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/pm_domain.h> > +#include <linux/delay.h> > +#include <linux/soc/mediatek/infracfg.h> > +#include <dt-bindings/power/mt8173-power.h> > +#include <linux/mfd/syscon.h> > + > +#define SPM_VDE_PWR_CON 0x0210 > +#define SPM_MFG_PWR_CON 0x0214 > +#define SPM_VEN_PWR_CON 0x0230 > +#define SPM_ISP_PWR_CON 0x0238 > +#define SPM_DIS_PWR_CON 0x023c > +#define SPM_VEN2_PWR_CON 0x0298 > +#define SPM_AUDIO_PWR_CON 0x029c > +#define SPM_MFG_2D_PWR_CON 0x02c0 > +#define SPM_MFG_ASYNC_PWR_CON 0x02c4 > +#define SPM_USB_PWR_CON 0x02cc > +#define SPM_PWR_STATUS 0x060c > +#define SPM_PWR_STATUS_2ND 0x0610 > + > +#define PWR_RST_B_BIT BIT(0) > +#define PWR_ISO_BIT BIT(1) > +#define PWR_ON_BIT BIT(2) > +#define PWR_ON_2ND_BIT BIT(3) > +#define PWR_CLK_DIS_BIT BIT(4) > + > +#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 VEN2_PWR_STA_MASK BIT(20) > +#define VEN_PWR_STA_MASK BIT(21) > +#define MFG_2D_PWR_STA_MASK BIT(22) > +#define MFG_ASYNC_PWR_STA_MASK BIT(23) > +#define AUDIO_PWR_STA_MASK BIT(24) > +#define USB_PWR_STA_MASK BIT(25) > + > +struct scp_domain_data { > + const char *name; > + u32 sta_mask; > + int ctl_offs; > + u32 sram_pdn_bits; > + u32 sram_pdn_ack_bits; > + u32 bus_prot_mask; > + int id; > + const char *clk_name; > +}; These fields are only used at init time: name (the pointer, the actual string is pointed to by genpd->name) id clk_name You could split those off into a separate __initconst struct. > + > +static const struct scp_domain_data scp_domain_data[] = { > + { > + .id = MT8173_POWER_DOMAIN_VDE, > + .name = "vde", > + .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_name = "vdec", > + }, { > + .id = MT8173_POWER_DOMAIN_VEN, Is there any reason you do not keep these in ".id" order: VDE, MFG, VEN, ISP, DIS, VEN2, AUDIO, MFG_2D, MFG_ASYNC, USB If you want to make the ID defines explicit in the code, you can instead get rid of the '.id' field, and use the defines as indices while initializing the scp_domain_data array: static const struct scp_domain_data scp_domain_data[] = { [MT8173_POWER_DOMAIN_VDE] = { }, [MT8173_POWER_DOMAIN_MFG] = { }, ... }; > + .name = "ven", > + .sta_mask = VEN_PWR_STA_MASK, > + .ctl_offs = SPM_VEN_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(15, 12), > + .clk_name = "venc", > + }, { > + .id = MT8173_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), > + }, { > + .id = MT8173_POWER_DOMAIN_DIS, > + .name = "disp", Perhaps .id/.name/.clk should be "MM" to match the corresponding clock define (CLK_TOP_MM_SEL) & prot bits. > + .sta_mask = DIS_PWR_STA_MASK, > + .ctl_offs = SPM_DIS_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(12, 12), > + .clk_name = "disp", > + .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MM_M0 | > + MT8173_TOP_AXI_PROT_EN_MM_M1, > + }, { > + .id = MT8173_POWER_DOMAIN_VEN2, > + .name = "ven2", Perhaps .id/.name/.clk should be "VENC_LT"/"venc_lt" to match the corresponding clock define (CLK_TOP_VENC_LT_SEL). > + .sta_mask = VEN2_PWR_STA_MASK, > + .ctl_offs = SPM_VEN2_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(15, 12), > + .clk_name = "ven2", > + }, { > + .id = MT8173_POWER_DOMAIN_AUDIO, > + .name = "audio", > + .sta_mask = AUDIO_PWR_STA_MASK, > + .ctl_offs = SPM_AUDIO_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(15, 12), > + }, { > + .id = MT8173_POWER_DOMAIN_MFG_ASYNC, > + .name = "mfg_async", > + .sta_mask = MFG_ASYNC_PWR_STA_MASK, > + .ctl_offs = SPM_MFG_ASYNC_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = 0, > + .clk_name = "mfg", > + }, { > + .id = MT8173_POWER_DOMAIN_MFG_2D, > + .name = "mfg_2d", > + .sta_mask = MFG_2D_PWR_STA_MASK, > + .ctl_offs = SPM_MFG_2D_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(13, 12), > + .clk_name = "mfg", > + }, { > + .id = MT8173_POWER_DOMAIN_MFG, > + .name = "mfg", > + .sta_mask = MFG_PWR_STA_MASK, > + .ctl_offs = SPM_MFG_PWR_CON, > + .sram_pdn_bits = GENMASK(13, 8), > + .sram_pdn_ack_bits = GENMASK(21, 16), > + .clk_name = "mfg", > + .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MFG_S | > + MT8173_TOP_AXI_PROT_EN_MFG_M0 | > + MT8173_TOP_AXI_PROT_EN_MFG_M1 | > + MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT, > + }, { > + .id = MT8173_POWER_DOMAIN_USB, > + .name = "usb", > + .sta_mask = USB_PWR_STA_MASK, > + .ctl_offs = SPM_USB_PWR_CON, > + .sram_pdn_bits = GENMASK(11, 8), > + .sram_pdn_ack_bits = GENMASK(15, 12), > + }, > +}; > + > +#define NUM_DOMAINS ARRAY_SIZE(scp_domain_data) > + > +struct scp; > + > +struct scp_domain { > + struct generic_pm_domain pmd; Could you name this "genpd"... that seems to be what most other drivers use. > + const struct scp_domain_data *data; > + struct scp *scp; > + struct clk *clk; > +}; > + > +struct scp { > + struct scp_domain domains[NUM_DOMAINS]; > + struct genpd_onecell_data pd_data; After probe() you don't use pd_data. Why do we need it in this struct? > + struct device *dev; > + void __iomem *base; > + struct regmap *infracfg; > +}; > + > +static int scpsys_power_on(struct generic_pm_domain *genpd) > +{ > + struct scp_domain *scpd = container_of(genpd, struct scp_domain, pmd); > + struct scp *scp = scpd->scp; > + const struct scp_domain_data *data = scpd->data; > + unsigned long expired; > + void __iomem *ctl_addr = scpd->scp->base + data->ctl_offs; > + u32 sram_pdn_ack = data->sram_pdn_ack_bits; > + u32 val; > + int ret; > + > + if (scpd->clk) { > + ret = clk_prepare_enable(scpd->clk); > + if (ret) > + return ret; > + } > + > + val = readl(ctl_addr); > + val |= PWR_ON_BIT; > + writel(val, ctl_addr); > + val |= PWR_ON_2ND_BIT; > + writel(val, ctl_addr); > + > + /* wait until PWR_ACK = 1 */ > + expired = jiffies + HZ; > + while (!(readl(scp->base + SPM_PWR_STATUS) & data->sta_mask) || > + !(readl(scp->base + SPM_PWR_STATUS_2ND) & data->sta_mask)) { > + cpu_relax(); > + if (time_after(jiffies, expired)) { > + ret = -EIO; > + goto out; > + } > + } > + > + val &= ~PWR_CLK_DIS_BIT; > + writel(val, ctl_addr); > + > + val &= ~PWR_ISO_BIT; > + writel(val, ctl_addr); > + > + val |= PWR_RST_B_BIT; > + writel(val, ctl_addr); > + > + val &= ~data->sram_pdn_bits; > + writel(val, ctl_addr); > + > + /* wait until SRAM_PDN_ACK all 0 */ > + expired = jiffies + HZ; > + while (sram_pdn_ack && (readl(ctl_addr) & sram_pdn_ack)) { > + cpu_relax(); > + if (time_after(jiffies, expired)) { > + ret = -EIO; > + goto out; > + } > + } > + > + if (data->bus_prot_mask) { > + ret = mtk_infracfg_clear_bus_protection(scp->infracfg, > + data->bus_prot_mask); > + if (ret) > + return ret; > + } > + > + return 0; > +out: > + dev_err(scp->dev, "Failed to power on domain %s\n", scpd->data->name); genpd->name should be sufficient > + > + return ret; > +} > + > +static int scpsys_power_off(struct generic_pm_domain *genpd) > +{ > + struct scp_domain *scpd = container_of(genpd, struct scp_domain, pmd); > + struct scp *scp = scpd->scp; > + const struct scp_domain_data *data = scpd->data; > + unsigned long expired; > + void __iomem *ctl_addr = scpd->scp->base + data->ctl_offs; > + u32 sram_pdn_ack = data->sram_pdn_ack_bits; > + u32 val; > + int ret; > + > + if (data->bus_prot_mask) { > + ret = mtk_infracfg_set_bus_protection(scp->infracfg, > + data->bus_prot_mask); > + if (ret) > + return ret; > + } > + > + val = readl(ctl_addr); > + val |= data->sram_pdn_bits; > + writel(val, ctl_addr); > + > + /* wait until SRAM_PDN_ACK all 1 */ > + expired = jiffies + HZ; > + while ((readl(ctl_addr) & sram_pdn_ack) != sram_pdn_ack) { > + cpu_relax(); > + if (time_after(jiffies, expired)) { > + ret = -EIO; > + goto out; > + } > + } > + > + val |= PWR_ISO_BIT; > + writel(val, ctl_addr); > + > + val &= ~PWR_RST_B_BIT; > + writel(val, ctl_addr); > + > + val |= PWR_CLK_DIS_BIT; > + writel(val, ctl_addr); > + > + val &= ~PWR_ON_BIT; > + writel(val, ctl_addr); > + > + val &= ~PWR_ON_2ND_BIT; > + writel(val, ctl_addr); > + > + /* wait until PWR_ACK = 0 */ > + expired = jiffies + HZ; > + while ((readl(scp->base + SPM_PWR_STATUS) & data->sta_mask) || > + (readl(scp->base + SPM_PWR_STATUS_2ND) & data->sta_mask)) { > + cpu_relax(); > + if (time_after(jiffies, expired)) { > + ret = -EIO; > + goto out; > + } > + } > + > + if (scpd->clk) > + clk_disable_unprepare(scpd->clk); > + > + return 0; > + > +out: > + dev_err(scp->dev, "Failed to power off domain %s\n", scpd->data->name); > + > + return ret; > +} > + > +static int scpsys_probe(struct platform_device *pdev) > +{ > + struct genpd_onecell_data *pd_data; > + struct resource *res; > + int i; > + struct scp *scp; > + > + scp = devm_kzalloc(&pdev->dev, sizeof(*scp), GFP_KERNEL); > + if (!scp) > + return -ENOMEM; > + > + scp->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + scp->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(scp->base)) > + return PTR_ERR(scp->base); > + > + pd_data = &scp->pd_data; > + > + pd_data->domains = devm_kzalloc(&pdev->dev, > + sizeof(*pd_data->domains) * NUM_DOMAINS, GFP_KERNEL); > + if (!pd_data->domains) > + return -ENOMEM; > + > + scp->infracfg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, > + "infracfg"); > + if (IS_ERR(scp->infracfg)) { > + dev_err(&pdev->dev, "Cannot find infracfg controller: %ld\n", > + PTR_ERR(scp->infracfg)); > + return PTR_ERR(scp->infracfg); > + } > + > + pd_data->num_domains = NUM_DOMAINS; > + > + for (i = 0; i < NUM_DOMAINS; i++) { > + struct scp_domain *scpd = &scp->domains[i]; > + > + if (scp_domain_data[i].clk_name) { > + const char *name = scp_domain_data[i].clk_name; > + > + scpd->clk = devm_clk_get(&pdev->dev, name); > + if (IS_ERR(scpd->clk)) { > + dev_err(&pdev->dev, "Failed to get %s clk: %ld\n", > + name, PTR_ERR(scpd->clk)); > + return PTR_ERR(scpd->clk); > + } > + } > + } > + > + for (i = 0; i < NUM_DOMAINS; i++) { > + struct scp_domain *scpd = &scp->domains[i]; > + struct generic_pm_domain *pmd = &scpd->pmd; > + > + pd_data->domains[scp_domain_data[i].id] = pmd; > + scpd->data = &scp_domain_data[i]; > + scpd->scp = scp; > + > + pmd->name = scp_domain_data[i].name; > + pmd->power_off = scpsys_power_off; > + pmd->power_on = scpsys_power_on; > + pmd->power_off_latency_ns = 20000; > + pmd->power_on_latency_ns = 20000; Where did these latency values come from? > + > + pm_genpd_init(pmd, NULL, true); I'm not sure how this works... does this mean that all power domains initially off? > + > + /* > + * If PM is disabled turn on all domains by default so that > + * consumers can work. > + */ > + if (!IS_ENABLED(CONFIG_PM)) > + pmd->power_on(pmd); > + } > + > + pm_genpd_add_subdomain(&scp->domains[MT8173_POWER_DOMAIN_MFG_ASYNC].pmd, > + &scp->domains[MT8173_POWER_DOMAIN_MFG_2D].pmd); > + pm_genpd_add_subdomain(&scp->domains[MT8173_POWER_DOMAIN_MFG_2D].pmd, > + &scp->domains[MT8173_POWER_DOMAIN_MFG].pmd); > + (1) nit: the continuation lines needs 1 more indent. (2) Why aren't you checking for errors? (3) The indexes here are wrong after you re-ordered scp_domain_data[]... scp->domains[] is ordered by scp_domain_data[] index, not ".id" order like pd_data->domains[]. So, I think you want: pm_genpd_add_subdomain(&pd_data->domains[MT8173_POWER_DOMAIN_MFG_ASYNC], &pd_data->domains[MT8173_POWER_DOMAIN_MFG_2D]); pm_genpd_add_subdomain(&pd_data->domains[MT8173_POWER_DOMAIN_MFG_2D], &pd_data->domains[MT8173_POWER_DOMAIN_MFG]); Best Regards, -Dan > + return of_genpd_add_provider_onecell(pdev->dev.of_node, pd_data); > +} > + > +static struct of_device_id of_scpsys_match_tbl[] = { > + { > + .compatible = "mediatek,mt8173-scpsys", > + }, { > + /* sentinel */ > + } > +}; > +MODULE_DEVICE_TABLE(of, of_scpsys_match_tbl); > + > +static struct platform_driver scpsys_drv = { > + .driver = { > + .name = "mtk-scpsys", > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(of_scpsys_match_tbl), > + }, > + .probe = scpsys_probe, > +}; > + > +module_platform_driver(scpsys_drv); > + > +MODULE_AUTHOR("Sascha Hauer, Pengutronix"); > +MODULE_DESCRIPTION("MediaTek MT8173 scpsys driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/dt-bindings/power/mt8173-power.h b/include/dt-bindings/power/mt8173-power.h > new file mode 100644 > index 0000000..88715f2 > --- /dev/null > +++ b/include/dt-bindings/power/mt8173-power.h > @@ -0,0 +1,15 @@ > +#ifndef _DT_BINDINGS_POWER_MT8183_POWER_H > +#define _DT_BINDINGS_POWER_MT8183_POWER_H > + > +#define MT8173_POWER_DOMAIN_VDE 0 > +#define MT8173_POWER_DOMAIN_MFG 1 > +#define MT8173_POWER_DOMAIN_VEN 2 > +#define MT8173_POWER_DOMAIN_ISP 3 > +#define MT8173_POWER_DOMAIN_DIS 4 > +#define MT8173_POWER_DOMAIN_VEN2 5 > +#define MT8173_POWER_DOMAIN_AUDIO 6 > +#define MT8173_POWER_DOMAIN_MFG_2D 7 > +#define MT8173_POWER_DOMAIN_MFG_ASYNC 8 > +#define MT8173_POWER_DOMAIN_USB 9 > + > +#endif /* _DT_BINDINGS_POWER_MT8183_POWER_H */ > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
Hi Sascha, On Tue, May 12, 2015 at 3:23 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > This adds support for some miscellaneous bits of the infracfg controller. > The mtk_infracfg_set/clear_bus_protection functions are necessary for > the scpsys power domain driver to handle the bus protection bits which > are contained in the infacfg register space. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/soc/mediatek/Kconfig | 9 +++++ > drivers/soc/mediatek/Makefile | 1 + > drivers/soc/mediatek/mtk-infracfg.c | 80 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 90 insertions(+) > create mode 100644 drivers/soc/mediatek/mtk-infracfg.c > > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig > index bcdb22d..6fae66f 100644 > --- a/drivers/soc/mediatek/Kconfig > +++ b/drivers/soc/mediatek/Kconfig > @@ -9,3 +9,12 @@ config MTK_PMIC_WRAP > Say yes here to add support for MediaTek PMIC Wrapper found > on different MediaTek SoCs. The PMIC wrapper is a proprietary > hardware to connect the PMIC. > + > +config MTK_INFRACFG nit: Could you alphabetize these config options - so this one before MTK_PMIC_WRAP > + tristate "MediaTek INFRACFG Support" > + depends on ARCH_MEDIATEK I've seen several drivers like this now: depends on ARCH_MEDIATEK || COMPILE_TEST > + select REGMAP > + help > + Say yes here to add support for the MediaTek INFRACFG controller. The > + INFRACFG controller contains various infrastructure registers not > + directly associated to any device. > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile > index ecaf4de..ce39119 100644 > --- a/drivers/soc/mediatek/Makefile > +++ b/drivers/soc/mediatek/Makefile > @@ -1 +1,2 @@ > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o > +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o alphabetize here, too. > diff --git a/drivers/soc/mediatek/mtk-infracfg.c b/drivers/soc/mediatek/mtk-infracfg.c > new file mode 100644 > index 0000000..b3ebfae > --- /dev/null > +++ b/drivers/soc/mediatek/mtk-infracfg.c > @@ -0,0 +1,80 @@ > +#include <linux/regmap.h> > +#include <linux/export.h> > +#include <linux/jiffies.h> > +#include <linux/soc/mediatek/infracfg.h> > +#include <asm/processor.h> and... alphabetize headers here. I'm not sure if people care, but I find it makes it much easier to merge/add things later if these lists are already sorted. Same "please alphabetize" comments for the mtk-scpsys patch, so I won't repeat them. > + > +#define INFRA_TOPAXI_PROTECTEN 0x0220 > +#define INFRA_TOPAXI_PROTECTSTA1 0x0228 > + > +/** > + * mtk_infracfg_set_bus_protection - enable bus protection > + * @regmap: The infracfg regmap > + * @mask: The mask containing the protection bits to be enabled. > + * > + * This function enables the bus protection bits for disabled power > + * domains so that the system does not hanf when some unit accesses the > + * bus while in power down. > + */ > +int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask) > +{ > + unsigned long expired; > + u32 val; > + int ret; > + > + regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, mask); > + > + expired = jiffies + HZ; > + > + while (1) { > + ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val); > + if (ret) > + return ret; > + > + if ((val & mask) == mask) > + break; > + > + cpu_relax(); > + if (time_after(jiffies, expired)) > + return -EIO; I think we should check for timeout first, and then cpu_relax() if there is still time left (here and in mtk_infracfg_clear_bus_protection()). Otherwise we end up doing one final cpu_relax() without rechecking the register we are polling (again, I have the same comment for the timeout loops in mtk-scpsys). Also, shouldn't we return -ETIMEOUT if we timeout? Thanks! -Dan > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(mtk_infracfg_set_bus_protection); > + > +/** > + * mtk_infracfg_clear_bus_protection - disable bus protection > + * @regmap: The infracfg regmap > + * @mask: The mask containing the protection bits to be disabled. > + * > + * This function disables the bus protection bits previously enabled with > + * mtk_infracfg_set_bus_protection. > + */ > +int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask) > +{ > + unsigned long expired; > + int ret; > + > + regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, 0); > + > + expired = jiffies + HZ; > + > + while (1) { > + u32 val; > + > + ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val); > + if (ret) > + return ret; > + > + if (!(val & mask)) > + break; > + > + cpu_relax(); > + if (time_after(jiffies, expired)) > + return -EIO; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(mtk_infracfg_clear_bus_protection); > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
Hi Daniel, On Fri, May 15, 2015 at 10:17:33PM +0800, Daniel Kurtz wrote: > Hi Sascha, > > On Tue, May 12, 2015 at 3:23 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > > This adds support for some miscellaneous bits of the infracfg controller. > > The mtk_infracfg_set/clear_bus_protection functions are necessary for > > the scpsys power domain driver to handle the bus protection bits which > > are contained in the infacfg register space. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > drivers/soc/mediatek/Kconfig | 9 +++++ > > drivers/soc/mediatek/Makefile | 1 + > > drivers/soc/mediatek/mtk-infracfg.c | 80 +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 90 insertions(+) > > create mode 100644 drivers/soc/mediatek/mtk-infracfg.c > > > > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig > > index bcdb22d..6fae66f 100644 > > --- a/drivers/soc/mediatek/Kconfig > > +++ b/drivers/soc/mediatek/Kconfig > > @@ -9,3 +9,12 @@ config MTK_PMIC_WRAP > > Say yes here to add support for MediaTek PMIC Wrapper found > > on different MediaTek SoCs. The PMIC wrapper is a proprietary > > hardware to connect the PMIC. > > + > > +config MTK_INFRACFG > > nit: Could you alphabetize these config options - so this one before > MTK_PMIC_WRAP > > > + tristate "MediaTek INFRACFG Support" > > + depends on ARCH_MEDIATEK > > I've seen several drivers like this now: > > depends on ARCH_MEDIATEK || COMPILE_TEST > > > > + select REGMAP > > + help > > + Say yes here to add support for the MediaTek INFRACFG controller. The > > + INFRACFG controller contains various infrastructure registers not > > + directly associated to any device. > > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile > > index ecaf4de..ce39119 100644 > > --- a/drivers/soc/mediatek/Makefile > > +++ b/drivers/soc/mediatek/Makefile > > @@ -1 +1,2 @@ > > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o > > +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o > > alphabetize here, too. > > > diff --git a/drivers/soc/mediatek/mtk-infracfg.c b/drivers/soc/mediatek/mtk-infracfg.c > > new file mode 100644 > > index 0000000..b3ebfae > > --- /dev/null > > +++ b/drivers/soc/mediatek/mtk-infracfg.c > > @@ -0,0 +1,80 @@ > > +#include <linux/regmap.h> > > +#include <linux/export.h> > > +#include <linux/jiffies.h> > > +#include <linux/soc/mediatek/infracfg.h> > > +#include <asm/processor.h> > > and... alphabetize headers here. > > I'm not sure if people care, but I find it makes it much easier to > merge/add things later if these lists are already sorted. > Same "please alphabetize" comments for the mtk-scpsys patch, so I > won't repeat them. > > > + > > +#define INFRA_TOPAXI_PROTECTEN 0x0220 > > +#define INFRA_TOPAXI_PROTECTSTA1 0x0228 > > + > > +/** > > + * mtk_infracfg_set_bus_protection - enable bus protection > > + * @regmap: The infracfg regmap > > + * @mask: The mask containing the protection bits to be enabled. > > + * > > + * This function enables the bus protection bits for disabled power > > + * domains so that the system does not hanf when some unit accesses the > > + * bus while in power down. > > + */ > > +int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask) > > +{ > > + unsigned long expired; > > + u32 val; > > + int ret; > > + > > + regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, mask); > > + > > + expired = jiffies + HZ; > > + > > + while (1) { > > + ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val); > > + if (ret) > > + return ret; > > + > > + if ((val & mask) == mask) > > + break; > > + > > + cpu_relax(); > > + if (time_after(jiffies, expired)) > > + return -EIO; > > I think we should check for timeout first, and then cpu_relax() if > there is still time left (here and in > mtk_infracfg_clear_bus_protection()). Otherwise we end up doing one > final cpu_relax() without rechecking the register we are polling > (again, I have the same comment for the timeout loops in mtk-scpsys). I think cpu_relax() delays execution in the order of microseconds (I don't actually know, just a guess), so if the timeout is a second the order doesn't really matter. What can happen though is an interrupt after the (val & mask) test but before the timeout check. So to be truly correct we have to repeat the (val & mask) test after the time_after() check. Is that what you want? > > Also, shouldn't we return -ETIMEOUT if we timeout? I dunno. Probably the operation operation timed out because of an IO error. I'll change it to -ETIMEDOUT. Sascha
On Mon, May 18, 2015 at 4:16 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > Hi Daniel, > > On Fri, May 15, 2015 at 10:17:33PM +0800, Daniel Kurtz wrote: >> Hi Sascha, >> >> On Tue, May 12, 2015 at 3:23 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote: >> > This adds support for some miscellaneous bits of the infracfg controller. >> > The mtk_infracfg_set/clear_bus_protection functions are necessary for >> > the scpsys power domain driver to handle the bus protection bits which >> > are contained in the infacfg register space. >> > >> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >> > --- >> > drivers/soc/mediatek/Kconfig | 9 +++++ >> > drivers/soc/mediatek/Makefile | 1 + >> > drivers/soc/mediatek/mtk-infracfg.c | 80 +++++++++++++++++++++++++++++++++++++ >> > 3 files changed, 90 insertions(+) >> > create mode 100644 drivers/soc/mediatek/mtk-infracfg.c >> > >> > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig >> > index bcdb22d..6fae66f 100644 >> > --- a/drivers/soc/mediatek/Kconfig >> > +++ b/drivers/soc/mediatek/Kconfig >> > @@ -9,3 +9,12 @@ config MTK_PMIC_WRAP >> > Say yes here to add support for MediaTek PMIC Wrapper found >> > on different MediaTek SoCs. The PMIC wrapper is a proprietary >> > hardware to connect the PMIC. >> > + >> > +config MTK_INFRACFG >> >> nit: Could you alphabetize these config options - so this one before >> MTK_PMIC_WRAP >> >> > + tristate "MediaTek INFRACFG Support" >> > + depends on ARCH_MEDIATEK >> >> I've seen several drivers like this now: >> >> depends on ARCH_MEDIATEK || COMPILE_TEST >> >> >> > + select REGMAP >> > + help >> > + Say yes here to add support for the MediaTek INFRACFG controller. The >> > + INFRACFG controller contains various infrastructure registers not >> > + directly associated to any device. >> > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile >> > index ecaf4de..ce39119 100644 >> > --- a/drivers/soc/mediatek/Makefile >> > +++ b/drivers/soc/mediatek/Makefile >> > @@ -1 +1,2 @@ >> > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o >> > +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o >> >> alphabetize here, too. >> >> > diff --git a/drivers/soc/mediatek/mtk-infracfg.c b/drivers/soc/mediatek/mtk-infracfg.c >> > new file mode 100644 >> > index 0000000..b3ebfae >> > --- /dev/null >> > +++ b/drivers/soc/mediatek/mtk-infracfg.c >> > @@ -0,0 +1,80 @@ >> > +#include <linux/regmap.h> >> > +#include <linux/export.h> >> > +#include <linux/jiffies.h> >> > +#include <linux/soc/mediatek/infracfg.h> >> > +#include <asm/processor.h> >> >> and... alphabetize headers here. >> >> I'm not sure if people care, but I find it makes it much easier to >> merge/add things later if these lists are already sorted. >> Same "please alphabetize" comments for the mtk-scpsys patch, so I >> won't repeat them. >> >> > + >> > +#define INFRA_TOPAXI_PROTECTEN 0x0220 >> > +#define INFRA_TOPAXI_PROTECTSTA1 0x0228 >> > + >> > +/** >> > + * mtk_infracfg_set_bus_protection - enable bus protection >> > + * @regmap: The infracfg regmap >> > + * @mask: The mask containing the protection bits to be enabled. >> > + * >> > + * This function enables the bus protection bits for disabled power >> > + * domains so that the system does not hanf when some unit accesses the >> > + * bus while in power down. >> > + */ >> > +int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask) >> > +{ >> > + unsigned long expired; >> > + u32 val; >> > + int ret; >> > + >> > + regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, mask); >> > + >> > + expired = jiffies + HZ; >> > + >> > + while (1) { >> > + ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val); >> > + if (ret) >> > + return ret; >> > + >> > + if ((val & mask) == mask) >> > + break; >> > + >> > + cpu_relax(); >> > + if (time_after(jiffies, expired)) >> > + return -EIO; >> >> I think we should check for timeout first, and then cpu_relax() if >> there is still time left (here and in >> mtk_infracfg_clear_bus_protection()). Otherwise we end up doing one >> final cpu_relax() without rechecking the register we are polling >> (again, I have the same comment for the timeout loops in mtk-scpsys). > > I think cpu_relax() delays execution in the order of microseconds (I > don't actually know, just a guess), so if the timeout is a second the > order doesn't really matter. What can happen though is an interrupt > after the (val & mask) test but before the timeout check. So to be > truly correct we have to repeat the (val & mask) test after the > time_after() check. Is that what you want? I'm not following, why would you need to repeat (val & mask) test after time_after? What does an interrupt have to do with it? Can you show a code snippet with what exactly you are proposing? -Dan >> Also, shouldn't we return -ETIMEOUT if we timeout? > > I dunno. Probably the operation operation timed out because of an IO > error. I'll change it to -ETIMEDOUT. > > Sascha > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue, May 19, 2015 at 02:54:41PM +0800, Daniel Kurtz wrote: > >> > + while (1) { > >> > + ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val); > >> > + if (ret) > >> > + return ret; > >> > + > >> > + if ((val & mask) == mask) > >> > + break; > >> > + > >> > + cpu_relax(); > >> > + if (time_after(jiffies, expired)) > >> > + return -EIO; > >> > >> I think we should check for timeout first, and then cpu_relax() if > >> there is still time left (here and in > >> mtk_infracfg_clear_bus_protection()). Otherwise we end up doing one > >> final cpu_relax() without rechecking the register we are polling > >> (again, I have the same comment for the timeout loops in mtk-scpsys). > > > > I think cpu_relax() delays execution in the order of microseconds (I > > don't actually know, just a guess), so if the timeout is a second the > > order doesn't really matter. What can happen though is an interrupt > > after the (val & mask) test but before the timeout check. So to be > > truly correct we have to repeat the (val & mask) test after the > > time_after() check. Is that what you want? > > I'm not following, why would you need to repeat (val & mask) test > after time_after? > What does an interrupt have to do with it? > Can you show a code snippet with what exactly you are proposing? Consider you have this timeout loop: while (1) { if (success()) break; if (time_after(jiffies, expired)) return -ETIMEDOUT; } Now when an interupt comes in between success() and time_after() then it can happen that the delay caused by the interrupt makes the code timeout even though success() might have become true in the meantime. So to be correct you have to: while (1) { if (success()) break; if (time_after(jiffies, expired)) { if (success()) break; return -ETIMEDOUT; } Or, if you don't want to repeat the termination condition: bool timeout = false; while (1) { if (success()) break; if (timeout) return -ETIMEDOUT; if (time_after(jiffies, expired)) timeout = true; } Anyway, with the timeout of one second used here this is all academic. Sascha
On Fri, May 15, 2015 at 10:17:21PM +0800, Daniel Kurtz wrote: > On Tue, May 12, 2015 at 3:23 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > > This adds a power domain driver for the Mediatek SCPSYS unit. > > > > The System Control Processor System (SCPSYS) has several power > > management related tasks in the system. The tasks include thermal > > measurement, dynamic voltage frequency scaling (DVFS), interrupt > > filter and lowlevel sleep control. The System Power Manager (SPM) > > inside the SCPSYS is for the MTCMOS power domain control. > > > > For now this driver only adds power domain support, the more > > advanced features are not yet supported. The driver implements > > the generic PM domain device tree bindings, the first user will > > most likely be the Mediatek AFE audio driver. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > .../devicetree/bindings/soc/mediatek/scpsys.txt | 2 + > > drivers/soc/mediatek/Kconfig | 8 + > > drivers/soc/mediatek/Makefile | 1 + > > drivers/soc/mediatek/mtk-scpsys.c | 416 +++++++++++++++++++++ > > include/dt-bindings/power/mt8173-power.h | 15 + > > 5 files changed, 442 insertions(+) > > create mode 100644 drivers/soc/mediatek/mtk-scpsys.c > > create mode 100644 include/dt-bindings/power/mt8173-power.h > > > > diff --git a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt > > index 4764a03..87f2091 100644 > > --- a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt > > +++ b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt > > @@ -15,6 +15,7 @@ Required properties: > > - compatible: Must be "mediatek,mt8173-scpsys" > > - #power-domain-cells: Must be 1 > > - reg: Address range of the SCPSYS unit > > +- infracfg: must contain a phandle to the infracfg controller > > > > Example: > > > > @@ -22,6 +23,7 @@ Example: > > #power-domain-cells = <1>; > > compatible = "mediatek,mt8173-scpsys"; > > reg = <0 0x10006000 0 0x1000>; > > + infracfg = <&infracfg>; > > }; > > > > Example consumer: > > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig > > index 6fae66f..1386c79 100644 > > --- a/drivers/soc/mediatek/Kconfig > > +++ b/drivers/soc/mediatek/Kconfig > > @@ -18,3 +18,11 @@ config MTK_INFRACFG > > Say yes here to add support for the MediaTek INFRACFG controller. The > > INFRACFG controller contains various infrastructure registers not > > directly associated to any device. > > + > > +config MTK_SCPSYS > > + tristate "MediaTek SCPSYS Support" > > + depends on MTK_INFRACFG > > Should this be: > depends on ARCH_MEDIATEK || COMPILE_TEST > selects MTK_INFRACFG > > > + select REGMAP > > + help > > + Say yes here to add support for the MediaTek SCPSYS power domain > > + driver. > > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile > > index ce39119..f8eebab 100644 > > --- a/drivers/soc/mediatek/Makefile > > +++ b/drivers/soc/mediatek/Makefile > > @@ -1,2 +1,3 @@ > > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o > > obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o > > +obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o > > diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c > > new file mode 100644 > > index 0000000..c42c7f1 > > --- /dev/null > > +++ b/drivers/soc/mediatek/mtk-scpsys.c > > @@ -0,0 +1,416 @@ > > +/* > > + * Copyright (c) 2015 Pengutronix, Sascha Hauer <kernel@pengutronix.de> > > + * > > + * 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/clk.h> > > +#include <linux/io.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/of_device.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/pm_domain.h> > > +#include <linux/delay.h> > > +#include <linux/soc/mediatek/infracfg.h> > > +#include <dt-bindings/power/mt8173-power.h> > > +#include <linux/mfd/syscon.h> > > + > > +#define SPM_VDE_PWR_CON 0x0210 > > +#define SPM_MFG_PWR_CON 0x0214 > > +#define SPM_VEN_PWR_CON 0x0230 > > +#define SPM_ISP_PWR_CON 0x0238 > > +#define SPM_DIS_PWR_CON 0x023c > > +#define SPM_VEN2_PWR_CON 0x0298 > > +#define SPM_AUDIO_PWR_CON 0x029c > > +#define SPM_MFG_2D_PWR_CON 0x02c0 > > +#define SPM_MFG_ASYNC_PWR_CON 0x02c4 > > +#define SPM_USB_PWR_CON 0x02cc > > +#define SPM_PWR_STATUS 0x060c > > +#define SPM_PWR_STATUS_2ND 0x0610 > > + > > +#define PWR_RST_B_BIT BIT(0) > > +#define PWR_ISO_BIT BIT(1) > > +#define PWR_ON_BIT BIT(2) > > +#define PWR_ON_2ND_BIT BIT(3) > > +#define PWR_CLK_DIS_BIT BIT(4) > > + > > +#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 VEN2_PWR_STA_MASK BIT(20) > > +#define VEN_PWR_STA_MASK BIT(21) > > +#define MFG_2D_PWR_STA_MASK BIT(22) > > +#define MFG_ASYNC_PWR_STA_MASK BIT(23) > > +#define AUDIO_PWR_STA_MASK BIT(24) > > +#define USB_PWR_STA_MASK BIT(25) > > + > > +struct scp_domain_data { > > + const char *name; > > + u32 sta_mask; > > + int ctl_offs; > > + u32 sram_pdn_bits; > > + u32 sram_pdn_ack_bits; > > + u32 bus_prot_mask; > > + int id; > > + const char *clk_name; > > +}; > > These fields are only used at init time: > name (the pointer, the actual string is pointed to by genpd->name) > id > clk_name > > You could split those off into a separate __initconst struct. Is this really worth it? > > > + > > +static const struct scp_domain_data scp_domain_data[] = { > > + { > > + .id = MT8173_POWER_DOMAIN_VDE, > > + .name = "vde", > > + .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_name = "vdec", > > + }, { > > + .id = MT8173_POWER_DOMAIN_VEN, > > Is there any reason you do not keep these in ".id" order: > VDE, MFG, VEN, ISP, DIS, VEN2, AUDIO, MFG_2D, MFG_ASYNC, USB The reason is simply that with CONFIG_PM disabled we have to turn on the domains in the correct order, otherwise the system hangs. I can renumber the defines though if you like. > > + }, { > > + .id = MT8173_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), > > + }, { > > + .id = MT8173_POWER_DOMAIN_DIS, > > + .name = "disp", > > Perhaps .id/.name/.clk should be "MM" to match the corresponding clock > define (CLK_TOP_MM_SEL) & prot bits. > > > + .sta_mask = DIS_PWR_STA_MASK, > > + .ctl_offs = SPM_DIS_PWR_CON, > > + .sram_pdn_bits = GENMASK(11, 8), > > + .sram_pdn_ack_bits = GENMASK(12, 12), > > + .clk_name = "disp", > > + .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MM_M0 | > > + MT8173_TOP_AXI_PROT_EN_MM_M1, > > + }, { > > + .id = MT8173_POWER_DOMAIN_VEN2, > > + .name = "ven2", > > Perhaps .id/.name/.clk should be "VENC_LT"/"venc_lt" to match the > corresponding clock define (CLK_TOP_VENC_LT_SEL). > > > + .sta_mask = VEN2_PWR_STA_MASK, > > + .ctl_offs = SPM_VEN2_PWR_CON, > > + .sram_pdn_bits = GENMASK(11, 8), > > + .sram_pdn_ack_bits = GENMASK(15, 12), > > + .clk_name = "ven2", > > + }, { > > + .id = MT8173_POWER_DOMAIN_AUDIO, > > + .name = "audio", > > + .sta_mask = AUDIO_PWR_STA_MASK, > > + .ctl_offs = SPM_AUDIO_PWR_CON, > > + .sram_pdn_bits = GENMASK(11, 8), > > + .sram_pdn_ack_bits = GENMASK(15, 12), > > + }, { > > + .id = MT8173_POWER_DOMAIN_MFG_ASYNC, > > + .name = "mfg_async", > > + .sta_mask = MFG_ASYNC_PWR_STA_MASK, > > + .ctl_offs = SPM_MFG_ASYNC_PWR_CON, > > + .sram_pdn_bits = GENMASK(11, 8), > > + .sram_pdn_ack_bits = 0, > > + .clk_name = "mfg", > > + }, { > > + .id = MT8173_POWER_DOMAIN_MFG_2D, > > + .name = "mfg_2d", > > + .sta_mask = MFG_2D_PWR_STA_MASK, > > + .ctl_offs = SPM_MFG_2D_PWR_CON, > > + .sram_pdn_bits = GENMASK(11, 8), > > + .sram_pdn_ack_bits = GENMASK(13, 12), > > + .clk_name = "mfg", > > + }, { > > + .id = MT8173_POWER_DOMAIN_MFG, > > + .name = "mfg", > > + .sta_mask = MFG_PWR_STA_MASK, > > + .ctl_offs = SPM_MFG_PWR_CON, > > + .sram_pdn_bits = GENMASK(13, 8), > > + .sram_pdn_ack_bits = GENMASK(21, 16), > > + .clk_name = "mfg", > > + .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MFG_S | > > + MT8173_TOP_AXI_PROT_EN_MFG_M0 | > > + MT8173_TOP_AXI_PROT_EN_MFG_M1 | > > + MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT, > > + }, { > > + .id = MT8173_POWER_DOMAIN_USB, > > + .name = "usb", > > + .sta_mask = USB_PWR_STA_MASK, > > + .ctl_offs = SPM_USB_PWR_CON, > > + .sram_pdn_bits = GENMASK(11, 8), > > + .sram_pdn_ack_bits = GENMASK(15, 12), > > + }, > > +}; > > + > > +#define NUM_DOMAINS ARRAY_SIZE(scp_domain_data) > > + > > +struct scp; > > + > > +struct scp_domain { > > + struct generic_pm_domain pmd; > > Could you name this "genpd"... that seems to be what most other drivers use. ok > > > + const struct scp_domain_data *data; > > + struct scp *scp; > > + struct clk *clk; > > +}; > > + > > +struct scp { > > + struct scp_domain domains[NUM_DOMAINS]; > > + struct genpd_onecell_data pd_data; > > After probe() you don't use pd_data. > Why do we need it in this struct? Just to save another extra allocation for it. > > > + struct device *dev; > > + void __iomem *base; > > + struct regmap *infracfg; > > +}; > > + > > +static int scpsys_power_on(struct generic_pm_domain *genpd) > > +{ > > + struct scp_domain *scpd = container_of(genpd, struct scp_domain, pmd); > > + struct scp *scp = scpd->scp; > > + const struct scp_domain_data *data = scpd->data; > > + unsigned long expired; > > + void __iomem *ctl_addr = scpd->scp->base + data->ctl_offs; > > + u32 sram_pdn_ack = data->sram_pdn_ack_bits; > > + u32 val; > > + int ret; > > + > > + if (scpd->clk) { > > + ret = clk_prepare_enable(scpd->clk); > > + if (ret) > > + return ret; > > + } > > + > > + val = readl(ctl_addr); > > + val |= PWR_ON_BIT; > > + writel(val, ctl_addr); > > + val |= PWR_ON_2ND_BIT; > > + writel(val, ctl_addr); > > + > > + /* wait until PWR_ACK = 1 */ > > + expired = jiffies + HZ; > > + while (!(readl(scp->base + SPM_PWR_STATUS) & data->sta_mask) || > > + !(readl(scp->base + SPM_PWR_STATUS_2ND) & data->sta_mask)) { > > + cpu_relax(); > > + if (time_after(jiffies, expired)) { > > + ret = -EIO; > > + goto out; > > + } > > + } > > + > > + val &= ~PWR_CLK_DIS_BIT; > > + writel(val, ctl_addr); > > + > > + val &= ~PWR_ISO_BIT; > > + writel(val, ctl_addr); > > + > > + val |= PWR_RST_B_BIT; > > + writel(val, ctl_addr); > > + > > + val &= ~data->sram_pdn_bits; > > + writel(val, ctl_addr); > > + > > + /* wait until SRAM_PDN_ACK all 0 */ > > + expired = jiffies + HZ; > > + while (sram_pdn_ack && (readl(ctl_addr) & sram_pdn_ack)) { > > + cpu_relax(); > > + if (time_after(jiffies, expired)) { > > + ret = -EIO; > > + goto out; > > + } > > + } > > + > > + if (data->bus_prot_mask) { > > + ret = mtk_infracfg_clear_bus_protection(scp->infracfg, > > + data->bus_prot_mask); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > +out: > > + dev_err(scp->dev, "Failed to power on domain %s\n", scpd->data->name); > > genpd->name should be sufficient ok > > > + > > + return ret; > > +} > > + > > +static int scpsys_power_off(struct generic_pm_domain *genpd) > > +{ > > + struct scp_domain *scpd = container_of(genpd, struct scp_domain, pmd); > > + struct scp *scp = scpd->scp; > > + const struct scp_domain_data *data = scpd->data; > > + unsigned long expired; > > + void __iomem *ctl_addr = scpd->scp->base + data->ctl_offs; > > + u32 sram_pdn_ack = data->sram_pdn_ack_bits; > > + u32 val; > > + int ret; > > + > > + if (data->bus_prot_mask) { > > + ret = mtk_infracfg_set_bus_protection(scp->infracfg, > > + data->bus_prot_mask); > > + if (ret) > > + return ret; > > + } > > + > > + val = readl(ctl_addr); > > + val |= data->sram_pdn_bits; > > + writel(val, ctl_addr); > > + > > + /* wait until SRAM_PDN_ACK all 1 */ > > + expired = jiffies + HZ; > > + while ((readl(ctl_addr) & sram_pdn_ack) != sram_pdn_ack) { > > + cpu_relax(); > > + if (time_after(jiffies, expired)) { > > + ret = -EIO; > > + goto out; > > + } > > + } > > + > > + val |= PWR_ISO_BIT; > > + writel(val, ctl_addr); > > + > > + val &= ~PWR_RST_B_BIT; > > + writel(val, ctl_addr); > > + > > + val |= PWR_CLK_DIS_BIT; > > + writel(val, ctl_addr); > > + > > + val &= ~PWR_ON_BIT; > > + writel(val, ctl_addr); > > + > > + val &= ~PWR_ON_2ND_BIT; > > + writel(val, ctl_addr); > > + > > + /* wait until PWR_ACK = 0 */ > > + expired = jiffies + HZ; > > + while ((readl(scp->base + SPM_PWR_STATUS) & data->sta_mask) || > > + (readl(scp->base + SPM_PWR_STATUS_2ND) & data->sta_mask)) { > > + cpu_relax(); > > + if (time_after(jiffies, expired)) { > > + ret = -EIO; > > + goto out; > > + } > > + } > > + > > + if (scpd->clk) > > + clk_disable_unprepare(scpd->clk); > > + > > + return 0; > > + > > +out: > > + dev_err(scp->dev, "Failed to power off domain %s\n", scpd->data->name); > > + > > + return ret; > > +} > > + > > +static int scpsys_probe(struct platform_device *pdev) > > +{ > > + struct genpd_onecell_data *pd_data; > > + struct resource *res; > > + int i; > > + struct scp *scp; > > + > > + scp = devm_kzalloc(&pdev->dev, sizeof(*scp), GFP_KERNEL); > > + if (!scp) > > + return -ENOMEM; > > + > > + scp->dev = &pdev->dev; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + scp->base = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(scp->base)) > > + return PTR_ERR(scp->base); > > + > > + pd_data = &scp->pd_data; > > + > > + pd_data->domains = devm_kzalloc(&pdev->dev, > > + sizeof(*pd_data->domains) * NUM_DOMAINS, GFP_KERNEL); > > + if (!pd_data->domains) > > + return -ENOMEM; > > + > > + scp->infracfg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, > > + "infracfg"); > > + if (IS_ERR(scp->infracfg)) { > > + dev_err(&pdev->dev, "Cannot find infracfg controller: %ld\n", > > + PTR_ERR(scp->infracfg)); > > + return PTR_ERR(scp->infracfg); > > + } > > + > > + pd_data->num_domains = NUM_DOMAINS; > > + > > + for (i = 0; i < NUM_DOMAINS; i++) { > > + struct scp_domain *scpd = &scp->domains[i]; > > + > > + if (scp_domain_data[i].clk_name) { > > + const char *name = scp_domain_data[i].clk_name; > > + > > + scpd->clk = devm_clk_get(&pdev->dev, name); > > + if (IS_ERR(scpd->clk)) { > > + dev_err(&pdev->dev, "Failed to get %s clk: %ld\n", > > + name, PTR_ERR(scpd->clk)); > > + return PTR_ERR(scpd->clk); > > + } > > + } > > + } > > + > > + for (i = 0; i < NUM_DOMAINS; i++) { > > + struct scp_domain *scpd = &scp->domains[i]; > > + struct generic_pm_domain *pmd = &scpd->pmd; > > + > > + pd_data->domains[scp_domain_data[i].id] = pmd; > > + scpd->data = &scp_domain_data[i]; > > + scpd->scp = scp; > > + > > + pmd->name = scp_domain_data[i].name; > > + pmd->power_off = scpsys_power_off; > > + pmd->power_on = scpsys_power_on; > > + pmd->power_off_latency_ns = 20000; > > + pmd->power_on_latency_ns = 20000; > > Where did these latency values come from? >From reducing them to 0 and seeing where the end results end up being. The power domain code increases the times automatically when the values are exceeded. > > > + > > + pm_genpd_init(pmd, NULL, true); > > I'm not sure how this works... does this mean that all power domains > initially off? Some are off, others are on. I'll change that to: - call scpsys_power_on for all domains so I know they are turned on - call pm_genpd_init(pmd, NULL, false); - Let the pm core disable the unused domains in the late_initcall That's the only way I found to properly sync the hardware state with the software state also with regard to the clocks. > > > + > > + /* > > + * If PM is disabled turn on all domains by default so that > > + * consumers can work. > > + */ > > + if (!IS_ENABLED(CONFIG_PM)) > > + pmd->power_on(pmd); > > + } > > + > > + pm_genpd_add_subdomain(&scp->domains[MT8173_POWER_DOMAIN_MFG_ASYNC].pmd, > > + &scp->domains[MT8173_POWER_DOMAIN_MFG_2D].pmd); > > + pm_genpd_add_subdomain(&scp->domains[MT8173_POWER_DOMAIN_MFG_2D].pmd, > > + &scp->domains[MT8173_POWER_DOMAIN_MFG].pmd); > > + > > (1) nit: the continuation lines needs 1 more indent. > > (2) Why aren't you checking for errors? I'll check for errors next round. However, I cannot bail out in this case since once pm_genpd_init() is called the domain cannot be unregistered anymore. Sascha
On Tue, May 19, 2015 at 3:45 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > On Tue, May 19, 2015 at 02:54:41PM +0800, Daniel Kurtz wrote: >> >> > + while (1) { >> >> > + ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val); >> >> > + if (ret) >> >> > + return ret; >> >> > + >> >> > + if ((val & mask) == mask) >> >> > + break; >> >> > + >> >> > + cpu_relax(); >> >> > + if (time_after(jiffies, expired)) >> >> > + return -EIO; >> >> >> >> I think we should check for timeout first, and then cpu_relax() if >> >> there is still time left (here and in >> >> mtk_infracfg_clear_bus_protection()). Otherwise we end up doing one >> >> final cpu_relax() without rechecking the register we are polling >> >> (again, I have the same comment for the timeout loops in mtk-scpsys). >> > >> > I think cpu_relax() delays execution in the order of microseconds (I >> > don't actually know, just a guess), so if the timeout is a second the >> > order doesn't really matter. What can happen though is an interrupt >> > after the (val & mask) test but before the timeout check. So to be >> > truly correct we have to repeat the (val & mask) test after the >> > time_after() check. Is that what you want? >> >> I'm not following, why would you need to repeat (val & mask) test >> after time_after? >> What does an interrupt have to do with it? >> Can you show a code snippet with what exactly you are proposing? > > Consider you have this timeout loop: > > while (1) { > if (success()) > break; > > if (time_after(jiffies, expired)) > return -ETIMEDOUT; > } > > Now when an interupt comes in between success() and time_after() then it > can happen that the delay caused by the interrupt makes the code timeout > even though success() might have become true in the meantime. So to be > correct you have to: I agree - I was confused because you only mentioned repeating the "(val & mask) test", not re-reading the register, which is the important bit. For other drivers, I've seen "wait_for()" macros, like below, which do exactly as you suggest above: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/rockchip-iommu.c#n110 > > while (1) { > if (success()) > break; > > if (time_after(jiffies, expired)) { > if (success()) > break; > return -ETIMEDOUT; > } > > Or, if you don't want to repeat the termination condition: > > bool timeout = false; > > while (1) { > if (success()) > break; > > if (timeout) > return -ETIMEDOUT; > > if (time_after(jiffies, expired)) > timeout = true; > } > > Anyway, with the timeout of one second used here this is all academic. I totally agree that this is academic for the loops here and in SCPSYS, where the timeout is arbitrary and long. -Dan > > Sascha > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
2015-05-19 12:30 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > On Fri, May 15, 2015 at 10:17:21PM +0800, Daniel Kurtz wrote: >> On Tue, May 12, 2015 at 3:23 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote: >> > This adds a power domain driver for the Mediatek SCPSYS unit. >> > >> > The System Control Processor System (SCPSYS) has several power >> > management related tasks in the system. The tasks include thermal >> > measurement, dynamic voltage frequency scaling (DVFS), interrupt >> > filter and lowlevel sleep control. The System Power Manager (SPM) >> > inside the SCPSYS is for the MTCMOS power domain control. >> > >> > For now this driver only adds power domain support, the more >> > advanced features are not yet supported. The driver implements >> > the generic PM domain device tree bindings, the first user will >> > most likely be the Mediatek AFE audio driver. >> > >> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >> > --- >> > .../devicetree/bindings/soc/mediatek/scpsys.txt | 2 + >> > drivers/soc/mediatek/Kconfig | 8 + >> > drivers/soc/mediatek/Makefile | 1 + >> > drivers/soc/mediatek/mtk-scpsys.c | 416 +++++++++++++++++++++ >> > include/dt-bindings/power/mt8173-power.h | 15 + >> > 5 files changed, 442 insertions(+) >> > create mode 100644 drivers/soc/mediatek/mtk-scpsys.c >> > create mode 100644 include/dt-bindings/power/mt8173-power.h >> > >> > diff --git a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt >> > index 4764a03..87f2091 100644 >> > --- a/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt >> > +++ b/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt >> > @@ -15,6 +15,7 @@ Required properties: >> > - compatible: Must be "mediatek,mt8173-scpsys" >> > - #power-domain-cells: Must be 1 >> > - reg: Address range of the SCPSYS unit >> > +- infracfg: must contain a phandle to the infracfg controller >> > >> > Example: >> > >> > @@ -22,6 +23,7 @@ Example: >> > #power-domain-cells = <1>; >> > compatible = "mediatek,mt8173-scpsys"; >> > reg = <0 0x10006000 0 0x1000>; >> > + infracfg = <&infracfg>; >> > }; >> > >> > Example consumer: >> > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig >> > index 6fae66f..1386c79 100644 >> > --- a/drivers/soc/mediatek/Kconfig >> > +++ b/drivers/soc/mediatek/Kconfig >> > @@ -18,3 +18,11 @@ config MTK_INFRACFG >> > Say yes here to add support for the MediaTek INFRACFG controller. The >> > INFRACFG controller contains various infrastructure registers not >> > directly associated to any device. >> > + >> > +config MTK_SCPSYS >> > + tristate "MediaTek SCPSYS Support" >> > + depends on MTK_INFRACFG >> >> Should this be: >> depends on ARCH_MEDIATEK || COMPILE_TEST >> selects MTK_INFRACFG >> >> > + select REGMAP >> > + help >> > + Say yes here to add support for the MediaTek SCPSYS power domain >> > + driver. >> > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile >> > index ce39119..f8eebab 100644 >> > --- a/drivers/soc/mediatek/Makefile >> > +++ b/drivers/soc/mediatek/Makefile >> > @@ -1,2 +1,3 @@ >> > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o >> > obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o >> > +obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o >> > diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c >> > new file mode 100644 >> > index 0000000..c42c7f1 >> > --- /dev/null >> > +++ b/drivers/soc/mediatek/mtk-scpsys.c >> > @@ -0,0 +1,416 @@ >> > +/* >> > + * Copyright (c) 2015 Pengutronix, Sascha Hauer <kernel@pengutronix.de> >> > + * >> > + * 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/clk.h> >> > +#include <linux/io.h> >> > +#include <linux/kernel.h> >> > +#include <linux/module.h> >> > +#include <linux/of_device.h> >> > +#include <linux/platform_device.h> >> > +#include <linux/regmap.h> >> > +#include <linux/pm_domain.h> >> > +#include <linux/delay.h> >> > +#include <linux/soc/mediatek/infracfg.h> >> > +#include <dt-bindings/power/mt8173-power.h> >> > +#include <linux/mfd/syscon.h> >> > + >> > +#define SPM_VDE_PWR_CON 0x0210 >> > +#define SPM_MFG_PWR_CON 0x0214 >> > +#define SPM_VEN_PWR_CON 0x0230 >> > +#define SPM_ISP_PWR_CON 0x0238 >> > +#define SPM_DIS_PWR_CON 0x023c >> > +#define SPM_VEN2_PWR_CON 0x0298 >> > +#define SPM_AUDIO_PWR_CON 0x029c >> > +#define SPM_MFG_2D_PWR_CON 0x02c0 >> > +#define SPM_MFG_ASYNC_PWR_CON 0x02c4 >> > +#define SPM_USB_PWR_CON 0x02cc >> > +#define SPM_PWR_STATUS 0x060c >> > +#define SPM_PWR_STATUS_2ND 0x0610 >> > + >> > +#define PWR_RST_B_BIT BIT(0) >> > +#define PWR_ISO_BIT BIT(1) >> > +#define PWR_ON_BIT BIT(2) >> > +#define PWR_ON_2ND_BIT BIT(3) >> > +#define PWR_CLK_DIS_BIT BIT(4) >> > + >> > +#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 VEN2_PWR_STA_MASK BIT(20) >> > +#define VEN_PWR_STA_MASK BIT(21) >> > +#define MFG_2D_PWR_STA_MASK BIT(22) >> > +#define MFG_ASYNC_PWR_STA_MASK BIT(23) >> > +#define AUDIO_PWR_STA_MASK BIT(24) >> > +#define USB_PWR_STA_MASK BIT(25) >> > + >> > +struct scp_domain_data { >> > + const char *name; >> > + u32 sta_mask; >> > + int ctl_offs; >> > + u32 sram_pdn_bits; >> > + u32 sram_pdn_ack_bits; >> > + u32 bus_prot_mask; >> > + int id; >> > + const char *clk_name; >> > +}; >> >> These fields are only used at init time: >> name (the pointer, the actual string is pointed to by genpd->name) >> id >> clk_name >> >> You could split those off into a separate __initconst struct. > > Is this really worth it? > >> >> > + >> > +static const struct scp_domain_data scp_domain_data[] = { >> > + { >> > + .id = MT8173_POWER_DOMAIN_VDE, >> > + .name = "vde", >> > + .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_name = "vdec", >> > + }, { >> > + .id = MT8173_POWER_DOMAIN_VEN, >> >> Is there any reason you do not keep these in ".id" order: >> VDE, MFG, VEN, ISP, DIS, VEN2, AUDIO, MFG_2D, MFG_ASYNC, USB > > The reason is simply that with CONFIG_PM disabled we have to turn on the > domains in the correct order, otherwise the system hangs. I can renumber > the defines though if you like. > >> > + }, { >> > + .id = MT8173_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), >> > + }, { >> > + .id = MT8173_POWER_DOMAIN_DIS, >> > + .name = "disp", >> >> Perhaps .id/.name/.clk should be "MM" to match the corresponding clock >> define (CLK_TOP_MM_SEL) & prot bits. > > >> >> > + .sta_mask = DIS_PWR_STA_MASK, >> > + .ctl_offs = SPM_DIS_PWR_CON, >> > + .sram_pdn_bits = GENMASK(11, 8), >> > + .sram_pdn_ack_bits = GENMASK(12, 12), >> > + .clk_name = "disp", >> > + .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MM_M0 | >> > + MT8173_TOP_AXI_PROT_EN_MM_M1, >> > + }, { >> > + .id = MT8173_POWER_DOMAIN_VEN2, >> > + .name = "ven2", >> >> Perhaps .id/.name/.clk should be "VENC_LT"/"venc_lt" to match the >> corresponding clock define (CLK_TOP_VENC_LT_SEL). >> >> > + .sta_mask = VEN2_PWR_STA_MASK, >> > + .ctl_offs = SPM_VEN2_PWR_CON, >> > + .sram_pdn_bits = GENMASK(11, 8), >> > + .sram_pdn_ack_bits = GENMASK(15, 12), >> > + .clk_name = "ven2", >> > + }, { >> > + .id = MT8173_POWER_DOMAIN_AUDIO, >> > + .name = "audio", >> > + .sta_mask = AUDIO_PWR_STA_MASK, >> > + .ctl_offs = SPM_AUDIO_PWR_CON, >> > + .sram_pdn_bits = GENMASK(11, 8), >> > + .sram_pdn_ack_bits = GENMASK(15, 12), >> > + }, { >> > + .id = MT8173_POWER_DOMAIN_MFG_ASYNC, >> > + .name = "mfg_async", >> > + .sta_mask = MFG_ASYNC_PWR_STA_MASK, >> > + .ctl_offs = SPM_MFG_ASYNC_PWR_CON, >> > + .sram_pdn_bits = GENMASK(11, 8), >> > + .sram_pdn_ack_bits = 0, >> > + .clk_name = "mfg", >> > + }, { >> > + .id = MT8173_POWER_DOMAIN_MFG_2D, >> > + .name = "mfg_2d", >> > + .sta_mask = MFG_2D_PWR_STA_MASK, >> > + .ctl_offs = SPM_MFG_2D_PWR_CON, >> > + .sram_pdn_bits = GENMASK(11, 8), >> > + .sram_pdn_ack_bits = GENMASK(13, 12), >> > + .clk_name = "mfg", >> > + }, { >> > + .id = MT8173_POWER_DOMAIN_MFG, >> > + .name = "mfg", >> > + .sta_mask = MFG_PWR_STA_MASK, >> > + .ctl_offs = SPM_MFG_PWR_CON, >> > + .sram_pdn_bits = GENMASK(13, 8), >> > + .sram_pdn_ack_bits = GENMASK(21, 16), >> > + .clk_name = "mfg", >> > + .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MFG_S | >> > + MT8173_TOP_AXI_PROT_EN_MFG_M0 | >> > + MT8173_TOP_AXI_PROT_EN_MFG_M1 | >> > + MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT, >> > + }, { >> > + .id = MT8173_POWER_DOMAIN_USB, >> > + .name = "usb", >> > + .sta_mask = USB_PWR_STA_MASK, >> > + .ctl_offs = SPM_USB_PWR_CON, >> > + .sram_pdn_bits = GENMASK(11, 8), >> > + .sram_pdn_ack_bits = GENMASK(15, 12), >> > + }, >> > +}; >> > + >> > +#define NUM_DOMAINS ARRAY_SIZE(scp_domain_data) >> > + >> > +struct scp; >> > + >> > +struct scp_domain { >> > + struct generic_pm_domain pmd; >> >> Could you name this "genpd"... that seems to be what most other drivers use. > > ok > >> >> > + const struct scp_domain_data *data; >> > + struct scp *scp; >> > + struct clk *clk; >> > +}; >> > + >> > +struct scp { >> > + struct scp_domain domains[NUM_DOMAINS]; >> > + struct genpd_onecell_data pd_data; >> >> After probe() you don't use pd_data. >> Why do we need it in this struct? > > Just to save another extra allocation for it. > >> >> > + struct device *dev; >> > + void __iomem *base; >> > + struct regmap *infracfg; >> > +}; >> > + >> > +static int scpsys_power_on(struct generic_pm_domain *genpd) >> > +{ >> > + struct scp_domain *scpd = container_of(genpd, struct scp_domain, pmd); >> > + struct scp *scp = scpd->scp; >> > + const struct scp_domain_data *data = scpd->data; >> > + unsigned long expired; >> > + void __iomem *ctl_addr = scpd->scp->base + data->ctl_offs; >> > + u32 sram_pdn_ack = data->sram_pdn_ack_bits; >> > + u32 val; >> > + int ret; >> > + >> > + if (scpd->clk) { >> > + ret = clk_prepare_enable(scpd->clk); >> > + if (ret) >> > + return ret; >> > + } >> > + >> > + val = readl(ctl_addr); >> > + val |= PWR_ON_BIT; >> > + writel(val, ctl_addr); >> > + val |= PWR_ON_2ND_BIT; >> > + writel(val, ctl_addr); >> > + >> > + /* wait until PWR_ACK = 1 */ >> > + expired = jiffies + HZ; >> > + while (!(readl(scp->base + SPM_PWR_STATUS) & data->sta_mask) || >> > + !(readl(scp->base + SPM_PWR_STATUS_2ND) & data->sta_mask)) { >> > + cpu_relax(); >> > + if (time_after(jiffies, expired)) { >> > + ret = -EIO; >> > + goto out; >> > + } >> > + } >> > + >> > + val &= ~PWR_CLK_DIS_BIT; >> > + writel(val, ctl_addr); >> > + >> > + val &= ~PWR_ISO_BIT; >> > + writel(val, ctl_addr); >> > + >> > + val |= PWR_RST_B_BIT; >> > + writel(val, ctl_addr); >> > + >> > + val &= ~data->sram_pdn_bits; >> > + writel(val, ctl_addr); >> > + >> > + /* wait until SRAM_PDN_ACK all 0 */ >> > + expired = jiffies + HZ; >> > + while (sram_pdn_ack && (readl(ctl_addr) & sram_pdn_ack)) { >> > + cpu_relax(); >> > + if (time_after(jiffies, expired)) { >> > + ret = -EIO; >> > + goto out; >> > + } >> > + } >> > + >> > + if (data->bus_prot_mask) { >> > + ret = mtk_infracfg_clear_bus_protection(scp->infracfg, >> > + data->bus_prot_mask); >> > + if (ret) >> > + return ret; >> > + } >> > + >> > + return 0; >> > +out: >> > + dev_err(scp->dev, "Failed to power on domain %s\n", scpd->data->name); >> >> genpd->name should be sufficient > > ok > >> >> > + >> > + return ret; >> > +} >> > + >> > +static int scpsys_power_off(struct generic_pm_domain *genpd) >> > +{ >> > + struct scp_domain *scpd = container_of(genpd, struct scp_domain, pmd); >> > + struct scp *scp = scpd->scp; >> > + const struct scp_domain_data *data = scpd->data; >> > + unsigned long expired; >> > + void __iomem *ctl_addr = scpd->scp->base + data->ctl_offs; >> > + u32 sram_pdn_ack = data->sram_pdn_ack_bits; >> > + u32 val; >> > + int ret; >> > + >> > + if (data->bus_prot_mask) { >> > + ret = mtk_infracfg_set_bus_protection(scp->infracfg, >> > + data->bus_prot_mask); >> > + if (ret) >> > + return ret; >> > + } >> > + >> > + val = readl(ctl_addr); >> > + val |= data->sram_pdn_bits; >> > + writel(val, ctl_addr); >> > + >> > + /* wait until SRAM_PDN_ACK all 1 */ >> > + expired = jiffies + HZ; >> > + while ((readl(ctl_addr) & sram_pdn_ack) != sram_pdn_ack) { >> > + cpu_relax(); >> > + if (time_after(jiffies, expired)) { >> > + ret = -EIO; >> > + goto out; >> > + } >> > + } >> > + >> > + val |= PWR_ISO_BIT; >> > + writel(val, ctl_addr); >> > + >> > + val &= ~PWR_RST_B_BIT; >> > + writel(val, ctl_addr); >> > + >> > + val |= PWR_CLK_DIS_BIT; >> > + writel(val, ctl_addr); >> > + >> > + val &= ~PWR_ON_BIT; >> > + writel(val, ctl_addr); >> > + >> > + val &= ~PWR_ON_2ND_BIT; >> > + writel(val, ctl_addr); >> > + >> > + /* wait until PWR_ACK = 0 */ >> > + expired = jiffies + HZ; >> > + while ((readl(scp->base + SPM_PWR_STATUS) & data->sta_mask) || >> > + (readl(scp->base + SPM_PWR_STATUS_2ND) & data->sta_mask)) { >> > + cpu_relax(); >> > + if (time_after(jiffies, expired)) { >> > + ret = -EIO; >> > + goto out; >> > + } >> > + } >> > + >> > + if (scpd->clk) >> > + clk_disable_unprepare(scpd->clk); >> > + >> > + return 0; >> > + >> > +out: >> > + dev_err(scp->dev, "Failed to power off domain %s\n", scpd->data->name); >> > + >> > + return ret; >> > +} >> > + >> > +static int scpsys_probe(struct platform_device *pdev) >> > +{ >> > + struct genpd_onecell_data *pd_data; >> > + struct resource *res; >> > + int i; >> > + struct scp *scp; >> > + >> > + scp = devm_kzalloc(&pdev->dev, sizeof(*scp), GFP_KERNEL); >> > + if (!scp) >> > + return -ENOMEM; >> > + >> > + scp->dev = &pdev->dev; >> > + >> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> > + scp->base = devm_ioremap_resource(&pdev->dev, res); >> > + if (IS_ERR(scp->base)) >> > + return PTR_ERR(scp->base); >> > + >> > + pd_data = &scp->pd_data; >> > + >> > + pd_data->domains = devm_kzalloc(&pdev->dev, >> > + sizeof(*pd_data->domains) * NUM_DOMAINS, GFP_KERNEL); >> > + if (!pd_data->domains) >> > + return -ENOMEM; >> > + >> > + scp->infracfg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, >> > + "infracfg"); >> > + if (IS_ERR(scp->infracfg)) { >> > + dev_err(&pdev->dev, "Cannot find infracfg controller: %ld\n", >> > + PTR_ERR(scp->infracfg)); >> > + return PTR_ERR(scp->infracfg); >> > + } >> > + >> > + pd_data->num_domains = NUM_DOMAINS; >> > + >> > + for (i = 0; i < NUM_DOMAINS; i++) { >> > + struct scp_domain *scpd = &scp->domains[i]; >> > + >> > + if (scp_domain_data[i].clk_name) { >> > + const char *name = scp_domain_data[i].clk_name; >> > + >> > + scpd->clk = devm_clk_get(&pdev->dev, name); >> > + if (IS_ERR(scpd->clk)) { >> > + dev_err(&pdev->dev, "Failed to get %s clk: %ld\n", >> > + name, PTR_ERR(scpd->clk)); >> > + return PTR_ERR(scpd->clk); >> > + } >> > + } >> > + } >> > + >> > + for (i = 0; i < NUM_DOMAINS; i++) { >> > + struct scp_domain *scpd = &scp->domains[i]; >> > + struct generic_pm_domain *pmd = &scpd->pmd; >> > + >> > + pd_data->domains[scp_domain_data[i].id] = pmd; >> > + scpd->data = &scp_domain_data[i]; >> > + scpd->scp = scp; >> > + >> > + pmd->name = scp_domain_data[i].name; >> > + pmd->power_off = scpsys_power_off; >> > + pmd->power_on = scpsys_power_on; >> > + pmd->power_off_latency_ns = 20000; >> > + pmd->power_on_latency_ns = 20000; >> >> Where did these latency values come from? > > From reducing them to 0 and seeing where the end results end up being. > The power domain code increases the times automatically when the values > are exceeded. > >> >> > + >> > + pm_genpd_init(pmd, NULL, true); >> >> I'm not sure how this works... does this mean that all power domains >> initially off? > > Some are off, others are on. I'll change that to: > > - call scpsys_power_on for all domains so I know they are turned on > - call pm_genpd_init(pmd, NULL, false); > - Let the pm core disable the unused domains in the late_initcall > > That's the only way I found to properly sync the hardware state with the > software state also with regard to the clocks. > >> >> > + >> > + /* >> > + * If PM is disabled turn on all domains by default so that >> > + * consumers can work. >> > + */ >> > + if (!IS_ENABLED(CONFIG_PM)) >> > + pmd->power_on(pmd); >> > + } >> > + >> > + pm_genpd_add_subdomain(&scp->domains[MT8173_POWER_DOMAIN_MFG_ASYNC].pmd, >> > + &scp->domains[MT8173_POWER_DOMAIN_MFG_2D].pmd); >> > + pm_genpd_add_subdomain(&scp->domains[MT8173_POWER_DOMAIN_MFG_2D].pmd, >> > + &scp->domains[MT8173_POWER_DOMAIN_MFG].pmd); >> > + >> >> (1) nit: the continuation lines needs 1 more indent. >> >> (2) Why aren't you checking for errors? > > I'll check for errors next round. However, I cannot bail out in this > case since once pm_genpd_init() is called the domain cannot be > unregistered anymore. We should think of implementing this. I had a quick look on it, and from what I understand as long as you don't have any subdomains, devices and masters added, it's just deleting an element from a linked list. Maybe we should split this in pm_genpd_prepare and pm_genpd_add, with the last one actually adding the generic_pm_domain to gpd_list. This way we could also get rid of the two for loops. Regrads, Matthias
On Tue, May 19, 2015 at 01:06:10PM +0200, Matthias Brugger wrote: > 2015-05-19 12:30 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > > > > I'll check for errors next round. However, I cannot bail out in this > > case since once pm_genpd_init() is called the domain cannot be > > unregistered anymore. > > We should think of implementing this. > I had a quick look on it, and from what I understand as long as you > don't have any subdomains, devices and masters added, it's just > deleting an element from a linked list. In my error path I would have subdomains added though, so I would need that to be handled. > Maybe we should split this in pm_genpd_prepare and pm_genpd_add, with > the last one actually adding the generic_pm_domain to gpd_list. > > This way we could also get rid of the two for loops. Well, with pm_genpd_prepare and pm_genpd_add I would still need two loops, one for preparing and one for adding. I'm not sure how useful it really is to be able to unregister power domains. For now I suggest to continue with the domains that are successfully registered. I'll look into it should the pm domain maintainers request it. Sascha
2015-05-20 16:03 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > On Tue, May 19, 2015 at 01:06:10PM +0200, Matthias Brugger wrote: >> 2015-05-19 12:30 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: >> > >> > I'll check for errors next round. However, I cannot bail out in this >> > case since once pm_genpd_init() is called the domain cannot be >> > unregistered anymore. >> >> We should think of implementing this. >> I had a quick look on it, and from what I understand as long as you >> don't have any subdomains, devices and masters added, it's just >> deleting an element from a linked list. > > In my error path I would have subdomains added though, so I would need > that to be handled. Well we could bail out, when pm_genpd_add_subdomain fails. But the actual implementations don't do that neither at the moment. > >> Maybe we should split this in pm_genpd_prepare and pm_genpd_add, with >> the last one actually adding the generic_pm_domain to gpd_list. >> >> This way we could also get rid of the two for loops. > > Well, with pm_genpd_prepare and pm_genpd_add I would still need two > loops, one for preparing and one for adding. > > I'm not sure how useful it really is to be able to unregister power > domains. For now I suggest to continue with the domains that are > successfully registered. I'll look into it should the pm domain > maintainers request it. Alright.
Sascha Hauer <s.hauer@pengutronix.de> writes: > This adds support for some miscellaneous bits of the infracfg controller. > The mtk_infracfg_set/clear_bus_protection functions are necessary for > the scpsys power domain driver to handle the bus protection bits which > are contained in the infacfg register space. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> IMO, this feature still needs more documentation. From an earlier exchange with James Liao, he was able to get some more details from the HW designer[1] which I think belong in comments somewhere in this driver. Kevin [1] http://lists.infradead.org/pipermail/linux-mediatek/2015-March/000142.html
On Tue, May 26, 2015 at 04:12:06PM -0700, Kevin Hilman wrote: > Sascha Hauer <s.hauer@pengutronix.de> writes: > > > This adds support for some miscellaneous bits of the infracfg controller. > > The mtk_infracfg_set/clear_bus_protection functions are necessary for > > the scpsys power domain driver to handle the bus protection bits which > > are contained in the infacfg register space. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > IMO, this feature still needs more documentation. From an earlier > exchange with James Liao, he was able to get some more details from the > HW designer[1] which I think belong in comments somewhere in this > driver. Ok, I'll add something like this. Sascha