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@xxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@xxxxxxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxx>


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