[PATCHv2 0/4] Add support for replicators which loses context on clock removal

[PATCHv2 0/4] Add support for replicators which loses context on clock removal

[Sai Prakash Ranjan]

This series is mainly to add support for replicators
which lose context on removing AMBA clock like on SC7180
SoC where replicator in AOSS domain loses context.

Patch 1 and 2 are minor cleanups to use macros.
Patch 3 and 4 adds support for replicators which loses
context on removing AMBA clock.

Previous version is here - https://lore.kernel.org/patchwork/patch/1239923/
More discussion is found here - https://lore.kernel.org/patchwork/patch/1231182/
There were no patch 1 and patch 2 in v1.

Sai Prakash Ranjan (4):
  coresight: replicator: Use CS_AMBA_ID macro for id table
  coresight: catu: Use CS_AMBA_ID macro for id table
  coresight: replicator: Reset replicator if context is lost
  dt-bindings: arm: coresight: Add optional property to replicators

 .../devicetree/bindings/arm/coresight.txt     |  6 ++
 drivers/hwtracing/coresight/coresight-catu.c  |  5 +-
 .../coresight/coresight-replicator.c          | 66 +++++++++++--------
 3 files changed, 46 insertions(+), 31 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCHv2 1/4] coresight: replicator: Use CS_AMBA_ID macro for id table

[Sai Prakash Ranjan]

Use CS_AMBA_ID macro for dynamic replicator AMBA id table
instead of open coding.

Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
---
 drivers/hwtracing/coresight/coresight-replicator.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
index e7dc1c31d20d..c619b456f55a 100644
--- a/drivers/hwtracing/coresight/coresight-replicator.c
+++ b/drivers/hwtracing/coresight/coresight-replicator.c
@@ -348,16 +348,9 @@ static int dynamic_replicator_probe(struct amba_device *adev,
 }
 
 static const struct amba_id dynamic_replicator_ids[] = {
-	{
-		.id     = 0x000bb909,
-		.mask   = 0x000fffff,
-	},
-	{
-		/* Coresight SoC-600 */
-		.id     = 0x000bb9ec,
-		.mask   = 0x000fffff,
-	},
-	{ 0, 0 },
+	CS_AMBA_ID(0x000bb909),
+	CS_AMBA_ID(0x000bb9ec),		/* Coresight SoC-600 */
+	{},
 };
 
 static struct amba_driver dynamic_replicator_driver = {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCHv2 1/4] coresight: replicator: Use CS_AMBA_ID macro for id table

[Mike Leach]

On Tue, 19 May 2020 at 14:36, Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> Use CS_AMBA_ID macro for dynamic replicator AMBA id table
> instead of open coding.
>
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> ---
>  drivers/hwtracing/coresight/coresight-replicator.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
> index e7dc1c31d20d..c619b456f55a 100644
> --- a/drivers/hwtracing/coresight/coresight-replicator.c
> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> @@ -348,16 +348,9 @@ static int dynamic_replicator_probe(struct amba_device *adev,
>  }
>
>  static const struct amba_id dynamic_replicator_ids[] = {
> -       {
> -               .id     = 0x000bb909,
> -               .mask   = 0x000fffff,
> -       },
> -       {
> -               /* Coresight SoC-600 */
> -               .id     = 0x000bb9ec,
> -               .mask   = 0x000fffff,
> -       },
> -       { 0, 0 },
> +       CS_AMBA_ID(0x000bb909),
> +       CS_AMBA_ID(0x000bb9ec),         /* Coresight SoC-600 */
> +       {},
>  };
>

Reviewed by: Mike Leach <mike.leach@linaro.org>

>  static struct amba_driver dynamic_replicator_driver = {
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
> _______________________________________________
> CoreSight mailing list
> CoreSight@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/coresight



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

[PATCHv2 2/4] coresight: catu: Use CS_AMBA_ID macro for id table

[Sai Prakash Ranjan]

Use CS_AMBA_ID macro for coresight catu AMBA id table
instead of open coding.

Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
---
 drivers/hwtracing/coresight/coresight-catu.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
index 16ebf38a9f66..1801804a7762 100644
--- a/drivers/hwtracing/coresight/coresight-catu.c
+++ b/drivers/hwtracing/coresight/coresight-catu.c
@@ -568,10 +568,7 @@ static int catu_probe(struct amba_device *adev, const struct amba_id *id)
 }
 
 static struct amba_id catu_ids[] = {
-	{
-		.id	= 0x000bb9ee,
-		.mask	= 0x000fffff,
-	},
+	CS_AMBA_ID(0x000bb9ee),
 	{},
 };
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCHv2 2/4] coresight: catu: Use CS_AMBA_ID macro for id table

[Mike Leach]

On Tue, 19 May 2020 at 14:36, Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> Use CS_AMBA_ID macro for coresight catu AMBA id table
> instead of open coding.
>
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> ---
>  drivers/hwtracing/coresight/coresight-catu.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> index 16ebf38a9f66..1801804a7762 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.c
> +++ b/drivers/hwtracing/coresight/coresight-catu.c
> @@ -568,10 +568,7 @@ static int catu_probe(struct amba_device *adev, const struct amba_id *id)
>  }
>
>  static struct amba_id catu_ids[] = {
> -       {
> -               .id     = 0x000bb9ee,
> -               .mask   = 0x000fffff,
> -       },
> +       CS_AMBA_ID(0x000bb9ee),
>         {},
>  };
>

Reviewed by: Mike Leach <mike.leach@linaro.org>

> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

[PATCHv2 3/4] coresight: replicator: Reset replicator if context is lost

[Sai Prakash Ranjan]

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;
 	}
 
+	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

Re: [PATCHv2 3/4] coresight: replicator: Reset replicator if context is lost

[Mike Leach]

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>


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCHv2 3/4] coresight: replicator: Reset replicator if context is lost

[Sai Prakash Ranjan]

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

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCHv2 3/4] coresight: replicator: Reset replicator if context is lost

[Mathieu Poirier]

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
> 

Re: [PATCHv2 3/4] coresight: replicator: Reset replicator if context is lost

[Sai Prakash Ranjan]

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
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCHv2 4/4] dt-bindings: arm: coresight: Add optional property to replicators

[Sai Prakash Ranjan]

Add an optional boolean property "qcom,replicator-loses-context" to
identify replicators which loses context when AMBA clocks are removed
in certain configurable replicator designs.

Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
---
 Documentation/devicetree/bindings/arm/coresight.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
index 846f6daae71b..b598a5f0037d 100644
--- a/Documentation/devicetree/bindings/arm/coresight.txt
+++ b/Documentation/devicetree/bindings/arm/coresight.txt
@@ -121,6 +121,12 @@ its hardware characteristcs.
 	* interrupts : Exactly one SPI may be listed for reporting the address
 	  error
 
+* Optional property for configurable replicators:
+
+	* qcom,replicator-loses-context: boolean. Indicates that the replicator
+	  will lose register context when AMBA clock is removed which is observed
+	  in some replicator designs.
+
 Graph bindings for Coresight
 -------------------------------
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCHv2 4/4] dt-bindings: arm: coresight: Add optional property to replicators

[Mike Leach]

On Tue, 19 May 2020 at 14:37, Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> Add an optional boolean property "qcom,replicator-loses-context" to
> identify replicators which loses context when AMBA clocks are removed
> in certain configurable replicator designs.
>
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/arm/coresight.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> index 846f6daae71b..b598a5f0037d 100644
> --- a/Documentation/devicetree/bindings/arm/coresight.txt
> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> @@ -121,6 +121,12 @@ its hardware characteristcs.
>         * interrupts : Exactly one SPI may be listed for reporting the address
>           error
>
> +* Optional property for configurable replicators:
> +
> +       * qcom,replicator-loses-context: boolean. Indicates that the replicator
> +         will lose register context when AMBA clock is removed which is observed
> +         in some replicator designs.
> +
>  Graph bindings for Coresight
>  -------------------------------
>

Reviewed-by Mike Leach <mike.leach@linaro.org>

> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
> _______________________________________________
> CoreSight mailing list
> CoreSight@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/coresight



--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK