Re: [PATCH v2 2/3] firmware: qcom: scm: Remove core, iface and bus clocks dependency
On 11/09/2016 09:55 PM, Bjorn Andersson wrote:
On Wed 09 Nov 17:47 PST 2016, Stephen Boyd wrote:
On 11/03, Sarangdhar Joshi wrote:
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index d79fecd..844e90d 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available);
static int qcom_scm_probe(struct platform_device *pdev)
{
struct qcom_scm *scm;
+ uint32_t clks;
If this was unsigned long flags;
I did look at this too and could only find a mixture of ways people have
done this. Isn't the correct type for this intptr_t?
That's true. There are lot of variations of how it's done. I had
referred one of the gpio driver for this. I think it's safe to use
unsigned long instead as Stephen suggested.
Btw I don't see intptr_t defined in include/linux/types.h.
Regards,
Sarang
Regards,
Bjorn
int ret;
scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
if (!scm)
return -ENOMEM;
- scm->core_clk = devm_clk_get(&pdev->dev, "core");
- if (IS_ERR(scm->core_clk)) {
- if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
- return PTR_ERR(scm->core_clk);
+ clks = (uint32_t)((uintptr_t)of_device_get_match_data(&pdev->dev));
then this could just be a cast to unsigned long?
+ if (clks & SCM_HAS_CORE_CLK) {
+ scm->core_clk = devm_clk_get(&pdev->dev, "core");
+ if (IS_ERR(scm->core_clk)) {
+ if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
+ return PTR_ERR(scm->core_clk);
- scm->core_clk = NULL;
+ scm->core_clk = NULL;
+ }
}
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH v2 2/3] firmware: qcom: scm: Remove core, iface and bus clocks dependency
On 11/09/2016 05:47 PM, Stephen Boyd wrote:
On 11/03, Sarangdhar Joshi wrote:
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index d79fecd..844e90d 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available);
static int qcom_scm_probe(struct platform_device *pdev)
{
struct qcom_scm *scm;
+ uint32_t clks;
If this was unsigned long flags;
int ret;
scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
if (!scm)
return -ENOMEM;
- scm->core_clk = devm_clk_get(&pdev->dev, "core");
- if (IS_ERR(scm->core_clk)) {
- if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
- return PTR_ERR(scm->core_clk);
+ clks = (uint32_t)((uintptr_t)of_device_get_match_data(&pdev->dev));
then this could just be a cast to unsigned long?
I saw quite a few places in kernel where similar type casts are used
(e.g gpio drivers) and ended up using uint32_t. uintptr_t is nothing but
typedef of an unsigned long. Probably it's a good idea to just use
unsigned long instead.
+ if (clks & SCM_HAS_CORE_CLK) {
+ scm->core_clk = devm_clk_get(&pdev->dev, "core");
+ if (IS_ERR(scm->core_clk)) {
+ if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
+ return PTR_ERR(scm->core_clk);
- scm->core_clk = NULL;
+ scm->core_clk = NULL;
+ }
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH v2 2/3] firmware: qcom: scm: Remove core, iface and bus clocks dependency
On 11/09, Bjorn Andersson wrote:
> On Wed 09 Nov 17:47 PST 2016, Stephen Boyd wrote:
>
> > On 11/03, Sarangdhar Joshi wrote:
> > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > > index d79fecd..844e90d 100644
> > > --- a/drivers/firmware/qcom_scm.c
> > > +++ b/drivers/firmware/qcom_scm.c
> > > @@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available);
> > > static int qcom_scm_probe(struct platform_device *pdev)
> > > {
> > > struct qcom_scm *scm;
> > > + uint32_t clks;
> >
> > If this was unsigned long flags;
> >
>
> I did look at this too and could only find a mixture of ways people have
> done this. Isn't the correct type for this intptr_t?
>
Well unsigned long == sizeof(kernel pointer) on all Linux
platforms so it's safe to do the cast. For example, struct
timer_list stores data in unsigned long. I guess this is a
'kernelism' though? Perhaps we should update
Documentation/CodingStyle if anyone cares.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH v2 2/3] firmware: qcom: scm: Remove core, iface and bus clocks dependency
On Wed 09 Nov 17:47 PST 2016, Stephen Boyd wrote:
> On 11/03, Sarangdhar Joshi wrote:
> > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > index d79fecd..844e90d 100644
> > --- a/drivers/firmware/qcom_scm.c
> > +++ b/drivers/firmware/qcom_scm.c
> > @@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available);
> > static int qcom_scm_probe(struct platform_device *pdev)
> > {
> > struct qcom_scm *scm;
> > + uint32_t clks;
>
> If this was unsigned long flags;
>
I did look at this too and could only find a mixture of ways people have
done this. Isn't the correct type for this intptr_t?
Regards,
Bjorn
> > int ret;
> >
> > scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
> > if (!scm)
> > return -ENOMEM;
> >
> > - scm->core_clk = devm_clk_get(&pdev->dev, "core");
> > - if (IS_ERR(scm->core_clk)) {
> > - if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
> > - return PTR_ERR(scm->core_clk);
> > + clks = (uint32_t)((uintptr_t)of_device_get_match_data(&pdev->dev));
>
> then this could just be a cast to unsigned long?
>
> > + if (clks & SCM_HAS_CORE_CLK) {
> > + scm->core_clk = devm_clk_get(&pdev->dev, "core");
> > + if (IS_ERR(scm->core_clk)) {
> > + if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
> > + return PTR_ERR(scm->core_clk);
> >
> > - scm->core_clk = NULL;
> > + scm->core_clk = NULL;
> > + }
> > }
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
Re: [PATCH v2 2/3] firmware: qcom: scm: Remove core, iface and bus clocks dependency
On 11/03, Sarangdhar Joshi wrote:
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index d79fecd..844e90d 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available);
> static int qcom_scm_probe(struct platform_device *pdev)
> {
> struct qcom_scm *scm;
> + uint32_t clks;
If this was unsigned long flags;
> int ret;
>
> scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
> if (!scm)
> return -ENOMEM;
>
> - scm->core_clk = devm_clk_get(&pdev->dev, "core");
> - if (IS_ERR(scm->core_clk)) {
> - if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
> - return PTR_ERR(scm->core_clk);
> + clks = (uint32_t)((uintptr_t)of_device_get_match_data(&pdev->dev));
then this could just be a cast to unsigned long?
> + if (clks & SCM_HAS_CORE_CLK) {
> + scm->core_clk = devm_clk_get(&pdev->dev, "core");
> + if (IS_ERR(scm->core_clk)) {
> + if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
> + return PTR_ERR(scm->core_clk);
>
> - scm->core_clk = NULL;
> + scm->core_clk = NULL;
> + }
> }
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v2 2/3] firmware: qcom: scm: Remove core, iface and bus clocks dependency
Core, iface and bus clocks are not required to be voted from SCM
driver for some of the Qualcomm chipsets. Remove dependency on
these clocks from driver.
Suggested-by: Bjorn Andersson
Signed-off-by: Sarangdhar Joshi
---
drivers/firmware/qcom_scm.c | 49 ++---
1 file changed, 37 insertions(+), 12 deletions(-)
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index d79fecd..844e90d 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -28,6 +28,10 @@
#include "qcom_scm.h"
+#define SCM_HAS_CORE_CLK BIT(0)
+#define SCM_HAS_IFACE_CLK BIT(1)
+#define SCM_HAS_BUS_CLKBIT(2)
+
struct qcom_scm {
struct device *dev;
struct clk *core_clk;
@@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available);
static int qcom_scm_probe(struct platform_device *pdev)
{
struct qcom_scm *scm;
+ uint32_t clks;
int ret;
scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
if (!scm)
return -ENOMEM;
- scm->core_clk = devm_clk_get(&pdev->dev, "core");
- if (IS_ERR(scm->core_clk)) {
- if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
- return PTR_ERR(scm->core_clk);
+ clks = (uint32_t)((uintptr_t)of_device_get_match_data(&pdev->dev));
+ if (clks & SCM_HAS_CORE_CLK) {
+ scm->core_clk = devm_clk_get(&pdev->dev, "core");
+ if (IS_ERR(scm->core_clk)) {
+ if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
+ return PTR_ERR(scm->core_clk);
- scm->core_clk = NULL;
+ scm->core_clk = NULL;
+ }
}
- if (of_device_is_compatible(pdev->dev.of_node, "qcom,scm")) {
+ if (clks & SCM_HAS_IFACE_CLK) {
scm->iface_clk = devm_clk_get(&pdev->dev, "iface");
if (IS_ERR(scm->iface_clk)) {
if (PTR_ERR(scm->iface_clk) != -EPROBE_DEFER)
- dev_err(&pdev->dev, "failed to acquire iface
clk\n");
+ dev_err(&pdev->dev,
+ "failed to acquire iface clk\n");
return PTR_ERR(scm->iface_clk);
}
+ }
+ if (clks & SCM_HAS_BUS_CLK) {
scm->bus_clk = devm_clk_get(&pdev->dev, "bus");
if (IS_ERR(scm->bus_clk)) {
if (PTR_ERR(scm->bus_clk) != -EPROBE_DEFER)
- dev_err(&pdev->dev, "failed to acquire bus
clk\n");
+ dev_err(&pdev->dev,
+ "failed to acquire bus clk\n");
return PTR_ERR(scm->bus_clk);
}
}
@@ -429,10 +441,23 @@ static int qcom_scm_probe(struct platform_device *pdev)
}
static const struct of_device_id qcom_scm_dt_match[] = {
- { .compatible = "qcom,scm-apq8064",},
- { .compatible = "qcom,scm-msm8660",},
- { .compatible = "qcom,scm-msm8960",},
- { .compatible = "qcom,scm",},
+ { .compatible = "qcom,scm-apq8064",
+ .data = (void *) SCM_HAS_CORE_CLK,
+ },
+ { .compatible = "qcom,scm-msm8660",
+ .data = (void *) SCM_HAS_CORE_CLK,
+ },
+ { .compatible = "qcom,scm-msm8960",
+ .data = (void *) SCM_HAS_CORE_CLK,
+ },
+ { .compatible = "qcom,scm-msm8996",
+ .data = NULL, /* no clocks */
+ },
+ { .compatible = "qcom,scm",
+ .data = (void *)(SCM_HAS_CORE_CLK
+ | SCM_HAS_IFACE_CLK
+ | SCM_HAS_BUS_CLK),
+ },
{}
};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
