Message-ID: <CAJ9a7VjAoUmMG9pLEzE_rMSpOjwVOi-ZCinF87n9H0JgfMDsiQ@mail.gmail.com>
Date:   Tue, 12 May 2020 22:52:33 +0100
From:   Mike Leach <mike.leach@...aro.org>
To:     Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>
Cc:     Suzuki K Poulose <suzuki.poulose@....com>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm-msm@...r.kernel.org,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH] coresight: dynamic-replicator: Fix handling of multiple connections

HI Sai,

On Tue, 12 May 2020 at 18:46, Sai Prakash Ranjan
<saiprakash.ranjan@...eaurora.org> wrote:
>
> Hi Mike,
>
> On 2020-05-12 17:19, Mike Leach wrote:
> [...]
>
> >> >>
> >> >> Sorry for hurrying up and sending the patch -
> >> >> https://lore.kernel.org/patchwork/patch/1239923/.
> >> >> I will send v2 based on further feedbacks here or there.
> >> >>
> >> >>>
> >> >>> 1) does this replicator part have a unique ID that differs from the
> >> >>> standard ARM designed replicators?
> >> >>> If so perhaps link the modification into this. (even if the part no
> >> >>> in
> >> >>> PIDR0/1 is the same the UCI should be different for a different
> >> >>> implementation)
> >> >>>
> > I have reviewed the replicator driver, and compared to all the other CS
> > drivers.
> > This driver appears to be the only one that sets hardware values in
> > probe() and expects them to remain in place on enable, and uses that
> > state for programming decisions later, despite telling the PM
> > infrastructure that it is clear to suspend the device.
> >
> > Now we have a system where the replicator hardware is behaving
> > differently under the driver, but is it behaving unreasonably?
>
> Thanks for taking your time to review this. For new replicator behaving
> unreasonably, I think the assumption that the context is not lost on
> disabling clock is flawed since its implementation defined. Is such
> assumption documented in any TRM?
>

Looking at the AMBA driver there is a comment there that AMBA does not
lose state when clocks are removed. This is consistent with the AMBA
protocol spec which states that AMBA slaves can only be accessed /
read / write on various strobe signals,  or state reset on PRESET
signal, all timed by the rising edge of the bus clock. state changes
are not permitted on clock events alone. Given this static nature of
AMBA slaves then removing the clock should not have any effect.

The AMBA driver only /drivers/amba/bus.c  gives permission to
remove/restore the clocks from the devices (pm_suspend pm_resume
callbacks) - this reduces the power consumption of these devices if
the clock is not running, but state must be retained.

> >> >>
> >> >> pid=0x2bb909 for both replicators. So part number is same.
> >> >> UCI will be different for different implementation(QCOM maybe
> >> >> different from ARM),
> >> >> but will it be different for different replicators under the same
> >> >> impl(i.e., on QCOM).
> >> >
> >> > May be use PIDR4.DES_2 to match the Implementor and apply the work
> >> > around for all QCOM replicators ?
> >> >
> >> > To me that sounds the best option.
> >> >
> >>
> >
> > I agree, if it can be established that the register values that make
> > up UCI (pid0-4, devarch, devtype, PID:CLASS==0x9), can correctly
> > identify the parts then a flag can be set in the probe() function and
> > acted on during the enable() function.
> >
>
> So here I have a doubt as to why we need to use UCI because PID =
> 0x2bb909
> and CID = 0xb105900d are same for both replicators, so UCI won't
> identify the
> different replicators(in same implementation i.e., on QCOM) here.
> Am I missing something?
>
> Thats why I think Suzuki suggested to use PIDR4_DES2 and check for QCOM
> impl
> and add a workaround for all replicators, something like below: (will
> need cleaning)
>
> #define PIDR4_DES2      0xFD0
>
> if (FIELD_GET(GENMASK(3, 0), readl_relaxed(drvdata->base + PIDR4_DES2))
> == 0x4)
>         id0val = id1val = 0xff;
>

Please look at the CoreSight components specification 3.0 (ARM IHI
0029E) Section B2.1.2 which describes the Unique Component Identifier
(UCI).
As mentioned above this consists of a combination of bits from
multiple registers, including PIDR4.

> ... and the rest as you suggested.
>
> >
> > This was a design decision made by the original driver writer. A
> > normal AMBA device should not lose context due to clock removal (see
> > drivers/amba/bus.c), so resetting in probe means this operation is
> > done only once, rather than add overhead in the enable() function,and
> > later decisions can be made according to the state of the registers
> > set.
> >
> > As you have pointed out, for this replicator implementation  the
> > context is unfortunately not retained when clocks are removed - so an
> > alternative method is required.
> >
> > perhaps something like:-
> >
> > probe()
> > ...
> > if (match_id_non_persistent_state_regs(ID))
> >     drvdata->check_filter_val_on_enable;
> > ....
> >
> > and a re-write of enable:-
> >
> > enable()
> > ...
> > CS_UNLOCK()
> > id0val = read(IDFILTER0);
> > id1val = read(IDFILTER1);
> >
> > /* some replicator designs lose context when AMBA clocks are removed -
> > check for this */
> > if (drvdata->check_filter_val_on_enable && (id0val == id1val == 0x0))
> >    id0val = id1val = 0xff;
> >
> > if(id0xal == id1val == 0xff)
> >    rc =  claim_device()
> >
> > if (!rc)
> >    switch (outport)
> >       case 0: id0val  = 0x0; break
> >       case 1: id1va; = 0x0; break;
> >      default: rc = -EINVAL;
> >
> > if (!rc)
> >    write(id0val);
> >    write(id1val);
> > CS_LOCK()
> > return rc;
> > ....
> >
>
> Thanks for this detailed idea for workaround. I will add this once we
> know whether we need to use UCI or PIDR4_DES2.
>
> > Given that the access to the enable() function is predicated on a
> > reference count per active port, there is also a case for dropping the
> > check_filter_val_on_enable flag completely - once one port is active,
> > then the device will remain enabled until both ports are inactive.
> > This still allows for future development of selective filtering per
> > port.
> >
> > One other point here - there is a case as I mentioned above for moving
> > to a stored value model for the driver - as this is the only coresight
> > driver that appears to set state in the probe() function rather than
> > write all on enable.
> > This however would necessitate a more comprehensive re-write.
> >
>
> I would defer this to experts as you or suzuki will have more idea
> regarding this than me.
>
> Thanks,
> Sai
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member
> of Code Aurora Forum, hosted by The Linux Foundation

Thanks

Mike

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

Powered by blists - more mailing lists