| Message ID | 5426556.OzZIXLrofJ@wuerfel (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2014-07-25 at 12:19 +0200, Arnd Bergmann wrote: > On Friday 25 July 2014 12:55:59 Mika Westerberg wrote: > > On Fri, Jul 25, 2014 at 12:07:14PM +0300, Mika Westerberg wrote: > > > On Fri, Jul 25, 2014 at 10:38:36AM +0200, Arnd Bergmann wrote: > > > > On Friday 25 July 2014 11:22:49 Mika Westerberg wrote: [] > > Something like this? Arnd, this dependency to certain DMA driver looks really bad. If we go that way, can we split that part to [another] module and make it dependent to DW_DMAC? Or shall we introduce a dmaengine type field in the platform data and dynamically choose proper filter-whatever-function to get the channel? > > Hock Leong / Chiaue Ee, are you able to check if this works on your BYT > > machines? > What I think you got wrong here (by following my bad advice) is the master > number. Looking at the code for dw_dma, I think src_master needs to be '1' > for your driver. On some SoCs we have up to 4 masters. It's blurry for me how the SPI should choose those masters. Currently it works fine, but I suspect there are [might be] performance issues. What about AVR32 case? We have to fix drivers as well there. > the dw_dma driver can be simplified a little by removing the special > case for the request line setting. Yes, this part I like.
On Friday 25 July 2014 13:45:47 Andy Shevchenko wrote: > On Fri, 2014-07-25 at 12:19 +0200, Arnd Bergmann wrote: > > On Friday 25 July 2014 12:55:59 Mika Westerberg wrote: > > > On Fri, Jul 25, 2014 at 12:07:14PM +0300, Mika Westerberg wrote: > > > > On Fri, Jul 25, 2014 at 10:38:36AM +0200, Arnd Bergmann wrote: > > > > > On Friday 25 July 2014 11:22:49 Mika Westerberg wrote: > > [] > > > > Something like this? > > Arnd, this dependency to certain DMA driver looks really bad. > > If we go that way, can we split that part to [another] module and make > it dependent to DW_DMAC? I don't see what you gain from that. The PCI ID will tell you which DMA engine is being used. The driver already hardcodes a slave_id based on the PCI ID today, and the > Or shall we introduce a dmaengine type field in the platform data and > dynamically choose proper filter-whatever-function to get the channel? We already have an interface for this, in the form of dma_request_slave_channel(), which takes a string identifier that is used to look up all that information in either device tree or ACPI. It wouldn't be unreasonable to add a third path in there to handle hardcoded platform devices, but that's a lot of work. Note that you still need to encode a reference to the dma engine in some way to do this right. The current code (with or without Mika's patch) will break as soon as you have multiple DMA engine devices. The current plan I think is to convert all platforms to use DT or ACPI so they get the right data from tables passed by the platform. I'm a bit puzzled about why Intel wants to support the non-ACPI non-DT case again. If we have to support this case anyway, what good will ACPI do us on those platforms? > > > Hock Leong / Chiaue Ee, are you able to check if this works on your BYT > > > machines? > > > What I think you got wrong here (by following my bad advice) is the master > > number. Looking at the code for dw_dma, I think src_master needs to be '1' > > for your driver. > > On some SoCs we have up to 4 masters. It's blurry for me how the SPI > should choose those masters. Currently it works fine, but I suspect > there are [might be] performance issues. I think it works because the dw-dma defaults to the values used by the specific implementation in your hardware. > What about AVR32 case? We have to fix drivers as well there. which ones? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> The current plan I think is to convert all platforms to use DT > or ACPI so they get the right data from tables passed by the > platform. I'm a bit puzzled about why Intel wants to support the > non-ACPI non-DT case again. If we have to support this case anyway, > what good will ACPI do us on those platforms? Because some industries move very very slowly so still don't believe in requiring ACPI or ACPI capable operating systems. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 25, 2014 at 12:19:02PM +0200, Arnd Bergmann wrote: > On Friday 25 July 2014 12:55:59 Mika Westerberg wrote: > > On Fri, Jul 25, 2014 at 12:07:14PM +0300, Mika Westerberg wrote: > > > On Fri, Jul 25, 2014 at 10:38:36AM +0200, Arnd Bergmann wrote: > > > > On Friday 25 July 2014 11:22:49 Mika Westerberg wrote: > > > > > > All you need to do is change your filter function to take the > > > > > > slave id from pxa_spi_info and stick it in there, e.g. > > > > > > > > > > > > static bool pxa2xx_spi_dw_dma_filter(struct dma_chan *chan, void *param) > > > > > > { > > > > > > const struct pxa2xx_spi_master *pdata = param; > > > > > > struct dw_dma_chan *dwc = to_dw_dma_chan(chan); > > > > > > > > > > > > dwc->request_line = fargs->req; > > > > > > dwc->src_master = 0; > > > > > > dwc->dst_master = 0; > > > > > > > > > > > > return 1; > > > > > > } > > > > > > > > > > Oh man. That makes pxa2xx_spi dependent on a certain specific DMA engine > > > > > driver. > > > > > > > > I think you can improve this by putting the filter function (and a pointer > > > > to it) into the pxa2xx_spi_master data provided by the PCI driver. > > > > > > Indeed, that looks better. It still makes the PCI part of the driver > > > dependent on a particular DMA engine driver but is certainly better than > > > the core pxa2xx_spi driver. > > > > Something like this? > > > > Hock Leong / Chiaue Ee, are you able to check if this works on your BYT > > machines? > > I think I found a small bug, and one detail that could be improved. > > > @@ -321,12 +310,14 @@ int pxa2xx_spi_dma_setup(struct driver_data *drv_data) > > return -ENOMEM; > > > > drv_data->tx_chan = dma_request_slave_channel_compat(mask, > > - pxa2xx_spi_dma_filter, pdata, dev, "tx"); > > + pdata->dma_filter, pdata->dma_filter_param, > > + dev, "tx"); > > if (!drv_data->tx_chan) > > return -ENODEV; > > If you change this part to pass '(void *)info->tx_slave_id' rather than > pdata->dma_filter_param, you can simplify the next code: OK > > +static bool pxa2xx_spi_pci_dma_filter(struct dma_chan *chan, void *param) > > +{ > > + struct dw_dma_chan *dwc = to_dw_dma_chan(chan); > > + const struct pxa_spi_info *info = param; > > + > > + if (chan->chan_id == info->tx_chan_id) > > + dwc->request_line = info->tx_slave_id; > > + else if (chan->chan_id == info->rx_chan_id) > > + dwc->request_line = info->rx_slave_id; > > + else > > + return false; > > + > > + dwc->src_master = 0; > > + dwc->dst_master = 0; > > + > > + return true; > > +} > > to disregard the tx_chan_id, which is really arbitrary as far as I can tell. > > What I think you got wrong here (by following my bad advice) is the master > number. Looking at the code for dw_dma, I think src_master needs to be '1' > for your driver. Right, I just copied your example. I'll change it to '1' in the next revision. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-07-25 at 17:55 +0200, Arnd Bergmann wrote: > On Friday 25 July 2014 13:45:47 Andy Shevchenko wrote: > > On Fri, 2014-07-25 at 12:19 +0200, Arnd Bergmann wrote: > > > On Friday 25 July 2014 12:55:59 Mika Westerberg wrote: > > > > On Fri, Jul 25, 2014 at 12:07:14PM +0300, Mika Westerberg wrote: > > > > > On Fri, Jul 25, 2014 at 10:38:36AM +0200, Arnd Bergmann wrote: > > > > > > On Friday 25 July 2014 11:22:49 Mika Westerberg wrote: > > > > [] > > > > > > Something like this? > > > > Arnd, this dependency to certain DMA driver looks really bad. > > > > If we go that way, can we split that part to [another] module and make > > it dependent to DW_DMAC? > > I don't see what you gain from that. The PCI ID will tell you which DMA > engine is being used. The driver already hardcodes a slave_id based on > the PCI ID today, and the "...and the..."? > > > Or shall we introduce a dmaengine type field in the platform data and > > dynamically choose proper filter-whatever-function to get the channel? > > We already have an interface for this, in the form of > dma_request_slave_channel(), which takes a string identifier that > is used to look up all that information in either device tree or > ACPI. It wouldn't be unreasonable to add a third path in there > to handle hardcoded platform devices, but that's a lot of work. > Note that you still need to encode a reference to the dma engine > in some way to do this right. The current code (with or without Mika's > patch) will break as soon as you have multiple DMA engine devices. What about to keep PCI case still valid? We can pass struct pci_dev (or actual struct device) of DMA controller to filter proper device. > The current plan I think is to convert all platforms to use DT > or ACPI so they get the right data from tables passed by the > platform. Good to know the road map. [] > > > What I think you got wrong here (by following my bad advice) is the master > > > number. Looking at the code for dw_dma, I think src_master needs to be '1' > > > for your driver. > > > > On some SoCs we have up to 4 masters. It's blurry for me how the SPI > > should choose those masters. Currently it works fine, but I suspect > > there are [might be] performance issues. > > I think it works because the dw-dma defaults to the values used by > the specific implementation in your hardware. > > What about AVR32 case? We have to fix drivers as well there. > which ones? arch/avr32/mach-at32ap/at32ap700x.c:1332:at32_add_device_mci It seems opaque for me if it's used anywhere.
On Mon, 2014-07-28 at 14:06 +0300, Andy Shevchenko wrote: > On Fri, 2014-07-25 at 17:55 +0200, Arnd Bergmann wrote: > > On Friday 25 July 2014 13:45:47 Andy Shevchenko wrote: [] > > > > What I think you got wrong here (by following my bad advice) is the master > > > > number. Looking at the code for dw_dma, I think src_master needs to be '1' > > > > for your driver. > > > > > > On some SoCs we have up to 4 masters. It's blurry for me how the SPI > > > should choose those masters. Currently it works fine, but I suspect > > > there are [might be] performance issues. > > > > I think it works because the dw-dma defaults to the values used by > > the specific implementation in your hardware. [Missed in previous email] Actually the defaults came from original driver for AVR32 case. Regarding to DW DMA databook the AHB masters could be used each by any channel, though it depends on what AHB layer is bound to (and corresponding peripheral device). Thus, like I said we might have the [minor] performance issues if we use, for example, two out of four masters. -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On Monday 28 July 2014 14:06:20 Andy Shevchenko wrote: > On Fri, 2014-07-25 at 17:55 +0200, Arnd Bergmann wrote: > > On Friday 25 July 2014 13:45:47 Andy Shevchenko wrote: > > > On Fri, 2014-07-25 at 12:19 +0200, Arnd Bergmann wrote: > > > > On Friday 25 July 2014 12:55:59 Mika Westerberg wrote: > > > > > On Fri, Jul 25, 2014 at 12:07:14PM +0300, Mika Westerberg wrote: > > > > > > On Fri, Jul 25, 2014 at 10:38:36AM +0200, Arnd Bergmann wrote: > > > > > > > On Friday 25 July 2014 11:22:49 Mika Westerberg wrote: > > > > > > [] > > > > > > > > Something like this? > > > > > > Arnd, this dependency to certain DMA driver looks really bad. > > > > > > If we go that way, can we split that part to [another] module and make > > > it dependent to DW_DMAC? > > > > I don't see what you gain from that. The PC ID will tell you which DMA > > engine is being used. The driver already hardcodes a slave_id based on > > the PCI ID today, and the > > "...and the..."? Sorry for missing that. the slave ID only makes sense in combination with the master device it is used in, so the dependency exists already. > > > Or shall we introduce a dmaengine type field in the platform data and > > > dynamically choose proper filter-whatever-function to get the channel? > > > > We already have an interface for this, in the form of > > dma_request_slave_channel(), which takes a string identifier that > > is used to look up all that information in either device tree or > > ACPI. It wouldn't be unreasonable to add a third path in there > > to handle hardcoded platform devices, but that's a lot of work. > > Note that you still need to encode a reference to the dma engine > > in some way to do this right. The current code (with or without Mika's > > patch) will break as soon as you have multiple DMA engine devices. > > What about to keep PCI case still valid? We can pass struct pci_dev (or > actual struct device) of DMA controller to filter proper device. Yes, that would be best. IIRC you have a single PCI device that instantiates both the SPI and the DMA engine as subdevices (I haven't looked at the code again), so that information would be readily available. > > > > What I think you got wrong here (by following my bad advice) is the master > > > > number. Looking at the code for dw_dma, I think src_master needs to be '1' > > > > for your driver. > > > > > > On some SoCs we have up to 4 masters. It's blurry for me how the SPI > > > should choose those masters. Currently it works fine, but I suspect > > > there are [might be] performance issues. > > > > I think it works because the dw-dma defaults to the values used by > > the specific implementation in your hardware. > > > > > > What about AVR32 case? We have to fix drivers as well there. > > > which ones? > > arch/avr32/mach-at32ap/at32ap700x.c:1332:at32_add_device_mci > > It seems opaque for me if it's used anywhere. It seems correct to me. This is the filter function used by atmel_mci: static bool atmci_filter(struct dma_chan *chan, void *pdata) { struct mci_platform_data *sl_pdata = pdata; struct mci_dma_data *sl; if (!sl_pdata) return false; sl = sl_pdata->dma_slave; if (sl && find_slave_dev(sl) == chan->device->dev) { chan->private = slave_data_ptr(sl); return true; } else { return false; } } It sets the dw_dma_slave information from slave_data_ptr(sl) here, and does not attempt to set a slave_id at all, the slave_config call only sets the required fields. Do you still see a problem here? > Actually the defaults came from original driver for AVR32 case. > > Regarding to DW DMA databook the AHB masters could be used each by any > channel, though it depends on what AHB layer is bound to (and > corresponding peripheral device). > > Thus, like I said we might have the [minor] performance issues if we > use, for example, two out of four masters. I don't see how: We have four cases that I am aware of, and all seem to handle this right: a) request line and masters are all configured from device-tree. b) request line is configured from ACPI, masters are autoconfigured c) request line and masters are all configured from avr32/arm32 platform data. d) request line and masters are all configured from Bay Trail PCI ID (Implemented by Mika's patch) The ACPI case could be extended if we ever get an ACPI based system that has more than two masters. In the other cases, the same source of information that configures the request line also configures the masters, so it's up to the system integration to pick the best setting. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-07-28 at 14:47 +0200, Arnd Bergmann wrote: > On Monday 28 July 2014 14:06:20 Andy Shevchenko wrote: > > On Fri, 2014-07-25 at 17:55 +0200, Arnd Bergmann wrote: > > > On Friday 25 July 2014 13:45:47 Andy Shevchenko wrote: > > > > On Fri, 2014-07-25 at 12:19 +0200, Arnd Bergmann wrote: > > > > > On Friday 25 July 2014 12:55:59 Mika Westerberg wrote: > > > > Arnd, this dependency to certain DMA driver looks really bad. > > > > > > > > If we go that way, can we split that part to [another] module and make > > > > it dependent to DW_DMAC? > > > > > > I don't see what you gain from that. The PC ID will tell you which DMA > > > engine is being used. The driver already hardcodes a slave_id based on > > > the PCI ID today, and the > > > > "...and the..."? > > Sorry for missing that. > > the slave ID only makes sense in combination with the master device > it is used in, so the dependency exists already. Yes, this is plain data that could be nicely fit in the pci driver. > > > > > What I think you got wrong here (by following my bad advice) is the master > > > > > number. Looking at the code for dw_dma, I think src_master needs to be '1' > > > > > for your driver. > > > > > > > > On some SoCs we have up to 4 masters. It's blurry for me how the SPI > > > > should choose those masters. Currently it works fine, but I suspect > > > > there are [might be] performance issues. > > > > > > I think it works because the dw-dma defaults to the values used by > > > the specific implementation in your hardware. > > > > > > > > > > What about AVR32 case? We have to fix drivers as well there. > > > > > which ones? > > > > arch/avr32/mach-at32ap/at32ap700x.c:1332:at32_add_device_mci > > > > It seems opaque for me if it's used anywhere. > > It seems correct to me. This is the filter function used by atmel_mci: > > static bool atmci_filter(struct dma_chan *chan, void *pdata) > { > struct mci_platform_data *sl_pdata = pdata; > struct mci_dma_data *sl; > > if (!sl_pdata) > return false; > > sl = sl_pdata->dma_slave; > if (sl && find_slave_dev(sl) == chan->device->dev) { > chan->private = slave_data_ptr(sl); > return true; > } else { > return false; > } > } > > It sets the dw_dma_slave information from slave_data_ptr(sl) here, > and does not attempt to set a slave_id at all, the slave_config > call only sets the required fields. > > Do you still see a problem here? Sorry, I was talking about master defaults regarding to your patch that proposes to remove that part from dw_dmac. I mean it should be set in function mentioned early. > > Actually the defaults came from original driver for AVR32 case. > > > > Regarding to DW DMA databook the AHB masters could be used each by any > > channel, though it depends on what AHB layer is bound to (and > > corresponding peripheral device). > > > > Thus, like I said we might have the [minor] performance issues if we > > use, for example, two out of four masters. > > I don't see how: We have four cases that I am aware of, and all seem > to handle this right: > > a) request line and masters are all configured from device-tree. > b) request line is configured from ACPI, masters are autoconfigured They are just defaults. Say, hardcoded. We just get master bus width and number of masters we have, but we don't use them. > c) request line and masters are all configured from avr32/arm32 > platform data. > d) request line and masters are all configured from Bay Trail > PCI ID (Implemented by Mika's patch) Actually here I just recall the case when we might have different number of masters and use the same IP block [of SPI controller], how could you distinguish that case? How to provide proper masters? It's luckily not a problem right now, but still potential one in some cases. > The ACPI case could be extended if we ever get an ACPI based system > that has more than two masters. > In the other cases, the same source of information that configures > the request line also configures the masters, so it's up to the > system integration to pick the best setting. Yeah, ACPI / DT cases work / will work fine, since we can get any information we need from there.
On Monday 28 July 2014 16:34:04 Andy Shevchenko wrote: > On Mon, 2014-07-28 at 14:47 +0200, Arnd Bergmann wrote: > > On Monday 28 July 2014 14:06:20 Andy Shevchenko wrote: > > static bool atmci_filter(struct dma_chan *chan, void *pdata) > > { > > struct mci_platform_data *sl_pdata = pdata; > > struct mci_dma_data *sl; > > > > if (!sl_pdata) > > return false; > > > > sl = sl_pdata->dma_slave; > > if (sl && find_slave_dev(sl) == chan->device->dev) { > > chan->private = slave_data_ptr(sl); > > return true; > > } else { > > return false; > > } > > } > > > > It sets the dw_dma_slave information from slave_data_ptr(sl) here, > > and does not attempt to set a slave_id at all, the slave_config > > call only sets the required fields. > > > > Do you still see a problem here? > > Sorry, I was talking about master defaults regarding to your patch that > proposes to remove that part from dw_dmac. I mean it should be set in > function mentioned early. Ok. So atmci_filter is still correct because chan->private contains a pointer to a structure with both request and master settings, right? > > c) request line and masters are all configured from avr32/arm32 > > platform data. > > d) request line and masters are all configured from Bay Trail > > PCI ID (Implemented by Mika's patch) > > Actually here I just recall the case when we might have different number > of masters and use the same IP block [of SPI controller], how could you > distinguish that case? How to provide proper masters? > > It's luckily not a problem right now, but still potential one in some > cases. Do you mean case d? I think for each PCI ID, you will have to add the correct settings to some table that is used in the filter function. Are there any cases you know where we can't hardcode them? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c index 1af731b83b3f..674f69fe6662 100644 --- a/drivers/dma/dw/core.c +++ b/drivers/dma/dw/core.c @@ -37,24 +37,6 @@ * support descriptor writeback. */ -static inline bool is_request_line_unset(struct dw_dma_chan *dwc) -{ - return dwc->request_line == (typeof(dwc->request_line))~0; -} - -static inline void dwc_set_masters(struct dw_dma_chan *dwc) -{ - struct dw_dma *dw = to_dw_dma(dwc->chan.device); - struct dw_dma_slave *dws = dwc->chan.private; - unsigned char mmax = dw->nr_masters - 1; - - if (!is_request_line_unset(dwc)) - return; - - dwc->src_master = min_t(unsigned char, mmax, dwc_get_sms(dws)); - dwc->dst_master = min_t(unsigned char, mmax, dwc_get_dms(dws)); -} - #define DWC_DEFAULT_CTLLO(_chan) ({ \ struct dw_dma_chan *_dwc = to_dw_dma_chan(_chan); \ struct dma_slave_config *_sconfig = &_dwc->dma_sconfig; \ @@ -967,10 +949,6 @@ set_runtime_config(struct dma_chan *chan, struct dma_slave_config *sconfig) memcpy(&dwc->dma_sconfig, sconfig, sizeof(*sconfig)); dwc->direction = sconfig->direction; - /* Take the request line from slave_id member */ - if (is_request_line_unset(dwc)) - dwc->request_line = sconfig->slave_id; - convert_burst(&dwc->dma_sconfig.src_maxburst); convert_burst(&dwc->dma_sconfig.dst_maxburst); @@ -1123,8 +1101,6 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan) * doesn't mean what you think it means), and status writeback. */ - dwc_set_masters(dwc); - spin_lock_irqsave(&dwc->lock, flags); i = dwc->descs_allocated; while (dwc->descs_allocated < NR_DESCS_PER_CHANNEL) {