Re: [PATCH v2 2/2] memory: tegra: Introduce memory client hot reset API

From: Dmitry Osipenko
Date: Mon Feb 19 2018 - 07:44:12 EST


On 19.02.2018 15:35, Dmitry Osipenko wrote:
> On 13.02.2018 14:24, Thierry Reding wrote:
>> On Mon, Feb 12, 2018 at 08:06:31PM +0300, Dmitry Osipenko wrote:
>>> In order to reset busy HW properly, memory controller needs to be
>>> involved, otherwise it possible to get corrupted memory if HW was reset
>>> during DMA. Introduce memory client 'hot reset' API that will be used
>>> for resetting busy HW. The primary users are memory clients related to
>>> video (decoder/encoder/camera) and graphics (2d/3d).
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>>> ---
>>> drivers/memory/tegra/mc.c | 249 ++++++++++++++++++++++++++++++++++++++++
>>> drivers/memory/tegra/tegra114.c | 25 ++++
>>> drivers/memory/tegra/tegra124.c | 32 ++++++
>>> drivers/memory/tegra/tegra20.c | 23 ++++
>>> drivers/memory/tegra/tegra210.c | 27 +++++
>>> drivers/memory/tegra/tegra30.c | 25 ++++
>>> include/soc/tegra/mc.h | 77 +++++++++++++
>>> 7 files changed, 458 insertions(+)
>>
>> As discussed on IRC, I typed up a variant of this in an attempt to fix
>> an unrelated bug report. The code is here:
>>
>> https://github.com/thierryreding/linux/commit/e104e703cb75dfe742b2e051194a0af8ddefcd03
>>
>> I think we can make this without adding a custom API. The reset control
>> API should work just fine. The above version doesn't take into account
>> some of Tegra20's quirks, but I think it should still work for Tegra20
>> with just slightly different implementations for ->assert() and
>> ->deassert().
>>
>> A couple of more specific comments below.
>>
>>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>>> index 187a9005351b..9838f588d64d 100644
>>> --- a/drivers/memory/tegra/mc.c
>>> +++ b/drivers/memory/tegra/mc.c
>>> @@ -7,11 +7,13 @@
>>> */
>>>
>>> #include <linux/clk.h>
>>> +#include <linux/delay.h>
>>> #include <linux/interrupt.h>
>>> #include <linux/kernel.h>
>>> #include <linux/module.h>
>>> #include <linux/of.h>
>>> #include <linux/platform_device.h>
>>> +#include <linux/reset.h>
>>> #include <linux/slab.h>
>>> #include <linux/sort.h>
>>>
>>> @@ -81,6 +83,172 @@ static const struct of_device_id tegra_mc_of_match[] = {
>>> };
>>> MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
>>>
>>> +static int terga_mc_flush_dma(struct tegra_mc *mc, unsigned int id)
>>> +{
>>> + unsigned int hw_id = mc->soc->modules[id].hw_id;
>>> + u32 value, reg_poll = mc->soc->reg_client_flush_status;
>>> + int retries = 3;
>>> +
>>> + value = mc_readl(mc, mc->soc->reg_client_ctrl);
>>> +
>>> + if (mc->soc->tegra20)
>>> + value &= ~BIT(hw_id);
>>> + else
>>> + value |= BIT(hw_id);
>>> +
>>> + /* block clients DMA requests */
>>> + mc_writel(mc, value, mc->soc->reg_client_ctrl);
>>> +
>>> + /* wait for completion of the outstanding DMA requests */
>>> + if (mc->soc->tegra20) {
>>> + while (mc_readl(mc, reg_poll + hw_id * sizeof(u32)) != 0) {
>>> + if (!retries--)
>>> + return -EBUSY;
>>> +
>>> + usleep_range(1000, 2000);
>>> + }
>>> + } else {
>>> + while ((mc_readl(mc, reg_poll) & BIT(hw_id)) == 0) {
>>> + if (!retries--)
>>> + return -EBUSY;
>>> +
>>> + usleep_range(1000, 2000);
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> I think this suffers from too much unification. The programming model is
>> too different to stash this into a single function implementation and as
>> a result this becomes very difficult to read. In my experience it's more
>> readable to split this into separate implementations and pass around
>> pointers to them.
>>
>>> +
>>> +static int terga_mc_unblock_dma(struct tegra_mc *mc, unsigned int id)
>>> +{
>>> + unsigned int hw_id = mc->soc->modules[id].hw_id;
>>> + u32 value;
>>> +
>>> + value = mc_readl(mc, mc->soc->reg_client_ctrl);
>>> +
>>> + if (mc->soc->tegra20)
>>> + value |= BIT(hw_id);
>>> + else
>>> + value &= ~BIT(hw_id);
>>> +
>>> + mc_writel(mc, value, mc->soc->reg_client_ctrl);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int terga_mc_hotreset_assert(struct tegra_mc *mc, unsigned int id)
>>> +{
>>> + unsigned int hw_id = mc->soc->modules[id].hw_id;
>>> + u32 value;
>>> +
>>> + if (mc->soc->tegra20) {
>>> + value = mc_readl(mc, mc->soc->reg_client_hotresetn);
>>> +
>>> + mc_writel(mc, value & ~BIT(hw_id),
>>> + mc->soc->reg_client_hotresetn);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int terga_mc_hotreset_deassert(struct tegra_mc *mc, unsigned int id)
>>> +{
>>> + unsigned int hw_id = mc->soc->modules[id].hw_id;
>>> + u32 value;
>>> +
>>> + if (mc->soc->tegra20) {
>>> + value = mc_readl(mc, mc->soc->reg_client_hotresetn);
>>> +
>>> + mc_writel(mc, value | BIT(hw_id),
>>> + mc->soc->reg_client_hotresetn);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> The same goes for these. I think we can do this much more easily by
>> providing reset controller API ->assert() and ->deassert()
>> implementations for Tegra20 and Tegra30+, and then register the reset
>> controller device using the ops stored in the MC SoC structure.
>>
>>> +static int tegra_mc_hot_reset_assert(struct tegra_mc *mc, unsigned int id,
>>> + struct reset_control *rst)
>>> +{
>>> + int err;
>>> +
>>> + /*
>>> + * Block clients DMA requests and wait for completion of the
>>> + * outstanding requests.
>>> + */
>>> + err = terga_mc_flush_dma(mc, id);
>>> + if (err) {
>>> + dev_err(mc->dev, "Failed to flush DMA: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + /* put in reset HW that corresponds to the memory client */
>>> + err = reset_control_assert(rst);
>>> + if (err) {
>>> + dev_err(mc->dev, "Failed to assert HW reset: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + /* clear the client requests sitting before arbitration */
>>> + err = terga_mc_hotreset_assert(mc, id);
>>> + if (err) {
>>> + dev_err(mc->dev, "Failed to hot reset client: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> This is very strictly according to the TRM, but I don't see a reason why
>> you couldn't stash the DMA blocking and the hot reset into a ->assert()
>> implementation...
>>
>>> +
>>> +static int tegra_mc_hot_reset_deassert(struct tegra_mc *mc, unsigned int id,
>>> + struct reset_control *rst)
>>> +{
>>> + int err;
>>> +
>>> + /* take out client from hot reset */
>>> + err = terga_mc_hotreset_deassert(mc, id);
>>> + if (err) {
>>> + dev_err(mc->dev, "Failed to deassert hot reset: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + /* take out from reset corresponding clients HW */
>>> + err = reset_control_deassert(rst);
>>> + if (err) {
>>> + dev_err(mc->dev, "Failed to deassert HW reset: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + /* allow new DMA requests to proceed to arbitration */
>>> + err = terga_mc_unblock_dma(mc, id);
>>> + if (err) {
>>> + dev_err(mc->dev, "Failed to unblock client: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> ... and the hot reset deassertion and the DMA unblocking into a
>> ->deassert() implementation.
>>
>> I think the important part is that DMA is blocked and the requests are
>> cleared before the module reset, and similarily that the hot reset is
>> released and DMA is unblocked after the module reset.
>
> Okay, let's try the way you're suggesting and see if anything breaks..

Although, it would be awesome if you could ask somebody who is familiar with the
actual HW implementation. I can imagine that HW is simplified and expecting SW
to take that into account, there could be hazards.