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@xxxxxxxxx > > > > 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 > >
