| Message ID | c2e02d0c92b081c05b91d07ec17e648c40af3897.1589894597.git.saiprakash.ranjan@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for replicators which loses context on clock removal | expand |
Hi Sai, On Tue, 19 May 2020 at 14:36, Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> wrote: > > On some QCOM SoCs, replicators in Always-On domain loses its > context as soon as the clock is disabled. Currently as a part > of pm_runtime workqueue, clock is disabled after the replicator > is initialized by amba_pm_runtime_suspend assuming that context > is not lost which is not true for replicators with such > limitations. So add a new property "qcom,replicator-loses-context" > to identify such replicators and reset them. > > Suggested-by: Mike Leach <mike.leach@linaro.org> > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> > --- > > Added Mike's suggested by for parts other than the DT property. > Perhaps I should add Co-developed-by Mike since the full skeletal > was given by Mike. I can add that if required on the next version. > > --- > .../coresight/coresight-replicator.c | 53 +++++++++++++------ > 1 file changed, 36 insertions(+), 17 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c > index c619b456f55a..ba66160c8140 100644 > --- a/drivers/hwtracing/coresight/coresight-replicator.c > +++ b/drivers/hwtracing/coresight/coresight-replicator.c > @@ -38,6 +38,7 @@ struct replicator_drvdata { > struct clk *atclk; > struct coresight_device *csdev; > spinlock_t spinlock; > + bool check_idfilter_val; > }; > > static void dynamic_replicator_reset(struct replicator_drvdata *drvdata) > @@ -66,29 +67,43 @@ static int dynamic_replicator_enable(struct replicator_drvdata *drvdata, > int inport, int outport) > { > int rc = 0; > - u32 reg; > - > - switch (outport) { > - case 0: > - reg = REPLICATOR_IDFILTER0; > - break; > - case 1: > - reg = REPLICATOR_IDFILTER1; > - break; > - default: > - WARN_ON(1); > - return -EINVAL; > - } > + u32 id0val, id1val; > > CS_UNLOCK(drvdata->base); > > - if ((readl_relaxed(drvdata->base + REPLICATOR_IDFILTER0) == 0xff) && > - (readl_relaxed(drvdata->base + REPLICATOR_IDFILTER1) == 0xff)) > + id0val = readl_relaxed(drvdata->base + REPLICATOR_IDFILTER0); > + id1val = readl_relaxed(drvdata->base + REPLICATOR_IDFILTER1); > + > + /* > + * Some replicator designs lose context when AMBA clocks are removed, > + * so have a check for this. > + */ > + if (drvdata->check_idfilter_val && id0val == 0x0 && id1val == 0x0) > + id0val = id1val = 0xff; > + > + if (id0val == 0xff && id1val == 0xff) > rc = coresight_claim_device_unlocked(drvdata->base); > > + if (!rc) { > + switch (outport) { > + case 0: > + id0val = 0x0; > + break; > + case 1: > + id1val = 0x0; > + break; > + default: > + WARN_ON(1); > + rc = -EINVAL; > + } > + } > + > /* Ensure that the outport is enabled. */ > - if (!rc) > - writel_relaxed(0x00, drvdata->base + reg); > + if (!rc) { > + writel_relaxed(id0val, drvdata->base + REPLICATOR_IDFILTER0); > + writel_relaxed(id1val, drvdata->base + REPLICATOR_IDFILTER1); > + } > + > CS_LOCK(drvdata->base); > > return rc; > @@ -239,6 +254,10 @@ static int replicator_probe(struct device *dev, struct resource *res) > desc.groups = replicator_groups;On HolidayOn Holiday > } > > + if (fwnode_property_present(dev_fwnode(dev), > + "qcom,replicator-loses-context")) > + drvdata->check_idfilter_val = true; > + > dev_set_drvdata(dev, drvdata); > > pdata = coresight_get_platform_data(dev); > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation > Suggested-by is just fine - I didn't actually try out my suggested code after all! Reviewed-by Mike Leach <mike.leach@linaro.org>
Hi Mike, On 2020-05-21 21:38, Mike Leach wrote: > Hi Sai, > > On Tue, 19 May 2020 at 14:36, Sai Prakash Ranjan > <saiprakash.ranjan@codeaurora.org> wrote: >> >> On some QCOM SoCs, replicators in Always-On domain loses its >> context as soon as the clock is disabled. Currently as a part >> of pm_runtime workqueue, clock is disabled after the replicator >> is initialized by amba_pm_runtime_suspend assuming that context >> is not lost which is not true for replicators with such >> limitations. So add a new property "qcom,replicator-loses-context" >> to identify such replicators and reset them. >> >> Suggested-by: Mike Leach <mike.leach@linaro.org> >> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> >> --- >> >> Added Mike's suggested by for parts other than the DT property. >> Perhaps I should add Co-developed-by Mike since the full skeletal >> was given by Mike. I can add that if required on the next version. >> [...] > > Suggested-by is just fine - I didn't actually try out my suggested > code after all! > > Reviewed-by Mike Leach <mike.leach@linaro.org> Sure, thanks for the review. Thanks, Sai
Hi Sai, On Tue, May 19, 2020 at 07:06:02PM +0530, Sai Prakash Ranjan wrote: > On some QCOM SoCs, replicators in Always-On domain loses its > context as soon as the clock is disabled. Currently as a part > of pm_runtime workqueue, clock is disabled after the replicator > is initialized by amba_pm_runtime_suspend assuming that context > is not lost which is not true for replicators with such > limitations. So add a new property "qcom,replicator-loses-context" > to identify such replicators and reset them. > > Suggested-by: Mike Leach <mike.leach@linaro.org> > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> > --- > > Added Mike's suggested by for parts other than the DT property. > Perhaps I should add Co-developed-by Mike since the full skeletal > was given by Mike. I can add that if required on the next version. I will let Mike decide what he wants to do - I'm fine either way. > > --- > .../coresight/coresight-replicator.c | 53 +++++++++++++------ > 1 file changed, 36 insertions(+), 17 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c > index c619b456f55a..ba66160c8140 100644 > --- a/drivers/hwtracing/coresight/coresight-replicator.c > +++ b/drivers/hwtracing/coresight/coresight-replicator.c > @@ -38,6 +38,7 @@ struct replicator_drvdata { > struct clk *atclk; > struct coresight_device *csdev; > spinlock_t spinlock; > + bool check_idfilter_val; Please add documentation for the new field, the same way other fields are documented. > }; > > static void dynamic_replicator_reset(struct replicator_drvdata *drvdata) > @@ -66,29 +67,43 @@ static int dynamic_replicator_enable(struct replicator_drvdata *drvdata, > int inport, int outport) > { > int rc = 0; > - u32 reg; > - > - switch (outport) { > - case 0: > - reg = REPLICATOR_IDFILTER0; > - break; > - case 1: > - reg = REPLICATOR_IDFILTER1; > - break; > - default: > - WARN_ON(1); > - return -EINVAL; > - } > + u32 id0val, id1val; > > CS_UNLOCK(drvdata->base); > > - if ((readl_relaxed(drvdata->base + REPLICATOR_IDFILTER0) == 0xff) && > - (readl_relaxed(drvdata->base + REPLICATOR_IDFILTER1) == 0xff)) > + id0val = readl_relaxed(drvdata->base + REPLICATOR_IDFILTER0); > + id1val = readl_relaxed(drvdata->base + REPLICATOR_IDFILTER1); > + > + /* > + * Some replicator designs lose context when AMBA clocks are removed, > + * so have a check for this. > + */ > + if (drvdata->check_idfilter_val && id0val == 0x0 && id1val == 0x0) > + id0val = id1val = 0xff; > + > + if (id0val == 0xff && id1val == 0xff) > rc = coresight_claim_device_unlocked(drvdata->base); > > + if (!rc) { > + switch (outport) { > + case 0: > + id0val = 0x0; > + break; > + case 1: > + id1val = 0x0; > + break; > + default: > + WARN_ON(1); > + rc = -EINVAL; > + } > + } > + > /* Ensure that the outport is enabled. */ > - if (!rc) > - writel_relaxed(0x00, drvdata->base + reg); > + if (!rc) { > + writel_relaxed(id0val, drvdata->base + REPLICATOR_IDFILTER0); > + writel_relaxed(id1val, drvdata->base + REPLICATOR_IDFILTER1); > + } > + > CS_LOCK(drvdata->base); > > return rc; > @@ -239,6 +254,10 @@ static int replicator_probe(struct device *dev, struct resource *res) > desc.groups = replicator_groups; > } > > + if (fwnode_property_present(dev_fwnode(dev), > + "qcom,replicator-loses-context")) > + drvdata->check_idfilter_val = true; > + The header <linux/property.h> needs to be added for function fwnode_property_present(). What is the clock situation with other QC components like funnels? Have they also been designed the same way? If so the binding should probably be "qcom,component-loses-context", otherwise what you have suggested will work just fine. My goal here is to avoid having "qcom,replicator-loses-context" and "qcom,funnel-loses-context". Lastly, I have applied patch 1 and 2 of this set to my tree so no need to resend them again with the next revision. Thanks, Mathieu > dev_set_drvdata(dev, drvdata); > > pdata = coresight_get_platform_data(dev); > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation >
Hi Mathieu, On 2020-05-22 23:10, Mathieu Poirier wrote: > Hi Sai, > > On Tue, May 19, 2020 at 07:06:02PM +0530, Sai Prakash Ranjan wrote: >> On some QCOM SoCs, replicators in Always-On domain loses its >> context as soon as the clock is disabled. Currently as a part >> of pm_runtime workqueue, clock is disabled after the replicator >> is initialized by amba_pm_runtime_suspend assuming that context >> is not lost which is not true for replicators with such >> limitations. So add a new property "qcom,replicator-loses-context" >> to identify such replicators and reset them. >> >> Suggested-by: Mike Leach <mike.leach@linaro.org> >> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> >> --- >> >> Added Mike's suggested by for parts other than the DT property. >> Perhaps I should add Co-developed-by Mike since the full skeletal >> was given by Mike. I can add that if required on the next version. > > I will let Mike decide what he wants to do - I'm fine either way. > Mike was ok with suggested-by, so I will go with that. >> >> --- >> .../coresight/coresight-replicator.c | 53 >> +++++++++++++------ >> 1 file changed, 36 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c >> b/drivers/hwtracing/coresight/coresight-replicator.c >> index c619b456f55a..ba66160c8140 100644 >> --- a/drivers/hwtracing/coresight/coresight-replicator.c >> +++ b/drivers/hwtracing/coresight/coresight-replicator.c >> @@ -38,6 +38,7 @@ struct replicator_drvdata { >> struct clk *atclk; >> struct coresight_device *csdev; >> spinlock_t spinlock; >> + bool check_idfilter_val; > > Please add documentation for the new field, the same way other fields > are > documented. > Sure will add that. >> }; >> >> static void dynamic_replicator_reset(struct replicator_drvdata >> *drvdata) >> @@ -66,29 +67,43 @@ static int dynamic_replicator_enable(struct >> replicator_drvdata *drvdata, >> int inport, int outport) >> { >> int rc = 0; >> - u32 reg; >> - >> - switch (outport) { >> - case 0: >> - reg = REPLICATOR_IDFILTER0; >> - break; >> - case 1: >> - reg = REPLICATOR_IDFILTER1; >> - break; >> - default: >> - WARN_ON(1); >> - return -EINVAL; >> - } >> + u32 id0val, id1val; >> >> CS_UNLOCK(drvdata->base); >> >> - if ((readl_relaxed(drvdata->base + REPLICATOR_IDFILTER0) == 0xff) && >> - (readl_relaxed(drvdata->base + REPLICATOR_IDFILTER1) == 0xff)) >> + id0val = readl_relaxed(drvdata->base + REPLICATOR_IDFILTER0); >> + id1val = readl_relaxed(drvdata->base + REPLICATOR_IDFILTER1); >> + >> + /* >> + * Some replicator designs lose context when AMBA clocks are >> removed, >> + * so have a check for this. >> + */ >> + if (drvdata->check_idfilter_val && id0val == 0x0 && id1val == 0x0) >> + id0val = id1val = 0xff; >> + >> + if (id0val == 0xff && id1val == 0xff) >> rc = coresight_claim_device_unlocked(drvdata->base); >> >> + if (!rc) { >> + switch (outport) { >> + case 0: >> + id0val = 0x0; >> + break; >> + case 1: >> + id1val = 0x0; >> + break; >> + default: >> + WARN_ON(1); >> + rc = -EINVAL; >> + } >> + } >> + >> /* Ensure that the outport is enabled. */ >> - if (!rc) >> - writel_relaxed(0x00, drvdata->base + reg); >> + if (!rc) { >> + writel_relaxed(id0val, drvdata->base + REPLICATOR_IDFILTER0); >> + writel_relaxed(id1val, drvdata->base + REPLICATOR_IDFILTER1); >> + } >> + >> CS_LOCK(drvdata->base); >> >> return rc; >> @@ -239,6 +254,10 @@ static int replicator_probe(struct device *dev, >> struct resource *res) >> desc.groups = replicator_groups; >> } >> >> + if (fwnode_property_present(dev_fwnode(dev), >> + "qcom,replicator-loses-context")) >> + drvdata->check_idfilter_val = true; >> + > > The header <linux/property.h> needs to be added for function > fwnode_property_present(). > Sure will add in next version. > What is the clock situation with other QC components like funnels? > Have they > also been designed the same way? If so the binding should probably be > "qcom,component-loses-context", otherwise what you have suggested will > work just > fine. My goal here is to avoid having "qcom,replicator-loses-context" > and > "qcom,funnel-loses-context". > Yes I understand it is quite ugly, but AFAIK we do not have any SoCs already released and coming in near future with such limitations in funnels or other components. So I will stick to the replicator specific property. > Lastly, I have applied patch 1 and 2 of this set to my tree so no need > to resend > them again with the next revision. > Thanks for reviewing these patches. Thanks, Sai
diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c index c619b456f55a..ba66160c8140 100644 --- a/drivers/hwtracing/coresight/coresight-replicator.c +++ b/drivers/hwtracing/coresight/coresight-replicator.c @@ -38,6 +38,7 @@ struct replicator_drvdata { struct clk *atclk; struct coresight_device *csdev; spinlock_t spinlock; + bool check_idfilter_val; }; static void dynamic_replicator_reset(struct replicator_drvdata *drvdata) @@ -66,29 +67,43 @@ static int dynamic_replicator_enable(struct replicator_drvdata *drvdata, int inport, int outport) { int rc = 0; - u32 reg; - - switch (outport) { - case 0: - reg = REPLICATOR_IDFILTER0; - break; - case 1: - reg = REPLICATOR_IDFILTER1; - break; - default: - WARN_ON(1); - return -EINVAL; - } + u32 id0val, id1val; CS_UNLOCK(drvdata->base); - if ((readl_relaxed(drvdata->base + REPLICATOR_IDFILTER0) == 0xff) && - (readl_relaxed(drvdata->base + REPLICATOR_IDFILTER1) == 0xff)) + id0val = readl_relaxed(drvdata->base + REPLICATOR_IDFILTER0); + id1val = readl_relaxed(drvdata->base + REPLICATOR_IDFILTER1); + + /* + * Some replicator designs lose context when AMBA clocks are removed, + * so have a check for this. + */ + if (drvdata->check_idfilter_val && id0val == 0x0 && id1val == 0x0) + id0val = id1val = 0xff; + + if (id0val == 0xff && id1val == 0xff) rc = coresight_claim_device_unlocked(drvdata->base); + if (!rc) { + switch (outport) { + case 0: + id0val = 0x0; + break; + case 1: + id1val = 0x0; + break; + default: + WARN_ON(1); + rc = -EINVAL; + } + } + /* Ensure that the outport is enabled. */ - if (!rc) - writel_relaxed(0x00, drvdata->base + reg); + if (!rc) { + writel_relaxed(id0val, drvdata->base + REPLICATOR_IDFILTER0); + writel_relaxed(id1val, drvdata->base + REPLICATOR_IDFILTER1); + } + CS_LOCK(drvdata->base); return rc; @@ -239,6 +254,10 @@ static int replicator_probe(struct device *dev, struct resource *res) desc.groups = replicator_groups; } + if (fwnode_property_present(dev_fwnode(dev), + "qcom,replicator-loses-context")) + drvdata->check_idfilter_val = true; + dev_set_drvdata(dev, drvdata); pdata = coresight_get_platform_data(dev);
On some QCOM SoCs, replicators in Always-On domain loses its context as soon as the clock is disabled. Currently as a part of pm_runtime workqueue, clock is disabled after the replicator is initialized by amba_pm_runtime_suspend assuming that context is not lost which is not true for replicators with such limitations. So add a new property "qcom,replicator-loses-context" to identify such replicators and reset them. Suggested-by: Mike Leach <mike.leach@linaro.org> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> --- Added Mike's suggested by for parts other than the DT property. Perhaps I should add Co-developed-by Mike since the full skeletal was given by Mike. I can add that if required on the next version. --- .../coresight/coresight-replicator.c | 53 +++++++++++++------ 1 file changed, 36 insertions(+), 17 deletions(-)