Message-ID: <ZvFecC6u0rFczFR9@smile.fi.intel.com>
Date: Mon, 23 Sep 2024 15:26:24 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Serge Semin <fancer.lancer@...il.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 02:57:27PM +0300, Serge Semin wrote:
> 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:

...

> > > 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).

Not sure about weirdness, but IIRC those Intel ones should support that
configuration as well, as the peripheral and memory busses are the same
at the end.

> > > 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,

Correct, I explicitly mentioned that that neither of proposed solutions cover
this right now.

> 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.

Right. And I propose to follow this way as long as we have no other devices
in the wild. I.o.w. let's solve the problem when it appears.

> 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 would even say in many sophisticated DMA controller cases... :-(

> 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.(

This sounds way too far from KISS.

> 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.

Yes, I still prefer mine.

> 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.

I don't think that is correct place for it. The platform data is specific
to the DMA controller as a whole and having there the master configuration
will mean to have the arrays of them. This OTOH will break the OF setup
where this comes from the slave descriptions and may not be provided with
DMA controller, making it imbalanced. Yes, I may agree with you that chip data
is not a good place either, but at least it isolates the case to PCI + ACPI /
pure ACPI devices (and in particular we won't need to alter Intel Quark case).

Ideally, we should parse the additional properties from ACPI for this kind
of DMA controllers to get this from the _slave_ resources. Currently this is
not done, but anyone may propose a such (would you like to volunteer?).

...

TL;DR: If you are okay with your authorship in v3, I prefer it over other
versions with the explanations given in this email thread.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists