gpcv2 driver is NOT just used on i.MX7D which has Cortex-A7
cores, but also on i.MX8MQ/i.MX8MM platforms which use Cortex-A53
cores, so let's use A_CORE instread of A7 to avoid confusion.
Signed-off-by: Anson Huang <[email protected]>
---
drivers/soc/imx/gpcv2.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 6ef18cf..0e31465 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -20,14 +20,14 @@
#include <linux/regulator/consumer.h>
#include <dt-bindings/power/imx7-power.h>
-#define GPC_LPCR_A7_BSC 0x000
+#define GPC_LPCR_A_CORE_BSC 0x000
#define GPC_PGC_CPU_MAPPING 0x0ec
-#define USB_HSIC_PHY_A7_DOMAIN BIT(6)
-#define USB_OTG2_PHY_A7_DOMAIN BIT(5)
-#define USB_OTG1_PHY_A7_DOMAIN BIT(4)
-#define PCIE_PHY_A7_DOMAIN BIT(3)
-#define MIPI_PHY_A7_DOMAIN BIT(2)
+#define USB_HSIC_PHY_A_CORE_DOMAIN BIT(6)
+#define USB_OTG2_PHY_A_CORE_DOMAIN BIT(5)
+#define USB_OTG1_PHY_A_CORE_DOMAIN BIT(4)
+#define PCIE_PHY_A_CORE_DOMAIN BIT(3)
+#define MIPI_PHY_A_CORE_DOMAIN BIT(2)
#define GPC_PU_PGC_SW_PUP_REQ 0x0f8
#define GPC_PU_PGC_SW_PDN_REQ 0x104
@@ -167,7 +167,7 @@ static const struct imx7_pgc_domain imx7_pgc_domains[] = {
},
.bits = {
.pxx = MIPI_PHY_SW_Pxx_REQ,
- .map = MIPI_PHY_A7_DOMAIN,
+ .map = MIPI_PHY_A_CORE_DOMAIN,
},
.voltage = 1000000,
.pgc = PGC_MIPI,
@@ -179,7 +179,7 @@ static const struct imx7_pgc_domain imx7_pgc_domains[] = {
},
.bits = {
.pxx = PCIE_PHY_SW_Pxx_REQ,
- .map = PCIE_PHY_A7_DOMAIN,
+ .map = PCIE_PHY_A_CORE_DOMAIN,
},
.voltage = 1000000,
.pgc = PGC_PCIE,
@@ -191,7 +191,7 @@ static const struct imx7_pgc_domain imx7_pgc_domains[] = {
},
.bits = {
.pxx = USB_HSIC_PHY_SW_Pxx_REQ,
- .map = USB_HSIC_PHY_A7_DOMAIN,
+ .map = USB_HSIC_PHY_A_CORE_DOMAIN,
},
.voltage = 1200000,
.pgc = PGC_USB_HSIC,
@@ -261,7 +261,7 @@ builtin_platform_driver(imx7_pgc_domain_driver)
static int imx_gpcv2_probe(struct platform_device *pdev)
{
static const struct regmap_range yes_ranges[] = {
- regmap_reg_range(GPC_LPCR_A7_BSC,
+ regmap_reg_range(GPC_LPCR_A_CORE_BSC,
GPC_M4_PU_PDN_FLG),
regmap_reg_range(GPC_PGC_CTRL(PGC_MIPI),
GPC_PGC_SR(PGC_MIPI)),
--
2.7.4
i.MX8MQ and i.MX8MM share same gpc module with i.MX7D, they
can reuse gpcv2 pgc driver for power domain control, this
patch renames all functions and structure definitions started
with "imx7" to "imx", and check machine type to pass platform
specific power domain data for power domain driver, thus make
gpcv2 pgc driver more generic for i.MX platforms.
Signed-off-by: Anson Huang <[email protected]>
---
drivers/soc/imx/gpcv2.c | 68 +++++++++++++++++++++++++++++--------------------
1 file changed, 40 insertions(+), 28 deletions(-)
diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 0e31465..0e33cb5 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -53,7 +53,7 @@
#define GPC_PGC_CTRL_PCR BIT(0)
-struct imx7_pgc_domain {
+struct imx_pgc_domain {
struct generic_pm_domain genpd;
struct regmap *regmap;
struct regulator *regulator;
@@ -69,11 +69,11 @@ struct imx7_pgc_domain {
struct device *dev;
};
-static int imx7_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
+static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
bool on)
{
- struct imx7_pgc_domain *domain = container_of(genpd,
- struct imx7_pgc_domain,
+ struct imx_pgc_domain *domain = container_of(genpd,
+ struct imx_pgc_domain,
genpd);
unsigned int offset = on ?
GPC_PU_PGC_SW_PUP_REQ : GPC_PU_PGC_SW_PDN_REQ;
@@ -150,17 +150,17 @@ static int imx7_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
return ret;
}
-static int imx7_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd)
+static int imx_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd)
{
- return imx7_gpc_pu_pgc_sw_pxx_req(genpd, true);
+ return imx_gpc_pu_pgc_sw_pxx_req(genpd, true);
}
-static int imx7_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd)
+static int imx_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd)
{
- return imx7_gpc_pu_pgc_sw_pxx_req(genpd, false);
+ return imx_gpc_pu_pgc_sw_pxx_req(genpd, false);
}
-static const struct imx7_pgc_domain imx7_pgc_domains[] = {
+static const struct imx_pgc_domain imx7_pgc_domains[] = {
[IMX7_POWER_DOMAIN_MIPI_PHY] = {
.genpd = {
.name = "mipi-phy",
@@ -198,9 +198,9 @@ static const struct imx7_pgc_domain imx7_pgc_domains[] = {
},
};
-static int imx7_pgc_domain_probe(struct platform_device *pdev)
+static int imx_pgc_domain_probe(struct platform_device *pdev)
{
- struct imx7_pgc_domain *domain = pdev->dev.platform_data;
+ struct imx_pgc_domain *domain = pdev->dev.platform_data;
int ret;
domain->dev = &pdev->dev;
@@ -233,9 +233,9 @@ static int imx7_pgc_domain_probe(struct platform_device *pdev)
return ret;
}
-static int imx7_pgc_domain_remove(struct platform_device *pdev)
+static int imx_pgc_domain_remove(struct platform_device *pdev)
{
- struct imx7_pgc_domain *domain = pdev->dev.platform_data;
+ struct imx_pgc_domain *domain = pdev->dev.platform_data;
of_genpd_del_provider(domain->dev->of_node);
pm_genpd_remove(&domain->genpd);
@@ -243,23 +243,24 @@ static int imx7_pgc_domain_remove(struct platform_device *pdev)
return 0;
}
-static const struct platform_device_id imx7_pgc_domain_id[] = {
- { "imx7-pgc-domain", },
+static const struct platform_device_id imx_pgc_domain_id[] = {
+ { "imx-pgc-domain", },
{ },
};
-static struct platform_driver imx7_pgc_domain_driver = {
+static struct platform_driver imx_pgc_domain_driver = {
.driver = {
- .name = "imx7-pgc",
+ .name = "imx-pgc",
},
- .probe = imx7_pgc_domain_probe,
- .remove = imx7_pgc_domain_remove,
- .id_table = imx7_pgc_domain_id,
+ .probe = imx_pgc_domain_probe,
+ .remove = imx_pgc_domain_remove,
+ .id_table = imx_pgc_domain_id,
};
-builtin_platform_driver(imx7_pgc_domain_driver)
+builtin_platform_driver(imx_pgc_domain_driver)
static int imx_gpcv2_probe(struct platform_device *pdev)
{
+ static const struct imx_pgc_domain *imx_pgc_domains;
static const struct regmap_range yes_ranges[] = {
regmap_reg_range(GPC_LPCR_A_CORE_BSC,
GPC_M4_PU_PDN_FLG),
@@ -287,6 +288,7 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
struct regmap *regmap;
struct resource *res;
void __iomem *base;
+ int pgc_max_index;
int ret;
pgc_np = of_get_child_by_name(dev->of_node, "pgc");
@@ -307,9 +309,19 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
return ret;
}
+ if (of_machine_is_compatible("fsl,imx7d")) {
+ pgc_max_index = ARRAY_SIZE(imx7_pgc_domains);
+ imx_pgc_domains = imx7_pgc_domains;
+ }
+
+ if (!imx_pgc_domains) {
+ dev_err(&pdev->dev, "no device match found\n");
+ return -ENODEV;
+ }
+
for_each_child_of_node(pgc_np, np) {
struct platform_device *pd_pdev;
- struct imx7_pgc_domain *domain;
+ struct imx_pgc_domain *domain;
u32 domain_index;
ret = of_property_read_u32(np, "reg", &domain_index);
@@ -319,14 +331,14 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
return ret;
}
- if (domain_index >= ARRAY_SIZE(imx7_pgc_domains)) {
+ if (domain_index >= pgc_max_index) {
dev_warn(dev,
"Domain index %d is out of bounds\n",
domain_index);
continue;
}
- pd_pdev = platform_device_alloc("imx7-pgc-domain",
+ pd_pdev = platform_device_alloc("imx-pgc-domain",
domain_index);
if (!pd_pdev) {
dev_err(dev, "Failed to allocate platform device\n");
@@ -335,8 +347,8 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
}
ret = platform_device_add_data(pd_pdev,
- &imx7_pgc_domains[domain_index],
- sizeof(imx7_pgc_domains[domain_index]));
+ &imx_pgc_domains[domain_index],
+ sizeof(imx_pgc_domains[domain_index]));
if (ret) {
platform_device_put(pd_pdev);
of_node_put(np);
@@ -345,8 +357,8 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
domain = pd_pdev->dev.platform_data;
domain->regmap = regmap;
- domain->genpd.power_on = imx7_gpc_pu_pgc_sw_pup_req;
- domain->genpd.power_off = imx7_gpc_pu_pgc_sw_pdn_req;
+ domain->genpd.power_on = imx_gpc_pu_pgc_sw_pup_req;
+ domain->genpd.power_off = imx_gpc_pu_pgc_sw_pdn_req;
pd_pdev->dev.parent = dev;
pd_pdev->dev.of_node = np;
--
2.7.4
Andrey,
Are you fine with these two patches?
Shawn
On Mon, Aug 06, 2018 at 02:39:48PM +0800, Anson Huang wrote:
> gpcv2 driver is NOT just used on i.MX7D which has Cortex-A7
> cores, but also on i.MX8MQ/i.MX8MM platforms which use Cortex-A53
> cores, so let's use A_CORE instread of A7 to avoid confusion.
>
> Signed-off-by: Anson Huang <[email protected]>
> ---
> drivers/soc/imx/gpcv2.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index 6ef18cf..0e31465 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -20,14 +20,14 @@
> #include <linux/regulator/consumer.h>
> #include <dt-bindings/power/imx7-power.h>
>
> -#define GPC_LPCR_A7_BSC 0x000
> +#define GPC_LPCR_A_CORE_BSC 0x000
>
> #define GPC_PGC_CPU_MAPPING 0x0ec
> -#define USB_HSIC_PHY_A7_DOMAIN BIT(6)
> -#define USB_OTG2_PHY_A7_DOMAIN BIT(5)
> -#define USB_OTG1_PHY_A7_DOMAIN BIT(4)
> -#define PCIE_PHY_A7_DOMAIN BIT(3)
> -#define MIPI_PHY_A7_DOMAIN BIT(2)
> +#define USB_HSIC_PHY_A_CORE_DOMAIN BIT(6)
> +#define USB_OTG2_PHY_A_CORE_DOMAIN BIT(5)
> +#define USB_OTG1_PHY_A_CORE_DOMAIN BIT(4)
> +#define PCIE_PHY_A_CORE_DOMAIN BIT(3)
> +#define MIPI_PHY_A_CORE_DOMAIN BIT(2)
>
> #define GPC_PU_PGC_SW_PUP_REQ 0x0f8
> #define GPC_PU_PGC_SW_PDN_REQ 0x104
> @@ -167,7 +167,7 @@ static const struct imx7_pgc_domain imx7_pgc_domains[] = {
> },
> .bits = {
> .pxx = MIPI_PHY_SW_Pxx_REQ,
> - .map = MIPI_PHY_A7_DOMAIN,
> + .map = MIPI_PHY_A_CORE_DOMAIN,
> },
> .voltage = 1000000,
> .pgc = PGC_MIPI,
> @@ -179,7 +179,7 @@ static const struct imx7_pgc_domain imx7_pgc_domains[] = {
> },
> .bits = {
> .pxx = PCIE_PHY_SW_Pxx_REQ,
> - .map = PCIE_PHY_A7_DOMAIN,
> + .map = PCIE_PHY_A_CORE_DOMAIN,
> },
> .voltage = 1000000,
> .pgc = PGC_PCIE,
> @@ -191,7 +191,7 @@ static const struct imx7_pgc_domain imx7_pgc_domains[] = {
> },
> .bits = {
> .pxx = USB_HSIC_PHY_SW_Pxx_REQ,
> - .map = USB_HSIC_PHY_A7_DOMAIN,
> + .map = USB_HSIC_PHY_A_CORE_DOMAIN,
> },
> .voltage = 1200000,
> .pgc = PGC_USB_HSIC,
> @@ -261,7 +261,7 @@ builtin_platform_driver(imx7_pgc_domain_driver)
> static int imx_gpcv2_probe(struct platform_device *pdev)
> {
> static const struct regmap_range yes_ranges[] = {
> - regmap_reg_range(GPC_LPCR_A7_BSC,
> + regmap_reg_range(GPC_LPCR_A_CORE_BSC,
> GPC_M4_PU_PDN_FLG),
> regmap_reg_range(GPC_PGC_CTRL(PGC_MIPI),
> GPC_PGC_SR(PGC_MIPI)),
> --
> 2.7.4
>
On Sun, Aug 5, 2018 at 11:45 PM Anson Huang <[email protected]> wrote:
>
> i.MX8MQ and i.MX8MM share same gpc module with i.MX7D, they
> can reuse gpcv2 pgc driver for power domain control, this
> patch renames all functions and structure definitions started
> with "imx7" to "imx", and check machine type to pass platform
> specific power domain data for power domain driver, thus make
> gpcv2 pgc driver more generic for i.MX platforms.
>
Just for the sake of
> Signed-off-by: Anson Huang <[email protected]>
> ---
> drivers/soc/imx/gpcv2.c | 68 +++++++++++++++++++++++++++++--------------------
> 1 file changed, 40 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index 0e31465..0e33cb5 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -53,7 +53,7 @@
>
> #define GPC_PGC_CTRL_PCR BIT(0)
>
> -struct imx7_pgc_domain {
> +struct imx_pgc_domain {
> struct generic_pm_domain genpd;
> struct regmap *regmap;
> struct regulator *regulator;
> @@ -69,11 +69,11 @@ struct imx7_pgc_domain {
> struct device *dev;
> };
>
> -static int imx7_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
> +static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
> bool on)
> {
> - struct imx7_pgc_domain *domain = container_of(genpd,
> - struct imx7_pgc_domain,
> + struct imx_pgc_domain *domain = container_of(genpd,
> + struct imx_pgc_domain,
> genpd);
> unsigned int offset = on ?
> GPC_PU_PGC_SW_PUP_REQ : GPC_PU_PGC_SW_PDN_REQ;
> @@ -150,17 +150,17 @@ static int imx7_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
> return ret;
> }
>
> -static int imx7_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd)
> +static int imx_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd)
> {
> - return imx7_gpc_pu_pgc_sw_pxx_req(genpd, true);
> + return imx_gpc_pu_pgc_sw_pxx_req(genpd, true);
> }
>
> -static int imx7_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd)
> +static int imx_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd)
> {
> - return imx7_gpc_pu_pgc_sw_pxx_req(genpd, false);
> + return imx_gpc_pu_pgc_sw_pxx_req(genpd, false);
> }
>
> -static const struct imx7_pgc_domain imx7_pgc_domains[] = {
> +static const struct imx_pgc_domain imx7_pgc_domains[] = {
> [IMX7_POWER_DOMAIN_MIPI_PHY] = {
> .genpd = {
> .name = "mipi-phy",
> @@ -198,9 +198,9 @@ static const struct imx7_pgc_domain imx7_pgc_domains[] = {
> },
> };
>
> -static int imx7_pgc_domain_probe(struct platform_device *pdev)
> +static int imx_pgc_domain_probe(struct platform_device *pdev)
> {
> - struct imx7_pgc_domain *domain = pdev->dev.platform_data;
> + struct imx_pgc_domain *domain = pdev->dev.platform_data;
> int ret;
>
> domain->dev = &pdev->dev;
> @@ -233,9 +233,9 @@ static int imx7_pgc_domain_probe(struct platform_device *pdev)
> return ret;
> }
>
> -static int imx7_pgc_domain_remove(struct platform_device *pdev)
> +static int imx_pgc_domain_remove(struct platform_device *pdev)
> {
> - struct imx7_pgc_domain *domain = pdev->dev.platform_data;
> + struct imx_pgc_domain *domain = pdev->dev.platform_data;
>
> of_genpd_del_provider(domain->dev->of_node);
> pm_genpd_remove(&domain->genpd);
> @@ -243,23 +243,24 @@ static int imx7_pgc_domain_remove(struct platform_device *pdev)
> return 0;
> }
>
> -static const struct platform_device_id imx7_pgc_domain_id[] = {
> - { "imx7-pgc-domain", },
> +static const struct platform_device_id imx_pgc_domain_id[] = {
> + { "imx-pgc-domain", },
> { },
> };
>
> -static struct platform_driver imx7_pgc_domain_driver = {
> +static struct platform_driver imx_pgc_domain_driver = {
> .driver = {
> - .name = "imx7-pgc",
> + .name = "imx-pgc",
> },
> - .probe = imx7_pgc_domain_probe,
> - .remove = imx7_pgc_domain_remove,
> - .id_table = imx7_pgc_domain_id,
> + .probe = imx_pgc_domain_probe,
> + .remove = imx_pgc_domain_remove,
> + .id_table = imx_pgc_domain_id,
> };
> -builtin_platform_driver(imx7_pgc_domain_driver)
> +builtin_platform_driver(imx_pgc_domain_driver)
>
> static int imx_gpcv2_probe(struct platform_device *pdev)
> {
> + static const struct imx_pgc_domain *imx_pgc_domains;
> static const struct regmap_range yes_ranges[] = {
> regmap_reg_range(GPC_LPCR_A_CORE_BSC,
> GPC_M4_PU_PDN_FLG),
> @@ -287,6 +288,7 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
> struct regmap *regmap;
> struct resource *res;
> void __iomem *base;
> + int pgc_max_index;
> int ret;
>
> pgc_np = of_get_child_by_name(dev->of_node, "pgc");
> @@ -307,9 +309,19 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
> return ret;
> }
>
> + if (of_machine_is_compatible("fsl,imx7d")) {
> + pgc_max_index = ARRAY_SIZE(imx7_pgc_domains);
> + imx_pgc_domains = imx7_pgc_domains;
> + }
Is there any reason to do this explicit call to
of_machine_is_compatible() as opposed to passing necessary data via
.data in imx_gpcv2_dt_ids[]? The latter seems like a more
straightforward way of passing variant specific driver info
> +
> + if (!imx_pgc_domains) {
> + dev_err(&pdev->dev, "no device match found\n");
> + return -ENODEV;
> + }
And doing so would also allow you to drop the check above.
Other that this seems like a reasonable change:
Acked-by: Andrey Smirnov <[email protected]>
Thanks,
Andrey Smirnov
On Sun, Aug 5, 2018 at 11:46 PM Anson Huang <[email protected]> wrote:
>
> gpcv2 driver is NOT just used on i.MX7D which has Cortex-A7
> cores, but also on i.MX8MQ/i.MX8MM platforms which use Cortex-A53
> cores, so let's use A_CORE instread of A7 to avoid confusion.
>
> Signed-off-by: Anson Huang <[email protected]>
Looks reasonable to me:
Acked-by: Andrey Smirnov <[email protected]>
> ---
> drivers/soc/imx/gpcv2.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index 6ef18cf..0e31465 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -20,14 +20,14 @@
> #include <linux/regulator/consumer.h>
> #include <dt-bindings/power/imx7-power.h>
>
> -#define GPC_LPCR_A7_BSC 0x000
> +#define GPC_LPCR_A_CORE_BSC 0x000
>
> #define GPC_PGC_CPU_MAPPING 0x0ec
> -#define USB_HSIC_PHY_A7_DOMAIN BIT(6)
> -#define USB_OTG2_PHY_A7_DOMAIN BIT(5)
> -#define USB_OTG1_PHY_A7_DOMAIN BIT(4)
> -#define PCIE_PHY_A7_DOMAIN BIT(3)
> -#define MIPI_PHY_A7_DOMAIN BIT(2)
> +#define USB_HSIC_PHY_A_CORE_DOMAIN BIT(6)
> +#define USB_OTG2_PHY_A_CORE_DOMAIN BIT(5)
> +#define USB_OTG1_PHY_A_CORE_DOMAIN BIT(4)
> +#define PCIE_PHY_A_CORE_DOMAIN BIT(3)
> +#define MIPI_PHY_A_CORE_DOMAIN BIT(2)
>
> #define GPC_PU_PGC_SW_PUP_REQ 0x0f8
> #define GPC_PU_PGC_SW_PDN_REQ 0x104
> @@ -167,7 +167,7 @@ static const struct imx7_pgc_domain imx7_pgc_domains[] = {
> },
> .bits = {
> .pxx = MIPI_PHY_SW_Pxx_REQ,
> - .map = MIPI_PHY_A7_DOMAIN,
> + .map = MIPI_PHY_A_CORE_DOMAIN,
> },
> .voltage = 1000000,
> .pgc = PGC_MIPI,
> @@ -179,7 +179,7 @@ static const struct imx7_pgc_domain imx7_pgc_domains[] = {
> },
> .bits = {
> .pxx = PCIE_PHY_SW_Pxx_REQ,
> - .map = PCIE_PHY_A7_DOMAIN,
> + .map = PCIE_PHY_A_CORE_DOMAIN,
> },
> .voltage = 1000000,
> .pgc = PGC_PCIE,
> @@ -191,7 +191,7 @@ static const struct imx7_pgc_domain imx7_pgc_domains[] = {
> },
> .bits = {
> .pxx = USB_HSIC_PHY_SW_Pxx_REQ,
> - .map = USB_HSIC_PHY_A7_DOMAIN,
> + .map = USB_HSIC_PHY_A_CORE_DOMAIN,
> },
> .voltage = 1200000,
> .pgc = PGC_USB_HSIC,
> @@ -261,7 +261,7 @@ builtin_platform_driver(imx7_pgc_domain_driver)
> static int imx_gpcv2_probe(struct platform_device *pdev)
> {
> static const struct regmap_range yes_ranges[] = {
> - regmap_reg_range(GPC_LPCR_A7_BSC,
> + regmap_reg_range(GPC_LPCR_A_CORE_BSC,
> GPC_M4_PU_PDN_FLG),
> regmap_reg_range(GPC_PGC_CTRL(PGC_MIPI),
> GPC_PGC_SR(PGC_MIPI)),
> --
> 2.7.4
>
On Sun, Aug 26, 2018 at 7:14 PM Shawn Guo <[email protected]> wrote:
>
> Andrey,
>
> Are you fine with these two patches?
>
I made a small comment on 2/2, but otherwise both patches seem
reasonable (Acks provided in separate reply).
Let me know if you need anything else from me.
Thanks,
Andrey Smirnov
On Mon, Aug 27, 2018 at 3:51 PM Andrey Smirnov <[email protected]> wrote:
>
> On Sun, Aug 5, 2018 at 11:45 PM Anson Huang <[email protected]> wrote:
> >
> > i.MX8MQ and i.MX8MM share same gpc module with i.MX7D, they
> > can reuse gpcv2 pgc driver for power domain control, this
> > patch renames all functions and structure definitions started
> > with "imx7" to "imx", and check machine type to pass platform
> > specific power domain data for power domain driver, thus make
> > gpcv2 pgc driver more generic for i.MX platforms.
> >
>
> Just for the sake of
Oops, forgot to type out the question I had about i.MX8MQ GPC in
general. I've noticed that vendor tree for i.MX8MQ has a separate
driver for GPC that relies on code in ARM Trusted Firmware binary blob
to do the actual switching. Do you by any chances know the relation
between this code and the driver I describe? Are they mutually
exclusive or complimentary (I assume the former)? Will the ATF-based
driver be eventually deprecated?
Thanks,
Andrey Smirnov
>
>
> > Signed-off-by: Anson Huang <[email protected]>
> > ---
> > drivers/soc/imx/gpcv2.c | 68 +++++++++++++++++++++++++++++--------------------
> > 1 file changed, 40 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > index 0e31465..0e33cb5 100644
> > --- a/drivers/soc/imx/gpcv2.c
> > +++ b/drivers/soc/imx/gpcv2.c
> > @@ -53,7 +53,7 @@
> >
> > #define GPC_PGC_CTRL_PCR BIT(0)
> >
> > -struct imx7_pgc_domain {
> > +struct imx_pgc_domain {
> > struct generic_pm_domain genpd;
> > struct regmap *regmap;
> > struct regulator *regulator;
> > @@ -69,11 +69,11 @@ struct imx7_pgc_domain {
> > struct device *dev;
> > };
> >
> > -static int imx7_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
> > +static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
> > bool on)
> > {
> > - struct imx7_pgc_domain *domain = container_of(genpd,
> > - struct imx7_pgc_domain,
> > + struct imx_pgc_domain *domain = container_of(genpd,
> > + struct imx_pgc_domain,
> > genpd);
> > unsigned int offset = on ?
> > GPC_PU_PGC_SW_PUP_REQ : GPC_PU_PGC_SW_PDN_REQ;
> > @@ -150,17 +150,17 @@ static int imx7_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
> > return ret;
> > }
> >
> > -static int imx7_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd)
> > +static int imx_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd)
> > {
> > - return imx7_gpc_pu_pgc_sw_pxx_req(genpd, true);
> > + return imx_gpc_pu_pgc_sw_pxx_req(genpd, true);
> > }
> >
> > -static int imx7_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd)
> > +static int imx_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd)
> > {
> > - return imx7_gpc_pu_pgc_sw_pxx_req(genpd, false);
> > + return imx_gpc_pu_pgc_sw_pxx_req(genpd, false);
> > }
> >
> > -static const struct imx7_pgc_domain imx7_pgc_domains[] = {
> > +static const struct imx_pgc_domain imx7_pgc_domains[] = {
> > [IMX7_POWER_DOMAIN_MIPI_PHY] = {
> > .genpd = {
> > .name = "mipi-phy",
> > @@ -198,9 +198,9 @@ static const struct imx7_pgc_domain imx7_pgc_domains[] = {
> > },
> > };
> >
> > -static int imx7_pgc_domain_probe(struct platform_device *pdev)
> > +static int imx_pgc_domain_probe(struct platform_device *pdev)
> > {
> > - struct imx7_pgc_domain *domain = pdev->dev.platform_data;
> > + struct imx_pgc_domain *domain = pdev->dev.platform_data;
> > int ret;
> >
> > domain->dev = &pdev->dev;
> > @@ -233,9 +233,9 @@ static int imx7_pgc_domain_probe(struct platform_device *pdev)
> > return ret;
> > }
> >
> > -static int imx7_pgc_domain_remove(struct platform_device *pdev)
> > +static int imx_pgc_domain_remove(struct platform_device *pdev)
> > {
> > - struct imx7_pgc_domain *domain = pdev->dev.platform_data;
> > + struct imx_pgc_domain *domain = pdev->dev.platform_data;
> >
> > of_genpd_del_provider(domain->dev->of_node);
> > pm_genpd_remove(&domain->genpd);
> > @@ -243,23 +243,24 @@ static int imx7_pgc_domain_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > -static const struct platform_device_id imx7_pgc_domain_id[] = {
> > - { "imx7-pgc-domain", },
> > +static const struct platform_device_id imx_pgc_domain_id[] = {
> > + { "imx-pgc-domain", },
> > { },
> > };
> >
> > -static struct platform_driver imx7_pgc_domain_driver = {
> > +static struct platform_driver imx_pgc_domain_driver = {
> > .driver = {
> > - .name = "imx7-pgc",
> > + .name = "imx-pgc",
> > },
> > - .probe = imx7_pgc_domain_probe,
> > - .remove = imx7_pgc_domain_remove,
> > - .id_table = imx7_pgc_domain_id,
> > + .probe = imx_pgc_domain_probe,
> > + .remove = imx_pgc_domain_remove,
> > + .id_table = imx_pgc_domain_id,
> > };
> > -builtin_platform_driver(imx7_pgc_domain_driver)
> > +builtin_platform_driver(imx_pgc_domain_driver)
> >
> > static int imx_gpcv2_probe(struct platform_device *pdev)
> > {
> > + static const struct imx_pgc_domain *imx_pgc_domains;
> > static const struct regmap_range yes_ranges[] = {
> > regmap_reg_range(GPC_LPCR_A_CORE_BSC,
> > GPC_M4_PU_PDN_FLG),
> > @@ -287,6 +288,7 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
> > struct regmap *regmap;
> > struct resource *res;
> > void __iomem *base;
> > + int pgc_max_index;
> > int ret;
> >
> > pgc_np = of_get_child_by_name(dev->of_node, "pgc");
> > @@ -307,9 +309,19 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
> > return ret;
> > }
> >
> > + if (of_machine_is_compatible("fsl,imx7d")) {
> > + pgc_max_index = ARRAY_SIZE(imx7_pgc_domains);
> > + imx_pgc_domains = imx7_pgc_domains;
> > + }
>
> Is there any reason to do this explicit call to
> of_machine_is_compatible() as opposed to passing necessary data via
> .data in imx_gpcv2_dt_ids[]? The latter seems like a more
> straightforward way of passing variant specific driver info
>
> > +
> > + if (!imx_pgc_domains) {
> > + dev_err(&pdev->dev, "no device match found\n");
> > + return -ENODEV;
> > + }
>
> And doing so would also allow you to drop the check above.
>
> Other that this seems like a reasonable change:
>
> Acked-by: Andrey Smirnov <[email protected]>
>
> Thanks,
> Andrey Smirnov
Hi, Andrey
Anson Huang
Best Regards!
> -----Original Message-----
> From: Andrey Smirnov <[email protected]>
> Sent: Tuesday, August 28, 2018 7:04 AM
> To: Anson Huang <[email protected]>
> Cc: Shawn Guo <[email protected]>; Sascha Hauer
> <[email protected]>; Sascha Hauer <[email protected]>; Fabio
> Estevam <[email protected]>; linux-arm-kernel
> <[email protected]>; linux-kernel
> <[email protected]>; dl-linux-imx <[email protected]>
> Subject: Re: [PATCH 2/2] soc: imx: gpcv2: make pgc driver more generic for
> other i.MX platforms
>
> On Mon, Aug 27, 2018 at 3:51 PM Andrey Smirnov
> <[email protected]> wrote:
> >
> > On Sun, Aug 5, 2018 at 11:45 PM Anson Huang <[email protected]>
> wrote:
> > >
> > > i.MX8MQ and i.MX8MM share same gpc module with i.MX7D, they can
> > > reuse gpcv2 pgc driver for power domain control, this patch renames
> > > all functions and structure definitions started with "imx7" to
> > > "imx", and check machine type to pass platform specific power domain
> > > data for power domain driver, thus make
> > > gpcv2 pgc driver more generic for i.MX platforms.
> > >
> >
> > Just for the sake of
>
> Oops, forgot to type out the question I had about i.MX8MQ GPC in general. I've
> noticed that vendor tree for i.MX8MQ has a separate driver for GPC that relies
> on code in ARM Trusted Firmware binary blob to do the actual switching. Do
> you by any chances know the relation between this code and the driver I
> describe? Are they mutually exclusive or complimentary (I assume the former)?
> Will the ATF-based driver be eventually deprecated?
Yes, our internal NXP tree currently put all GPC registers operation in ARM Trusted Firmware, and
Linux kernel has a gpc-psci.c which is a virtual GPC driver to call SMC and trap into ARM Trusted Firmware
whenever it wants to read/write GPC registers. But for upstream, we plan to reuse i.MX7D's
GPC driver for power domain control, since the GPC registers for power domain control are
independent with other low power mode's control registers, it is NOT necessary to introduce
another virtual GPC driver to call ARM Trusted Firmware for power domain control.
So yes, ATF-based gpc driver for power domain control will be deprecated when we upstream the ATF for
i.MX8MQ series SoCs. We prefer to reuse the i.MX7D's GPC driver in upstream Linux kernel. If you are OK
with this, I will send out a V2 patch set to address your comments, thanks.
Anson.
>
> Thanks,
> Andrey Smirnov
>
> >
> >
> > > Signed-off-by: Anson Huang <[email protected]>
> > > ---
> > > drivers/soc/imx/gpcv2.c | 68
> > > +++++++++++++++++++++++++++++--------------------
> > > 1 file changed, 40 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c index
> > > 0e31465..0e33cb5 100644
> > > --- a/drivers/soc/imx/gpcv2.c
> > > +++ b/drivers/soc/imx/gpcv2.c
> > > @@ -53,7 +53,7 @@
> > >
> > > #define GPC_PGC_CTRL_PCR BIT(0)
> > >
> > > -struct imx7_pgc_domain {
> > > +struct imx_pgc_domain {
> > > struct generic_pm_domain genpd;
> > > struct regmap *regmap;
> > > struct regulator *regulator; @@ -69,11 +69,11 @@ struct
> > > imx7_pgc_domain {
> > > struct device *dev;
> > > };
> > >
> > > -static int imx7_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain
> > > *genpd,
> > > +static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain
> > > +*genpd,
> > > bool on) {
> > > - struct imx7_pgc_domain *domain = container_of(genpd,
> > > - struct
> imx7_pgc_domain,
> > > + struct imx_pgc_domain *domain = container_of(genpd,
> > > + struct
> > > + imx_pgc_domain,
> > > genpd);
> > > unsigned int offset = on ?
> > > GPC_PU_PGC_SW_PUP_REQ :
> GPC_PU_PGC_SW_PDN_REQ; @@
> > > -150,17 +150,17 @@ static int imx7_gpc_pu_pgc_sw_pxx_req(struct
> generic_pm_domain *genpd,
> > > return ret;
> > > }
> > >
> > > -static int imx7_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain
> > > *genpd)
> > > +static int imx_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain
> > > +*genpd)
> > > {
> > > - return imx7_gpc_pu_pgc_sw_pxx_req(genpd, true);
> > > + return imx_gpc_pu_pgc_sw_pxx_req(genpd, true);
> > > }
> > >
> > > -static int imx7_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain
> > > *genpd)
> > > +static int imx_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain
> > > +*genpd)
> > > {
> > > - return imx7_gpc_pu_pgc_sw_pxx_req(genpd, false);
> > > + return imx_gpc_pu_pgc_sw_pxx_req(genpd, false);
> > > }
> > >
> > > -static const struct imx7_pgc_domain imx7_pgc_domains[] = {
> > > +static const struct imx_pgc_domain imx7_pgc_domains[] = {
> > > [IMX7_POWER_DOMAIN_MIPI_PHY] = {
> > > .genpd = {
> > > .name = "mipi-phy",
> > > @@ -198,9 +198,9 @@ static const struct imx7_pgc_domain
> imx7_pgc_domains[] = {
> > > },
> > > };
> > >
> > > -static int imx7_pgc_domain_probe(struct platform_device *pdev)
> > > +static int imx_pgc_domain_probe(struct platform_device *pdev)
> > > {
> > > - struct imx7_pgc_domain *domain = pdev->dev.platform_data;
> > > + struct imx_pgc_domain *domain = pdev->dev.platform_data;
> > > int ret;
> > >
> > > domain->dev = &pdev->dev;
> > > @@ -233,9 +233,9 @@ static int imx7_pgc_domain_probe(struct
> platform_device *pdev)
> > > return ret;
> > > }
> > >
> > > -static int imx7_pgc_domain_remove(struct platform_device *pdev)
> > > +static int imx_pgc_domain_remove(struct platform_device *pdev)
> > > {
> > > - struct imx7_pgc_domain *domain = pdev->dev.platform_data;
> > > + struct imx_pgc_domain *domain = pdev->dev.platform_data;
> > >
> > > of_genpd_del_provider(domain->dev->of_node);
> > > pm_genpd_remove(&domain->genpd); @@ -243,23 +243,24
> @@
> > > static int imx7_pgc_domain_remove(struct platform_device *pdev)
> > > return 0;
> > > }
> > >
> > > -static const struct platform_device_id imx7_pgc_domain_id[] = {
> > > - { "imx7-pgc-domain", },
> > > +static const struct platform_device_id imx_pgc_domain_id[] = {
> > > + { "imx-pgc-domain", },
> > > { },
> > > };
> > >
> > > -static struct platform_driver imx7_pgc_domain_driver = {
> > > +static struct platform_driver imx_pgc_domain_driver = {
> > > .driver = {
> > > - .name = "imx7-pgc",
> > > + .name = "imx-pgc",
> > > },
> > > - .probe = imx7_pgc_domain_probe,
> > > - .remove = imx7_pgc_domain_remove,
> > > - .id_table = imx7_pgc_domain_id,
> > > + .probe = imx_pgc_domain_probe,
> > > + .remove = imx_pgc_domain_remove,
> > > + .id_table = imx_pgc_domain_id,
> > > };
> > > -builtin_platform_driver(imx7_pgc_domain_driver)
> > > +builtin_platform_driver(imx_pgc_domain_driver)
> > >
> > > static int imx_gpcv2_probe(struct platform_device *pdev) {
> > > + static const struct imx_pgc_domain *imx_pgc_domains;
> > > static const struct regmap_range yes_ranges[] = {
> > > regmap_reg_range(GPC_LPCR_A_CORE_BSC,
> > > GPC_M4_PU_PDN_FLG), @@
> -287,6
> > > +288,7 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
> > > struct regmap *regmap;
> > > struct resource *res;
> > > void __iomem *base;
> > > + int pgc_max_index;
> > > int ret;
> > >
> > > pgc_np = of_get_child_by_name(dev->of_node, "pgc"); @@
> > > -307,9 +309,19 @@ static int imx_gpcv2_probe(struct platform_device
> *pdev)
> > > return ret;
> > > }
> > >
> > > + if (of_machine_is_compatible("fsl,imx7d")) {
> > > + pgc_max_index = ARRAY_SIZE(imx7_pgc_domains);
> > > + imx_pgc_domains = imx7_pgc_domains;
> > > + }
> >
> > Is there any reason to do this explicit call to
> > of_machine_is_compatible() as opposed to passing necessary data via
> > .data in imx_gpcv2_dt_ids[]? The latter seems like a more
> > straightforward way of passing variant specific driver info
> >
> > > +
> > > + if (!imx_pgc_domains) {
> > > + dev_err(&pdev->dev, "no device match found\n");
> > > + return -ENODEV;
> > > + }
> >
> > And doing so would also allow you to drop the check above.
> >
> > Other that this seems like a reasonable change:
> >
> > Acked-by: Andrey Smirnov <[email protected]>
> >
> > Thanks,
> > Andrey Smirnov
On Mon, Aug 27, 2018 at 7:32 PM Anson Huang <[email protected]> wrote:
>
> Hi, Andrey
>
> Anson Huang
> Best Regards!
>
>
> > -----Original Message-----
> > From: Andrey Smirnov <[email protected]>
> > Sent: Tuesday, August 28, 2018 7:04 AM
> > To: Anson Huang <[email protected]>
> > Cc: Shawn Guo <[email protected]>; Sascha Hauer
> > <[email protected]>; Sascha Hauer <[email protected]>; Fabio
> > Estevam <[email protected]>; linux-arm-kernel
> > <[email protected]>; linux-kernel
> > <[email protected]>; dl-linux-imx <[email protected]>
> > Subject: Re: [PATCH 2/2] soc: imx: gpcv2: make pgc driver more generic for
> > other i.MX platforms
> >
> > On Mon, Aug 27, 2018 at 3:51 PM Andrey Smirnov
> > <[email protected]> wrote:
> > >
> > > On Sun, Aug 5, 2018 at 11:45 PM Anson Huang <[email protected]>
> > wrote:
> > > >
> > > > i.MX8MQ and i.MX8MM share same gpc module with i.MX7D, they can
> > > > reuse gpcv2 pgc driver for power domain control, this patch renames
> > > > all functions and structure definitions started with "imx7" to
> > > > "imx", and check machine type to pass platform specific power domain
> > > > data for power domain driver, thus make
> > > > gpcv2 pgc driver more generic for i.MX platforms.
> > > >
> > >
> > > Just for the sake of
> >
> > Oops, forgot to type out the question I had about i.MX8MQ GPC in general. I've
> > noticed that vendor tree for i.MX8MQ has a separate driver for GPC that relies
> > on code in ARM Trusted Firmware binary blob to do the actual switching. Do
> > you by any chances know the relation between this code and the driver I
> > describe? Are they mutually exclusive or complimentary (I assume the former)?
> > Will the ATF-based driver be eventually deprecated?
>
> Yes, our internal NXP tree currently put all GPC registers operation in ARM Trusted Firmware, and
> Linux kernel has a gpc-psci.c which is a virtual GPC driver to call SMC and trap into ARM Trusted Firmware
> whenever it wants to read/write GPC registers. But for upstream, we plan to reuse i.MX7D's
> GPC driver for power domain control, since the GPC registers for power domain control are
> independent with other low power mode's control registers, it is NOT necessary to introduce
> another virtual GPC driver to call ARM Trusted Firmware for power domain control.
>
> So yes, ATF-based gpc driver for power domain control will be deprecated when we upstream the ATF for
> i.MX8MQ series SoCs. We prefer to reuse the i.MX7D's GPC driver in upstream Linux kernel. If you are OK
> with this, I will send out a V2 patch set to address your comments, thanks.
By all means, I have no objections, the more this code can be reused
the better. I just wanted to confirm if I understood the relation
between two drivers and their future right, that's all.
Thanks,
Andrey Smirnov
Anson Huang
Best Regards!
> -----Original Message-----
> From: Andrey Smirnov <[email protected]>
> Sent: Tuesday, August 28, 2018 6:51 AM
> To: Anson Huang <[email protected]>
> Cc: Shawn Guo <[email protected]>; Sascha Hauer
> <[email protected]>; Sascha Hauer <[email protected]>; Fabio
> Estevam <[email protected]>; linux-arm-kernel
> <[email protected]>; linux-kernel
> <[email protected]>; dl-linux-imx <[email protected]>
> Subject: Re: [PATCH 2/2] soc: imx: gpcv2: make pgc driver more generic for
> other i.MX platforms
>
> On Sun, Aug 5, 2018 at 11:45 PM Anson Huang <[email protected]>
> wrote:
> >
> > i.MX8MQ and i.MX8MM share same gpc module with i.MX7D, they can reuse
> > gpcv2 pgc driver for power domain control, this patch renames all
> > functions and structure definitions started with "imx7" to "imx", and
> > check machine type to pass platform specific power domain data for
> > power domain driver, thus make
> > gpcv2 pgc driver more generic for i.MX platforms.
> >
>
> Just for the sake of
>
>
> > Signed-off-by: Anson Huang <[email protected]>
> > ---
> > drivers/soc/imx/gpcv2.c | 68
> > +++++++++++++++++++++++++++++--------------------
> > 1 file changed, 40 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c index
> > 0e31465..0e33cb5 100644
> > --- a/drivers/soc/imx/gpcv2.c
> > +++ b/drivers/soc/imx/gpcv2.c
> > @@ -53,7 +53,7 @@
> >
> > #define GPC_PGC_CTRL_PCR BIT(0)
> >
> > -struct imx7_pgc_domain {
> > +struct imx_pgc_domain {
> > struct generic_pm_domain genpd;
> > struct regmap *regmap;
> > struct regulator *regulator;
> > @@ -69,11 +69,11 @@ struct imx7_pgc_domain {
> > struct device *dev;
> > };
> >
> > -static int imx7_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain
> > *genpd,
> > +static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
> > bool on) {
> > - struct imx7_pgc_domain *domain = container_of(genpd,
> > - struct
> imx7_pgc_domain,
> > + struct imx_pgc_domain *domain = container_of(genpd,
> > + struct
> > + imx_pgc_domain,
> > genpd);
> > unsigned int offset = on ?
> > GPC_PU_PGC_SW_PUP_REQ :
> GPC_PU_PGC_SW_PDN_REQ; @@
> > -150,17 +150,17 @@ static int imx7_gpc_pu_pgc_sw_pxx_req(struct
> generic_pm_domain *genpd,
> > return ret;
> > }
> >
> > -static int imx7_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain
> > *genpd)
> > +static int imx_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd)
> > {
> > - return imx7_gpc_pu_pgc_sw_pxx_req(genpd, true);
> > + return imx_gpc_pu_pgc_sw_pxx_req(genpd, true);
> > }
> >
> > -static int imx7_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain
> > *genpd)
> > +static int imx_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd)
> > {
> > - return imx7_gpc_pu_pgc_sw_pxx_req(genpd, false);
> > + return imx_gpc_pu_pgc_sw_pxx_req(genpd, false);
> > }
> >
> > -static const struct imx7_pgc_domain imx7_pgc_domains[] = {
> > +static const struct imx_pgc_domain imx7_pgc_domains[] = {
> > [IMX7_POWER_DOMAIN_MIPI_PHY] = {
> > .genpd = {
> > .name = "mipi-phy",
> > @@ -198,9 +198,9 @@ static const struct imx7_pgc_domain
> imx7_pgc_domains[] = {
> > },
> > };
> >
> > -static int imx7_pgc_domain_probe(struct platform_device *pdev)
> > +static int imx_pgc_domain_probe(struct platform_device *pdev)
> > {
> > - struct imx7_pgc_domain *domain = pdev->dev.platform_data;
> > + struct imx_pgc_domain *domain = pdev->dev.platform_data;
> > int ret;
> >
> > domain->dev = &pdev->dev;
> > @@ -233,9 +233,9 @@ static int imx7_pgc_domain_probe(struct
> platform_device *pdev)
> > return ret;
> > }
> >
> > -static int imx7_pgc_domain_remove(struct platform_device *pdev)
> > +static int imx_pgc_domain_remove(struct platform_device *pdev)
> > {
> > - struct imx7_pgc_domain *domain = pdev->dev.platform_data;
> > + struct imx_pgc_domain *domain = pdev->dev.platform_data;
> >
> > of_genpd_del_provider(domain->dev->of_node);
> > pm_genpd_remove(&domain->genpd); @@ -243,23 +243,24 @@
> static
> > int imx7_pgc_domain_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > -static const struct platform_device_id imx7_pgc_domain_id[] = {
> > - { "imx7-pgc-domain", },
> > +static const struct platform_device_id imx_pgc_domain_id[] = {
> > + { "imx-pgc-domain", },
> > { },
> > };
> >
> > -static struct platform_driver imx7_pgc_domain_driver = {
> > +static struct platform_driver imx_pgc_domain_driver = {
> > .driver = {
> > - .name = "imx7-pgc",
> > + .name = "imx-pgc",
> > },
> > - .probe = imx7_pgc_domain_probe,
> > - .remove = imx7_pgc_domain_remove,
> > - .id_table = imx7_pgc_domain_id,
> > + .probe = imx_pgc_domain_probe,
> > + .remove = imx_pgc_domain_remove,
> > + .id_table = imx_pgc_domain_id,
> > };
> > -builtin_platform_driver(imx7_pgc_domain_driver)
> > +builtin_platform_driver(imx_pgc_domain_driver)
> >
> > static int imx_gpcv2_probe(struct platform_device *pdev) {
> > + static const struct imx_pgc_domain *imx_pgc_domains;
> > static const struct regmap_range yes_ranges[] = {
> > regmap_reg_range(GPC_LPCR_A_CORE_BSC,
> > GPC_M4_PU_PDN_FLG), @@
> -287,6 +288,7
> > @@ static int imx_gpcv2_probe(struct platform_device *pdev)
> > struct regmap *regmap;
> > struct resource *res;
> > void __iomem *base;
> > + int pgc_max_index;
> > int ret;
> >
> > pgc_np = of_get_child_by_name(dev->of_node, "pgc"); @@ -307,9
> > +309,19 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
> > return ret;
> > }
> >
> > + if (of_machine_is_compatible("fsl,imx7d")) {
> > + pgc_max_index = ARRAY_SIZE(imx7_pgc_domains);
> > + imx_pgc_domains = imx7_pgc_domains;
> > + }
>
> Is there any reason to do this explicit call to
> of_machine_is_compatible() as opposed to passing necessary data via .data in
> imx_gpcv2_dt_ids[]? The latter seems like a more straightforward way of
> passing variant specific driver info
The reason of NOT using .data is that currently gpcv2 driver uses a structure array
of imx7_pgc_domains[] which has no fixed length, the .data can ONLY pass the pointer
of the starting address of imx7_pgc_domains[], and we need to know the array size of
imx7_pgc_domains, how can we know it if we ONLY got the starting address of this array
passed from .data. Please advise, thanks.
Anson.
>
> > +
> > + if (!imx_pgc_domains) {
> > + dev_err(&pdev->dev, "no device match found\n");
> > + return -ENODEV;
> > + }
>
> And doing so would also allow you to drop the check above.
>
> Other that this seems like a reasonable change:
>
> Acked-by: Andrey Smirnov <[email protected]>
>
> Thanks,
> Andrey Smirnov
On Tue, Aug 28, 2018 at 12:28 AM Anson Huang <[email protected]> wrote:
>
>
>
> Anson Huang
> Best Regards!
>
>
> > -----Original Message-----
> > From: Andrey Smirnov <[email protected]>
> > Sent: Tuesday, August 28, 2018 6:51 AM
> > To: Anson Huang <[email protected]>
> > Cc: Shawn Guo <[email protected]>; Sascha Hauer
> > <[email protected]>; Sascha Hauer <[email protected]>; Fabio
> > Estevam <[email protected]>; linux-arm-kernel
> > <[email protected]>; linux-kernel
> > <[email protected]>; dl-linux-imx <[email protected]>
> > Subject: Re: [PATCH 2/2] soc: imx: gpcv2: make pgc driver more generic for
> > other i.MX platforms
> >
> > On Sun, Aug 5, 2018 at 11:45 PM Anson Huang <[email protected]>
> > wrote:
> > >
> > > i.MX8MQ and i.MX8MM share same gpc module with i.MX7D, they can reuse
> > > gpcv2 pgc driver for power domain control, this patch renames all
> > > functions and structure definitions started with "imx7" to "imx", and
> > > check machine type to pass platform specific power domain data for
> > > power domain driver, thus make
> > > gpcv2 pgc driver more generic for i.MX platforms.
> > >
> >
> > Just for the sake of
> >
> >
> > > Signed-off-by: Anson Huang <[email protected]>
> > > ---
> > > drivers/soc/imx/gpcv2.c | 68
> > > +++++++++++++++++++++++++++++--------------------
> > > 1 file changed, 40 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c index
> > > 0e31465..0e33cb5 100644
> > > --- a/drivers/soc/imx/gpcv2.c
> > > +++ b/drivers/soc/imx/gpcv2.c
> > > @@ -53,7 +53,7 @@
> > >
> > > #define GPC_PGC_CTRL_PCR BIT(0)
> > >
> > > -struct imx7_pgc_domain {
> > > +struct imx_pgc_domain {
> > > struct generic_pm_domain genpd;
> > > struct regmap *regmap;
> > > struct regulator *regulator;
> > > @@ -69,11 +69,11 @@ struct imx7_pgc_domain {
> > > struct device *dev;
> > > };
> > >
> > > -static int imx7_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain
> > > *genpd,
> > > +static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
> > > bool on) {
> > > - struct imx7_pgc_domain *domain = container_of(genpd,
> > > - struct
> > imx7_pgc_domain,
> > > + struct imx_pgc_domain *domain = container_of(genpd,
> > > + struct
> > > + imx_pgc_domain,
> > > genpd);
> > > unsigned int offset = on ?
> > > GPC_PU_PGC_SW_PUP_REQ :
> > GPC_PU_PGC_SW_PDN_REQ; @@
> > > -150,17 +150,17 @@ static int imx7_gpc_pu_pgc_sw_pxx_req(struct
> > generic_pm_domain *genpd,
> > > return ret;
> > > }
> > >
> > > -static int imx7_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain
> > > *genpd)
> > > +static int imx_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd)
> > > {
> > > - return imx7_gpc_pu_pgc_sw_pxx_req(genpd, true);
> > > + return imx_gpc_pu_pgc_sw_pxx_req(genpd, true);
> > > }
> > >
> > > -static int imx7_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain
> > > *genpd)
> > > +static int imx_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd)
> > > {
> > > - return imx7_gpc_pu_pgc_sw_pxx_req(genpd, false);
> > > + return imx_gpc_pu_pgc_sw_pxx_req(genpd, false);
> > > }
> > >
> > > -static const struct imx7_pgc_domain imx7_pgc_domains[] = {
> > > +static const struct imx_pgc_domain imx7_pgc_domains[] = {
> > > [IMX7_POWER_DOMAIN_MIPI_PHY] = {
> > > .genpd = {
> > > .name = "mipi-phy",
> > > @@ -198,9 +198,9 @@ static const struct imx7_pgc_domain
> > imx7_pgc_domains[] = {
> > > },
> > > };
> > >
> > > -static int imx7_pgc_domain_probe(struct platform_device *pdev)
> > > +static int imx_pgc_domain_probe(struct platform_device *pdev)
> > > {
> > > - struct imx7_pgc_domain *domain = pdev->dev.platform_data;
> > > + struct imx_pgc_domain *domain = pdev->dev.platform_data;
> > > int ret;
> > >
> > > domain->dev = &pdev->dev;
> > > @@ -233,9 +233,9 @@ static int imx7_pgc_domain_probe(struct
> > platform_device *pdev)
> > > return ret;
> > > }
> > >
> > > -static int imx7_pgc_domain_remove(struct platform_device *pdev)
> > > +static int imx_pgc_domain_remove(struct platform_device *pdev)
> > > {
> > > - struct imx7_pgc_domain *domain = pdev->dev.platform_data;
> > > + struct imx_pgc_domain *domain = pdev->dev.platform_data;
> > >
> > > of_genpd_del_provider(domain->dev->of_node);
> > > pm_genpd_remove(&domain->genpd); @@ -243,23 +243,24 @@
> > static
> > > int imx7_pgc_domain_remove(struct platform_device *pdev)
> > > return 0;
> > > }
> > >
> > > -static const struct platform_device_id imx7_pgc_domain_id[] = {
> > > - { "imx7-pgc-domain", },
> > > +static const struct platform_device_id imx_pgc_domain_id[] = {
> > > + { "imx-pgc-domain", },
> > > { },
> > > };
> > >
> > > -static struct platform_driver imx7_pgc_domain_driver = {
> > > +static struct platform_driver imx_pgc_domain_driver = {
> > > .driver = {
> > > - .name = "imx7-pgc",
> > > + .name = "imx-pgc",
> > > },
> > > - .probe = imx7_pgc_domain_probe,
> > > - .remove = imx7_pgc_domain_remove,
> > > - .id_table = imx7_pgc_domain_id,
> > > + .probe = imx_pgc_domain_probe,
> > > + .remove = imx_pgc_domain_remove,
> > > + .id_table = imx_pgc_domain_id,
> > > };
> > > -builtin_platform_driver(imx7_pgc_domain_driver)
> > > +builtin_platform_driver(imx_pgc_domain_driver)
> > >
> > > static int imx_gpcv2_probe(struct platform_device *pdev) {
> > > + static const struct imx_pgc_domain *imx_pgc_domains;
> > > static const struct regmap_range yes_ranges[] = {
> > > regmap_reg_range(GPC_LPCR_A_CORE_BSC,
> > > GPC_M4_PU_PDN_FLG), @@
> > -287,6 +288,7
> > > @@ static int imx_gpcv2_probe(struct platform_device *pdev)
> > > struct regmap *regmap;
> > > struct resource *res;
> > > void __iomem *base;
> > > + int pgc_max_index;
> > > int ret;
> > >
> > > pgc_np = of_get_child_by_name(dev->of_node, "pgc"); @@ -307,9
> > > +309,19 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
> > > return ret;
> > > }
> > >
> > > + if (of_machine_is_compatible("fsl,imx7d")) {
> > > + pgc_max_index = ARRAY_SIZE(imx7_pgc_domains);
> > > + imx_pgc_domains = imx7_pgc_domains;
> > > + }
> >
> > Is there any reason to do this explicit call to
> > of_machine_is_compatible() as opposed to passing necessary data via .data in
> > imx_gpcv2_dt_ids[]? The latter seems like a more straightforward way of
> > passing variant specific driver info
>
> The reason of NOT using .data is that currently gpcv2 driver uses a structure array
> of imx7_pgc_domains[] which has no fixed length, the .data can ONLY pass the pointer
> of the starting address of imx7_pgc_domains[], and we need to know the array size of
> imx7_pgc_domains, how can we know it if we ONLY got the starting address of this array
> passed from .data. Please advise, thanks.
>
Unless I am missing something, you should be able to do something like:
struct imx_pgc_domain_data {
struct imx_pgc_domain *domains;
size_t domains_num;
};
const struct imx_pgc_domain_data imx7_pgc_domain_data = {
.domains = imx7_pgc_domains,
.domains_num = ARRAY_SIZE(imx7_pgc_domains),
};
and then just pass imx7_pgc_domain_data via .data. That's a pretty
common idiom in kernel driver.
Thanks,
Andrey Smirnov
Anson Huang
Best Regards!
> -----Original Message-----
> From: Andrey Smirnov <[email protected]>
> Sent: Tuesday, August 28, 2018 4:01 PM
> To: Anson Huang <[email protected]>
> Cc: Shawn Guo <[email protected]>; Sascha Hauer
> <[email protected]>; Sascha Hauer <[email protected]>; Fabio
> Estevam <[email protected]>; linux-arm-kernel
> <[email protected]>; linux-kernel
> <[email protected]>; dl-linux-imx <[email protected]>
> Subject: Re: [PATCH 2/2] soc: imx: gpcv2: make pgc driver more generic for
> other i.MX platforms
>
> On Tue, Aug 28, 2018 at 12:28 AM Anson Huang <[email protected]>
> wrote:
> >
> >
> >
> > Anson Huang
> > Best Regards!
> >
> >
> > > -----Original Message-----
> > > From: Andrey Smirnov <[email protected]>
> > > Sent: Tuesday, August 28, 2018 6:51 AM
> > > To: Anson Huang <[email protected]>
> > > Cc: Shawn Guo <[email protected]>; Sascha Hauer
> > > <[email protected]>; Sascha Hauer <[email protected]>;
> > > Fabio Estevam <[email protected]>; linux-arm-kernel
> > > <[email protected]>; linux-kernel
> > > <[email protected]>; dl-linux-imx <[email protected]>
> > > Subject: Re: [PATCH 2/2] soc: imx: gpcv2: make pgc driver more
> > > generic for other i.MX platforms
> > >
> > > On Sun, Aug 5, 2018 at 11:45 PM Anson Huang <[email protected]>
> > > wrote:
> > > >
> > > > i.MX8MQ and i.MX8MM share same gpc module with i.MX7D, they can
> > > > reuse
> > > > gpcv2 pgc driver for power domain control, this patch renames all
> > > > functions and structure definitions started with "imx7" to "imx",
> > > > and check machine type to pass platform specific power domain data
> > > > for power domain driver, thus make
> > > > gpcv2 pgc driver more generic for i.MX platforms.
> > > >
> > >
> > > Just for the sake of
> > >
> > >
> > > > Signed-off-by: Anson Huang <[email protected]>
> > > > ---
> > > > drivers/soc/imx/gpcv2.c | 68
> > > > +++++++++++++++++++++++++++++--------------------
> > > > 1 file changed, 40 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > > > index
> > > > 0e31465..0e33cb5 100644
> > > > --- a/drivers/soc/imx/gpcv2.c
> > > > +++ b/drivers/soc/imx/gpcv2.c
> > > > @@ -53,7 +53,7 @@
> > > >
> > > > #define GPC_PGC_CTRL_PCR BIT(0)
> > > >
> > > > -struct imx7_pgc_domain {
> > > > +struct imx_pgc_domain {
> > > > struct generic_pm_domain genpd;
> > > > struct regmap *regmap;
> > > > struct regulator *regulator; @@ -69,11 +69,11 @@ struct
> > > > imx7_pgc_domain {
> > > > struct device *dev;
> > > > };
> > > >
> > > > -static int imx7_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain
> > > > *genpd,
> > > > +static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain
> > > > +*genpd,
> > > > bool on) {
> > > > - struct imx7_pgc_domain *domain = container_of(genpd,
> > > > - struct
> > > imx7_pgc_domain,
> > > > + struct imx_pgc_domain *domain = container_of(genpd,
> > > > + struct
> > > > + imx_pgc_domain,
> > > >
> genpd);
> > > > unsigned int offset = on ?
> > > > GPC_PU_PGC_SW_PUP_REQ :
> > > GPC_PU_PGC_SW_PDN_REQ; @@
> > > > -150,17 +150,17 @@ static int imx7_gpc_pu_pgc_sw_pxx_req(struct
> > > generic_pm_domain *genpd,
> > > > return ret;
> > > > }
> > > >
> > > > -static int imx7_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain
> > > > *genpd)
> > > > +static int imx_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain
> > > > +*genpd)
> > > > {
> > > > - return imx7_gpc_pu_pgc_sw_pxx_req(genpd, true);
> > > > + return imx_gpc_pu_pgc_sw_pxx_req(genpd, true);
> > > > }
> > > >
> > > > -static int imx7_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain
> > > > *genpd)
> > > > +static int imx_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain
> > > > +*genpd)
> > > > {
> > > > - return imx7_gpc_pu_pgc_sw_pxx_req(genpd, false);
> > > > + return imx_gpc_pu_pgc_sw_pxx_req(genpd, false);
> > > > }
> > > >
> > > > -static const struct imx7_pgc_domain imx7_pgc_domains[] = {
> > > > +static const struct imx_pgc_domain imx7_pgc_domains[] = {
> > > > [IMX7_POWER_DOMAIN_MIPI_PHY] = {
> > > > .genpd = {
> > > > .name = "mipi-phy",
> > > > @@ -198,9 +198,9 @@ static const struct imx7_pgc_domain
> > > imx7_pgc_domains[] = {
> > > > },
> > > > };
> > > >
> > > > -static int imx7_pgc_domain_probe(struct platform_device *pdev)
> > > > +static int imx_pgc_domain_probe(struct platform_device *pdev)
> > > > {
> > > > - struct imx7_pgc_domain *domain = pdev->dev.platform_data;
> > > > + struct imx_pgc_domain *domain = pdev->dev.platform_data;
> > > > int ret;
> > > >
> > > > domain->dev = &pdev->dev;
> > > > @@ -233,9 +233,9 @@ static int imx7_pgc_domain_probe(struct
> > > platform_device *pdev)
> > > > return ret;
> > > > }
> > > >
> > > > -static int imx7_pgc_domain_remove(struct platform_device *pdev)
> > > > +static int imx_pgc_domain_remove(struct platform_device *pdev)
> > > > {
> > > > - struct imx7_pgc_domain *domain = pdev->dev.platform_data;
> > > > + struct imx_pgc_domain *domain = pdev->dev.platform_data;
> > > >
> > > > of_genpd_del_provider(domain->dev->of_node);
> > > > pm_genpd_remove(&domain->genpd); @@ -243,23 +243,24
> @@
> > > static
> > > > int imx7_pgc_domain_remove(struct platform_device *pdev)
> > > > return 0;
> > > > }
> > > >
> > > > -static const struct platform_device_id imx7_pgc_domain_id[] = {
> > > > - { "imx7-pgc-domain", },
> > > > +static const struct platform_device_id imx_pgc_domain_id[] = {
> > > > + { "imx-pgc-domain", },
> > > > { },
> > > > };
> > > >
> > > > -static struct platform_driver imx7_pgc_domain_driver = {
> > > > +static struct platform_driver imx_pgc_domain_driver = {
> > > > .driver = {
> > > > - .name = "imx7-pgc",
> > > > + .name = "imx-pgc",
> > > > },
> > > > - .probe = imx7_pgc_domain_probe,
> > > > - .remove = imx7_pgc_domain_remove,
> > > > - .id_table = imx7_pgc_domain_id,
> > > > + .probe = imx_pgc_domain_probe,
> > > > + .remove = imx_pgc_domain_remove,
> > > > + .id_table = imx_pgc_domain_id,
> > > > };
> > > > -builtin_platform_driver(imx7_pgc_domain_driver)
> > > > +builtin_platform_driver(imx_pgc_domain_driver)
> > > >
> > > > static int imx_gpcv2_probe(struct platform_device *pdev) {
> > > > + static const struct imx_pgc_domain *imx_pgc_domains;
> > > > static const struct regmap_range yes_ranges[] = {
> > > > regmap_reg_range(GPC_LPCR_A_CORE_BSC,
> > > > GPC_M4_PU_PDN_FLG), @@
> > > -287,6 +288,7
> > > > @@ static int imx_gpcv2_probe(struct platform_device *pdev)
> > > > struct regmap *regmap;
> > > > struct resource *res;
> > > > void __iomem *base;
> > > > + int pgc_max_index;
> > > > int ret;
> > > >
> > > > pgc_np = of_get_child_by_name(dev->of_node, "pgc"); @@
> > > > -307,9
> > > > +309,19 @@ static int imx_gpcv2_probe(struct platform_device
> > > > +*pdev)
> > > > return ret;
> > > > }
> > > >
> > > > + if (of_machine_is_compatible("fsl,imx7d")) {
> > > > + pgc_max_index = ARRAY_SIZE(imx7_pgc_domains);
> > > > + imx_pgc_domains = imx7_pgc_domains;
> > > > + }
> > >
> > > Is there any reason to do this explicit call to
> > > of_machine_is_compatible() as opposed to passing necessary data via
> > > .data in imx_gpcv2_dt_ids[]? The latter seems like a more
> > > straightforward way of passing variant specific driver info
> >
> > The reason of NOT using .data is that currently gpcv2 driver uses a
> > structure array of imx7_pgc_domains[] which has no fixed length, the
> > .data can ONLY pass the pointer of the starting address of
> > imx7_pgc_domains[], and we need to know the array size of
> > imx7_pgc_domains, how can we know it if we ONLY got the starting address
> of this array passed from .data. Please advise, thanks.
> >
>
> Unless I am missing something, you should be able to do something like:
>
> struct imx_pgc_domain_data {
> struct imx_pgc_domain *domains;
> size_t domains_num;
> };
>
> const struct imx_pgc_domain_data imx7_pgc_domain_data = {
> .domains = imx7_pgc_domains,
> .domains_num = ARRAY_SIZE(imx7_pgc_domains), };
>
> and then just pass imx7_pgc_domain_data via .data. That's a pretty common
> idiom in kernel driver.
>
> Thanks,
> Andrey Smirnov
Thanks Andrey. I just thought we can avoid another warp for the structure, if
we can NOT, I will add a wrap in V2 patch set as you suggested, thanks.
Anson.