Re: [PATCH v1 1/3] soc/tegra: fuse: Fix reading registers using DMA on Tegra20

[Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On 26/09/17 18:31, Dmitry Osipenko wrote:
> On 26.09.2017 19:08, Stephen Warren wrote:
>> On 09/26/2017 08:08 AM, Jon Hunter wrote:
>>> Hi Dmitry,
>>>
>>> On 25/09/17 23:35, Dmitry Osipenko wrote:
>>>> DMA config is incorrect, because of it DMA transfer is never issued and
>>>> tegra20_fuse_read() always returns 0x0.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>>   drivers/soc/tegra/fuse/fuse-tegra.c   | 1 +
>>>>   drivers/soc/tegra/fuse/fuse-tegra20.c | 3 ++-
>>>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/soc/tegra/fuse/fuse-tegra.c
>>>> b/drivers/soc/tegra/fuse/fuse-tegra.c
>>>> index b7c552e3133c..73a3a2c74021 100644
>>>> --- a/drivers/soc/tegra/fuse/fuse-tegra.c
>>>> +++ b/drivers/soc/tegra/fuse/fuse-tegra.c
>>>> @@ -132,6 +132,7 @@ static int tegra_fuse_probe(struct platform_device *pdev)
>>>>         /* take over the memory region from the early initialization */
>>>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +    fuse->phys = res->start;
>>>>       fuse->base = devm_ioremap_resource(&pdev->dev, res);
>>>>       if (IS_ERR(fuse->base))
>>>>           return PTR_ERR(fuse->base);
>>>> diff --git a/drivers/soc/tegra/fuse/fuse-tegra20.c
>>>> b/drivers/soc/tegra/fuse/fuse-tegra20.c
>>>> index 294413a969a0..a33f48c06771 100644
>>>> --- a/drivers/soc/tegra/fuse/fuse-tegra20.c
>>>> +++ b/drivers/soc/tegra/fuse/fuse-tegra20.c
>>>> @@ -59,7 +59,7 @@ static u32 tegra20_fuse_read(struct tegra_fuse *fuse,
>>>> unsigned int offset)
>>>>         mutex_lock(&fuse->apbdma.lock);
>>>>   -    fuse->apbdma.config.src_addr = fuse->apbdma.phys + FUSE_BEGIN + offset;
>>>> +    fuse->apbdma.config.src_addr = fuse->phys + FUSE_BEGIN + offset;
>>>>         err = dmaengine_slave_config(fuse->apbdma.chan, &fuse->apbdma.config);
>>>>       if (err)
>>>> @@ -119,6 +119,7 @@ static int tegra20_fuse_probe(struct tegra_fuse *fuse)
>>>>       fuse->apbdma.config.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>>>>       fuse->apbdma.config.src_maxburst = 1;
>>>>       fuse->apbdma.config.dst_maxburst = 1;
>>>> +    fuse->apbdma.config.direction = DMA_DEV_TO_MEM;
>>>>         init_completion(&fuse->apbdma.wait);
>>>>       mutex_init(&fuse->apbdma.lock);
>>>
>>> Thanks for the fix.
>>>
>>> When booting the mainline on Tegra20 Trimslice, I only see the
>>> tegra20_fuse_read_early() called and not the tegra20_fuse_read(). That's
>>> not to say it is not needed, but I am wondering if we really need this
>>> complex tegra20_fuse_read() using DMA and whether we should just have
>>> the normal fuse->read() call tegra20_fuse_read_early() as well to
>>> simplify matters?
>>>
>>> Maybe Thierry or Stephen know the history here?
>>
>> There's some HW bug related to reading the fuse registers from the CPU. The fix
>> was implemented long ago by Olof; see the git commit description pasted below.
>> IIRC, the code directly reads the fuse registers before the point where DMA is
>> available (the argument being we can't do anything else, and this period of time
>> is short so the risk hopefully low), but once DMA is available, it is used to
>> avoid the HW bug. The bug was apparently fixed in Tegra30; see the other commit
>> description pasted below.
>>
>>> commit e2f91578b35347341482f6af9e4fcf3174531efd
>>> Author: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>
>>> Date:   Wed Oct 12 23:52:29 2011 -0700
>>>
>>>     ARM: tegra: use APB DMA for accessing APB devices
>>>         Tegra2 hangs if APB registers are accessed from the cpu during an
>>>     apb dma operation. The workaround is to use apb dma to read/write the
>>>     registers instead.
>>>         There is a dependency loop between fuses, clocks, and APBDMA.  If dma
>>>     is enabled, fuse reads must go through APBDMA to avoid corruption due
>>>     to a hw bug.  APBDMA requires a clock to be enabled.  Clocks must read
>>>     a fuse to determine allowable cpu frequencies.
>>>         Separate out the fuse DMA initialization, and allow the fuse read
>>>     and write functions to be called without using DMA before the DMA
>>>     initialization has been completed.  Access to the fuses before APBDMA
>>>     is initialized won't hit the hardware bug because nothing else can be
>>>     using DMA.
>>>         Original fuse registar access code from Varun Wadekar
>>>     <vwadekar-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>, improved by Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>>     and later moved to separate driver by Jon Mayo <jmayo-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>.
>>>         Major refactoring/cleanup by Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>.
>>>         Changes since v1:
>>>         * fix 'return false' on error condition
>>>     * dequeue dma ops in case of timeout
>>>         From: Jon Mayo <jmayo-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>.
>>>     Signed-off-by: Jon Mayo <jmayo-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>.
>>>     Signed-off-by: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>
>>>     Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>>> commit b861c275ea5cfeab32241c3c92a203579d5699ff
>>> Author: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> Date:   Wed Jun 20 18:06:34 2012 +0530
>>>
>>>     ARM: tegra: apbio access using dma for tegra20 only
>>>         The Tegra20 HW issue with accessing APBIO registers (such
>>>     as fuse registers) directly from the CPU concurrently with
>>>     APB DMA accesses has been fixed in Tegra30 and later chips.
>>>         Access these registers directly from the CPU on Tegra30
>>>     and later, and apply the workaround only for Tegra20.
>>>         Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>     Tested-by: Chaitanya Bandi <bandik-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>     Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Thank you very much for the clarification.

Ditto, being lazy I had only looked back as far as when it move to
drivers/soc/tegra, should have looked further!

Great, so looks like a good fix.

Cheers
Jon

-- 
nvpublic