From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jon Hunter Subject: Re: [PATCH v1 1/3] soc/tegra: fuse: Fix reading registers using DMA on Tegra20 Date: Tue, 26 Sep 2017 21:53:49 +0100 Message-ID: <853a9b19-c43b-daac-aeb9-6409e4e52d47@nvidia.com> References: <27dadd0335aac71c9d4d613c33a6a1d0a285afa4.1506378772.git.digetx@gmail.com> <87d33561-d959-7444-552d-7226adf29eb4@nvidia.com> <9921a66d-90f0-5a69-2da3-dc2c355eb86a@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: <9921a66d-90f0-5a69-2da3-dc2c355eb86a-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Content-Language: en-US Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Dmitry Osipenko , Stephen Warren , Thierry Reding Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Stephen Warren List-Id: linux-tegra@vger.kernel.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 >>>> --- >>>>   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 >>> 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 >>>     , improved by Colin Cross >>>     and later moved to separate driver by Jon Mayo . >>>         Major refactoring/cleanup by Olof Johansson . >>>         Changes since v1: >>>         * fix 'return false' on error condition >>>     * dequeue dma ops in case of timeout >>>         From: Jon Mayo . >>>     Signed-off-by: Jon Mayo . >>>     Signed-off-by: Olof Johansson >>>     Acked-by: Stephen Warren >> >>> commit b861c275ea5cfeab32241c3c92a203579d5699ff >>> Author: Laxman Dewangan >>> 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 >>>     Tested-by: Chaitanya Bandi >>>     Signed-off-by: Stephen Warren > > 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