| Message ID | 1371762407-24544-6-git-send-email-joelagnel@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 6/21/2013 2:36 AM, Joel A Fernandes wrote: > From: Joel A Fernandes <agnel.joel@gmail.com> > > Build TI_EDMA by default for ARCH_DAVINCI and ARCH_OMAP2PLUS > > Signed-off-by: Joel A Fernandes <joelagnel@ti.com> You should sign-off with author e-mail address. > --- > arch/arm/Kconfig | 1 + > arch/arm/mach-omap2/Kconfig | 1 + > drivers/dma/Kconfig | 2 +- > 3 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index b1c66a4..7d58cd9 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -841,6 +841,7 @@ config ARCH_DAVINCI > select HAVE_IDE > select NEED_MACH_GPIO_H > select TI_PRIV_EDMA > + select DMADEVICES It is generally a bad idea to force select on something that can be enabled using menuconfig. Unless used carefully, select causes "unmet direct dependency" warnings which folks are already fighting hard to fix. This leads to what Russell referred in the past as "select madness" [1] In this particular case, it is perfectly okay to have a DaVinci platform without DMA engine support. Drivers figure out they don't have DMA support and switch to PIO mode. Add this in defconfig if its useful for most folks using the platform, but don't force it for everyone through select. > select USE_OF > select ZONE_DMA > help > diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig > index f91b07f..c02f083 100644 > --- a/arch/arm/mach-omap2/Kconfig > +++ b/arch/arm/mach-omap2/Kconfig > @@ -17,6 +17,7 @@ config ARCH_OMAP2PLUS > select PROC_DEVICETREE if PROC_FS > select SOC_BUS > select SPARSE_IRQ > + select DMADEVICES > select TI_PRIV_EDMA > select USE_OF > help > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig > index 3215a3c..b2d6f15 100644 > --- a/drivers/dma/Kconfig > +++ b/drivers/dma/Kconfig > @@ -216,7 +216,7 @@ config TI_EDMA > depends on ARCH_DAVINCI || ARCH_OMAP > select DMA_ENGINE > select DMA_VIRTUAL_CHANNELS > - default n > + default y Can't see why DMA support should default to y. Thanks, Sekhar [1] http://lkml.org/lkml/2013/3/4/114
On Fri, Jun 21, 2013 at 5:16 AM, Sekhar Nori <nsekhar@ti.com> wrote: > On 6/21/2013 2:36 AM, Joel A Fernandes wrote: >> From: Joel A Fernandes <agnel.joel@gmail.com> >> >> Build TI_EDMA by default for ARCH_DAVINCI and ARCH_OMAP2PLUS >> >> Signed-off-by: Joel A Fernandes <joelagnel@ti.com> > > You should sign-off with author e-mail address. > >> --- >> arch/arm/Kconfig | 1 + >> arch/arm/mach-omap2/Kconfig | 1 + >> drivers/dma/Kconfig | 2 +- >> 3 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig >> index b1c66a4..7d58cd9 100644 >> --- a/arch/arm/Kconfig >> +++ b/arch/arm/Kconfig >> @@ -841,6 +841,7 @@ config ARCH_DAVINCI >> select HAVE_IDE >> select NEED_MACH_GPIO_H >> select TI_PRIV_EDMA >> + select DMADEVICES > > It is generally a bad idea to force select on something that can be > enabled using menuconfig. Unless used carefully, select causes "unmet > direct dependency" warnings which folks are already fighting hard to > fix. This leads to what Russell referred in the past as "select madness" [1] Are you concerned with bloat issues? I know your point of view but the idea was to build these options by default for these platforms even though in some cases it might not be used. I have seen folks including myself select the wrong option. Having the build system automatically select the correct option for the most common cases can be very useful I feel and not require manual configuration. > In this particular case, it is perfectly okay to have a DaVinci platform > without DMA engine support. Drivers figure out they don't have DMA > support and switch to PIO mode. Drivers can still use PIO mode if they wish even though DMA is built into the kernel right. >> select USE_OF >> select ZONE_DMA >> help >> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig >> index f91b07f..c02f083 100644 >> --- a/arch/arm/mach-omap2/Kconfig >> +++ b/arch/arm/mach-omap2/Kconfig >> @@ -17,6 +17,7 @@ config ARCH_OMAP2PLUS >> select PROC_DEVICETREE if PROC_FS >> select SOC_BUS >> select SPARSE_IRQ >> + select DMADEVICES >> select TI_PRIV_EDMA >> select USE_OF >> help >> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig >> index 3215a3c..b2d6f15 100644 >> --- a/drivers/dma/Kconfig >> +++ b/drivers/dma/Kconfig >> @@ -216,7 +216,7 @@ config TI_EDMA >> depends on ARCH_DAVINCI || ARCH_OMAP >> select DMA_ENGINE >> select DMA_VIRTUAL_CHANNELS >> - default n >> + default y > > Can't see why DMA support should default to y. > For same reason as above. Thanks, Joel
On Friday 21 June 2013, Joel A Fernandes wrote: > >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > >> index b1c66a4..7d58cd9 100644 > >> --- a/arch/arm/Kconfig > >> +++ b/arch/arm/Kconfig > >> @@ -841,6 +841,7 @@ config ARCH_DAVINCI > >> select HAVE_IDE > >> select NEED_MACH_GPIO_H > >> select TI_PRIV_EDMA > >> + select DMADEVICES > > > > It is generally a bad idea to force select on something that can be > > enabled using menuconfig. Unless used carefully, select causes "unmet > > direct dependency" warnings which folks are already fighting hard to > > fix. This leads to what Russell referred in the past as "select madness" [1] > > Are you concerned with bloat issues? I know your point of view but the idea > was to build these options by default for these platforms even though > in some cases > it might not be used. I have seen folks including myself select the wrong > option. Having the build system automatically select the correct option for the > most common cases can be very useful I feel and not require manual > configuration. For defaults, you should use the defconfig, not 'select' in Kconfig. A lot of the 'select' statements are actually wrong because they do not take dependencies into account where A selects B but not C, and B depends on C, which leads to broken builds when C is disabled by a user (or randconfig). You should only ever use 'select' from platforms on silent options that are not themselves user selectable like the above 'HAVE_IDE' and 'NEED_MACH_GPIO_H'. Arnd
On Fri, Jun 21, 2013 at 9:00 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 21 June 2013, Joel A Fernandes wrote: >> >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig >> >> index b1c66a4..7d58cd9 100644 >> >> --- a/arch/arm/Kconfig >> >> +++ b/arch/arm/Kconfig >> >> @@ -841,6 +841,7 @@ config ARCH_DAVINCI >> >> select HAVE_IDE >> >> select NEED_MACH_GPIO_H >> >> select TI_PRIV_EDMA >> >> + select DMADEVICES >> > >> > It is generally a bad idea to force select on something that can be >> > enabled using menuconfig. Unless used carefully, select causes "unmet >> > direct dependency" warnings which folks are already fighting hard to >> > fix. This leads to what Russell referred in the past as "select madness" [1] >> >> Are you concerned with bloat issues? I know your point of view but the idea >> was to build these options by default for these platforms even though >> in some cases >> it might not be used. I have seen folks including myself select the wrong >> option. Having the build system automatically select the correct option for the >> most common cases can be very useful I feel and not require manual >> configuration. > > For defaults, you should use the defconfig, not 'select' in Kconfig. > > A lot of the 'select' statements are actually wrong because they > do not take dependencies into account where A selects B but not C, > and B depends on C, which leads to broken builds when C is disabled > by a user (or randconfig). I haven't come across this problem but- are you saying there is a shortcoming in Kbuild/Kconfig that selects an option even if its dependency is not met? The problem with defconfig is also too many options I feel for a common case. CONFIG_DMADEVICES=y CONFIG_TI_EDMA=y Most if not all future OMAPs from will use EDMA. Why not we can be explicit about it and just built it in anyway. If ARCH_OMAP and DMADEVICES are selected, then we can just build EDMA in by default. I agree maybe the option can be dropped from Davinci but I suggest let's keep it for OMAP. Is that ok? Thanks, Joel
On Friday 21 June 2013, Joel A Fernandes wrote: > I haven't come across this problem but- are you saying there is a > shortcoming in Kbuild/Kconfig that selects an option even if its > dependency is not met? Well, the shortcoming is that it lets you specify impossible contraints. You get a warning from Kconfig when building such a configuration, but then it continues. > The problem with defconfig is also too many options I feel for a common case. > CONFIG_DMADEVICES=y > CONFIG_TI_EDMA=y > > Most if not all future OMAPs from will use EDMA. Why not we can be > explicit about it and just built it in anyway. If ARCH_OMAP and > DMADEVICES are selected, then we can just build EDMA in by default. It's just not how we do things. Kconfig is a mess because we are not consistent in the way this is done. > I agree maybe the option can be dropped from Davinci but I suggest > let's keep it for OMAP. Is that ok? No, I would still like you to not add it to either one. I'm spending a lot of my time tracking down incorrect 'select' statements and I'd rather spend it in a different way. I've had to a number of 'select' statements from OMAP in the past, please don't add any new ones unless there is a strict build dependency (which normally should not exist). Arnd
On Fri, Jun 21, 2013 at 9:32 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 21 June 2013, Joel A Fernandes wrote: >> I haven't come across this problem but- are you saying there is a >> shortcoming in Kbuild/Kconfig that selects an option even if its >> dependency is not met? > > Well, the shortcoming is that it lets you specify impossible > contraints. You get a warning from Kconfig when building > such a configuration, but then it continues. > >> The problem with defconfig is also too many options I feel for a common case. >> CONFIG_DMADEVICES=y >> CONFIG_TI_EDMA=y >> >> Most if not all future OMAPs from will use EDMA. Why not we can be >> explicit about it and just built it in anyway. If ARCH_OMAP and >> DMADEVICES are selected, then we can just build EDMA in by default. > > It's just not how we do things. Kconfig is a mess because we are > not consistent in the way this is done. > >> I agree maybe the option can be dropped from Davinci but I suggest >> let's keep it for OMAP. Is that ok? > > No, I would still like you to not add it to either one. I'm spending > a lot of my time tracking down incorrect 'select' statements and I'd > rather spend it in a different way. I've had to a number of 'select' > statements from OMAP in the past, please don't add any new ones > unless there is a strict build dependency (which normally should not > exist). I think we are talking about different things, I agree the 'select DMADEVICES' can be dropped but lets please keep the default y option (not adding new select statements, just saying that if someone select DMADEVICES in menuconfig and if they're ARCH_OMAP=1 , then default to EDMA). This will simply allow people to have a default. Thanks. Thanks Joel
On Friday 21 June 2013, Joel A Fernandes wrote: > I think we are talking about different things, I agree the 'select > DMADEVICES' can be dropped but lets please keep the default y option > (not adding new select statements, just saying that if someone select > DMADEVICES in menuconfig and if they're ARCH_OMAP=1 , then default to > EDMA). This will simply allow people to have a default. Thanks. Yes, that's ok. Also, if the driver doesn't strictly depend on these platforms but can build on anything, I would write it as config TI_EDMA tristate "TI EDMA support" default m if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2 select DMA_ENGINE select DMA_VIRTUAL_CHANNELS Arnd
Hi Arnd, On Fri, Jun 21, 2013 at 1:44 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 21 June 2013, Joel A Fernandes wrote: >> I think we are talking about different things, I agree the 'select >> DMADEVICES' can be dropped but lets please keep the default y option >> (not adding new select statements, just saying that if someone select >> DMADEVICES in menuconfig and if they're ARCH_OMAP=1 , then default to >> EDMA). This will simply allow people to have a default. Thanks. > > Yes, that's ok. Ok, thanks. I will follow up with a patch in my next submissions that builds it. Perhaps a: default y if 'ARCH_OMAP2PLUS' and leave the existing as it is... default n if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2' would make most sense to me. Basically EDMA is seen on current and all new OMAP2PLUS. > > config TI_EDMA > tristate "TI EDMA support" > default m if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2 > select DMA_ENGINE > select DMA_VIRTUAL_CHANNELS MMC depends on EDMA specially on AM33xx there's no PIO mode AFAIK. The 'm' option will require some initramfs to load the module when needing to MMC boot, I suggest lets leave it as y. Thanks, Joel
On Friday 21 June 2013, Joel A Fernandes wrote: > Hi Arnd, > > On Fri, Jun 21, 2013 at 1:44 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Friday 21 June 2013, Joel A Fernandes wrote: > >> I think we are talking about different things, I agree the 'select > >> DMADEVICES' can be dropped but lets please keep the default y option > >> (not adding new select statements, just saying that if someone select > >> DMADEVICES in menuconfig and if they're ARCH_OMAP=1 , then default to > >> EDMA). This will simply allow people to have a default. Thanks. > > > > Yes, that's ok. > > Ok, thanks. I will follow up with a patch in my next submissions that builds it. > > Perhaps a: > default y if 'ARCH_OMAP2PLUS' > > and leave the existing as it is... > default n if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2' > > would make most sense to me. Basically EDMA is seen on current and all > new OMAP2PLUS. Ok > > config TI_EDMA > > tristate "TI EDMA support" > > default m if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2 > > select DMA_ENGINE > > select DMA_VIRTUAL_CHANNELS > > > MMC depends on EDMA specially on AM33xx there's no PIO mode AFAIK. The > 'm' option will require some initramfs to load the module when needing > to MMC boot, I suggest lets leave it as y. > Ah, right: you still export a filter function from the edma driver and use it in slave drivers: drivers/mmc/host/davinci_mmc.c: dma_request_slave_channel_compat(mask, edma_filter_fn, drivers/mmc/host/davinci_mmc.c: dma_request_slave_channel_compat(mask, edma_filter_fn, drivers/spi/spi-davinci.c: dspi->dma_rx = dma_request_channel(mask, edma_filter_fn, drivers/spi/spi-davinci.c: dspi->dma_tx = dma_request_channel(mask, edma_filter_fn, As long as this is the case, you have to be careful with the dependencies to make sure that davinci_mmc and spi-davinci either depend on TI_EDMA, or edma_filter_fn gets defined to NULL when you are building for a DT-only platform. Arnd
>> > config TI_EDMA >> > tristate "TI EDMA support" >> > default m if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2 >> > select DMA_ENGINE >> > select DMA_VIRTUAL_CHANNELS >> >> >> MMC depends on EDMA specially on AM33xx there's no PIO mode AFAIK. The >> 'm' option will require some initramfs to load the module when needing >> to MMC boot, I suggest lets leave it as y. >> > > Ah, right: you still export a filter function from the edma driver > and use it in slave drivers: > > drivers/mmc/host/davinci_mmc.c: dma_request_slave_channel_compat(mask, edma_filter_fn, > drivers/mmc/host/davinci_mmc.c: dma_request_slave_channel_compat(mask, edma_filter_fn, > drivers/spi/spi-davinci.c: dspi->dma_rx = dma_request_channel(mask, edma_filter_fn, > drivers/spi/spi-davinci.c: dspi->dma_tx = dma_request_channel(mask, edma_filter_fn, > > As long as this is the case, you have to be careful with the dependencies > to make sure that davinci_mmc and spi-davinci either depend on TI_EDMA, or > edma_filter_fn gets defined to NULL when you are building for a DT-only platform. Yes sure, right now they are defined as follows in include/linux/edma.h: #if defined(CONFIG_TI_EDMA) || defined(CONFIG_TI_EDMA_MODULE) bool edma_filter_fn(struct dma_chan *, void *); #else static inline bool edma_filter_fn(struct dma_chan *chan, void *param) { return false; } #endif This also has the side effect of causing DMA requests to fail if TI_EDMA is not built, causing frustration for a lot of people some of whom don't want to deal with DMA so I think it is OK to build the driver in by default as it is (and will be) used by a lot of OMAP2PLUS. Thanks, Joel
On 6/22/2013 3:23 AM, Joel A Fernandes wrote: > Hi Arnd, > > On Fri, Jun 21, 2013 at 1:44 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Friday 21 June 2013, Joel A Fernandes wrote: >>> I think we are talking about different things, I agree the 'select >>> DMADEVICES' can be dropped but lets please keep the default y option >>> (not adding new select statements, just saying that if someone select >>> DMADEVICES in menuconfig and if they're ARCH_OMAP=1 , then default to >>> EDMA). This will simply allow people to have a default. Thanks. >> >> Yes, that's ok. > > Ok, thanks. I will follow up with a patch in my next submissions that builds it. > > Perhaps a: > default y if 'ARCH_OMAP2PLUS' > > and leave the existing as it is... > default n if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2' > > would make most sense to me. Basically EDMA is seen on current and all > new OMAP2PLUS. OMAP2PLUS devices like OMAP3/4 do not have EDMA so this is not really true. > >> >> config TI_EDMA >> tristate "TI EDMA support" >> default m if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2 >> select DMA_ENGINE >> select DMA_VIRTUAL_CHANNELS > > > > MMC depends on EDMA specially on AM33xx there's no PIO mode AFAIK. The > 'm' option will require some initramfs to load the module when needing > to MMC boot, I suggest lets leave it as y. But there is no reason why it cannot work with PIO, right? Sounds like the right fix is in driver. Thanks, Sekhar
On 6/24/2013 4:53 PM, Sekhar Nori wrote: > On 6/22/2013 3:23 AM, Joel A Fernandes wrote: >> Hi Arnd, >> >> On Fri, Jun 21, 2013 at 1:44 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Friday 21 June 2013, Joel A Fernandes wrote: >>>> I think we are talking about different things, I agree the 'select >>>> DMADEVICES' can be dropped but lets please keep the default y option >>>> (not adding new select statements, just saying that if someone select >>>> DMADEVICES in menuconfig and if they're ARCH_OMAP=1 , then default to >>>> EDMA). This will simply allow people to have a default. Thanks. >>> >>> Yes, that's ok. >> >> Ok, thanks. I will follow up with a patch in my next submissions that builds it. >> >> Perhaps a: >> default y if 'ARCH_OMAP2PLUS' >> >> and leave the existing as it is... >> default n if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2' >> >> would make most sense to me. Basically EDMA is seen on current and all >> new OMAP2PLUS. > > OMAP2PLUS devices like OMAP3/4 do not have EDMA so this is not really true. More accurately, there is EDMA hardware on these devices, but its usage is limited to DSP. ARM uses SDMA instead. Thanks, Sekhar
On 6/22/2013 8:23 AM, Joel A Fernandes wrote: >>>> config TI_EDMA >>>> tristate "TI EDMA support" >>>> default m if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2 >>>> select DMA_ENGINE >>>> select DMA_VIRTUAL_CHANNELS >>> >>> >>> MMC depends on EDMA specially on AM33xx there's no PIO mode AFAIK. The >>> 'm' option will require some initramfs to load the module when needing >>> to MMC boot, I suggest lets leave it as y. >>> >> >> Ah, right: you still export a filter function from the edma driver >> and use it in slave drivers: >> >> drivers/mmc/host/davinci_mmc.c: dma_request_slave_channel_compat(mask, edma_filter_fn, >> drivers/mmc/host/davinci_mmc.c: dma_request_slave_channel_compat(mask, edma_filter_fn, >> drivers/spi/spi-davinci.c: dspi->dma_rx = dma_request_channel(mask, edma_filter_fn, >> drivers/spi/spi-davinci.c: dspi->dma_tx = dma_request_channel(mask, edma_filter_fn, >> >> As long as this is the case, you have to be careful with the dependencies >> to make sure that davinci_mmc and spi-davinci either depend on TI_EDMA, or >> edma_filter_fn gets defined to NULL when you are building for a DT-only platform. > > Yes sure, right now they are defined as follows in include/linux/edma.h: > > #if defined(CONFIG_TI_EDMA) || defined(CONFIG_TI_EDMA_MODULE) > bool edma_filter_fn(struct dma_chan *, void *); > #else > static inline bool edma_filter_fn(struct dma_chan *chan, void *param) > { > return false; > } > #endif > > This also has the side effect of causing DMA requests to fail if > TI_EDMA is not built, causing frustration for a lot of people some of > whom don't want to deal with DMA so I think it is OK to build the > driver in by default as it is (and will be) used by a lot of > OMAP2PLUS. Solution for this is to enable TI_EDMA in relevant defconfigs. Most folks who would get frustrated by such issues would be using defconfigs and for those who are building their configuration from scratch this will be pretty low in their list of worries. Thanks, Sekhar
On Mon, Jun 24, 2013 at 6:53 AM, Sekhar Nori <nsekhar@ti.com> wrote: > On 6/22/2013 8:23 AM, Joel A Fernandes wrote: >>>>> config TI_EDMA >>>>> tristate "TI EDMA support" >>>>> default m if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2 >>>>> select DMA_ENGINE >>>>> select DMA_VIRTUAL_CHANNELS >>>> >>>> >>>> MMC depends on EDMA specially on AM33xx there's no PIO mode AFAIK. The >>>> 'm' option will require some initramfs to load the module when needing >>>> to MMC boot, I suggest lets leave it as y. >>>> >>> >>> Ah, right: you still export a filter function from the edma driver >>> and use it in slave drivers: >>> >>> drivers/mmc/host/davinci_mmc.c: dma_request_slave_channel_compat(mask, edma_filter_fn, >>> drivers/mmc/host/davinci_mmc.c: dma_request_slave_channel_compat(mask, edma_filter_fn, >>> drivers/spi/spi-davinci.c: dspi->dma_rx = dma_request_channel(mask, edma_filter_fn, >>> drivers/spi/spi-davinci.c: dspi->dma_tx = dma_request_channel(mask, edma_filter_fn, >>> >>> As long as this is the case, you have to be careful with the dependencies >>> to make sure that davinci_mmc and spi-davinci either depend on TI_EDMA, or >>> edma_filter_fn gets defined to NULL when you are building for a DT-only platform. >> >> Yes sure, right now they are defined as follows in include/linux/edma.h: >> >> #if defined(CONFIG_TI_EDMA) || defined(CONFIG_TI_EDMA_MODULE) >> bool edma_filter_fn(struct dma_chan *, void *); >> #else >> static inline bool edma_filter_fn(struct dma_chan *chan, void *param) >> { >> return false; >> } >> #endif >> >> This also has the side effect of causing DMA requests to fail if >> TI_EDMA is not built, causing frustration for a lot of people some of >> whom don't want to deal with DMA so I think it is OK to build the >> driver in by default as it is (and will be) used by a lot of >> OMAP2PLUS. > > Solution for this is to enable TI_EDMA in relevant defconfigs. Most > folks who would get frustrated by such issues would be using defconfigs > and for those who are building their configuration from scratch this > will be pretty low in their list of worries. Yes, it is not in omap2plus_defconfig either. I will post a patch to add it to the same, and we can ignore the Kconfig defaults and select patches. Thanks, Joel
On Mon, Jun 24, 2013 at 6:23 AM, Sekhar Nori <nsekhar@ti.com> wrote: > On 6/22/2013 3:23 AM, Joel A Fernandes wrote: >> Hi Arnd, >> >> On Fri, Jun 21, 2013 at 1:44 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Friday 21 June 2013, Joel A Fernandes wrote: >>>> I think we are talking about different things, I agree the 'select >>>> DMADEVICES' can be dropped but lets please keep the default y option >>>> (not adding new select statements, just saying that if someone select >>>> DMADEVICES in menuconfig and if they're ARCH_OMAP=1 , then default to >>>> EDMA). This will simply allow people to have a default. Thanks. >>> >>> Yes, that's ok. >> >> Ok, thanks. I will follow up with a patch in my next submissions that builds it. >> >> Perhaps a: >> default y if 'ARCH_OMAP2PLUS' >> >> and leave the existing as it is... >> default n if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2' >> >> would make most sense to me. Basically EDMA is seen on current and all >> new OMAP2PLUS. > > OMAP2PLUS devices like OMAP3/4 do not have EDMA so this is not really true. Sure. Right now though, and from what I've seen, all future SoCs are supporting EDMA. >> >>> >>> config TI_EDMA >>> tristate "TI EDMA support" >>> default m if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2 >>> select DMA_ENGINE >>> select DMA_VIRTUAL_CHANNELS >> >> >> >> MMC depends on EDMA specially on AM33xx there's no PIO mode AFAIK. The >> 'm' option will require some initramfs to load the module when needing >> to MMC boot, I suggest lets leave it as y. > > But there is no reason why it cannot work with PIO, right? Sounds like > the right fix is in driver. I am not sure about this. I agree no reason it cannot do PIO. I will check. But even if it did, loading EDMA as a module will require omap_hsmmc to switch to DMA mode which I am sure the driver doesn't support today. Thanks, Joel
On Saturday 22 June 2013, Joel A Fernandes wrote: > > >> > config TI_EDMA > >> > tristate "TI EDMA support" > >> > default m if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2 > >> > select DMA_ENGINE > >> > select DMA_VIRTUAL_CHANNELS > >> > >> > >> MMC depends on EDMA specially on AM33xx there's no PIO mode AFAIK. The > >> 'm' option will require some initramfs to load the module when needing > >> to MMC boot, I suggest lets leave it as y. > >> > > > > Ah, right: you still export a filter function from the edma driver > > and use it in slave drivers: > > > > drivers/mmc/host/davinci_mmc.c: dma_request_slave_channel_compat(mask, edma_filter_fn, > > drivers/mmc/host/davinci_mmc.c: dma_request_slave_channel_compat(mask, edma_filter_fn, > > drivers/spi/spi-davinci.c: dspi->dma_rx = dma_request_channel(mask, edma_filter_fn, > > drivers/spi/spi-davinci.c: dspi->dma_tx = dma_request_channel(mask, edma_filter_fn, > > > > As long as this is the case, you have to be careful with the dependencies > > to make sure that davinci_mmc and spi-davinci either depend on TI_EDMA, or > > edma_filter_fn gets defined to NULL when you are building for a DT-only platform. > > Yes sure, right now they are defined as follows in include/linux/edma.h: > > #if defined(CONFIG_TI_EDMA) || defined(CONFIG_TI_EDMA_MODULE) > bool edma_filter_fn(struct dma_chan *, void *); > #else > static inline bool edma_filter_fn(struct dma_chan *chan, void *param) > { > return false; > } > #endif It's best to just define the filter function in the platform code and pass a pointer to it through platform data. The way you do it above makes the slave drivers inherently nonportable. Arnd
On Mon, Jun 24, 2013 at 3:28 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Saturday 22 June 2013, Joel A Fernandes wrote: >> >> >> > config TI_EDMA >> >> > tristate "TI EDMA support" >> >> > default m if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2 >> >> > select DMA_ENGINE >> >> > select DMA_VIRTUAL_CHANNELS >> >> >> >> >> >> MMC depends on EDMA specially on AM33xx there's no PIO mode AFAIK. The >> >> 'm' option will require some initramfs to load the module when needing >> >> to MMC boot, I suggest lets leave it as y. >> >> >> > >> > Ah, right: you still export a filter function from the edma driver >> > and use it in slave drivers: >> > >> > drivers/mmc/host/davinci_mmc.c: dma_request_slave_channel_compat(mask, edma_filter_fn, >> > drivers/mmc/host/davinci_mmc.c: dma_request_slave_channel_compat(mask, edma_filter_fn, >> > drivers/spi/spi-davinci.c: dspi->dma_rx = dma_request_channel(mask, edma_filter_fn, >> > drivers/spi/spi-davinci.c: dspi->dma_tx = dma_request_channel(mask, edma_filter_fn, >> > >> > As long as this is the case, you have to be careful with the dependencies >> > to make sure that davinci_mmc and spi-davinci either depend on TI_EDMA, or >> > edma_filter_fn gets defined to NULL when you are building for a DT-only platform. >> >> Yes sure, right now they are defined as follows in include/linux/edma.h: >> >> #if defined(CONFIG_TI_EDMA) || defined(CONFIG_TI_EDMA_MODULE) >> bool edma_filter_fn(struct dma_chan *, void *); >> #else >> static inline bool edma_filter_fn(struct dma_chan *chan, void *param) >> { >> return false; >> } >> #endif > > It's best to just define the filter function in the platform > code and pass a pointer to it through platform data. The way you do > it above makes the slave drivers inherently nonportable. But with DT-only platforms, can you really do that? Thanks, Joel
On Monday 24 June 2013, Joel A Fernandes wrote: > >> Yes sure, right now they are defined as follows in include/linux/edma.h: > >> > >> #if defined(CONFIG_TI_EDMA) || defined(CONFIG_TI_EDMA_MODULE) > >> bool edma_filter_fn(struct dma_chan *, void *); > >> #else > >> static inline bool edma_filter_fn(struct dma_chan *chan, void *param) > >> { > >> return false; > >> } > >> #endif > > > > It's best to just define the filter function in the platform > > code and pass a pointer to it through platform data. The way you do > > it above makes the slave drivers inherently nonportable. > > But with DT-only platforms, can you really do that? The nice thing about this is that with a DT-only platform, the filter function will automatically go away: you have no platform_data, which means that if you are using dma_request_slave_channel_compat, you just pass NULL as the filter and the filter-data, same as just calling dma_request_slave_channel. Arnd
> -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: Monday, June 24, 2013 4:08 PM > To: Joel A Fernandes > Cc: Nori, Sekhar; Fernandes, Joel A; Tony Lindgren; Matt Porter; Grant Likely; > Rob Herring; Vinod Koul; Mark Brown; Cousson, Benoit; Russell King; Rob > Landley; Andrew Morton; Jason Kridner; Koen Kooi; Devicetree Discuss; Linux > OMAP List; Linux ARM Kernel List; Linux DaVinci Kernel List; Linux Kernel > Mailing List; Linux Documentation List; Linux MMC List; Linux SPI Devel List > Subject: Re: [PATCH v12 05/11] edma: config: Enable config options for EDMA > > On Monday 24 June 2013, Joel A Fernandes wrote: > > >> Yes sure, right now they are defined as follows in include/linux/edma.h: > > >> > > >> #if defined(CONFIG_TI_EDMA) || defined(CONFIG_TI_EDMA_MODULE) > bool > > >> edma_filter_fn(struct dma_chan *, void *); #else static inline bool > > >> edma_filter_fn(struct dma_chan *chan, void *param) { return false; > > >> } #endif > > > > > > It's best to just define the filter function in the platform code > > > and pass a pointer to it through platform data. The way you do it > > > above makes the slave drivers inherently nonportable. > > > > But with DT-only platforms, can you really do that? > > The nice thing about this is that with a DT-only platform, the filter function will > automatically go away: you have no platform_data, which means that if you > are using dma_request_slave_channel_compat, you just pass NULL as the filter > and the filter-data, same as just calling dma_request_slave_channel. > [Joel] Ah yes! Thanks for that. Right, edma_filter_fn is not passed explicitly for DT case. Thanks, Joel
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index b1c66a4..7d58cd9 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -841,6 +841,7 @@ config ARCH_DAVINCI select HAVE_IDE select NEED_MACH_GPIO_H select TI_PRIV_EDMA + select DMADEVICES select USE_OF select ZONE_DMA help diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig index f91b07f..c02f083 100644 --- a/arch/arm/mach-omap2/Kconfig +++ b/arch/arm/mach-omap2/Kconfig @@ -17,6 +17,7 @@ config ARCH_OMAP2PLUS select PROC_DEVICETREE if PROC_FS select SOC_BUS select SPARSE_IRQ + select DMADEVICES select TI_PRIV_EDMA select USE_OF help diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig index 3215a3c..b2d6f15 100644 --- a/drivers/dma/Kconfig +++ b/drivers/dma/Kconfig @@ -216,7 +216,7 @@ config TI_EDMA depends on ARCH_DAVINCI || ARCH_OMAP select DMA_ENGINE select DMA_VIRTUAL_CHANNELS - default n + default y help Enable support for the TI EDMA controller. This DMA engine is found on TI DaVinci and AM33xx parts.