linux-kernel - Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <yw1xd1tz650x.fsf@unicorn.mansr.com>
Date:	Mon, 21 Dec 2015 18:16:46 +0000
From:	Måns Rullgård <mans@...sr.com>
To:	Andy Shevchenko <andy.shevchenko@...il.com>
Cc:	Viresh Kumar <viresh.linux@...il.com>,
	Julian Margetson <runaway@...dw.ms>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Tejun Heo <tj@...nel.org>, linux-ide@...r.kernel.org,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel

Andy Shevchenko <andy.shevchenko@...il.com> writes:

> On Mon, Dec 21, 2015 at 2:15 PM, Måns Rullgård <mans@...sr.com> wrote:
>> Andy Shevchenko <andy.shevchenko@...il.com> writes:
>>
>>> +Viresh
>>>
>>> On Mon, Dec 21, 2015 at 2:58 AM, Måns Rullgård <mans@...sr.com> wrote:
>>>> Andy Shevchenko <andy.shevchenko@...il.com> writes:
>>>>
>>>>> On Sun, Dec 20, 2015 at 8:49 PM, Måns Rullgård <mans@...sr.com> wrote:
>>>>>> Julian Margetson <runaway@...dw.ms> writes:
>>>>>>> On 12/20/2015 1:11 PM, Måns Rullgård wrote:
>>>>>>>> Julian Margetson <runaway@...dw.ms> writes:
>>>>>
>>>>>>> [   48.769671] ata3.00: failed command: READ FPDMA QUEUED
>>>>>>
>>>>>> Well, that didn't help.  I still think it's part of the problem, but
>>>>>> something else must be wrong as well.  The various Master Select fields
>>>>>> look like a good place to start.
>>>>>
>>>>> Master number (which is here would be either 1 or 0) should not affect
>>>>> as long as they are connected to the same AHB bus (I would be
>>>>> surprised if they are not).
>>>>
>>>> I think they are not.  The relevant part of the block diagram for the
>>>> 460EX looks something like this:
>>>>
>>>>       +-----+
>>>>       | CPU |
>>>>       +-----+
>>>>          |
>>>>  +---------------+
>>>>  |      BUS      |
>>>>  +---------------+
>>>>     |         |
>>>>  +-----+   +-----+
>>>>  | DMA |   | RAM |
>>>>  +-----+   +-----+
>>>>     |
>>>>  +------+
>>>>  | SATA |
>>>>  +------+
>>>>
>>>> The DMA-SATA link is private and ignores the address, which is the only
>>>> reason the driver can possibly work (it's programming a CPU virtual
>>>> address there).
>>>
>>> If you look at the original code the SMS and DMS are programmed
>>> statically independent on DMA direction, so LLP is programmed always
>>> to master 1. I don't think your scheme is reflecting this right. I
>>> could imagine two AHB buses, one of them connects CPU, SATA and RAM,
>>> and the other CPU and DMA.
>>
>> Check the code again.  The original code swaps SMS and DMS depending on
>> direction, and it sets LMS to 1.  Put differently, it always sets the
>> memory side 1 and the device side to 0.  The dw_dma driver sets SMS and
>> DMS to the src/dst_master values provided through dma_request_channel()
>> regardless of the current direction and LMS always zero.
>
> I used to have a patch to implement this in dw_dmac driver. However, I
> dropped it at some point. Seems we need it back and now I possible
> have a good explanation why.

Are you still able to find that patch?  Shouldn't be too hard to do from
scratch if not.

>> If those values didn't matter, why would the fields exist in the
>> first place?
>
> Because someone can have more than one AHB bus on the system and
> connect DMA to all of them (up to 4).

Which apparently these guys did.  Well, not a full-blown AHB bus, but
they seem to be using two master interfaces.

>>> In any case on all Intel SoCs and AVR32, and as far as I can tell on
>>> Spear13xx (Viresh?) there is not a case, that's why I hardly imagine
>>> that the problem is in master numbers by themselves.
>>
>> The 460EX is a PowerPC system.  Expect unusual topologies.
>
> Yeah, that's right.

BTW, there's a good reason for wiring it like this.  If the source and
destination are on different buses, the DMA engine can do a read and a
write in each cycle.  Otherwise the reads and writes have to be issued
alternately.

>>>>>> Also, the manual says the LLP_SRC_EN
>>>>>> and LLP_DST_EN flags should be cleared on the last in a chain of blocks.
>>>>>> The old sata_dwc driver does this whereas dw_dma does not.
>>>>>
>>>>> Easy to fix, however I can't get how it might affect.
>>>>
>>>> From the Atmel doc:
>>>>
>>>>   In Table 17-1 on page 185, all other combinations of LLPx.LOC = 0,
>>>>   CTLx.LLP_S_EN, CFGx.RELOAD_SR, CTLx.LLP_D_EN, and CFGx.RELOAD_DS are
>>>>   illegal, and causes indeterminate or erroneous behavior.
>>>
>>> I will check Synospys documentation later on.
>
> Yes, we have to clear those bits. I will do a patch or you already have one?

I'll send the patch soon.

>>>> Most likely nothing happens, but I think it ought to be fixed.  In fact,
>>>> I have a patch already.
>>>
>>> Good. Send with Fixes tag if it's upstream ready.
>>>
>>>> Come to think of it, I have an AVR32 dev somewhere.  Maybe I should dust
>>>> it off.
>>>
>>> I have ATNGW100.
>>
>> I have an AT32ATK1006.  Can you suggest a good test to exercise the DMA
>> engine?
>
> On that board I tried MMC (the only available user for me), though it
> is not reliable, I also tried the dmatest module.

Hmm, is there anywhere this damn driver actually works?  ;-)

-- 
Måns Rullgård
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/