2015-11-18 16:19:15

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support

The CPPI-4.1 driver selects TI_CPPI41, which is a dmaengine
driver and that may not be available when CONFIG_DMADEVICES
is not set:

warning: (USB_TI_CPPI41_DMA) selects TI_CPPI41 which has unmet direct dependencies (DMADEVICES && ARCH_OMAP)

This adds an extra dependency to avoid generating warnings in randconfig
builds. Ideally we'd remove the 'select' statement, but that has the
potential to break defconfig files.

Signed-off-by: Arnd Bergmann <[email protected]>
Fixes: 411dd19c682d ("usb: musb: Kconfig: Select the DMA driver if DMA mode of MUSB is enabled")

diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
index 1f2037bbeb0d..45c83baf675d 100644
--- a/drivers/usb/musb/Kconfig
+++ b/drivers/usb/musb/Kconfig
@@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA

config USB_TI_CPPI41_DMA
bool 'TI CPPI 4.1 (AM335x)'
- depends on ARCH_OMAP
+ depends on ARCH_OMAP && DMADEVICES
select TI_CPPI41

config USB_TUSB_OMAP_DMA


2015-11-18 18:29:30

by Bin Liu

[permalink] [raw]
Subject: Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support

Hi,

On Wed, Nov 18, 2015 at 10:18 AM, Arnd Bergmann <[email protected]> wrote:
> The CPPI-4.1 driver selects TI_CPPI41, which is a dmaengine
> driver and that may not be available when CONFIG_DMADEVICES
> is not set:
>
> warning: (USB_TI_CPPI41_DMA) selects TI_CPPI41 which has unmet direct dependencies (DMADEVICES && ARCH_OMAP)
>
> This adds an extra dependency to avoid generating warnings in randconfig
> builds. Ideally we'd remove the 'select' statement, but that has the
> potential to break defconfig files.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Fixes: 411dd19c682d ("usb: musb: Kconfig: Select the DMA driver if DMA mode of MUSB is enabled")
>
> diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
> index 1f2037bbeb0d..45c83baf675d 100644
> --- a/drivers/usb/musb/Kconfig
> +++ b/drivers/usb/musb/Kconfig
> @@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA
>
> config USB_TI_CPPI41_DMA
> bool 'TI CPPI 4.1 (AM335x)'
> - depends on ARCH_OMAP
> + depends on ARCH_OMAP && DMADEVICES
> select TI_CPPI41

I am not sure what the generic policy is, but instead of hiding
USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable
DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience
perspective.

Thanks,
-Bin.

>
> config USB_TUSB_OMAP_DMA
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2015-11-18 19:28:10

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support


Hi,

Bin Liu <[email protected]> writes:
> On Wed, Nov 18, 2015 at 10:18 AM, Arnd Bergmann <[email protected]> wrote:
>> The CPPI-4.1 driver selects TI_CPPI41, which is a dmaengine
>> driver and that may not be available when CONFIG_DMADEVICES
>> is not set:
>>
>> warning: (USB_TI_CPPI41_DMA) selects TI_CPPI41 which has unmet direct dependencies (DMADEVICES && ARCH_OMAP)
>>
>> This adds an extra dependency to avoid generating warnings in randconfig
>> builds. Ideally we'd remove the 'select' statement, but that has the
>> potential to break defconfig files.
>>
>> Signed-off-by: Arnd Bergmann <[email protected]>
>> Fixes: 411dd19c682d ("usb: musb: Kconfig: Select the DMA driver if DMA mode of MUSB is enabled")
>>
>> diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
>> index 1f2037bbeb0d..45c83baf675d 100644
>> --- a/drivers/usb/musb/Kconfig
>> +++ b/drivers/usb/musb/Kconfig
>> @@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA
>>
>> config USB_TI_CPPI41_DMA
>> bool 'TI CPPI 4.1 (AM335x)'
>> - depends on ARCH_OMAP
>> + depends on ARCH_OMAP && DMADEVICES
>> select TI_CPPI41
>
> I am not sure what the generic policy is, but instead of hiding
> USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable
> DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience
> perspective.

that would mean "select DMADEVICES" and that's frowned upon.

--
balbi


Attachments:
signature.asc (818.00 B)

2015-11-18 20:20:59

by Bin Liu

[permalink] [raw]
Subject: Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support

Hi,

On Wed, Nov 18, 2015 at 1:27 PM, Felipe Balbi <[email protected]> wrote:
>
> Hi,
>
> Bin Liu <[email protected]> writes:
>> On Wed, Nov 18, 2015 at 10:18 AM, Arnd Bergmann <[email protected]> wrote:
>>> The CPPI-4.1 driver selects TI_CPPI41, which is a dmaengine
>>> driver and that may not be available when CONFIG_DMADEVICES
>>> is not set:
>>>
>>> warning: (USB_TI_CPPI41_DMA) selects TI_CPPI41 which has unmet direct dependencies (DMADEVICES && ARCH_OMAP)
>>>
>>> This adds an extra dependency to avoid generating warnings in randconfig
>>> builds. Ideally we'd remove the 'select' statement, but that has the
>>> potential to break defconfig files.
>>>
>>> Signed-off-by: Arnd Bergmann <[email protected]>
>>> Fixes: 411dd19c682d ("usb: musb: Kconfig: Select the DMA driver if DMA mode of MUSB is enabled")
>>>
>>> diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
>>> index 1f2037bbeb0d..45c83baf675d 100644
>>> --- a/drivers/usb/musb/Kconfig
>>> +++ b/drivers/usb/musb/Kconfig
>>> @@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA
>>>
>>> config USB_TI_CPPI41_DMA
>>> bool 'TI CPPI 4.1 (AM335x)'
>>> - depends on ARCH_OMAP
>>> + depends on ARCH_OMAP && DMADEVICES
>>> select TI_CPPI41
>>
>> I am not sure what the generic policy is, but instead of hiding
>> USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable
>> DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience
>> perspective.
>
> that would mean "select DMADEVICES" and that's frowned upon.

Currently 'select DMADEVICES' is not in there. Will adding it fix the
dependency warning in randconfig? Sorry for the question, but I don't
know enough about Kconfig to get the answer.

Regards,
-Bin.

>
> --
> balbi

2015-11-18 20:28:31

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support

On Wednesday 18 November 2015 12:29:27 Bin Liu wrote:
> > diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
> > index 1f2037bbeb0d..45c83baf675d 100644
> > --- a/drivers/usb/musb/Kconfig
> > +++ b/drivers/usb/musb/Kconfig
> > @@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA
> >
> > config USB_TI_CPPI41_DMA
> > bool 'TI CPPI 4.1 (AM335x)'
> > - depends on ARCH_OMAP
> > + depends on ARCH_OMAP && DMADEVICES
> > select TI_CPPI41
>
> I am not sure what the generic policy is, but instead of hiding
> USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable
> DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience
> perspective.

General policy is that you should not 'select' a symbol that is
also user-visible, as that tends to cause dependency loops and
other problems when something is enabled without the user being
aware of that.

Ideally we should remove the 'select TI_CPPI41' here as well, but
what we could do instead is to make that a silent symbol and remove
the prompt so it always gets enabled implicitly when USB_TI_CPPI41_DMA
and DMADEVICES are both enabled.

Arnd

2015-11-18 20:39:11

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support


Hi,

Bin Liu <[email protected]> writes:
>> Bin Liu <[email protected]> writes:
>>> On Wed, Nov 18, 2015 at 10:18 AM, Arnd Bergmann <[email protected]> wrote:
>>>> The CPPI-4.1 driver selects TI_CPPI41, which is a dmaengine
>>>> driver and that may not be available when CONFIG_DMADEVICES
>>>> is not set:
>>>>
>>>> warning: (USB_TI_CPPI41_DMA) selects TI_CPPI41 which has unmet direct dependencies (DMADEVICES && ARCH_OMAP)
>>>>
>>>> This adds an extra dependency to avoid generating warnings in randconfig
>>>> builds. Ideally we'd remove the 'select' statement, but that has the
>>>> potential to break defconfig files.
>>>>
>>>> Signed-off-by: Arnd Bergmann <[email protected]>
>>>> Fixes: 411dd19c682d ("usb: musb: Kconfig: Select the DMA driver if DMA mode of MUSB is enabled")
>>>>
>>>> diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
>>>> index 1f2037bbeb0d..45c83baf675d 100644
>>>> --- a/drivers/usb/musb/Kconfig
>>>> +++ b/drivers/usb/musb/Kconfig
>>>> @@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA
>>>>
>>>> config USB_TI_CPPI41_DMA
>>>> bool 'TI CPPI 4.1 (AM335x)'
>>>> - depends on ARCH_OMAP
>>>> + depends on ARCH_OMAP && DMADEVICES
>>>> select TI_CPPI41
>>>
>>> I am not sure what the generic policy is, but instead of hiding
>>> USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable
>>> DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience
>>> perspective.
>>
>> that would mean "select DMADEVICES" and that's frowned upon.
>
> Currently 'select DMADEVICES' is not in there. Will adding it fix the
> dependency warning in randconfig? Sorry for the question, but I don't
> know enough about Kconfig to get the answer.

it certainly would, but we don't like to add "select XYZ" to Kconfig
because a select bypasses the dependency tree. Let me explain:

config A
tristate "A"
depends on B

config B
tristate "B"


config C
tristate "C"
select A


C can select A without B being enabled.

--
balbi


Attachments:
signature.asc (818.00 B)

2015-11-18 20:39:14

by Bin Liu

[permalink] [raw]
Subject: Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support

Hi,

On Wed, Nov 18, 2015 at 2:27 PM, Arnd Bergmann <[email protected]> wrote:
> On Wednesday 18 November 2015 12:29:27 Bin Liu wrote:
>> > diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
>> > index 1f2037bbeb0d..45c83baf675d 100644
>> > --- a/drivers/usb/musb/Kconfig
>> > +++ b/drivers/usb/musb/Kconfig
>> > @@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA
>> >
>> > config USB_TI_CPPI41_DMA
>> > bool 'TI CPPI 4.1 (AM335x)'
>> > - depends on ARCH_OMAP
>> > + depends on ARCH_OMAP && DMADEVICES
>> > select TI_CPPI41
>>
>> I am not sure what the generic policy is, but instead of hiding
>> USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable
>> DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience
>> perspective.
>
> General policy is that you should not 'select' a symbol that is
> also user-visible, as that tends to cause dependency loops and
> other problems when something is enabled without the user being
> aware of that.

Understood. Thanks. I am okay with this patch.

>
> Ideally we should remove the 'select TI_CPPI41' here as well, but
> what we could do instead is to make that a silent symbol and remove
> the prompt so it always gets enabled implicitly when USB_TI_CPPI41_DMA
> and DMADEVICES are both enabled.

But what if DMADEVICES was disabled and USB_TI_CPPI41_DMA was enabled?
I would think I had CPPI fully enabled for MUSB, but it didn't because
TI_CPPI41 was disabled.

I would think this patch is the test option so far, we might have to
document somewhere that to dmaengine has to be enabled to use MUSB
CPPI, but I am not sure where the best place is to document...

Regards,
-Bin.

>
> Arnd

2015-11-18 20:42:09

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support


Hi,

Arnd Bergmann <[email protected]> writes:
> On Wednesday 18 November 2015 12:29:27 Bin Liu wrote:
>> > diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
>> > index 1f2037bbeb0d..45c83baf675d 100644
>> > --- a/drivers/usb/musb/Kconfig
>> > +++ b/drivers/usb/musb/Kconfig
>> > @@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA
>> >
>> > config USB_TI_CPPI41_DMA
>> > bool 'TI CPPI 4.1 (AM335x)'
>> > - depends on ARCH_OMAP
>> > + depends on ARCH_OMAP && DMADEVICES
>> > select TI_CPPI41
>>
>> I am not sure what the generic policy is, but instead of hiding
>> USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable
>> DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience
>> perspective.
>
> General policy is that you should not 'select' a symbol that is
> also user-visible, as that tends to cause dependency loops and
> other problems when something is enabled without the user being
> aware of that.
>
> Ideally we should remove the 'select TI_CPPI41' here as well, but
> what we could do instead is to make that a silent symbol and remove
> the prompt so it always gets enabled implicitly when USB_TI_CPPI41_DMA
> and DMADEVICES are both enabled.

that should be perfect now that Tony L fixed this up so we can enable
all MUSB DMA Engines in a single zImage.

--
balbi


Attachments:
signature.asc (818.00 B)

2015-11-18 20:45:34

by Bin Liu

[permalink] [raw]
Subject: Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support

On Wed, Nov 18, 2015 at 2:38 PM, Felipe Balbi <[email protected]> wrote:
>
> Hi,
>
> Bin Liu <[email protected]> writes:
>>> Bin Liu <[email protected]> writes:
>>>> On Wed, Nov 18, 2015 at 10:18 AM, Arnd Bergmann <[email protected]> wrote:
>>>>> The CPPI-4.1 driver selects TI_CPPI41, which is a dmaengine
>>>>> driver and that may not be available when CONFIG_DMADEVICES
>>>>> is not set:
>>>>>
>>>>> warning: (USB_TI_CPPI41_DMA) selects TI_CPPI41 which has unmet direct dependencies (DMADEVICES && ARCH_OMAP)
>>>>>
>>>>> This adds an extra dependency to avoid generating warnings in randconfig
>>>>> builds. Ideally we'd remove the 'select' statement, but that has the
>>>>> potential to break defconfig files.
>>>>>
>>>>> Signed-off-by: Arnd Bergmann <[email protected]>
>>>>> Fixes: 411dd19c682d ("usb: musb: Kconfig: Select the DMA driver if DMA mode of MUSB is enabled")
>>>>>
>>>>> diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
>>>>> index 1f2037bbeb0d..45c83baf675d 100644
>>>>> --- a/drivers/usb/musb/Kconfig
>>>>> +++ b/drivers/usb/musb/Kconfig
>>>>> @@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA
>>>>>
>>>>> config USB_TI_CPPI41_DMA
>>>>> bool 'TI CPPI 4.1 (AM335x)'
>>>>> - depends on ARCH_OMAP
>>>>> + depends on ARCH_OMAP && DMADEVICES
>>>>> select TI_CPPI41
>>>>
>>>> I am not sure what the generic policy is, but instead of hiding
>>>> USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable
>>>> DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience
>>>> perspective.
>>>
>>> that would mean "select DMADEVICES" and that's frowned upon.
>>
>> Currently 'select DMADEVICES' is not in there. Will adding it fix the
>> dependency warning in randconfig? Sorry for the question, but I don't
>> know enough about Kconfig to get the answer.
>
> it certainly would, but we don't like to add "select XYZ" to Kconfig
> because a select bypasses the dependency tree. Let me explain:
>
> config A
> tristate "A"
> depends on B
>
> config B
> tristate "B"
>
>
> config C
> tristate "C"
> select A
>
>
> C can select A without B being enabled.

Thanks for the explanation, very clear.

>
> --
> balbi

2015-11-18 21:07:08

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support

On Wednesday 18 November 2015 14:39:10 Bin Liu wrote:
> > Ideally we should remove the 'select TI_CPPI41' here as well, but
> > what we could do instead is to make that a silent symbol and remove
> > the prompt so it always gets enabled implicitly when USB_TI_CPPI41_DMA
> > and DMADEVICES are both enabled.
>
> But what if DMADEVICES was disabled and USB_TI_CPPI41_DMA was enabled?
> I would think I had CPPI fully enabled for MUSB, but it didn't because
> TI_CPPI41 was disabled.

That would cause a runtime failure, just like any other configuration
that does not enable all the hardware you want to use.

> I would think this patch is the test option so far, we might have to
> document somewhere that to dmaengine has to be enabled to use MUSB
> CPPI, but I am not sure where the best place is to document...

There are hundreds of device drivers that use dmaengines as a
backend, we don't normally document this, just like we don't
document the fact that you need to enable the right gpio, irqchip,
timer, clock etc drivers for your platform.

Arnd