Message-ID: <7cy2ho5lysh4tqk3vqz6rv67dadsi33bszx234vpu7bvslnddp@ed6zxezx7nwf>
Date: Mon, 23 Sep 2024 14:57:27 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Viresh Kumar <vireshk@...nel.org>, Vinod Koul <vkoul@...nel.org>, stable@...r.kernel.org, 
	Ferry Toth <fntoth@...il.com>
Subject: Re: [PATCH v3 1/1] dmaengine: dw: Select only supported masters for
 ACPI devices

On Mon, Sep 23, 2024 at 11:21:37AM +0300, Andy Shevchenko wrote:
> On Mon, Sep 23, 2024 at 01:01:08AM +0300, Serge Semin wrote:
> > On Fri, Sep 20, 2024 at 06:56:17PM +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > Fix the problem by specifying the default master ID for both memory
> > > and peripheral devices in the driver data. Thus the issue noticed for
> > > the iDMA 32-bit controllers will be eliminated and the ACPI-probed
> > > DW DMA controllers will be configured with the correct master ID by
> > > default.
> 
> > > ---
> > > v3: rewrote to use driver_data
> > > v2: https://lore.kernel.org/r/20240919185151.7331-1-fancer.lancer@gmail.com
> > 
> > IMO v2 looked better for me.
> 
> I disagree, obviously, since I sent a v3.

> (I can drop your authorship and tags in v4)

No need in that. See my further comments.

> 
> > I am sure you know, but Master IDs is a
> > platform-specific thing specific for each slave/peripheral device
> > connected to the DMA controller. Depending on the chip design one
> > peripheral device can be accessed over the one master IDs, another
> > device/memory may have another master connected (can be up to four
> > master IDs in general). That's why the master IDs have been declared
> > in the dw_dma_slave structure.
> 
> Correct.
> 
> > So adding them to struct
> > dw_dma_chip_pdata doesn't seem like a good idea seeing it contains the
> > generic DW DMA controller info.
> 
> So far there is no evidence that the channels are integrated differently on
> the same DMA controller over all hardware that uses this IP.

I've got one device (which currently isn't supported by the vanilla
kernel) which DW DMA-controller have a weird feature of being able to
communicate with one of the peripherals via both available masters.)
So the master IDs order can differ from what is currently set by
default (for ACPI).

Anyway regarding the amount of the master I/F-es, yes, I failed to
find any platform with more than two masters synthesized in. But based
on the DW DMAC IP-core databook there can be up to four of them (see
the DMAH_NUM_MASTER_INT parameter).

> 
> > On the contrary my implementation
> > seems a bit more coherent since it just changes the default slave IDs
> > defined in the dw_dma_acpi_filter() method and initialized in the
> > dw_dma_slave instance without adding slave-specific fields to the
> > generic controller data.
> 

> The default enumeration for PCI + ACPI or pure ACPI devices is not
> changed with my patch, but actually makes it better (increases granularity).
> The defaults are platform specific and that's what driver_data is for.

Since it's a default setting for the particular controller then it
seems it would be better to signify that semantic in the fields name.
Moreover seeing it's a per-platform setup I would recomment to move
these fields to the dw_dma_platform_data structure instead.
(Please see my last comment regarding this.)

> 
> While you like your solution, the problem with it that it doesn't cover
> different orders, so it's half-baked solution, I think. Mine doesn't
> change the status quo about integration (see above) and has better approach
> regarding different ordering.

Well, your solution doesn't cover the different order of the master
IDs either, because the IDs order is still fixed but on the per-controller
basis. Yes, in that regard your approach is bit more comprehensive,
but it still remains half-baked since, as you said yourself further,
it doesn't cover the cases of the non-default master IDs combination.

My solution doesn't change the status quo either, but merely fixes the
case which is currently incorrectly handled in the
dw_dma_acpi_filter() method. The rest remains the same.
(See further before responding to this comment.)

> Both implementations have a flaw regarding per-channel master configuration.

Right, none of our approaches provide a complete solution of the
problem with a per-peripheral device master IDs setup. Based on this and
what was said in the previous comments chunk, I would normally prefer to
choose a simpler, more localised, less invasive fix, which is provided
in my version of the change. That's why I started the discussion
in the first place.
(Please see my last comment before answering to this one.)

> 
> > What seems like a much better alternative to the both approaches, is
> > to use the dw_dma_slave instance defined in the mrfld_spi_setup()
> > method for the Intel Merrifield SPI PXA2xx DMA-interface in
> > drivers/spi/spi-pxa2xx-pci.c. But AFAICT that data is left unused
> > since the DMA-engine handle and connection parameters are determined
> > by the channel name. Right? Is it possible to make use of the
> > filter-function specified to the dma_request_slave_channel_compat()
> > method?
> 

> Unfortunately no, in ACPI case the only data we use is the name (index) of
> the channel in the respective resources. Everything else is done automatically.

Right, but AFAICS ACPI doesn't provide the complete settings in this
case. I thought about some combined solution: retrieve the DMAC
channel via the standard name/index-based approach and then pass the
dw_dma_slave settings via the custom filter method specified by the
client driver, thus making use of what is implemented in the
Merrifield SPI PXA2xx driver. Alas implementing this approach would
require to alter the generic DMA-engine core somehow.(

In anyway if you prefer your version of the fix, fine by me. I've
provided my reasoning above. If it wasn't enough to persuade you I
won't be insisting on my change anymore especially seeing its your and
Varesh duty to maintain the driver. But, again IMO, it seems to be
better to add the default_{m,p}_master/d{p,m}_master/etc fields to the
dw_dma_platform_data structure since the platform-specific controller
settings have been consolidated in there. The dw_dma_chip_pdata
structure looks as more like generic driver data storage.

-Serge(y)

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Powered by blists - more mailing lists