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

[Julian Margetson <runaway@candw.ms>]

On 12/21/2015 8:16 AM, Måns Rullgård wrote:
> Julian Margetson <runaway@candw.ms> writes:
>
>> On 12/21/2015 4:40 AM, Andy Shevchenko wrote:
>>> +Viresh
>>>
>>> On Mon, Dec 21, 2015 at 2:58 AM, Måns Rullgård <mans@mansr.com> wrote:
>>>> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>>>>
>>>>> On Sun, Dec 20, 2015 at 8:49 PM, Måns Rullgård <mans@mansr.com> wrote:
>>>>>> Julian Margetson <runaway@candw.ms> writes:
>>>>>>> On 12/20/2015 1:11 PM, Måns Rullgård wrote:
>>>>>>>> Julian Margetson <runaway@candw.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.
>>>
>>> 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.
>>>
>>>>>> 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.
>>>
>>>> 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.
>>>
>>> P.S. Anyway we have to ask Julian to try the kernel with
>>> 8b3444852a2b58129 reverted.
>>>
>> git revert 8b3444852a2b58129
>> error: could not revert 8b34448... sata_dwc_460ex: move to generic DMA driver
>> hint: after resolving the conflicts, mark the corrected paths
>> hint: with 'git add <paths>' or 'git rm <paths>'
>> hint: and commit the result with 'git commit'
> Yeah, that won't work since there are numerous changes afterward.  Just
> revert the entire file back to 4.0 like this:
>
> $ git checkout v4.0 drivers/ata/sata_dwc_460ex.c
>
  CC [M]  drivers/ata/sata_dwc_460ex.o
drivers/ata/sata_dwc_460ex.c:467:36: error: macro "dma_request_channel" 
requires 3 arguments, but only 1 given
  static int dma_request_channel(void)
                                     ^
drivers/ata/sata_dwc_460ex.c:468:1: error: expected ‘=’, ‘,’, 
‘;’, ‘asm’ or ‘__attribute__’ before ‘{’ token
  {
  ^
drivers/ata/sata_dwc_460ex.c: In function ‘dma_dwc_xfer_setup’:
drivers/ata/sata_dwc_460ex.c:758:31: error: macro "dma_request_channel" 
requires 3 arguments, but only 1 given
   dma_ch = dma_request_channel();
                                ^
drivers/ata/sata_dwc_460ex.c:758:11: error: ‘dma_request_channel’ 
undeclared (first use in this function)
   dma_ch = dma_request_channel();
            ^
drivers/ata/sata_dwc_460ex.c:758:11: note: each undeclared identifier is 
reported only once for each function it appears in
drivers/ata/sata_dwc_460ex.c: In function ‘sata_dwc_dma_filter’:
drivers/ata/sata_dwc_460ex.c:1282:35: error: ‘struct 
sata_dwc_device_port’ has no member named ‘dws’
   struct dw_dma_slave *dws = hsdevp->dws;
                                    ^
drivers/ata/sata_dwc_460ex.c: In function ‘sata_dwc_port_start’:
drivers/ata/sata_dwc_460ex.c:1325:17: warning: unused variable 
‘mask’ [-Wunused-variable]
   dma_cap_mask_t mask;
                  ^
drivers/ata/sata_dwc_460ex.c: At top level:
drivers/ata/sata_dwc_460ex.c:345:28: warning: ‘sata_dwc_dma_dws’ 
defined but not used [-Wunused-variable]
  static struct dw_dma_slave sata_dwc_dma_dws = {
                             ^
drivers/ata/sata_dwc_460ex.c:1279:13: warning: ‘sata_dwc_dma_filter’ 
defined but not used [-Wunused-function]
  static bool sata_dwc_dma_filter(struct dma_chan *chan, void *param)
              ^
make[2]: *** [drivers/ata/sata_dwc_460ex.o] Error 1
make[1]: *** [drivers/ata] Error 2
make: *** [drivers] Error 2
make: *** Waiting for unfinished jobs....