Re: UAC2 gadget not recognized on Windows 10

[Jassi Brar]

On Wed, Jun 20, 2018 at 9:16 PM, jonsm...@gmail.com  wrote:
> On Wed, Jun 20, 2018 at 11:19 AM Jassi Brar  wrote:
>>
>> On Wed, Jun 20, 2018 at 8:32 PM, jonsm...@gmail.com  
>> wrote:
>> >
>> >
>> >
>> > On Wed, Jun 20, 2018 at 9:36 AM Jassi Brar  
>> > wrote:
>> >>
>> >> On Tue, Feb 13, 2018 at 3:02 PM, Robert Bielik  
>> >> wrote:
>> >> >> Enabling SOF interrupts will be a big pain :-) Well, enabling the
>> >> >> interrupt itself is a no-brainer, but it'll cause terrible CPU 
>> >> >> overload.
>> >> >
>> >> > Oh, I see. Hmm... would it be possible to allow upper levels to config 
>> >> > this dynamically ? I.e. for the ALSA subsystem there is no need for the 
>> >> > SOF timestamps, whereas for my proposal they would be needed.
>> >> >
>> >> >  And what kind of CPU overhead are we talking about ? The IRQs 
>> >> > shouldn't come more often than every 125 us, and all that is needed is 
>> >> > to take a timestamp value But I'm probably overlooking a lot of stuff...
>> >> >
>> >> I believe we could control data rate precisely enough for the
>> >> feeedback ep to be needed only for compatibility with Windows (Linux
>> >> is already tested to work fine, MacOS is reported to have worked when
>> >> the code was upstreamed.).
>> >
>> >
>> > Right now Windows 10 (all I have checked) refuses to even connect to a 
>> > Linux based UAC2 device. It appears to be complaining that no endpoint 
>> > supporting USB_ENDPOINT_USAGE_FEEDBACK mode is implement. My five minutes 
>> > of poking in the spec implies that implementing this is required for an 
>> > endpoint doing USB_ENDPOINT_SYNC_ASYNC.  Maybe this is something that can 
>> > be faked to the point of allowing Windows to connect to the UAC2 gadget 
>> > driver?
>> >
>> Thats what I meant.
>>
>> > The Windows error you get is:
>> > "The driver could not find a feedback endpoint for an asynchronous data 
>> > OUT endpoint on device \Device\."
>> >
>> For OUT, there might be some hoops to jump. Honestly it's been years
>> since I read the spec and got the code to work. I find it tempting to
>> look into Windows 10 as well, but I can't commit to a timeline.
>
> For my application there is no recording HW on the gadget. Maybe a
> quick way to fix this would be to make a flag to say that the gadget
> is output only? And then change the descriptors around?
>
There could be hacks to fool Windows but the right approach is to find
some real uac2 "USB-IN only" device and share the lsusb output. Then
we could kill some descriptor to pretend like that device.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: UAC2 gadget not recognized on Windows 10

[Jassi Brar]

On Wed, Jun 20, 2018 at 8:32 PM, jonsm...@gmail.com  wrote:
>
>
>
> On Wed, Jun 20, 2018 at 9:36 AM Jassi Brar  wrote:
>>
>> On Tue, Feb 13, 2018 at 3:02 PM, Robert Bielik  
>> wrote:
>> >> Enabling SOF interrupts will be a big pain :-) Well, enabling the
>> >> interrupt itself is a no-brainer, but it'll cause terrible CPU overload.
>> >
>> > Oh, I see. Hmm... would it be possible to allow upper levels to config 
>> > this dynamically ? I.e. for the ALSA subsystem there is no need for the 
>> > SOF timestamps, whereas for my proposal they would be needed.
>> >
>> >  And what kind of CPU overhead are we talking about ? The IRQs shouldn't 
>> > come more often than every 125 us, and all that is needed is to take a 
>> > timestamp value But I'm probably overlooking a lot of stuff...
>> >
>> I believe we could control data rate precisely enough for the
>> feeedback ep to be needed only for compatibility with Windows (Linux
>> is already tested to work fine, MacOS is reported to have worked when
>> the code was upstreamed.).
>
>
> Right now Windows 10 (all I have checked) refuses to even connect to a Linux 
> based UAC2 device. It appears to be complaining that no endpoint supporting 
> USB_ENDPOINT_USAGE_FEEDBACK mode is implement. My five minutes of poking in 
> the spec implies that implementing this is required for an endpoint doing 
> USB_ENDPOINT_SYNC_ASYNC.  Maybe this is something that can be faked to the 
> point of allowing Windows to connect to the UAC2 gadget driver?
>
Thats what I meant.

> The Windows error you get is:
> "The driver could not find a feedback endpoint for an asynchronous data OUT 
> endpoint on device \Device\."
>
For OUT, there might be some hoops to jump. Honestly it's been years
since I read the spec and got the code to work. I find it tempting to
look into Windows 10 as well, but I can't commit to a timeline.

Cheers!
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: UAC2 gadget not recognized on Windows 10

[Jassi Brar]

On Tue, Feb 13, 2018 at 3:02 PM, Robert Bielik  wrote:
>> Enabling SOF interrupts will be a big pain :-) Well, enabling the
>> interrupt itself is a no-brainer, but it'll cause terrible CPU overload.
>
> Oh, I see. Hmm... would it be possible to allow upper levels to config this 
> dynamically ? I.e. for the ALSA subsystem there is no need for the SOF 
> timestamps, whereas for my proposal they would be needed.
>
>  And what kind of CPU overhead are we talking about ? The IRQs shouldn't come 
> more often than every 125 us, and all that is needed is to take a timestamp 
> value  But I'm probably overlooking a lot of stuff...
>
I believe we could control data rate precisely enough for the
feeedback ep to be needed only for compatibility with Windows (Linux
is already tested to work fine, MacOS is reported to have worked when
the code was upstreamed.).

There was an implementation suggested by Alan Stern which would
maintain accurate rate over a reasonable period of time. But that was
rejected ... https://patches.linaro.org/patch/36270/
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 0/4] USB Audio Gadget refactoring

[Jassi Brar]

ac1_legacy.h|   82 ++
>  drivers/usb/gadget/legacy/Kconfig  |   15 +-
>  drivers/usb/gadget/legacy/audio.c  |   55 +-
>  15 files changed, 2494 insertions(+), 1348 deletions(-)
>  create mode 100644 Documentation/ABI/testing/configfs-usb-gadget-uac1_legacy
>  create mode 100644 drivers/usb/gadget/function/f_uac1_legacy.c
>  create mode 100644 drivers/usb/gadget/function/u_audio.c
>  create mode 100644 drivers/usb/gadget/function/u_audio.h
>  rename drivers/usb/gadget/function/{u_uac1.c => u_uac1_legacy.c} (98%)
>  create mode 100644 drivers/usb/gadget/function/u_uac1_legacy.h
>
Acked-by: Jassi Brar <jassisinghb...@gmail.com>

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/3] usb: gadget: f_uac2: split out audio core

[Jassi Brar]

On Tue, May 30, 2017 at 5:13 AM, Ruslan Bilovol
<ruslan.bilo...@gmail.com> wrote:
> On Mon, May 22, 2017 at 6:58 PM, Jassi Brar <jassisinghb...@gmail.com> wrote:
>> On Thu, May 18, 2017 at 4:07 AM, Ruslan Bilovol
>> <ruslan.bilo...@gmail.com> wrote:
>>> Abstract the peripheral side ALSA sound card code from
>>> the f_uac2 function into a component that can be called
>>> by various functions, so the various flavors can be split
>>> apart and selectively reused.
>>>
>>> Visible changes:
>>>  - add uac_params structure to pass audio paramteres for
>>>g_audio_setup
>>>  - make ALSA sound card's name configurable
>>>  - add [in/out]_ep_maxpsize
>>>  - allocate snd_uac_chip structure during g_audio_setup
>>>  - add u_audio_[start/stop]_[capture/playback] functions
>>>
>>> Signed-off-by: Ruslan Bilovol <ruslan.bilo...@gmail.com>
>>> ---
>>>  drivers/usb/gadget/Kconfig|   4 +
>>>  drivers/usb/gadget/function/Makefile  |   1 +
>>>  drivers/usb/gadget/function/f_uac2.c  | 721 
>>> --
>>>  drivers/usb/gadget/function/u_audio.c | 661 +++
>>>  drivers/usb/gadget/function/u_audio.h |  95 +
>>>  drivers/usb/gadget/legacy/Kconfig |   1 +
>>>  6 files changed, 846 insertions(+), 637 deletions(-)
>>>  create mode 100644 drivers/usb/gadget/function/u_audio.c
>>>  create mode 100644 drivers/usb/gadget/function/u_audio.h
>>>
>>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>>> index c164d6b..2ba0ace 100644
>>> --- a/drivers/usb/gadget/Kconfig
>>> +++ b/drivers/usb/gadget/Kconfig
>>> @@ -158,6 +158,9 @@ config USB_U_SERIAL
>>>  config USB_U_ETHER
>>> tristate
>>>
>>> +config USB_U_AUDIO
>>> +   tristate
>>> +
>>>  config USB_F_SERIAL
>>> tristate
>>>
>>> @@ -381,6 +384,7 @@ config USB_CONFIGFS_F_UAC2
>>> depends on SND
>>> select USB_LIBCOMPOSITE
>>> select SND_PCM
>>> +   select USB_U_AUDIO
>>> select USB_F_UAC2
>>> help
>>>   This Audio function is compatible with USB Audio Class
>>> diff --git a/drivers/usb/gadget/function/Makefile 
>>> b/drivers/usb/gadget/function/Makefile
>>> index cb8c225..b29f2ae 100644
>>> --- a/drivers/usb/gadget/function/Makefile
>>> +++ b/drivers/usb/gadget/function/Makefile
>>> @@ -32,6 +32,7 @@ usb_f_mass_storage-y  := f_mass_storage.o 
>>> storage_common.o
>>>  obj-$(CONFIG_USB_F_MASS_STORAGE)+= usb_f_mass_storage.o
>>>  usb_f_fs-y := f_fs.o
>>>  obj-$(CONFIG_USB_F_FS) += usb_f_fs.o
>>> +obj-$(CONFIG_USB_U_AUDIO)  += u_audio.o
>>>  usb_f_uac1-y   := f_uac1.o u_uac1.o
>>>  obj-$(CONFIG_USB_F_UAC1)   += usb_f_uac1.o
>>>  usb_f_uac2-y   := f_uac2.o
>>> diff --git a/drivers/usb/gadget/function/f_uac2.c 
>>> b/drivers/usb/gadget/function/f_uac2.c
>>> index d4565b5..059a14a 100644
>>> --- a/drivers/usb/gadget/function/f_uac2.c
>>> +++ b/drivers/usb/gadget/function/f_uac2.c
>>> @@ -15,10 +15,7 @@
>>>  #include 
>>>  #include 
>>>
>>> -#include 
>>> -#include 
>>> -#include 
>>> -
>>> +#include "u_audio.h"
>>>  #include "u_uac2.h"
>>>
>>>  /*
>>> @@ -50,455 +47,23 @@
>>>  #define UNFLW_CTRL 8
>>>  #define OVFLW_CTRL 10
>>>
>>> -struct uac2_req {
>>> -   struct uac2_rtd_params *pp; /* parent param */
>>> -   struct usb_request *req;
>>> -};
>>> -
>>> -struct uac2_rtd_params {
>>> -   struct snd_uac2_chip *uac2; /* parent chip */
>>> -   bool ep_enabled; /* if the ep is enabled */
>>> -   /* Size of the ring buffer */
>>> -   size_t dma_bytes;
>>> -   unsigned char *dma_area;
>>> -
>>> -   struct snd_pcm_substream *ss;
>>> -
>>> -   /* Ring buffer */
>>> -   ssize_t hw_ptr;
>>> -
>>> -   void *rbuf;
>>> -
>>> -   size_t period_size;
>>> -
>>> -   unsigned max_psize;
>>> -   struct uac2_req *ureq;
>>> -
>>> -   spinlock_t lock;
>>> -};
&g

Re: dwc2 audio gadget with high data rates

[Jassi Brar]

On Wed, May 24, 2017 at 12:22 AM, Francesco Lavra
 wrote:
> On Sun, May 21, 2017 at 4:42 PM, Francesco Lavra
>  wrote:
>> Hi,
>> I'm using the dwc2 OTG controller as a USB audio gadget (g_audio driver),
>> and I'm having trouble with making it work at high data rates, e.g. 192 kHz
>> sampling rate or 6 channels.
>> When I load the g_audio driver with the above parameters (p_srate=192000
>> p_chmask=0x3F), I get this error message "dwc2 ff58.usb:
>> dwc2_hsotg_ep_enable: No suitable fifo found", followed by a full stack
>> trace; as a result, the gadget driver is not operational. If I try to first
>> load the driver with its default parameters, then unload it and reload it
>> with the above parameters, then I don't see any error message, but the
>> driver misbehaves: when I send audio from the USB peripheral to a host, the
>> audio data is sent at the rate corresponding to the default parameters. For
>> example, a speaker-test run which is supposed to output audio for 3 seconds
>> to each channel takes a longer time, to be precise 36 seconds per channel,
>> i.e. 12 times the amount it should take. 12 is exactly the ratio between the
>> audio data rate with default parameters (48 kHz, 2 channels) and the rate
>> with the (p_srate=192000 p_chmask=0x3F) settings.
>> Using other combinations of parameter values, I get similar results, e.g.
>> the audio is 3 times slower if I use 6 channels with the default 48 kHz
>> sampling rate.
>>
>> So it looks like something gets misconfigured when the gadget driver is
>> loaded and unloaded multiple times with different parameters, but my main
>> question is about the "No suitable fifo found" error that I get when trying
>> to load the g_audio driver: does it stem for an inherent hardware limitation
>> of the dwc2 controller? is it specific to the SoC I'm using (Rockchip
>> rk3288)? or is it possible via code changes to the dwc2 driver to overcome
>> these issues?
>>
>> For the record, I'm using the latest kernel stable release (4.11.2).
>
> Since the g_audio module is now a legacy driver and there is a better
> way (configfs) for setting up a USB gadget as a UAC2 device, I tried
> following the configfs route, but unfortunately the end result is the
> same: above a certain audio data rate, when I bind the gadget to the
> UDC controller I get the "No suitable fifo found" error followed by a
> stack trace.
> I also tried to configure first the gadget with supported parameters
> (2 channels, 4 bytes per sample, 44.1 kHz), then disable the gadget
> and clean up its configuration, and then re-configure it with
> different parameter values (e.g. 96 kHz instead of 44.1): this time it
> doesn't give any error, but it is somehow using (at least in part) the
> older settings, because the audio flows at the same speed as it would
> flow with the older settings.
> So the behavior is the same as with the legacy g_audio driver. Any
> clue on what is going on? And is this a limitation of the UDC
> controller, or can the issue be solved with driver changes?
>
Some UDC controllers use fixed length buffer divided among all EPs so
no one EP may have large enough buffer to support high bandwidth
required, while others may have unnecessary space. So may be look into
the cause of message "No suitable fifo found"
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/3] usb: gadget: f_uac2: split out audio core

[Jassi Brar]

On Thu, May 18, 2017 at 4:07 AM, Ruslan Bilovol
 wrote:
> Abstract the peripheral side ALSA sound card code from
> the f_uac2 function into a component that can be called
> by various functions, so the various flavors can be split
> apart and selectively reused.
>
> Visible changes:
>  - add uac_params structure to pass audio paramteres for
>g_audio_setup
>  - make ALSA sound card's name configurable
>  - add [in/out]_ep_maxpsize
>  - allocate snd_uac_chip structure during g_audio_setup
>  - add u_audio_[start/stop]_[capture/playback] functions
>
> Signed-off-by: Ruslan Bilovol 
> ---
>  drivers/usb/gadget/Kconfig|   4 +
>  drivers/usb/gadget/function/Makefile  |   1 +
>  drivers/usb/gadget/function/f_uac2.c  | 721 
> --
>  drivers/usb/gadget/function/u_audio.c | 661 +++
>  drivers/usb/gadget/function/u_audio.h |  95 +
>  drivers/usb/gadget/legacy/Kconfig |   1 +
>  6 files changed, 846 insertions(+), 637 deletions(-)
>  create mode 100644 drivers/usb/gadget/function/u_audio.c
>  create mode 100644 drivers/usb/gadget/function/u_audio.h
>
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index c164d6b..2ba0ace 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -158,6 +158,9 @@ config USB_U_SERIAL
>  config USB_U_ETHER
> tristate
>
> +config USB_U_AUDIO
> +   tristate
> +
>  config USB_F_SERIAL
> tristate
>
> @@ -381,6 +384,7 @@ config USB_CONFIGFS_F_UAC2
> depends on SND
> select USB_LIBCOMPOSITE
> select SND_PCM
> +   select USB_U_AUDIO
> select USB_F_UAC2
> help
>   This Audio function is compatible with USB Audio Class
> diff --git a/drivers/usb/gadget/function/Makefile 
> b/drivers/usb/gadget/function/Makefile
> index cb8c225..b29f2ae 100644
> --- a/drivers/usb/gadget/function/Makefile
> +++ b/drivers/usb/gadget/function/Makefile
> @@ -32,6 +32,7 @@ usb_f_mass_storage-y  := f_mass_storage.o 
> storage_common.o
>  obj-$(CONFIG_USB_F_MASS_STORAGE)+= usb_f_mass_storage.o
>  usb_f_fs-y := f_fs.o
>  obj-$(CONFIG_USB_F_FS) += usb_f_fs.o
> +obj-$(CONFIG_USB_U_AUDIO)  += u_audio.o
>  usb_f_uac1-y   := f_uac1.o u_uac1.o
>  obj-$(CONFIG_USB_F_UAC1)   += usb_f_uac1.o
>  usb_f_uac2-y   := f_uac2.o
> diff --git a/drivers/usb/gadget/function/f_uac2.c 
> b/drivers/usb/gadget/function/f_uac2.c
> index d4565b5..059a14a 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -15,10 +15,7 @@
>  #include 
>  #include 
>
> -#include 
> -#include 
> -#include 
> -
> +#include "u_audio.h"
>  #include "u_uac2.h"
>
>  /*
> @@ -50,455 +47,23 @@
>  #define UNFLW_CTRL 8
>  #define OVFLW_CTRL 10
>
> -struct uac2_req {
> -   struct uac2_rtd_params *pp; /* parent param */
> -   struct usb_request *req;
> -};
> -
> -struct uac2_rtd_params {
> -   struct snd_uac2_chip *uac2; /* parent chip */
> -   bool ep_enabled; /* if the ep is enabled */
> -   /* Size of the ring buffer */
> -   size_t dma_bytes;
> -   unsigned char *dma_area;
> -
> -   struct snd_pcm_substream *ss;
> -
> -   /* Ring buffer */
> -   ssize_t hw_ptr;
> -
> -   void *rbuf;
> -
> -   size_t period_size;
> -
> -   unsigned max_psize;
> -   struct uac2_req *ureq;
> -
> -   spinlock_t lock;
> -};
> -
> -struct snd_uac2_chip {
> -   struct uac2_rtd_params p_prm;
> -   struct uac2_rtd_params c_prm;
> -
> -   struct snd_card *card;
> -   struct snd_pcm *pcm;
> -
> -   /* timekeeping for the playback endpoint */
> -   unsigned int p_interval;
> -   unsigned int p_residue;
> -
> -   /* pre-calculated values for playback iso completion */
> -   unsigned int p_pktsize;
> -   unsigned int p_pktsize_residue;
> -   unsigned int p_framesize;
> +struct f_uac2 {
> +   struct g_audio g_audio;
> +   u8 ac_intf, as_in_intf, as_out_intf;
> +   u8 ac_alt, as_in_alt, as_out_alt;   /* needed for get_alt() */
>  };
>
> -#define BUFF_SIZE_MAX  (PAGE_SIZE * 16)
> -#define PRD_SIZE_MAX   PAGE_SIZE
> -#define MIN_PERIODS4
> -
> -static struct snd_pcm_hardware uac2_pcm_hardware = {
> -   .info = SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER
> -| SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID
> -| SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
> -   .rates = SNDRV_PCM_RATE_CONTINUOUS,
> -   .periods_max = BUFF_SIZE_MAX / PRD_SIZE_MAX,
> -   .buffer_bytes_max = BUFF_SIZE_MAX,
> -   .period_bytes_max = PRD_SIZE_MAX,
> -   .periods_min = MIN_PERIODS,
> -};
> -
> -struct audio_dev {
> -   u8 ac_intf, ac_alt;
> -   u8 as_out_intf, as_out_alt;
> -   u8 as_in_intf, as_in_alt;
> -
> -   struct usb_ep *in_ep, 

[PATCH] usb: gadget: uvc: fix UVC_ATTR macro for UVCG_OPTS_ATTR

[Jassi Brar]

From: Jassi Brar <jaswinder.si...@linaro.org>

Fix a typo bug that prevents creation of
streaming_{interval,maxpacket,maxburst} nodes for invalid 'aname' node.

Fixes: 76e0da34 ("usb-gadget/uvc: use per-attribute show and store methods")
Signed-off-by: Jassi Brar <jaswinder.si...@linaro.org>
Cc: sta...@vger.kernel.org
Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
---
 drivers/usb/gadget/function/uvc_configfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index ecb5614..158849e 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -2245,7 +2245,7 @@ end:  
\
return ret; \
 }  \
\
-UVC_ATTR(f_uvc_opts_, cname, aname)
+UVC_ATTR(f_uvc_opts_, cname, cname)
 
 #define identity_conv(x) (x)
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: gadget: uvc: fix UVC_ATTR macro for UVCG_OPTS_ATTR

[Jassi Brar]

From: Jassi Brar <jaswinder.si...@linaro.org>

Typo in commit 76e0da34c7cec5a7d introduced a bug that prevents
creation of streaming_{interval,maxpacket,maxburst} nodes for
invalid 'aname' node.

Signed-off-by: Jassi Brar <jaswinder.si...@linaro.org>
---
 drivers/usb/gadget/function/uvc_configfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index ecb5614..158849e 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -2245,7 +2245,7 @@ end:  
\
return ret; \
 }  \
\
-UVC_ATTR(f_uvc_opts_, cname, aname)
+UVC_ATTR(f_uvc_opts_, cname, cname)
 
 #define identity_conv(x) (x)
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 0/5] USB Audio Gadget refactoring

[Jassi Brar]

On Tue, Jul 26, 2016 at 7:01 AM, Ruslan Bilovol
 wrote:
> On Fri, Jul 15, 2016 at 10:43 AM, Clemens Ladisch  wrote:
 On Tue, May 24, 2016 at 2:50 AM, Ruslan Bilovol
  wrote:
> it may break current usecase for some people
>>
>> And what are the benefits that justify breaking the kernel API?
>
>
> Main limitation with current f_uac1 design is - it can be used only on systems
> with real ALSA card present and can have only exact number of
> channels / sampling rate as sink card has.
> Yet it is not flexible - can't do audio processing between f_uac1 and the 
> card.
> Also if someone wants to bind f_uac1 it to another sound card he has to
> unload g_audio or reconfigure it through configfs - that means USB
> reenumeration on host device.
>
> If you have a "virtual sound card", audio processing is done in userspace
> and is more flexible. You even don't need to have a real sound card and
> can use some userspace application for playing/capturing audio samples.
> Moreover, existing f_uac2 (that is USB Audio Class 2.0 function
> implementation) already uses approach of "virtual sound card"
>
While I agree the virtual sound card approach is the right way, I am
not sure if we should break the userspace api that the existing UAC1
driver exposes. Maybe we should add another virtual-sound-card
exposing UAC1 driver ... and hopefully very similar to (or just port
of) the f_audio_source.c from android.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Jassi Brar]

On Thu, Oct 15, 2015 at 7:59 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Thu, 15 Oct 2015, Jassi Brar wrote:
>
>> On Wed, Oct 14, 2015 at 11:18 PM, Alan Stern <st...@rowland.harvard.edu> 
>> wrote:
>> > On Wed, 14 Oct 2015, Jassi Brar wrote:
>> >
>> >> BTW, should the gadget stack ever queue a Non-ZLP as reply to some
>> >> setup request that has USB_DIR_IN not set?
>> >
>> > Yes.  If USB_DIR_IN is not set then the control transfer is OUT, so the
>> > gadget needs to queue a request to receive some data from the host.
>> > That request will obviously need to be a non-ZLP.
>> >
>> By 'reply' I meant after reading out and parsing the
>> setup(control-out) request. I am sure we need to send a ZLP.
>
> You're wrong.  Consider what happens when the host wants to send 7
> bytes of data to the gadget using a control-OUT transfer:
>
> The gadget receives a SETUP packet.  The USB_DIR_IN bit is
> clear because this is an OUT transfer, and wLength is set to 7.
> Which is what you got, right?
>
> Next, the host will send the 7-byte data packet.  The gadget
> has to prepare to receive it, and it does so by submitting a
> 7-byte OUT request to ep0.  This happens within the setup
> handler.
>
> The data packet is sent and the gadget receives it.  The status
> stage for this transfer consists of a 0-length IN transaction,
>
I have been referring to this "0-length IN transaction" when I said
"need to send a zlp".

> which the UDC driver automatically queues as soon as the
> completion routine for the data packet returns.  The gadget
> driver isn't involved in the status stage (unfortunately).
>
OK, so that is a known 'feature', not a bug in gadget drivers as I
have been calling it. I was trying to make the f_acm.c send that
"0-length IN transaction" by making 'value=0'

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Jassi Brar]

On Wed, Oct 14, 2015 at 8:42 PM, Felipe Balbi <ba...@ti.com> wrote:
>
> Hi,
>
> jaswinder.si...@linaro.org writes:
>> From: Jassi Brar <jaswinder.si...@linaro.org>
>>
>> We must return 0 for any OUT setup request, otherwise
>> protocol error may occur.
>
> have you seen any such errors ?
>
Yes. While testing my new udc driver.


> Care to describe what problems you have seen and which UDC you were using,
> what's the exact scenario. How have you tested this ?
>
The udc I am writing the driver for is not yet public. To test my
driver at lowest level possible, I use 'usbmon' to capture and analyze
traffic since I don't have any hardware probe.

I see the following on gadget side
---
configfs-gadget gadget: non-core control req21.20 v i0001 l7
configfs-gadget gadget: acm ttyGS0 req21.20 v i0001 l7
configfs-gadget gadget: acm ttyGS0 completion, err -71
---

and the following on host side usbmon capture
---
88041ed01f00 540296433 S Co:3:023:0 s 21 20  0001 0007 7 =
8025 08
88041ed01f00 540296790 C Co:3:023:0 -71 0
---

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Jassi Brar]

On Wed, Oct 14, 2015 at 11:18 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Wed, 14 Oct 2015, Jassi Brar wrote:
>
>> BTW, should the gadget stack ever queue a Non-ZLP as reply to some
>> setup request that has USB_DIR_IN not set?
>
> Yes.  If USB_DIR_IN is not set then the control transfer is OUT, so the
> gadget needs to queue a request to receive some data from the host.
> That request will obviously need to be a non-ZLP.
>
By 'reply' I meant after reading out and parsing the
setup(control-out) request. I am sure we need to send a ZLP.

> Could this cause the problem you're seeing?  The host tries to send
> more data than the gadget is ready to receive?  (Although then the
> error code on the gadget side should be -75, not -71.)
>
Thanks, but as you said my problem is not that (I get protocol error
-71).  My problem is my udc driver actually tries to send a 7 byte
response to a control-out command.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Jassi Brar]

On Wed, Oct 14, 2015 at 9:18 PM, Felipe Balbi <ba...@ti.com> wrote:
>
> Hi,
>
> Jassi Brar <jassisinghb...@gmail.com> writes:
>> On Wed, Oct 14, 2015 at 8:42 PM, Felipe Balbi <ba...@ti.com> wrote:
>>>
>>> Hi,
>>>
>>> jaswinder.si...@linaro.org writes:
>>>> From: Jassi Brar <jaswinder.si...@linaro.org>
>>>>
>>>> We must return 0 for any OUT setup request, otherwise
>>>> protocol error may occur.
>>>
>>> have you seen any such errors ?
>>>
>> Yes. While testing my new udc driver.
>>
>>
>>> Care to describe what problems you have seen and which UDC you were using,
>>> what's the exact scenario. How have you tested this ?
>>>
>> The udc I am writing the driver for is not yet public. To test my
>> driver at lowest level possible, I use 'usbmon' to capture and analyze
>> traffic since I don't have any hardware probe.
>>
>> I see the following on gadget side
>> ---
>> configfs-gadget gadget: non-core control req21.20 v i0001 l7
>> configfs-gadget gadget: acm ttyGS0 req21.20 v i0001 l7
>> configfs-gadget gadget: acm ttyGS0 completion, err -71
>> ---
>>
>> and the following on host side usbmon capture
>> ---
>> 88041ed01f00 540296433 S Co:3:023:0 s 21 20  0001 0007 7 =
>> 8025 08
>> 88041ed01f00 540296790 C Co:3:023:0 -71 0
>> ---
>
> and you did you even consider this could be a bug in your new UDC driver
> at all ? This works fine with other UDCs.
>
I was happy too until I decided to look closely at the annoying print
on gadget side. This is a non-fatal error and visible only when
debugging is enabled, so every udc seems 'fine'

> How far are you in developing this new UDC driver ?
>
Its done and tested for fsg+acm composite and each alone.

> Did you run USBCV at all ?
>
No USBCV not yet. I borrowed Windows machine to test FSG but forgot
USBCV since it's been years I wrote last udc driver. Will give it a
try tomorrow but I don't think it could emulate the sequence I hit
with f_acm.

> Are you sure you're implementing EP0 handling correctly ? What
> sort of tests have you done with this UDC ? Is it working with testusb
> against a known good host (EHCI and XHCI should be good for that) ?
>
Well as I said I have been looking at low level transfers and
everything is good.

BTW, should the gadget stack ever queue a Non-ZLP as reply to some
setup request that has USB_DIR_IN not set?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Jassi Brar]

On Wed, Oct 14, 2015 at 10:40 PM, Felipe Balbi <ba...@ti.com> wrote:
>
> Hi,
>
> Jassi Brar <jassisinghb...@gmail.com> writes:
>> On Wed, Oct 14, 2015 at 9:18 PM, Felipe Balbi <ba...@ti.com> wrote:
>>>
>>> Hi,
>>>
>>> Jassi Brar <jassisinghb...@gmail.com> writes:
>>>> On Wed, Oct 14, 2015 at 8:42 PM, Felipe Balbi <ba...@ti.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> jaswinder.si...@linaro.org writes:
>>>>>> From: Jassi Brar <jaswinder.si...@linaro.org>
>>>>>>
>>>>>> We must return 0 for any OUT setup request, otherwise
>>>>>> protocol error may occur.
>>>>>
>>>>> have you seen any such errors ?
>>>>>
>>>> Yes. While testing my new udc driver.
>>>>
>>>>
>>>>> Care to describe what problems you have seen and which UDC you were using,
>>>>n> what's the exact scenario. How have you tested this ?
>>>>>
>>>> The udc I am writing the driver for is not yet public. To test my
>>>> driver at lowest level possible, I use 'usbmon' to capture and analyze
>>>> traffic since I don't have any hardware probe.
>>>>
>>>> I see the following on gadget side
>>>> ---
>>>> configfs-gadget gadget: non-core control req21.20 v i0001 l7
>>>> configfs-gadget gadget: acm ttyGS0 req21.20 v i0001 l7
>>>> configfs-gadget gadget: acm ttyGS0 completion, err -71
>>>> ---
>>>>
>>>> and the following on host side usbmon capture
>>>> ---
>>>> 88041ed01f00 540296433 S Co:3:023:0 s 21 20  0001 0007 7 =
>>>> 8025 08
>>>> 88041ed01f00 540296790 C Co:3:023:0 -71 0
>>>> ---
>>>
>>> and you did you even consider this could be a bug in your new UDC driver
>>> at all ? This works fine with other UDCs.
>>>
>> I was happy too until I decided to look closely at the annoying print
>> on gadget side. This is a non-fatal error and visible only when
>> debugging is enabled, so every udc seems 'fine'
>
> I tried with my sniffer and saw no stalls, nothing. My setup request to
> set line coding to 9600 baud worked just fine.
>
I am sure your udc driver will (need to) track stages of a transfer.
If you put some print in usb_ep_ops.queue() you will notice the stack
queues 7byte transfer but the udc driver silently drops it and send a
zlp here.

  I can move the change into my driver as well, but the point is
gadget should never try to _send_ non-zlp as reply to control-OUT
command.


>>> How far are you in developing this new UDC driver ?
>>>
>> Its done and tested for fsg+acm composite and each alone.
>
> stress tests ? Meaning, did you leave it running for a couple of weeks
> at least ?
>
I know I can't stress test enough, but this bug is 100% hit and
staring in the eye at protocol level.

>> BTW, should the gadget stack ever queue a Non-ZLP as reply to some
>> setup request that has USB_DIR_IN not set?
>
> What phase of the setup are we talking about ?
>
I said "_reply_ to setup request" so I meant status phase.

UDC driver receives the SETUP command as '21 20 00 00 01 00 07 00',
passes it onto composite, which hands it over to acm and which
pretends we need to return a 7byte packet to host (value = w_length =
7).
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V6 00/12] Tegra xHCI support

[Jassi Brar]

On 25 November 2014 at 05:47, Andrew Bresticker abres...@chromium.org wrote:
 This series adds support for xHCI on NVIDIA Tegra SoCs.  This includes:
  - patches 1, 2, and 3: minor cleanups for mailbox framework and xHCI,
  - patches 4 and 5: adding a driver for the mailbox used to communicate
with the xHCI controller's firmware,
  - patches 6 and 7: extending the XUSB pad controller driver to support
the USB PHY types (UTMI, HSIC, and USB3),
  - patches 8 and 9: adding a xHCI host-controller driver, and
  - patches 10, 11, and 12: updating the relevant DT files.

 The mailbox driver (patch 5) has a compile-time dependency on patch 2 and
 a run-time dependency on patch 3.  Both the PHY (patch 7) and host (patch 9)
 drivers have compile-time dependencies on the mailbox driver.  The host
 driver also has a run-time dependency on patch 1.  Because of this, this
 entire series should probably go through the Tegra tree.

Why shouldn't I pick 2  3 at least?

-Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V5 05/12] mailbox: Add NVIDIA Tegra XUSB mailbox driver

[Jassi Brar]

On 18 November 2014 at 04:11, Andrew Bresticker abres...@chromium.org wrote:

 +
 +static int tegra_xusb_mbox_send_data(struct mbox_chan *chan, void *data)
 +{
 +   struct tegra_xusb_mbox *mbox = to_tegra_mbox(chan-mbox);
 +   struct tegra_xusb_mbox_msg *msg = data;
 +   unsigned long flags;
 +   u32 reg, owner;
 +
 +   dev_dbg(mbox-mbox.dev, TX message %#x:%#x\n, msg-cmd, msg-data);
 +
 +   /* ACK/NAK must be sent with the controller as the mailbox owner */
 +   if (msg-cmd == MBOX_CMD_ACK || msg-cmd == MBOX_CMD_NAK)
 +   owner = MBOX_OWNER_FW;
 +   else
 +   owner = MBOX_OWNER_SW;
 +
 +   spin_lock_irqsave(mbox-lock, flags);
 +
 +   /* Acquire mailbox */
 +   if (mbox_readl(mbox, XUSB_CFG_ARU_MBOX_OWNER) != MBOX_OWNER_NONE) {
 +   dev_err(mbox-mbox.dev, Mailbox not idle\n);
 +   goto busy;
 +   }
 +   mbox_writel(mbox, owner, XUSB_CFG_ARU_MBOX_OWNER);
 +   if (mbox_readl(mbox, XUSB_CFG_ARU_MBOX_OWNER) != owner) {
 +   dev_err(mbox-mbox.dev, Failed to acquire mailbox);
 +   goto busy;
 +   }
 +
 +   mbox_writel(mbox, mbox_pack_msg(msg), XUSB_CFG_ARU_MBOX_DATA_IN);
 +   reg = mbox_readl(mbox, XUSB_CFG_ARU_MBOX_CMD);
 +   reg |= MBOX_INT_EN | MBOX_DEST_FALC;
 +   mbox_writel(mbox, reg, XUSB_CFG_ARU_MBOX_CMD);
 +
 +   spin_unlock_irqrestore(mbox-lock, flags);
 +
 +   return 0;
 +busy:
 +   spin_unlock_irqrestore(mbox-lock, flags);
 +   return -EBUSY;
 +}
 +
 +static int tegra_xusb_mbox_startup(struct mbox_chan *chan)
 +{
 +   return 0;
 +}
 +
 +static void tegra_xusb_mbox_shutdown(struct mbox_chan *chan)
 +{
 +}
 +
 +static bool tegra_xusb_mbox_last_tx_done(struct mbox_chan *chan)
 +{
 +   /*
 +* Transmissions are assumed to be completed as soon as they are
 +* written to the mailbox.
 +*/
 +   return true;

In .send_data() you you mark the channel busy by setting the
XUSB_CFG_ARU_MBOX_OWNER to !MBOX_OWNER_NONE, which remains so until
you get an IRQ. So maybe you should check for the OWNER_NONE flag in
.last_tx_done()?

-Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V5 07/12] pinctrl: tegra-xusb: Add USB PHY support

[Jassi Brar]

On 18 November 2014 at 04:11, Andrew Bresticker abres...@chromium.org wrote:
 In addition to the PCIe and SATA PHYs, the XUSB pad controller also
 supports 3 UTMI, 2 HSIC, and 2 USB3 PHYs.  Each USB3 PHY uses a single
 PCIe or SATA lane and is mapped to one of the three UTMI ports.

 The xHCI controller will also send messages intended for the PHY driver,
 so request and listen for messages on the mailbox's PHY channel.

 Signed-off-by: Andrew Bresticker abres...@chromium.org
 Acked-by: Linus Walleij linus.wall...@linaro.org
 Reviewed-by: Stephen Warren swar...@nvidia.com

Mailbox bits look good to me.

-Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V5 04/12] of: Add NVIDIA Tegra XUSB mailbox binding

[Jassi Brar]

On 18 November 2014 at 04:11, Andrew Bresticker abres...@chromium.org wrote:
 Add device-tree bindings for the Tegra XUSB mailbox which will be used
 for communication between the Tegra xHCI controller's firmware and the
 host processor.

 Signed-off-by: Andrew Bresticker abres...@chromium.org
 Reviewed-by: Stephen Warren swar...@nvidia.com

Acked-by: Jassi Brar jaswinder.si...@linaro.org
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/5] usb: gadget: f_uac2: restructure some code in afunc_set_alt()

[Jassi Brar]

Hi Felipe

On Sat, Aug 30, 2014 at 2:16 AM, Felipe Balbi ba...@ti.com wrote:
 Hi,

 On Wed, Aug 27, 2014 at 07:09:03PM +0200, Daniel Mack wrote:
 Restructure some code to make it easier to read.

 While at it, return -ENOMEM instead of -EINVAL if
 usb_ep_alloc_request() fails, and omit the logging in such cases
 (the mm core will complain loud enough).

 Signed-off-by: Daniel Mack zon...@gmail.com
 ---

 does this depend on anything ? It doesn't apply to my testing/next

There's v6 of the patchset here
http://www.spinics.net/lists/linux-usb/msg112769.html  though the 1-4
patches are probably same.

For Patchv6-5/5 we need
either http://www.spinics.net/lists/linux-usb/msg112773.html
or   http://www.spinics.net/lists/linux-usb/msg112913.html

I and Daniel feel strongly about how we implement data rate control.
Please share your decision making.

Thanks
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/5] usb: gadget: f_uac2: restructure some code in afunc_set_alt()

[Jassi Brar]

On Tue, Sep 2, 2014 at 7:59 PM, Felipe Balbi ba...@ti.com wrote:
 On Tue, Sep 02, 2014 at 04:04:14PM +0200, Daniel Mack wrote:
 Hi Felipe,

 On 08/29/2014 10:46 PM, Felipe Balbi wrote:
  On Wed, Aug 27, 2014 at 07:09:03PM +0200, Daniel Mack wrote:
  Restructure some code to make it easier to read.
 
  While at it, return -ENOMEM instead of -EINVAL if
  usb_ep_alloc_request() fails, and omit the logging in such cases
  (the mm core will complain loud enough).
 
  Signed-off-by: Daniel Mack zon...@gmail.com
  ---
 
  does this depend on anything ? It doesn't apply to my testing/next
 

 The reason it doesn't apply is that you already applied v6 of my series
 to your testing/next tree:


 https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/nextid=c52425b

 The currently pending discussion is about how to determine the size of
 the packets sent down to the host, and it only affects the last patch in
 the series. To summarize, here are the two approaches to do this:


 1) We pre-calculate a pattern of lengths which is then followed when
 sending the packets. This is what Jassi implemented in his alternative
 approach to my version here:

   http://www.spinics.net/lists/linux-usb/msg112913.html

 This idea comes at the price of storing the pre-calculated sequence and
 track its state, which currently means adding 163 unsigned ints to the

 that's quite a lot :-) The version is this thread uses only 5 unsigned
 ints though.

Sorry but doing the comparison in bytes, rather than variables, is silly.
If I had kzalloc()'ed just enough bytes, you think that would have
been 'optimal'? ;)

Anyways I guess I have to just suck it up...
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: gadget: f_uac2: modulate playback data rate

[Jassi Brar]

The UAC2 function driver currently responds to all packets at all times
with wMaxPacketSize packets. That results in way too fast audio
playback as the function driver (which is in fact supposed to define
the audio stream pace) delivers as fast as it can.

We need data rate to match, as accurately as possible, the sampling rate
expressed by the UAC2 topology to the Host.

We do this by sending packets of varying length (1 sample more than usual)
in a pattern so that we get the desired data rate over a period of a
second or sooner. The payload pattern is calculated only once (using
Alan's Algo), when the Host enables the interface, and saved in an
array so that the 2 ping-pong usb_requests directly index into the
pattern array to figure out the payload length they are supposed to
transfer next. Note that the increased overhead in agdev_iso_complete()
is almost zero.

Signed-off-by: Jassi Brar jaswinder.si...@linaro.org
---
 drivers/usb/gadget/function/f_uac2.c | 70 ++--
 1 file changed, 67 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uac2.c 
b/drivers/usb/gadget/function/f_uac2.c
index 246a778..84fd3b0 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -59,8 +59,15 @@ const char *uac2_name = snd_uac2;
 struct uac2_req {
struct uac2_rtd_params *pp; /* parent param */
struct usb_request *req;
+   unsigned idx; /* current element of length-pattern loop */
 };
 
+/*
+ * 5512.5Hz is going to need the maximum number of elements (80),
+ * in the length-pattern loop, among standard ALSA supported rates.
+ */
+#define MAX_LOOP_LEN   80
+
 struct uac2_rtd_params {
struct snd_uac2_chip *uac2; /* parent chip */
bool ep_enabled; /* if the ep is enabled */
@@ -80,6 +87,9 @@ struct uac2_rtd_params {
unsigned max_psize;
struct uac2_req ureq[USB_XFERS];
 
+   unsigned pattern[MAX_LOOP_LEN];
+   unsigned plen; /* valid entries in pattern[] */
+
spinlock_t lock;
 };
 
@@ -191,8 +201,12 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request 
*req)
 
spin_lock_irqsave(prm-lock, flags);
 
-   if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK)
+   if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
+   /* Update length for next payload */
+   ur-idx = (ur-idx + USB_XFERS) % prm-plen;
+   req-length = prm-pattern[ur-idx];
req-actual = req-length;
+   }
 
pending = prm-hw_ptr % prm-period_size;
pending += req-actual;
@@ -1066,6 +1080,31 @@ err:
return -EINVAL;
 }
 
+/*
+ * Find optimal pattern of payloads for a given number
+ * of samples and maximum sync period (in ms) over which
+ * we have to distribute them uniformly.
+ */
+static unsigned
+get_pattern(unsigned samples, unsigned sync, unsigned *pt)
+{
+   unsigned n, x = 0, i = 0, p = samples % sync;
+
+   do {
+   x += p;
+   n = samples / sync;
+   if (x = sync) {
+   n += 1;
+   x -= sync;
+   }
+   if (pt)
+   pt[i] = n;
+   i++;
+   } while (x);
+
+   return i;
+}
+
 static int
 afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
 {
@@ -1097,11 +1136,35 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, 
unsigned alt)
if (intf == agdev-as_out_intf) {
ep = agdev-out_ep;
prm = uac2-c_prm;
+   prm-plen = 1;
+   prm-pattern[0] = prm-max_psize;
config_ep_by_speed(gadget, fn, ep);
agdev-as_out_alt = alt;
} else if (intf == agdev-as_in_intf) {
+   struct f_uac2_opts *opts = agdev_to_uac2_opts(agdev);
+   unsigned intvl, rate;
+
+   if (gadget-speed == USB_SPEED_FULL)
+   intvl = (1  (fs_epin_desc.bInterval - 1)) * 1000;
+   else
+   intvl = (1  (hs_epin_desc.bInterval - 1)) * 125;
+
+   rate = opts-p_srate;
+   if (rate == 5512) { /* which implies 5512.5 practically */
+   rate = 55125;
+   intvl *= 10;
+   }
+
ep = agdev-in_ep;
prm = uac2-p_prm;
+   prm-plen = get_pattern(rate, intvl, NULL); /* dry run */
+   /* We try to support arbitrary rates too */
+   if (prm-plen  MAX_LOOP_LEN) {
+   prm-plen = 1;
+   prm-pattern[0] = rate / intvl;
+   } else {
+   prm-plen = get_pattern(rate, intvl, prm-pattern);
+   }
config_ep_by_speed(gadget, fn, ep);
agdev-as_in_alt = alt;
} else {
@@ -1125,12 +1188,13 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, 
unsigned alt

Re: [PATCH] usb: gadget: f_uac2: modulate playback data rate

[Jassi Brar]

On Fri, Aug 29, 2014 at 11:43 AM, Jassi Brar jaswinder.si...@linaro.org wrote:
 The UAC2 function driver currently responds to all packets at all times
 with wMaxPacketSize packets. That results in way too fast audio
 playback as the function driver (which is in fact supposed to define
 the audio stream pace) delivers as fast as it can.

 We need data rate to match, as accurately as possible, the sampling rate
 expressed by the UAC2 topology to the Host.

 We do this by sending packets of varying length (1 sample more than usual)
 in a pattern so that we get the desired data rate over a period of a
 second or sooner. The payload pattern is calculated only once (using
 Alan's Algo), when the Host enables the interface, and saved in an
 array so that the 2 ping-pong usb_requests directly index into the
 pattern array to figure out the payload length they are supposed to
 transfer next. Note that the increased overhead in agdev_iso_complete()
 is almost zero.

 Signed-off-by: Jassi Brar jaswinder.si...@linaro.org
 ---
  drivers/usb/gadget/function/f_uac2.c | 70 
 ++--
  1 file changed, 67 insertions(+), 3 deletions(-)

 diff --git a/drivers/usb/gadget/function/f_uac2.c 
 b/drivers/usb/gadget/function/f_uac2.c
 index 246a778..84fd3b0 100644
 --- a/drivers/usb/gadget/function/f_uac2.c
 +++ b/drivers/usb/gadget/function/f_uac2.c
 @@ -59,8 +59,15 @@ const char *uac2_name = snd_uac2;
  struct uac2_req {
 struct uac2_rtd_params *pp; /* parent param */
 struct usb_request *req;
 +   unsigned idx; /* current element of length-pattern loop */
  };

 +/*
 + * 5512.5Hz is going to need the maximum number of elements (80),
 + * in the length-pattern loop, among standard ALSA supported rates.
 + */
 +#define MAX_LOOP_LEN   80
 +
  struct uac2_rtd_params {
 struct snd_uac2_chip *uac2; /* parent chip */
 bool ep_enabled; /* if the ep is enabled */
 @@ -80,6 +87,9 @@ struct uac2_rtd_params {
 unsigned max_psize;
 struct uac2_req ureq[USB_XFERS];

 +   unsigned pattern[MAX_LOOP_LEN];
 +   unsigned plen; /* valid entries in pattern[] */
 +
 spinlock_t lock;
  };

 @@ -191,8 +201,12 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request 
 *req)

 spin_lock_irqsave(prm-lock, flags);

 -   if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK)
 +   if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
 +   /* Update length for next payload */
 +   ur-idx = (ur-idx + USB_XFERS) % prm-plen;
 +   req-length = prm-pattern[ur-idx];
 req-actual = req-length;
 +   }

 pending = prm-hw_ptr % prm-period_size;
 pending += req-actual;
 @@ -1066,6 +1080,31 @@ err:
 return -EINVAL;
  }

 +/*
 + * Find optimal pattern of payloads for a given number
 + * of samples and maximum sync period (in ms) over which
 + * we have to distribute them uniformly.
 + */
 +static unsigned
 +get_pattern(unsigned samples, unsigned sync, unsigned *pt)
 +{
 +   unsigned n, x = 0, i = 0, p = samples % sync;
 +
 +   do {
 +   x += p;
 +   n = samples / sync;
 +   if (x = sync) {
 +   n += 1;
 +   x -= sync;
 +   }
 +   if (pt)
 +   pt[i] = n;
 +   i++;
 +   } while (x);
 +
 +   return i;
 +}
 +
  static int
  afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
  {
 @@ -1097,11 +1136,35 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, 
 unsigned alt)
 if (intf == agdev-as_out_intf) {
 ep = agdev-out_ep;
 prm = uac2-c_prm;
 +   prm-plen = 1;
 +   prm-pattern[0] = prm-max_psize;
 config_ep_by_speed(gadget, fn, ep);
 agdev-as_out_alt = alt;
 } else if (intf == agdev-as_in_intf) {
 +   struct f_uac2_opts *opts = agdev_to_uac2_opts(agdev);
 +   unsigned intvl, rate;
 +
 +   if (gadget-speed == USB_SPEED_FULL)
 +   intvl = (1  (fs_epin_desc.bInterval - 1)) * 1000;
 +   else
 +   intvl = (1  (hs_epin_desc.bInterval - 1)) * 125;
 +
 +   rate = opts-p_srate;
 +   if (rate == 5512) { /* which implies 5512.5 practically */
 +   rate = 55125;
 +   intvl *= 10;
 +   }
 +
 ep = agdev-in_ep;
 prm = uac2-p_prm;
 +   prm-plen = get_pattern(rate, intvl, NULL); /* dry run */
 +   /* We try to support arbitrary rates too */
 +   if (prm-plen  MAX_LOOP_LEN) {
 +   prm-plen = 1;
 +   prm-pattern[0] = rate / intvl;
 +   } else {
 +   prm-plen = get_pattern(rate, intvl, prm-pattern);

   We need to multiply every element

Re: [PATCH] usb: gadget: f_uac2: modulate playback data rate

[Jassi Brar]

On Fri, Aug 29, 2014 at 1:21 PM, Jassi Brar jaswinder.si...@linaro.org wrote:

 +
 + rate = opts-p_srate;
 + if (rate == 5512) { /* which implies 5512.5 practically */
 + rate = 55125;
 + intvl *= 10;
 + }
 Well, I'd say that Alan's approach as implemented in my v6 is still more
 comprehensible for anyone who'll try to understand this later, and it
 doesn't make any assumption on run time values.

 I hope you realize this 5512 check has nothing to do with the Alan's
 approach  I can very well remove the check and it will still work!
  It is only an added feature to provide accurate data rate because
 ALSA API can not define fractional rates.

When ALSA says 5512Hz it is actually played at 5512.5Hz by a real codec.
So I think *any* f_uac2 patch, for the rate fix, should do the above
5512-5512.5 conversion.

-Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 5/5] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Wed, Aug 27, 2014 at 10:39 PM, Daniel Mack zon...@gmail.com wrote:
 The UAC2 function driver currently responds to all packets at all times
 with wMaxPacketSize packets. That results in way too fast audio
 playback as the function driver (which is in fact supposed to define
 the audio stream pace) delivers as fast as it can.

 Fix this by sizing each packet correctly with the following steps:

  a) Set the packet's size by dividing the nominal data rate by the
 playback endpoint's interval.

  b) If there is a residual value from the calculation in a), add
 it to a accumulator to keep track of it across packets.

  c) If the accumulator has gathered at least the number of bytes
 that are needed for one sample frame, increase the packet size.

 This way, the packet size calculation will get rid of any kind of
 imprecision that would otherwise occur with a simple division over
 time.

 Some of the variables that are needed while processing each packet
 are pre-computed for performance reasons.

 Signed-off-by: Daniel Mack zon...@gmail.com
 ---
  drivers/usb/gadget/function/f_uac2.c | 69 
 +---
  1 file changed, 65 insertions(+), 4 deletions(-)

 diff --git a/drivers/usb/gadget/function/f_uac2.c 
 b/drivers/usb/gadget/function/f_uac2.c
 index 246a778..a5a27a5 100644
 --- a/drivers/usb/gadget/function/f_uac2.c
 +++ b/drivers/usb/gadget/function/f_uac2.c
 @@ -92,6 +92,15 @@ struct snd_uac2_chip {

 struct snd_card *card;
 struct snd_pcm *pcm;
 +
 +   /* timekeeping for the playback endpoint */
 +   unsigned int p_interval;
 +   unsigned int p_residue;
 +
 +   /* pre-calculated values for playback iso completion */
 +   unsigned int p_pktsize;
 +   unsigned int p_pktsize_residue;
 +   unsigned int p_framesize;
  };

I admire Alan's simple algo. I couldn't have matched that.

However I am not sure how worth is the implementation if it requires 3
more members to avoid calculations simple enough to cause no
noticeable overhead. Once every millisecond is perfectly bearable IMO.
5 members in uac2 structure for only playback, look ugly.

regards,
-jassi



  #define BUFF_SIZE_MAX  (PAGE_SIZE * 16)
 @@ -191,8 +200,29 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request 
 *req)

 spin_lock_irqsave(prm-lock, flags);

 -   if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK)
 +   if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
 +   /*
 +* For each IN packet, take the quotient of the current data
 +* rate and the endpoint's interval as the base packet size.
 +* If there is a residue from this division, add it to the
 +* residue accumulator.
 +*/
 +   req-length = uac2-p_pktsize;
 +   uac2-p_residue += uac2-p_pktsize_residue;
 +
 +   /*
 +* Whenever there are more bytes in the accumulator than we
 +* need to add one more sample frame, increase this packet's
 +* size and decrease the accumulator.
 +*/
 +   if (uac2-p_residue / uac2-p_interval = uac2-p_framesize) {
 +   req-length += uac2-p_framesize;
 +   uac2-p_residue -= uac2-p_framesize *
 +  uac2-p_interval;
 +   }
 +
 req-actual = req-length;
 +   }

 pending = prm-hw_ptr % prm-period_size;
 pending += req-actual;
 @@ -346,6 +376,7 @@ static int uac2_pcm_open(struct snd_pcm_substream 
 *substream)
 c_srate = opts-c_srate;
 p_chmask = opts-p_chmask;
 c_chmask = opts-c_chmask;
 +   uac2-p_residue = 0;

 runtime-hw = uac2_pcm_hardware;

 @@ -1077,7 +1108,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, 
 unsigned alt)
 struct usb_request *req;
 struct usb_ep *ep;
 struct uac2_rtd_params *prm;
 -   int i;
 +   int req_len, i;

 /* No i/f has more than 2 alt settings */
 if (alt  1) {
 @@ -1099,11 +1130,41 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, 
 unsigned alt)
 prm = uac2-c_prm;
 config_ep_by_speed(gadget, fn, ep);
 agdev-as_out_alt = alt;
 +   req_len = prm-max_psize;
 } else if (intf == agdev-as_in_intf) {
 +   struct f_uac2_opts *opts = agdev_to_uac2_opts(agdev);
 +   unsigned int factor, rate;
 +   struct usb_endpoint_descriptor *ep_desc;
 +
 ep = agdev-in_ep;
 prm = uac2-p_prm;
 config_ep_by_speed(gadget, fn, ep);
 agdev-as_in_alt = alt;
 +
 +   /* pre-calculate the playback endpoint's interval */
 +   if (gadget-speed == USB_SPEED_FULL) {
 +   ep_desc = fs_epin_desc;
 +   factor = 1000;
 +  

Re: [PATCH v6 5/5] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Thu, Aug 28, 2014 at 3:33 PM, Daniel Mack dan...@zonque.org wrote:
 On 08/28/2014 11:33 AM, Jassi Brar wrote:
 On Wed, Aug 27, 2014 at 10:39 PM, Daniel Mack zon...@gmail.com wrote:

 diff --git a/drivers/usb/gadget/function/f_uac2.c 
 b/drivers/usb/gadget/function/f_uac2.c
 index 246a778..a5a27a5 100644
 --- a/drivers/usb/gadget/function/f_uac2.c
 +++ b/drivers/usb/gadget/function/f_uac2.c
 @@ -92,6 +92,15 @@ struct snd_uac2_chip {

 struct snd_card *card;
 struct snd_pcm *pcm;
 +
 +   /* timekeeping for the playback endpoint */
 +   unsigned int p_interval;
 +   unsigned int p_residue;
 +
 +   /* pre-calculated values for playback iso completion */
 +   unsigned int p_pktsize;
 +   unsigned int p_pktsize_residue;
 +   unsigned int p_framesize;
  };

 I admire Alan's simple algo. I couldn't have matched that.

 However I am not sure how worth is the implementation if it requires 3
 more members to avoid calculations simple enough to cause no
 noticeable overhead. Once every millisecond is perfectly bearable IMO.

 Alan is right. If we can avoid that, we should.

It started with avoiding storing values (even lesser overhead) and
doing things in runtime (code looking elegant).

 5 members in uac2 structure for only playback, look ugly.

 It's still superior to a number of unnecessary calculations done every
 millisecond which will come up with the same result every time. So I
 clearly favor that approach. Three more ints in a struct don't hurt
 really for something that's usually not instantiated more than once per
 system.

 Anyway, I'll leave it to Felipe whether to merge v5 or v6 :)

Please wait, let me cook up a patch that uses (a) Alan's algo, (b)
cause lesser churn and (c) is even 'faster'.

-jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 5/5] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Thu, Aug 28, 2014 at 3:47 PM, Daniel Mack dan...@zonque.org wrote:
 On 08/28/2014 12:10 PM, Jassi Brar wrote:
 On Thu, Aug 28, 2014 at 3:33 PM, Daniel Mack dan...@zonque.org wrote:

 It's still superior to a number of unnecessary calculations done every
 millisecond which will come up with the same result every time. So I
 clearly favor that approach. Three more ints in a struct don't hurt
 really for something that's usually not instantiated more than once per
 system.

 Anyway, I'll leave it to Felipe whether to merge v5 or v6 :)

 Please wait, let me cook up a patch that uses (a) Alan's algo, (b)
 cause lesser churn and (c) is even 'faster'.

 Alright.

 Please note, however, that you can't do the divison 'uac2-p_residue /
 uac2-p_interval' in a pre-calucated value, as that won't cover cases
 with a per-packet residual value that isn't a multiple of the frame
 size. Hence, the residue counter has to go in bytes, not frames, and for
 that reason, I left that division in the per-packet loop.

 Felipe, could you already merge patch 1-5 of the last series (v6)?

err.. 1-4 of v6 :) right?   Yes please, Felipe, we have no contention
on those nice patches.

Thanks
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 5/5] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Wed, Aug 27, 2014 at 12:20 PM, Daniel Mack dan...@zonque.org wrote:
 On 08/27/2014 06:08 AM, Jassi Brar wrote:
 On Wed, Aug 27, 2014 at 3:23 AM, Daniel Mack zon...@gmail.com wrote:

 +   uac2-p_interval = (1  (ep_desc-bInterval - 1)) * factor;
 +   req_len = rate / uac2-p_interval;

 +  if (opts-p_srate % uac2-p_interval)
 +  req_len += fsz;
 .

 +   uac2-p_residue = 0;
 } else {
 dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
 return -EINVAL;
 @@ -1128,7 +1188,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, 
 unsigned alt)

 req-zero = 0;
 req-context = prm-ureq[i];
 -   req-length = prm-max_psize;
 +   req-length = req_len;
 req-complete = agdev_iso_complete;
 req-buf = prm-rbuf + i * req-length;

   otherwise req[0]-buf might overlap req[1]-buf's first frame
 for when we need to send an extra frame.

 Hmm? The first USB_XFERS packets will only contain zeros, and we're only
 preparing those here. For every successive packet, the length is
 recalculated and the audio material is copied in accordingly before the
 requets is requeued. What buffers should overlap here?

For 44100/2/S16, req_len is 176 or 44 frames. But we need to send 45
frames in a packet occasionally.

req[0]-buf = rbuf + 0   and  req[1]-buf = rbuf + 176.
 But what if we req[0] needed to carry the packet with 45 frames?

-jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 5/5] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Wed, Aug 27, 2014 at 12:45 PM, Daniel Mack dan...@zonque.org wrote:
 On 08/27/2014 09:07 AM, Jassi Brar wrote:
 On Wed, Aug 27, 2014 at 12:20 PM, Daniel Mack dan...@zonque.org wrote:

 Hmm? The first USB_XFERS packets will only contain zeros, and we're only
 preparing those here. For every successive packet, the length is
 recalculated and the audio material is copied in accordingly before the
 requets is requeued. What buffers should overlap here?

 For 44100/2/S16, req_len is 176 or 44 frames. But we need to send 45
 frames in a packet occasionally.

 req[0]-buf = rbuf + 0   and  req[1]-buf = rbuf + 176.

 No. req[0]-buf = rbuf + 0 and req[1]-buf = rbuf + max_psize.

You patch does
-   req-length = prm-max_psize;
+   req-length = req_len;

Or did you send the wrong version of patch?

where req_len is calculated as
   req_len = rate / uac2-p_interval;

Clearly for 44.1/2/S16,  req_len evaluates to 176.

-jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 5/5] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Wed, Aug 27, 2014 at 3:23 AM, Daniel Mack zon...@gmail.com wrote:

 @@ -1099,11 +1139,31 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, 
 unsigned alt)
 prm = uac2-c_prm;
 config_ep_by_speed(gadget, fn, ep);
 agdev-as_out_alt = alt;
 +   req_len = prm-max_psize;
 } else if (intf == agdev-as_in_intf) {
 +   struct f_uac2_opts *opts = agdev_to_uac2_opts(agdev);
 +   unsigned int rate = opts-p_srate * opts-p_ssize *
 +   num_channels(opts-p_chmask);
 +   struct usb_endpoint_descriptor *ep_desc;
 +   unsigned int factor;
 +
 ep = agdev-in_ep;
 prm = uac2-p_prm;
 config_ep_by_speed(gadget, fn, ep);
 agdev-as_in_alt = alt;
 +
 +   /* pre-calculate the playback endpoint's interval */
 +   if (gadget-speed == USB_SPEED_FULL) {
 +   ep_desc = fs_epin_desc;
 +   factor = 1000;
 +   } else {
 +   ep_desc = hs_epin_desc;
 +   factor = 125;
 +   }
 +
 +   uac2-p_interval = (1  (ep_desc-bInterval - 1)) * factor;
 +   req_len = rate / uac2-p_interval;

(a)For 44.1/2/S16,  req_len = 176

 +   uac2-p_residue = 0;
 } else {
 dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
 return -EINVAL;
 @@ -1128,7 +1188,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, 
 unsigned alt)

 req-zero = 0;
 req-context = prm-ureq[i];
 -   req-length = prm-max_psize;
 +   req-length = req_len;
(b)req-length = req_len or 176

 req-complete = agdev_iso_complete;
 req-buf = prm-rbuf + i * req-length;

Here  req[0]-buf  is  req-length, which is 176 bytes from (b).

I hope this makes it clear.

-jassi



 }
 --
 2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 0/5] usb: gadget: f_uac2: cleanups and capture timing

[Jassi Brar]

On Wed, Aug 27, 2014 at 1:15 PM, Daniel Mack zon...@gmail.com wrote:
 Hi,

 this is v4 of the f_uac2 timing fixup series.

 Changes from v4:

 * Fix buffer setup in afunc_set_alt()

 Changes from v3:

 * add another patch (3/5) to introduce agdev_to_uac2_opts()
   which is also needed in 5/5
 * patch 5/5 only:
   move from a pre-calculated sequence of packet lengths to
   an accumulator that sums up the residual values from the
   packet size calculation and adds an extra sample frame
   now and then when the accumulator has gathered enough bytes.

 Changes from v2:

 * swap path 3 and 4, so that the ALSA buffer wrap around fix
   comes in first. It's not actually a bug fix for the current
   code, but more a preparation to allow for smaller packets.
 * use the p_ssize, p_chmask and p_srate for the length
   calculations
 * prepare a sequence of packet sizes to send, and alternate
   over them when sending the the IN packets. The actual commit
   log and the comments yield some more details on that.

 Changes from v1:

 * drop UAC_EP_CS_ATTR_FILL_MAX approach and rather size the
   packets correctly
 * add a patch to fix buffer wrap problems in the ALSA buffer
   logic, which wasn't needed before

 The first two patches are just cleanups.

 Thanks to Alan Stern and Jassi Brar for the feedback!


 Daniel

 Daniel Mack (5):
   usb: gadget: f_uac2: restructure some code in afunc_set_alt()
   usb: gadget: f_uac2: add short-hand for 'dev'
   usb: gadget: f_uac2: introduce agdev_to_uac2_opts
   usb: gadget: f_uac2: handle partial dma area wrap
   usb: gadget: f_uac2: send reasonably sized packets

 Acked-by: Jassi Brar jassisinghb...@gmail.com

BTW, against which tree is this patchset based?

Thanks
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver

[Jassi Brar]

On 27 August 2014 23:08, Andrew Bresticker abres...@chromium.org wrote:
 On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren swar...@wwwdotorg.org 
 wrote:

 I'm not even sure if it's appropriate for the low-level mailbox driver to
 know about the semantics of the message, rather than simply sending them on
 to the client driver? Perhaps when drivers register(?) for callbacks(?) for
 messages, they should state which types of messages they want to listen to?

 So there's not really a way for the client driver to tell the mailbox
 driver which types of messages it wants to listen to on a particular
 channel with the mailbox framework - it simply provides a way for
 clients to bind with channels.  I think there are a couple of options
 here, either: a) have a channel per message (as you mentioned in the
 previous patch), which allows the client to only register for messages
 (channels) it wants to handle, or b) extend the mailbox framework to
 allow shared channels so that both clients can receive messages on the
 single channel and handle messages appropriately.   The disadvantage
 of (a) is that the commands are firmware defined and could
 theoretically change between releases of the firmware, though I'm not
 sure how common that is in practice.  So that leaves (b) - Jassi, what
 do you think about having shared (non-exclusive) channels?

n++ ... 'mailbox' is one such device that imbibes properties of local
controller hardware and the remote firmware. Change in remote f/w's
behavior might require the controller driver to change besides our
client driver. A typical example is format of payloads (if
involved) a client and mailbox controller driver have direct
understanding of the packet format ... which is likely to change with
remote f/w.  So if the original concern why are we doing s/w protocol
stuff in controller driver? won't go away by providing for shared
channels (which would have its own tradeoffs).

BTW, on DMAEngine we are moving towards virtual channels backed by
limited physical ones ... your setup looks quite similar. So your
demuxer doesn't hurt my eyes at all.

-jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Tue, Aug 26, 2014 at 9:18 PM, Alan Stern st...@rowland.harvard.edu wrote:
 On Tue, 26 Aug 2014, Daniel Mack wrote:

 On 08/26/2014 05:08 PM, Alan Stern wrote:
  The normal approach is to perform a simple runtime calculation (no
  pre-allocated pattern).  It's not complex.
 
  Let S be the number of samples per second at the nominal transfer rate
  (for example, S = 44100).  Let R be the number of packets per second
  (1000 because you transfer one packet every millisecond).

 [...]

 Yes, I thought about that too. The pre-allocated approach is not much
 code either, and it also gives accurate values for all common audio
 sample rates, but maybe the runtime calculation nicer and easier to read
 in the end. I'll give that a try later.

 It has the advantage of working for all audio rates, not just the
 common ones.  And it doesn't require preallocation of quite so many
 transfer buffers (although they are generally small enough not to
 matter much).

I was indeed aware we could do the calculations in runtime, but I am
not sure one formula can get us optimal result for all scenarios. I
think we want to play with bInterval as well.

For ex, on extreme end, for 5512Hz we may want to double packet size
(from 22 bytes) in favor of halving ISO polls or even better.
Similarly for 192000Hz, we don't want to reject only because
wMaxPacketSize is 512... because we are likely running HS, so we could
set bInterval to 0.5ms and do 384 bytes packet.

I am still aware this all could be calculated and decided in runtime,
but I don't think many readers will find that easier to understand
than a static table.

BTW, we can do without increasing allocated usb requests.

Regards,
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Tue, Aug 26, 2014 at 10:00 PM, Jassi Brar jassisinghb...@gmail.com wrote:
 On Tue, Aug 26, 2014 at 9:18 PM, Alan Stern st...@rowland.harvard.edu wrote:
 On Tue, 26 Aug 2014, Daniel Mack wrote:

 On 08/26/2014 05:08 PM, Alan Stern wrote:
  The normal approach is to perform a simple runtime calculation (no
  pre-allocated pattern).  It's not complex.
 
  Let S be the number of samples per second at the nominal transfer rate
  (for example, S = 44100).  Let R be the number of packets per second
  (1000 because you transfer one packet every millisecond).

 [...]

 Yes, I thought about that too. The pre-allocated approach is not much
 code either, and it also gives accurate values for all common audio
 sample rates, but maybe the runtime calculation nicer and easier to read
 in the end. I'll give that a try later.

 It has the advantage of working for all audio rates, not just the
 common ones.  And it doesn't require preallocation of quite so many
 transfer buffers (although they are generally small enough not to
 matter much).

 I was indeed aware we could do the calculations in runtime, but I am
 not sure one formula can get us optimal result for all scenarios. I
 think we want to play with bInterval as well.

 For ex, on extreme end, for 5512Hz we may want to double packet size
 (from 22 bytes) in favor of halving ISO polls or even better.
 Similarly for 192000Hz, we don't want to reject only because
 wMaxPacketSize is 512... because we are likely running HS, so we could
 set bInterval to 0.5ms and do 384 bytes packet.

 I am still aware this all could be calculated and decided in runtime,
 but I don't think many readers will find that easier to understand
 than a static table.

Another example, for 44100Hz, calculations suggest synchronizing over
10ms as {9x176 + 1x180}, however we can keep better sync over 5ms as
{4x176+1x178}. Again we can calculate for even such optimizations but
only at the cost of readability.

Regards,
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Tue, Aug 26, 2014 at 1:46 PM, Daniel Mack zon...@gmail.com wrote:

 +static void
 +afunc_set_p_pktsize(struct usb_gadget *gadget, struct audio_dev *agdev)
 +{
 +   unsigned i, residue, rate, factor, interval, framesize, pktsize, len;
 +   struct snd_uac2_chip *uac2 = agdev-uac2;
 +   struct usb_endpoint_descriptor *ep_desc;
 +   struct f_uac2_opts *opts =
 +   container_of(agdev-func.fi, struct f_uac2_opts, func_inst);
 +
 +   if (gadget-speed == USB_SPEED_FULL) {
 +   ep_desc = fs_epin_desc;
 +   factor = 1000;
 +   } else {
 +   ep_desc = hs_epin_desc;
 +   factor = 125;
 +   }
 +
 +   /*
 +* Simply dividing the desired data rate by the number of available
 +* packets per seconds give rounding errors. Hence, we prepare a
 +* sequence of packet sizes that will be alternated over when sending
 +* the packets to the IN endpoint.
 +*/
 +   framesize = opts-p_ssize * num_channels(opts-p_chmask);
 +   interval = (1  (ep_desc-bInterval - 1)) * factor;

Or simply, interval = 1000 ?

 +   rate = opts-p_srate * framesize;
 +
 +   pktsize = min_t(unsigned, rate / interval, ep_desc-wMaxPacketSize);
 +
 +   /*
 +* If there's a residue from the modulo operation, calculate the index
 +* of the first packet that could be increased by one sample frame.
 +* This will be our sequence length, and the last member carries one
 +* frame more than the others. Hence, the shorter the sequence is, the
 +* more bytes will be transferred.
 +*/
 +   residue = (rate % interval) / framesize;
 +   if (residue)
 +   len = min_t(unsigned, interval / residue,
 +   ARRAY_SIZE(uac2-p_pktsize_seq));
 +   else
 +   len = 1;
 +
 +   /* Set all lengths in the sequence to the divided value ... */
 +   for (i = 0; i  len; i++)
 +   uac2-p_pktsize_seq[i] = pktsize;
 +
 +   /* ... and add another frame to the last sequence member */
 +   if (len  1)
 +   uac2-p_pktsize_seq[len - 1] += framesize;
 +
I see this takes care of only 44100Hz case. As I said in other post,
we should take care of all standard [5512.5, 192000] rates, possibly
also optimizing poll period.

Regards,
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Tue, Aug 26, 2014 at 1:38 PM, Daniel Mack dan...@zonque.org wrote:
 On 08/25/2014 07:22 PM, Jassi Brar wrote:
 On Mon, Aug 25, 2014 at 9:30 PM, Daniel Mack zon...@gmail.com wrote:

 @@ -187,7 +189,7 @@ agdev_iso_complete(struct usb_ep *ep, struct 
 usb_request *req)

 if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
 src = prm-dma_area + prm-hw_ptr;
 -   req-actual = req-length;
 +   req-length = req-actual = uac2-c_pktsize;

 This doesn't seem right.
 We asked req-length to be transmitted by the udc which shouldn't
 return until that is done. So at this point setting 'length' doesn't
 mean much. The original assignment to 'actual' is only because we want
 to ignore any issue that caused the udc to transmit fewer bytes (we
 drop that data).

 Looking at this again, setting req-length is in fact the right thing to
 do. We want to prepare a new packet of a specific length, so we have to
 let the udc driver know how much data is contains before we call
 usb_ep_queue() again.

I was coming from increasing usb requests to number of elements in the
pattern array. In which case we set length of each req just once in
afunc_set_alt() and leave iso_complete() *untouched*.

However, if we do want to keep using only 2 usb requests, then yes we
do have to reset the req-length for the next packet. But still that
should ideally be done at the end of iso_complete(). The
initialization of 2 requests' length in afunc_set_alt() should be done
to first 2 elements of pattern (though nothing observable changes).

 Note that this is for SNDRV_PCM_STREAM_PLAYBACK, so for the IN endpoint
 of the gadget. Reading your description again makes me believe you
 actually wanted to do that for the *capture* side, because this is were
 could possibly 'drop data', right?

By 'drop data' I meant we don't retry sending length-actual bytes.

Regards,
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Tue, Aug 26, 2014 at 11:07 PM, Alan Stern st...@rowland.harvard.edu wrote:
 On Tue, 26 Aug 2014, Jassi Brar wrote:

 On Tue, Aug 26, 2014 at 10:00 PM, Jassi Brar jassisinghb...@gmail.com 
 wrote:
  On Tue, Aug 26, 2014 at 9:18 PM, Alan Stern st...@rowland.harvard.edu 
  wrote:
  On Tue, 26 Aug 2014, Daniel Mack wrote:
 
  On 08/26/2014 05:08 PM, Alan Stern wrote:
   The normal approach is to perform a simple runtime calculation (no
   pre-allocated pattern).  It's not complex.
  
   Let S be the number of samples per second at the nominal transfer rate
   (for example, S = 44100).  Let R be the number of packets per second
   (1000 because you transfer one packet every millisecond).
 
  [...]
 
  Yes, I thought about that too. The pre-allocated approach is not much
  code either, and it also gives accurate values for all common audio
  sample rates, but maybe the runtime calculation nicer and easier to read
  in the end. I'll give that a try later.
 
  It has the advantage of working for all audio rates, not just the
  common ones.  And it doesn't require preallocation of quite so many
  transfer buffers (although they are generally small enough not to
  matter much).
 
  I was indeed aware we could do the calculations in runtime, but I am
  not sure one formula can get us optimal result for all scenarios. I
  think we want to play with bInterval as well.
 
  For ex, on extreme end, for 5512Hz we may want to double packet size
  (from 22 bytes) in favor of halving ISO polls or even better.
  Similarly for 192000Hz, we don't want to reject only because
  wMaxPacketSize is 512... because we are likely running HS, so we could
  set bInterval to 0.5ms and do 384 bytes packet.

 That's a separate matter.  Selecting the sample rate and the polling
 interval...  You can put those things in a static table or decide them
 at runtime.  Either way, once you have settled on those parameters, the
 computation I outlined earlier shows how to pack the samples into
 packets.

OK.

  I am still aware this all could be calculated and decided in runtime,
  but I don't think many readers will find that easier to understand
  than a static table.
 
 Another example, for 44100Hz, calculations suggest synchronizing over
 10ms as {9x176 + 1x180}, however we can keep better sync over 5ms as
 {4x176+1x178}.

 No, that's not good because it ends up splitting samples between
 packets.  You'd put 44 samples in the first four packets, then 44+(1/2)
 in the fifth, then (1/2)+43+(1/2) in the next four, and (1/2)+44 in the
 tenth (as opposed to 44 in nine packets and 45 in the tenth).

 Maybe UAC2 allows such things (I haven't read the spec), but in general
 it seems like a bad idea.  And IIRC, the USB-2 spec disallows it.

Hmm... yeah, frame-unaligned packets may mess up audio in case of
packet drops. Thanks.

 Again we can calculate for even such optimizations but
 only at the cost of readability.

 Anybody accustomed to audio programming will immediately recognize the
 pattern of the computation I outlined.

:) that is applicable against argument for readability for any subsystem.
Anyways, Daniel is doing the patch, I think he has the final call as
long we get best functionality.

Thanks
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 5/5] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Wed, Aug 27, 2014 at 3:23 AM, Daniel Mack zon...@gmail.com wrote:
 The UAC2 function driver currently responds to all packets at all times
 with wMaxPacketSize packets. That results in way too fast audio
 playback as the function driver (which is in fact supposed to define
 the audio stream pace) delivers as fast as it can.

 Fix this by sizing each packet correctly with the following steps:

  a) Set the packet's size by dividing the nominal data rate by the
 playback endpoint's interval.q

  b) If there is a residual value from the calculation in a), add
 it to a accumulator to keep track of it across packets.

  c) If the accumulator has gathered at least the number of bytes
 that are needed for one sample frame, increase the packet size.

 This way, the packet size calculation will get rid of any kind of
 imprecision that would otherwise occur with a simple division over
 time.

 Signed-off-by: Daniel Mack zon...@gmail.com
 ---
  drivers/usb/gadget/function/f_uac2.c | 66 
 ++--
  1 file changed, 63 insertions(+), 3 deletions(-)

 diff --git a/drivers/usb/gadget/function/f_uac2.c 
 b/drivers/usb/gadget/function/f_uac2.c
 index 246a778..06c364a 100644
 --- a/drivers/usb/gadget/function/f_uac2.c
 +++ b/drivers/usb/gadget/function/f_uac2.c
 @@ -92,6 +92,10 @@ struct snd_uac2_chip {

 struct snd_card *card;
 struct snd_pcm *pcm;
 +
 +   /* timekeeping for the playback endpoint */
 +   unsigned int p_interval;
 +   unsigned int p_residue;
  };

  #define BUFF_SIZE_MAX  (PAGE_SIZE * 16)
 @@ -191,8 +195,43 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request 
 *req)

 spin_lock_irqsave(prm-lock, flags);

 -   if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK)
 +   if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
 +   struct audio_dev *agdev = uac2_to_agdev(uac2);
 +   struct f_uac2_opts *opts = agdev_to_uac2_opts(agdev);
 +   unsigned int fsz = opts-p_ssize * 
 num_channels(opts-p_chmask);
 +   unsigned int rate = opts-p_srate * fsz;
 +
 +   /*
 +* For each IN packet, calculate the minimum packet size by
 +* dividing the nominal bytes per second as required by the
 +* current audio format by the endpoint's interval.
 +*/
 +   req-length = min_t(unsigned int,
 +   rate / uac2-p_interval, prm-max_psize);
 +
 +   /*
 +* If there is a residual value from the division, add it to
 +* the residue accumulator.
 +*/
 +   uac2-p_residue += rate % uac2-p_interval;
 +
 +   /*
 +* Whenever there are more bytes in the accumulator than we
 +* need to add one more sample frame, increase this packet's
 +* size and decrease the accumulator.
 +*/
 +   if (uac2-p_residue / uac2-p_interval = fsz) {
 +   req-length += fsz;
 +   uac2-p_residue -= fsz * uac2-p_interval;
 +   }
 +
 +   /*
 +* req-actual is used as copy length below. We can safely
 +* override it here as it is not looked at when the packet
 +* is resubmitted on an IN endpoint.
 +*/
 req-actual = req-length;
 +   }

 pending = prm-hw_ptr % prm-period_size;
 pending += req-actual;
 @@ -346,6 +385,7 @@ static int uac2_pcm_open(struct snd_pcm_substream 
 *substream)
 c_srate = opts-c_srate;
 p_chmask = opts-p_chmask;
 c_chmask = opts-c_chmask;
 +   uac2-p_residue = 0;

 runtime-hw = uac2_pcm_hardware;

 @@ -1077,7 +1117,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, 
 unsigned alt)
 struct usb_request *req;
 struct usb_ep *ep;
 struct uac2_rtd_params *prm;
 -   int i;
 +   int req_len, i;

 /* No i/f has more than 2 alt settings */
 if (alt  1) {
 @@ -1099,11 +1139,31 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, 
 unsigned alt)
 prm = uac2-c_prm;
 config_ep_by_speed(gadget, fn, ep);
 agdev-as_out_alt = alt;
 +   req_len = prm-max_psize;
 } else if (intf == agdev-as_in_intf) {
 +   struct f_uac2_opts *opts = agdev_to_uac2_opts(agdev);
+  unsigned int fsz = opts-p_ssize *
num_channels(opts-p_chmask);
+  unsigned int rate = opts-p_srate * fsz;
 +   struct usb_endpoint_descriptor *ep_desc;
 +   unsigned int factor;
 +
 ep = agdev-in_ep;
 prm = uac2-p_prm;
 config_ep_by_speed(gadget, fn, ep);
 agdev-as_in_alt = alt;
 +
 +   /* pre-calculate the playback endpoint's interval */
 +   

Re: [PATCH 0/4] usb: gadget: f_uac2: cleanups and capture timing

[Jassi Brar]

On Mon, Aug 25, 2014 at 2:14 PM, Daniel Mack dan...@zonque.org wrote:
 Hi Clemens,

 On 08/25/2014 09:15 AM, Clemens Ladisch wrote:
 Daniel Mack wrote:
 a) Linux snd-usb-audio currently pre-calculates the estimated packet
 sizes to expect from a USB device, and will only receive packets that
 have the expected size or are smaller.

 snd-usb-audio allows packets to be 25 % larger.

 Yes, but packets can't be larger than *that*.

 This can be worked around by setting the UAC_EP_CS_ATTR_FILL_MAX in
 the UAC2 endpoint descriptor

 The spec says about this flag (4.10.1.2):
 | when receiving data from an IN endpoint, the Host software must be
 | prepared to receive wMaxPacketSize bytes and discard any superfluous
 | zero bytes in the packet.
 | Note:  This bit can only be used for Type II formatted data. The data
 |stream must contain enough information (in a header) to
 |separate the actual data from the padded zero bytes.

 Right, I've read this too, and we're not using Type II, so we don't have
 header information to tell us the real payload. The idea was to just use
 an 0 or 512 bytes approach.

 send 0-byte packets from agdev_iso_complete() if the time passed since
 the last completed buffer does not allow for another one to be sent.

 Audio Formats, 2.1:
 | To indicate a temporary stop in the isochronous data stream ..., an
 | in-band Transfer Delimiter needs to be defined.  This specification
 | considers two situations to be a Transfer Delimiter. The first is
 | a zero-length data packet and the second is the absence of an
 | isochronous transfer

 2.3.1.1:
 | The goal must be to keep the instantaneous number of audio slots per
 | virtual frame, ni as close as possible to the average number of audio
 | slots per virtual frame, nav. [...] If the sampling rate is a constant,
 | the allowable variation on ni is limited to one audio slot, that is,
 | ∆ni = 1. This implies that all virtual frame packets must either
 | contain INT(nav) audio slots (small VFP) or INT(nav) + 1 (large VFP)
 | audio slots.

 This results in very stable timing behavior in my tests.

 But it increases jitter, and might not work with any other driver.

 You're right, that's also my concern.

 f_uac(2) *must* implement some mechanism to control the clock, i.e., the
 packet sizes.  (And this is not part of the standard ALSA API.)

 The easiest is probably really to just calculate correct packet sizes
 and stick to them. After all, the actual clock is really arbitrary, we
 just have to pick something that is in the range of the sample rate.

 I'll cook up an alternative patch and do some tests with Sebastian off-list.

How about configuring bInterval and wMaxPacketSize to get the desired rate?

For ex, 48KHz/2/S16, we need 192bytes/millisec. So we set bInterval=1
(or 4 for HS) and wMaxPacketSize=192 for that configuration.  For
44.1KHz/2/S16 we need 176.4bytes/millisec, so we set
wMaxPacketSize=178 and send packets of length in {176, 176, 176,
176,178} pattern.

Regards,
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] usb: gadget: f_uac2: cleanups and capture timing

[Jassi Brar]

On Mon, Aug 25, 2014 at 2:57 PM, Daniel Mack dan...@zonque.org wrote:
 On 08/25/2014 11:23 AM, Jassi Brar wrote:
 On Mon, Aug 25, 2014 at 2:14 PM, Daniel Mack dan...@zonque.org wrote:

 The easiest is probably really to just calculate correct packet sizes
 and stick to them. After all, the actual clock is really arbitrary, we
 just have to pick something that is in the range of the sample rate.

 I'll cook up an alternative patch and do some tests with Sebastian off-list.

 How about configuring bInterval and wMaxPacketSize to get the desired rate?

 For ex, 48KHz/2/S16, we need 192bytes/millisec. So we set bInterval=1
 (or 4 for HS) and wMaxPacketSize=192 for that configuration.  For
 44.1KHz/2/S16 we need 176.4bytes/millisec, so we set
 wMaxPacketSize=178 and send packets of length in {176, 176, 176,
 176,178} pattern.

 Yes, something like that. But you can't modify wMaxPacketSize in
 accordance to the sample rate and format, but just send short packets.

We can't change rate once the f_uac2 module is loaded. So
wMaxPacketSize changes only across module loads.

Cheers,
-jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] usb: gadget: f_uac2: cleanups and capture timing

[Jassi Brar]

On Mon, Aug 25, 2014 at 3:07 PM, Daniel Mack dan...@zonque.org wrote:
 On 08/25/2014 11:30 AM, Jassi Brar wrote:
 On Mon, Aug 25, 2014 at 2:57 PM, Daniel Mack dan...@zonque.org wrote:
 On 08/25/2014 11:23 AM, Jassi Brar wrote:
 On Mon, Aug 25, 2014 at 2:14 PM, Daniel Mack dan...@zonque.org wrote:

 The easiest is probably really to just calculate correct packet sizes
 and stick to them. After all, the actual clock is really arbitrary, we
 just have to pick something that is in the range of the sample rate.

 I'll cook up an alternative patch and do some tests with Sebastian 
 off-list.

 How about configuring bInterval and wMaxPacketSize to get the desired rate?

 For ex, 48KHz/2/S16, we need 192bytes/millisec. So we set bInterval=1
 (or 4 for HS) and wMaxPacketSize=192 for that configuration.  For
 44.1KHz/2/S16 we need 176.4bytes/millisec, so we set
 wMaxPacketSize=178 and send packets of length in {176, 176, 176,
 176,178} pattern.

 Yes, something like that. But you can't modify wMaxPacketSize in
 accordance to the sample rate and format, but just send short packets.

 We can't change rate once the f_uac2 module is loaded. So
 wMaxPacketSize changes only across module loads.

 Yes, but we shouldn't rely on wMaxPacketSize but really just send
 packets of the right size. This is what other USB audio devices do as well.

Yup, that should do too.  Setting wMaxPacketSize to just enough length
seemed more optimized and robust to me. But I have not strong feelings
here.

 And btw - we could also change the logic of f_uac2 so the sample rate
 can be changed at runtime. The only constaint is that the counterpart
 device on the gadget side must not be active when this happens. Whatever
 part of the system comes up first (USB or ALSA) defines the sample rate
 and the format. But I'll save that for later :)

Properly supporting multiple sampling rates required a topology more
complicated than now, so I skipped it. IIRC some 'clock selector' unit
needs to be added to have a UAC2 compliant method to change rates from
Host side.

Thanks
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Mon, Aug 25, 2014 at 9:30 PM, Daniel Mack zon...@gmail.com wrote:
 The UAC2 function driver currently responds to all packets at all times
 with wMaxPacketSize packets. That results in way too fast audio
 playback as the function driver (which is in fact supposed to define
 the audio stream pace) delivers as fast as it can.

 Fix this by pre-calculating the size of each packet to meet the
 requested sample rate and format. This won't be 100% accurate, but
 that's acceptable.

For rates like 44100 we will always hear clicks because we can not
transfer 176400bytes by packets of equal size over duration of 1
second. The inaccuracy here is due to the way we program, and not due
to system/bus load.

Have you tried the approach I suggested - {4x176, 1x178} pattern
packets, and does that not work? Please let me know if I am
overlooking something. Otherwise let us do the best we can (If you
want me to give that a try, I can in a day or two).

 @@ -187,7 +189,7 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request 
 *req)

 if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
 src = prm-dma_area + prm-hw_ptr;
 -   req-actual = req-length;
 +   req-length = req-actual = uac2-c_pktsize;

This doesn't seem right.
We asked req-length to be transmitted by the udc which shouldn't
return until that is done. So at this point setting 'length' doesn't
mean much. The original assignment to 'actual' is only because we want
to ignore any issue that caused the udc to transmit fewer bytes (we
drop that data).

I believe you want to do the following in afunc_set_alt().
  -  req-length = prm-max_psize;
 +  req-length =  uac2-c_pktsize;

which should render the patch-4/4 needless, I hope because there is
nowhere 512 in the code and neither did I assume any such fixed value.
We simply alloc 2 usb requests of wMaxPacketSize each and copy data
to/from the ALSA ring buffer. For you the wMaxPacketSize might be 512,
but the code should work for any value.

Regards,
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Mon, Aug 25, 2014 at 10:52 PM, Jassi Brar jassisinghb...@gmail.com wrote:
 On Mon, Aug 25, 2014 at 9:30 PM, Daniel Mack zon...@gmail.com wrote:
 The UAC2 function driver currently responds to all packets at all times
 with wMaxPacketSize packets. That results in way too fast audio
 playback as the function driver (which is in fact supposed to define
 the audio stream pace) delivers as fast as it can.

 Fix this by pre-calculating the size of each packet to meet the
 requested sample rate and format. This won't be 100% accurate, but
 that's acceptable.

 For rates like 44100 we will always hear clicks because we can not
 transfer 176400bytes by packets of equal size over duration of 1
 second. The inaccuracy here is due to the way we program, and not due
 to system/bus load.

 Have you tried the approach I suggested - {4x176, 1x178} pattern
 packets, and does that not work? Please let me know if I am
 overlooking something. Otherwise let us do the best we can (If you
 want me to give that a try, I can in a day or two).

 @@ -187,7 +189,7 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request 
 *req)

 if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
 src = prm-dma_area + prm-hw_ptr;
 -   req-actual = req-length;
 +   req-length = req-actual = uac2-c_pktsize;

 This doesn't seem right.
 We asked req-length to be transmitted by the udc which shouldn't
 return until that is done. So at this point setting 'length' doesn't
 mean much. The original assignment to 'actual' is only because we want
 to ignore any issue that caused the udc to transmit fewer bytes (we
 drop that data).

 I believe you want to do the following in afunc_set_alt().
   -  req-length = prm-max_psize;
  +  req-length =  uac2-c_pktsize;

Sorry I intended...
   -  prm-max_psize = hs_epin_desc.wMaxPacketSize;
   + prm-max_psize = agdev-uac2.c_pktsize;


Also USB-IN is capture for host, but in f_uac2 it is playback. So you
may want to do

- rate = opts-c_srate * opts-c_ssize * num_channels(opts-c_chmask);
- rate = opts-p_srate * opts-p_ssize * num_channels(opts-p_chmask);

BTW, why not do the same for USB-OUT as well? it shouldn't hurt.

Thanks
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Mon, Aug 25, 2014 at 11:40 PM, Daniel Mack dan...@zonque.org wrote:
 Hi,

 On 08/25/2014 07:22 PM, Jassi Brar wrote:
 On Mon, Aug 25, 2014 at 9:30 PM, Daniel Mack zon...@gmail.com wrote:
 The UAC2 function driver currently responds to all packets at all times
 with wMaxPacketSize packets. That results in way too fast audio
 playback as the function driver (which is in fact supposed to define
 the audio stream pace) delivers as fast as it can.

 Fix this by pre-calculating the size of each packet to meet the
 requested sample rate and format. This won't be 100% accurate, but
 that's acceptable.

 For rates like 44100 we will always hear clicks because we can not
 transfer 176400bytes by packets of equal size over duration of 1
 second.

 How do you test to hear those clicks? If you do arecord | aplay on the
 host, you will have underruns or overruns at some point, because the
 internal sound interface of the host runs at a different speed than the
 gadget. This, however, also happens when you use any other USB sound
 card, and is hence it not surprising.

 It doesn't really matter if we transfer 176000 or 176400 bytes per
 second, measured by the host's time base. After all, the internal sound
 card may also be off by some percentage, depending on how it is built.
 We shouldn't be too far off though, as we currently are.

 The inaccuracy here is due to the way we program, and not due
 to system/bus load.

 Sure, but rates across devices will never match, so it doesn't matter
 really. Two clocks on two devices will always drift, even if they're
 totally accurate by their own means. You have the same situation the
 other way around on the playback endpoint: the host plays at whatever
 speed it considers to be 176400 bytes/sec, which will never be exactly
 176400 bytes/s measured by the gadget's clock, because there is no
 feedback endpoint. Audio applications have to be aware of that, and if
 they need to synchronize two devices with their own clock each, they
 have to implement some sort of resampler.

A high-end device, that consumes exactly 176400 bytes per second, on
host is piped data captured from f_uac2. However we write the code so
that f_uac2 can send only 176000 bytes every second.

Theoretically ruing out 'perfection' unsettles me. We can do better
and is not much trouble.

 Have you tried the approach I suggested - {4x176, 1x178} pattern
 packets, and does that not work? Please let me know if I am
 overlooking something. Otherwise let us do the best we can (If you
 want me to give that a try, I can in a day or two).

 That would only be necessary if you want the gadget's playback device to
 run absolutely in sync with its system clock. And there's no need for
 that IMO.

And if we want to provide exactly 176400 bytes of audio data every
second to the host.


 @@ -187,7 +189,7 @@ agdev_iso_complete(struct usb_ep *ep, struct 
 usb_request *req)

 if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
 src = prm-dma_area + prm-hw_ptr;
 -   req-actual = req-length;
 +   req-length = req-actual = uac2-c_pktsize;

 This doesn't seem right.
 We asked req-length to be transmitted by the udc which shouldn't
 return until that is done. So at this point setting 'length' doesn't
 mean much.

 That's right. I had it fixed already. Seems I staged the wrong version.
 Will fix in v3, thanks!

 which should render the patch-4/4 needless, I hope because there is
 nowhere 512 in the code and neither did I assume any such fixed value.

 The maxsize variables are initialized to the endpoint's wMaxPacketSize,
 which is 512. So your audio packets will go in slices of 512, and so
 they'll always fit nicely into the dma buffer, which has 64k.

 We simply alloc 2 usb requests of wMaxPacketSize each and copy data
 to/from the ALSA ring buffer. For you the wMaxPacketSize might be 512,
 but the code should work for any value.

 Exactly, but with 65536 bytes in the DMA buffer, and 176 bytes in each
 packet, you will have an uneven wrap around around each 372th packet, so
 we need to address these cases.

I see, thanks. That is a bug fix then, and probably should have been
patch-3/4 instead.

Regards,
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets

[Jassi Brar]

On Tue, Aug 26, 2014 at 1:38 AM, Daniel Mack dan...@zonque.org wrote:
 On 08/25/2014 09:00 PM, Jassi Brar wrote:
 On Mon, Aug 25, 2014 at 11:40 PM, Daniel Mack dan...@zonque.org wrote:

 Sure, but rates across devices will never match, so it doesn't matter
 really. Two clocks on two devices will always drift, even if they're
 totally accurate by their own means. You have the same situation the
 other way around on the playback endpoint: the host plays at whatever
 speed it considers to be 176400 bytes/sec, which will never be exactly
 176400 bytes/s measured by the gadget's clock, because there is no
 feedback endpoint. Audio applications have to be aware of that, and if
 they need to synchronize two devices with their own clock each, they
 have to implement some sort of resampler.

 A high-end device, that consumes exactly 176400 bytes per second, on
 host is piped data captured from f_uac2. However we write the code so
 that f_uac2 can send only 176000 bytes every second.

 Theoretically ruing out 'perfection' unsettles me. We can do better
 and is not much trouble.

 Alright, you're right. I'll cook something up to alternate the sizes in
 order to get closer. Will be part of v3.

Cool... we can avoid runtime calculations by maybe picking the
pre-defined 'length pattern' at module load time to match the rate
selected. And have those many usb requests allocated and their
'length' initialized to the pattern. Then the rest of code would
remain unchanged. Though I won't be surprised if you have some better
idea.


 Exactly, but with 65536 bytes in the DMA buffer, and 176 bytes in each
 packet, you will have an uneven wrap around around each 372th packet, so
 we need to address these cases.

 I see, thanks. That is a bug fix then, and probably should have been
 patch-3/4 instead.

 It hasn't been a problem since, but only after the packet size change.
 But I can swap them around, no problem :)

Thanks, Its just the we wouldn't want the fix to get delayed with the
data-rate fix patch. Also maybe stable should be CC'ed on that patch?

Regards,
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is the command line commands to use UAC2 at USB client side?

[Jassi Brar]

On Tue, Aug 19, 2014 at 2:15 PM, Daniel Mack dan...@zonque.org wrote:
 On 08/19/2014 02:01 AM, Xuebing Wang wrote:

 root@imx6slevk:~#
 root@imx6slevk:~# arecord -f dat -t wav -D hw:2,0 | aplay -D hw:0,0
 Recording WAVE 'stdin' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo
 Playing WAVE 'stdin' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo
 Such a setup should work, I recently tried it myself.

 The other direction (capturing from host, playback on device), however,
 has a major problem as the device interface has no timing mechanism, and
 hence 'arecord | aplay' on the gadget side fails. I've prepared patches
 and a more comprehensive description for this, but I'm waiting for
 Andrzej's patches to be reviewed, as mine are based upon his.

Hmm... I tested 48KHz USB-IN without noise, 44.1KHz did show noise
though ... iirc

 Thanks. You were trying UAC2, right?

 Yes.

However if used with defaults(i.e, 64KHz USB-OUT and 48KHz USB-IN)
there should be no noise because samples divide up into frame nicely.

 Does it work with Windows 7/8 host?

 I have no idea, and no Windows box to test on.

IIRC Windows doesn't have native support for UAC2.

-jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is the command line commands to use UAC2 at USB client side?

[Jassi Brar]

On Tue, Aug 19, 2014 at 2:39 PM, Daniel Mack dan...@zonque.org wrote:
 On 08/19/2014 11:01 AM, Jassi Brar wrote:
 On Tue, Aug 19, 2014 at 2:15 PM, Daniel Mack dan...@zonque.org wrote:
 On 08/19/2014 02:01 AM, Xuebing Wang wrote:

 root@imx6slevk:~#
 root@imx6slevk:~# arecord -f dat -t wav -D hw:2,0 | aplay -D hw:0,0
 Recording WAVE 'stdin' : Signed 16 bit Little Endian, Rate 48000 Hz, 
 Stereo
 Playing WAVE 'stdin' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo
 Such a setup should work, I recently tried it myself.

 The other direction (capturing from host, playback on device), however,
 has a major problem as the device interface has no timing mechanism, and
 hence 'arecord | aplay' on the gadget side fails. I've prepared patches
 and a more comprehensive description for this, but I'm waiting for
 Andrzej's patches to be reviewed, as mine are based upon his.

 Hmm... I tested 48KHz USB-IN without noise, 44.1KHz did show noise
 though ... iirc

 With USB-IN, you're referring to arecord on the host side, and aplay on
 the gadget? Playing/ recording wave files on both sides worked fine for
 me. The only problem here is that once you link one side to a sink or
 source that expects audio to be transported at least roughly with the
 announces sample rate, things break because there is nothing that
 controls the timing. It's easy to fix, and as I said, I have patches for
 this that I'll send out shortly.

Its been quite some time now, but I think we designed the uac2 to rely
on USB's ISO packets' rate control to send and receive audio data at
announced sampling rate. For some rates though the data rate doesn't
become a multiple of packetsize and we see a periodic 'click' noise.
Probably we are talking about the same thing. Please try to CC me on
your patches.

 However, I thought Xuebing's setup is the other way around, right?

Yes, Xuebing tested USB-OUT.

Cheers,
-Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is the command line commands to use UAC2 at USB client side?

[Jassi Brar]

On Thu, Aug 14, 2014 at 12:05 PM, Xuebing Wang xbi...@gmail.com wrote:
 Hi Community,

 Based on Freescale platform , I am trying to use USB Audio Class version
 2.0.  Host can detect this UAC2 device.

 At device side (after modprobe g_audio):

 root@imx6slevk:~# aplay -l
  List of PLAYBACK Hardware Devices 
 card 0: wm8962audio [wm8962-audio], device 0: HiFi wm8962-0 []
   Subdevices: 1/1
   Subdevice #0: subdevice #0
 card 1: imxspdif [imx-spdif], device 0: S/PDIF PCM Playback dit-hifi-0 []
   Subdevices: 1/1
   Subdevice #0: subdevice #0
 card 2: UAC2Gadget [UAC2_Gadget], device 0: UAC2 PCM [UAC2 PCM]
   Subdevices: 1/1
   Subdevice #0: subdevice #0

 At device side, in driver/usb/gadget/Kconfig, it says before for
 CONFIG_USB_AUDIO
 -
   This driver doesn't expect any real Audio codec to be present
   on the device - the audio streams are simply sinked to and
   sourced from a virtual ALSA sound card created. The user-space
   application may choose to do whatever it wants with the data
   received from the USB Host and choose to provide whatever it
   wants as audio data to the USB Host.
 -

 My question is: at device side, how to configure sink/source for UAC2
 virtual ALSA sound card (hw:2,0, UAC2_Gadget)?

At OS level, select the UAC2 card like any other - Control Panel -
Sound settings. You want to select the same Card on Host as well as
Gadget side.

-Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is the command line commands to use UAC2 at USB client side?

[Jassi Brar]

On Fri, Aug 15, 2014 at 7:55 AM, Xuebing Wang xbi...@gmail.com wrote:

 On 08/15/2014 08:55 AM, Peter Chen wrote:



 At device side, in driver/usb/gadget/Kconfig, it says before for
 CONFIG_USB_AUDIO
 -
 This driver doesn't expect any real Audio codec to be

 present

 on the device - the audio streams are simply sinked to and
 sourced from a virtual ALSA sound card created. The user-

 space

 application may choose to do whatever it wants with the

 data

 received from the USB Host and choose to provide whatever

 it

 wants as audio data to the USB Host.
 -

 My question is: at device side, how to configure sink/source for UAC2
 virtual ALSA sound card (hw:2,0, UAC2_Gadget)?

 At OS level, select the UAC2 card like any other - Control Panel -
 Sound settings. You want to select the same Card on Host as well as
 Gadget side.

 -Jassi

 Thanks for your reply. At the device side, should I do something to
 redirect the virtual sound card to a real one?
 --

 I don't think you need, did you meet any problems?

 Peter


 Thanks Peter.

 My problem is that I can not hear any sound if playing wav from Ubuntu Host.

 I can hear sound if I play wav for the real sound card at the device side.

 For UAC1 (file u_uac1.c), usb audio stream is hard-coded to be directed to
 /dev/snd/pcmC0D0p. I am wondering for UAC2, should I do the similar?

That is actually a bad thing about UAC1. The piping should be done at
user level.
Please google for how to pipe output of 'arecord' into 'aplay'.
(BTW, there is nothing UAC specific about that)

-Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Work on Usb subsystem

[Jassi Brar]

On Sun, Aug 3, 2014 at 11:01 PM, Nick Krause xerofo...@gmail.com wrote:
 On Sun, Aug 3, 2014 at 4:50 AM, Jassi Brar jassisinghb...@gmail.com wrote:
 On Sun, Aug 3, 2014 at 11:07 AM, Nick Krause xerofo...@gmail.com wrote:
 Hey Guys,
 I assume after my other fuck ups , you assume I can't do kernel work
 and I understand. But if you want to give me a second case
 and allow me to help out with the usb subsystem that would be great too.

 Why do you need someone to 'allow' you to do something? If you find
 some bug or see some feature missing, feel free to send in the
 patches. Just don't expect them to be always accepted and be ready to
 take lessons gratefully. Before that you need to do your homework
 because a bunch of bad patches could dent the credibility just as good
 ones build it up.

 HTH,
 -Jassi

 Thanks Jassi,
 I was just wondering where to start, that's all.

Usually people start by understanding the subsystem they are
interested in. For ex, you may start by understanding how USB works,
how Linux's USB stack is organised, try to trace the enumeration
process and later flow of control (in the process learn about
tracering and debugging techniques in kernel). You may not have
written the code but you could still understand it well. If you want
to work upstream, read linux/Documentation/ and mailing-lists
live/archived ... esp mails from gods of relevant subsystem. People
may not always have time to personally explain things to you again.
 If you use Linux USB enough most probably you'll come across some
device that has issues with Linux or, even more likely, the Linux
driver available for the device is against some old version of Linux
which you may try to upgrade and _test_ and then submit a patch. I am
sure there is some todo-list online as well.
 Of course all this assumes one has good grasp on basic programming
and debugging techniques before starting out.

Cheers,
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] usb: gadget: f_uac1: Staticize local functions

[Jassi Brar]

On 5 August 2013 08:45, Jingoo Han jg1@samsung.com wrote:
 control_selector_init() is used only in this file.
 audio_bind_config() is used only in audio.c file to which
 f_uac1.c is included. Thus, these functions are staticized
 to fix the following warnings.

 drivers/usb/gadget/f_uac1.c:698:12: warning: symbol 'control_selector_init' 
 was not declared. Should it be static?
 drivers/usb/gadget/f_uac1.c:722:12: warning: symbol 'audio_bind_config' was 
 not declared. Should it be static?

 Signed-off-by: Jingoo Han jg1@samsung.com

Acked-by: Jassi Brar jaswinder.si...@linaro.org

 ---
  drivers/usb/gadget/f_uac1.c |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/drivers/usb/gadget/f_uac1.c b/drivers/usb/gadget/f_uac1.c
 index fa8ea4e..2b4c82d 100644
 --- a/drivers/usb/gadget/f_uac1.c
 +++ b/drivers/usb/gadget/f_uac1.c
 @@ -695,7 +695,7 @@ static int generic_get_cmd(struct usb_audio_control *con, 
 u8 cmd)
  }

  /* Todo: add more control selecotor dynamically */
 -int __init control_selector_init(struct f_audio *audio)
 +static int __init control_selector_init(struct f_audio *audio)
  {
 INIT_LIST_HEAD(audio-cs);
 list_add(feature_unit.list, audio-cs);
 @@ -719,7 +719,7 @@ int __init control_selector_init(struct f_audio *audio)
   *
   * Returns zero on success, else negative errno.
   */
 -int __init audio_bind_config(struct usb_configuration *c)
 +static int __init audio_bind_config(struct usb_configuration *c)
  {
 struct f_audio *audio;
 int status;
 --
 1.7.10.4





-- 
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] usb: gadget: u_uac1: add __user annotation

[Jassi Brar]

On 5 August 2013 08:44, Jingoo Han jg1@samsung.com wrote:
 Added __user annotation to fix the following sparse warning.

 drivers/usb/gadget/u_uac1.c:194:52: warning: incorrect type in argument 2 
 (different address spaces)
 drivers/usb/gadget/u_uac1.c:194:52:expected void const [noderef] 
 asn:1*buf
 drivers/usb/gadget/u_uac1.c:194:52:got void *buf

 Signed-off-by: Jingoo Han jg1@samsung.com

Acked-by: Jassi Brar jaswinder.si...@linaro.org

 ---
  drivers/usb/gadget/u_uac1.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/drivers/usb/gadget/u_uac1.c b/drivers/usb/gadget/u_uac1.c
 index c7d460f..7a55fea 100644
 --- a/drivers/usb/gadget/u_uac1.c
 +++ b/drivers/usb/gadget/u_uac1.c
 @@ -191,7 +191,7 @@ try_again:
 frames = bytes_to_frames(runtime, count);
 old_fs = get_fs();
 set_fs(KERNEL_DS);
 -   result = snd_pcm_lib_write(snd-substream, buf, frames);
 +   result = snd_pcm_lib_write(snd-substream, (void __user *)buf, 
 frames);
 if (result != frames) {
 ERROR(card, Playback error: %d\n, (int)result);
 set_fs(old_fs);
 --
 1.7.10.4





-- 
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: MUSB multiplatform work?

[Jassi Brar]

On Mon, Jun 17, 2013 at 7:13 PM, Vinod Koul vinod.k...@intel.com wrote:
 On Wed, Jun 12, 2013 at 05:27:53PM +0530, Jassi Brar wrote:
 IIRC,  TI's Sundaram Raju proposed a TI specific api to DMAEngine for
 this very purpose, which was generalized into
 device_prep_interleaved_dma().  Which I think should already be enough
 to serve the purpose. Is it not?
 The interleaved for having to get/set data from interleaved or an 2d array.
 Think of a raw image from camera where you need to get some region only and 
 skip
 rest. In those case interleaved API helps

IIRC I designed the interleaved api ;)  and I am sure it was not
designed solely for async, rather the first users of the api are
slave.

 Here this is just normal slave DMA with changing FIFO address and which just
 loops over the FIFO value

It is possible that I didn't understand the OMAP usecase and the
interleave api won't work, but if it does work please let us not
introduce yet another api for a 'normal' usecase.

In fact my bigger plan was to call the api 'generic' and have it
eventually consume most, if not all, dma_transaction_type's but then
we couldn't see the same and I got busy with non-dma stuff ...
anyways.

cheers.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: MUSB multiplatform work?

[Jassi Brar]

On 12 June 2013 15:35, Vinod Koul vinod.k...@intel.com wrote:
 On Thu, May 30, 2013 at 11:19:33PM +0200, Linus Walleij wrote:
 On Thu, May 30, 2013 at 10:31 PM, Tony Lindgren t...@atomide.com wrote:

  There are many devices where the device FIFO is memory mapped to the
  GPMC bus on omaps, like TUSB, OneNAND, smc911x etc. I believe the
  only reason why these have not been converted to the dmaengine is
  the fact that dmaengine cannot configure the DMA hardware to do
  autoincrement and loop over the device FIFO.

 OK that seems like something pretty generic that we could just add
 to the struct dma_slave_config actually, something like:

 u32 src_fifoloop;
 u32 dst_fifoloop;
 Yes for genric but not for loop. For most of our cases we are considering the
 FIFO address as a constant. Typically DMA controllers have this ability to
 perform incremental/decremental/constant FIFO address.

 What would make sense to have is:

 enum dmaengine_slave_addr_mode {
 DMAENGINE_SLAVE_ADDR_CONSTANT = 0,
 DMAENGINE_SLAVE_ADDR_INCREMANT,
 DMAENGINE_SLAVE_ADDR_DECREMENT,
 };

 and add these to struct dma_slave_config:
 enum dmaengine_slave_addr_mode src_mode;
 enum dmaengine_slave_addr_mode dstn_mode;

 For loopover we can have:
 u32 loop_counter;

 on 0 means no loop, and valid value tells when to loopover (offset).

 will this help for all of the above controllers and their conversion?

IIRC,  TI's Sundaram Raju proposed a TI specific api to DMAEngine for
this very purpose, which was generalized into
device_prep_interleaved_dma().  Which I think should already be enough
to serve the purpose. Is it not?

-jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: MUSB multiplatform work?

[Jassi Brar]

On 31 May 2013 02:01, Tony Lindgren t...@atomide.com wrote:
 * Linus Walleij linus.wall...@linaro.org [130530 13:27]:
 On Thu, May 30, 2013 at 10:18 PM, Tony Lindgren t...@atomide.com wrote:

  TUSB would work with dmaengine, but AFAIK we're still missing the
  dmaengine configuration options to support increasing device end
  FIFO address.

 Can you elaborate on this? What is the usecase here?

 There are many devices where the device FIFO is memory mapped to the
 GPMC bus on omaps, like TUSB, OneNAND, smc911x etc. I believe the
 only reason why these have not been converted to the dmaengine is
 the fact that dmaengine cannot configure the DMA hardware to do
 autoincrement and loop over the device FIFO.

Doesn't device_prep_interleaved_dma() help?

-jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: gadget: f_uac2: Fix broken prm to uac2 mapping

[Jassi Brar]

On Sat, Jun 1, 2013 at 2:47 AM, Felipe Balbi ba...@ti.com wrote:
 HI,

 On Thu, May 30, 2013 at 06:23:33PM +0530, Jassi Brar wrote:
 From: Jassi Brar jaswinder.si...@linaro.org

 prm_to_uac2() is broken because it tests against pointer it itself
 mapped onto, which will never be different.
 Fix the mapping by adding pointer to parent chip in each rtd param
 and removing the prm_to_uac2().

 Reported-by: Julien Rouviere jrouvi...@qualistream.com
 Signed-off-by: Jassi Brar jaswinder.si...@linaro.org

 when was this bug introduced ? Is it critical for v3.10 ? can it wait
 for v3.11 ? Does it need to go to stable ?

The bug has already been there. It is hit only in rare error path and
is not critical.

Thanks,
Jassi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] USB: gadget: f_uac2: Fix broken prm to uac2 mapping

[Jassi Brar]

From: Jassi Brar jaswinder.si...@linaro.org

prm_to_uac2() is broken because it tests against pointer it itself
mapped onto, which will never be different.
Fix the mapping by adding pointer to parent chip in each rtd param
and removing the prm_to_uac2().

Reported-by: Julien Rouviere jrouvi...@qualistream.com
Signed-off-by: Jassi Brar jaswinder.si...@linaro.org
---
 drivers/usb/gadget/f_uac2.c |   20 ++--
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/f_uac2.c b/drivers/usb/gadget/f_uac2.c
index c7468b6..2c858a8 100644
--- a/drivers/usb/gadget/f_uac2.c
+++ b/drivers/usb/gadget/f_uac2.c
@@ -90,6 +90,7 @@ struct uac2_req {
 };
 
 struct uac2_rtd_params {
+   struct snd_uac2_chip *uac2; /* parent chip */
bool ep_enabled; /* if the ep is enabled */
/* Size of the ring buffer */
size_t dma_bytes;
@@ -169,18 +170,6 @@ struct snd_uac2_chip *pdev_to_uac2(struct platform_device 
*p)
 }
 
 static inline
-struct snd_uac2_chip *prm_to_uac2(struct uac2_rtd_params *r)
-{
-   struct snd_uac2_chip *uac2 = container_of(r,
-   struct snd_uac2_chip, c_prm);
-
-   if (uac2-c_prm != r)
-   uac2 = container_of(r, struct snd_uac2_chip, p_prm);
-
-   return uac2;
-}
-
-static inline
 uint num_channels(uint chanmask)
 {
uint num = 0;
@@ -204,7 +193,7 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request 
*req)
struct uac2_req *ur = req-context;
struct snd_pcm_substream *substream;
struct uac2_rtd_params *prm = ur-pp;
-   struct snd_uac2_chip *uac2 = prm_to_uac2(prm);
+   struct snd_uac2_chip *uac2 = prm-uac2;
 
/* i/f shutting down */
if (!prm-ep_enabled)
@@ -896,7 +885,7 @@ struct cntrl_range_lay3 {
 static inline void
 free_ep(struct uac2_rtd_params *prm, struct usb_ep *ep)
 {
-   struct snd_uac2_chip *uac2 = prm_to_uac2(prm);
+   struct snd_uac2_chip *uac2 = prm-uac2;
int i;
 
prm-ep_enabled = false;
@@ -972,6 +961,9 @@ afunc_bind(struct usb_configuration *cfg, struct 
usb_function *fn)
}
agdev-in_ep-driver_data = agdev;
 
+   uac2-p_prm.uac2 = uac2;
+   uac2-c_prm.uac2 = uac2;
+
hs_epout_desc.bEndpointAddress = fs_epout_desc.bEndpointAddress;
hs_epout_desc.wMaxPacketSize = fs_epout_desc.wMaxPacketSize;
hs_epin_desc.bEndpointAddress = fs_epin_desc.bEndpointAddress;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] usb: gadget: f_uac2: Remove redundant platform_set_drvdata()

[Jassi Brar]

On 6 May 2013 17:07, Sachin Kamat sachin.ka...@linaro.org wrote:
 Commit 0998d06310 (device-core: Ensure drvdata = NULL when no
 driver is bound) removes the need to set driver data field to
 NULL.

 Signed-off-by: Sachin Kamat sachin.ka...@linaro.org
 Cc: Jaswinder Singh jaswinder.si...@linaro.org

Acked-by: Jaswinder Singh jaswinder.si...@linaro.org
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 23/23] mfd: omap-usb-host: Don't spam console on clk_set_parent failure

[Jassi Brar]

On 5 December 2012 19:43, Roger Quadros rog...@ti.com wrote:
 On 12/05/2012 03:42 PM, Sergei Shtylyov wrote:
 Hello.

 On 04-12-2012 18:31, Roger Quadros wrote:

 clk_set_parent is expected to fail on OMAP3 platforms. We don't
 consider that as fatal so don't spam console.

 Signed-off-by: Roger Quadros rog...@ti.com
 ---
   drivers/mfd/omap-usb-host.c |   18 +-
   1 files changed, 9 insertions(+), 9 deletions(-)

 diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
 index 0bb54393..e5257dc 100644
 --- a/drivers/mfd/omap-usb-host.c
 +++ b/drivers/mfd/omap-usb-host.c
 @@ -657,32 +657,32 @@ static int __devinit usbhs_omap_probe(struct
 platform_device *pdev)
   }

   if (is_ehci_phy_mode(pdata-port_mode[0])) {
 -/* for OMAP3 , the clk set paretn fails */
 +/* for OMAP3, clk_set_parent fails */
   ret = clk_set_parent(omap-utmi_clk[0],
   omap-xclk60mhsp1_ck);
   if (ret != 0)
 -dev_err(dev, xclk60mhsp1_ck set parent
 -failed error:%d\n, ret);
 +dev_dbg(dev, xclk60mhsp1_ck set parent failed : %d\n,
 +ret);
   } else if (is_ehci_tll_mode(pdata-port_mode[0])) {
   ret = clk_set_parent(omap-utmi_clk[0],
   omap-init_60m_fclk);
   if (ret != 0)
 -dev_err(dev, init_60m_fclk set parent
 -failed error:%d\n, ret);
 +dev_dbg(dev, P0 init_60m_fclk set parent failed: %d\n,
 +ret);
   }

   if (is_ehci_phy_mode(pdata-port_mode[1])) {
   ret = clk_set_parent(omap-utmi_clk[1],
   omap-xclk60mhsp2_ck);
   if (ret != 0)
 -dev_err(dev, xclk60mhsp2_ck set parent
 -failed error:%d\n, ret);
 +dev_dbg(dev, xclk60mhsp2_ck set parent failed : %d\n,
 +ret);
   } else if (is_ehci_tll_mode(pdata-port_mode[1])) {
   ret = clk_set_parent(omap-utmi_clk[1],
   omap-init_60m_fclk);
   if (ret != 0)
 -dev_err(dev, init_60m_fclk set parent
 -failed error:%d\n, ret);
 +dev_dbg(dev, P1 init_60m_fclk set parent failed: %d\n,
 +ret);

Hm, you sometimes put a space before colon in the error message and
 sometimes not. Inconsistent. :-)


 That was because it fit in 80 characters without the space. I'm not sure
 what is more important, fitting in 80 or consistency in the print
 message. Maybe i should have removed the spaces everywhere so that it is
 consistent as well. :)

prints are one thing where breaking the 80 column rule is acceptable.
Otherwise it fails grep'ing
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Jassi Brar]

On Mon, Dec 10, 2012 at 3:18 PM, Roger Quadros rog...@ti.com wrote:
 On 12/06/2012 04:34 PM, Jassi Brar wrote:

 Yes, this is where we can think of a generic PHY driver to make sure thy
 PHY has necessary resources enabled. In the Panda case it would be the
 PHY clock and RESET gpio.

 I wonder if ehci-omap should assume, besides regulator, a clock might
 also be need to setup for the transceiver chip.
 Maybe usbhs_bdata in board-omap4panda.c should have
 .reset_gpio_port[0]  = GPIO_HUB_NRESET ?


 Just like the regulator, reset_gpio_port has nothing to do with OMAP
 EHCI. So we would want to get rid of that too from the OMAP USB driver.

Looking at the code I realized we already book resources only for
populated ports. Saving power from LAN9514 chip would come from a
separate solution. So, for now when our transceiver, USB3320, has
simple hardwired configuration probably just platform_data/DT would
do. When we come across some programmable transceiver (like USB3503
over i2c), that might warrant a separate PHY driver. Still I don't
think we could have a 'generic' phy driver.


 The LAN95xx chip still needs to be powered up and the PHY driver isn't
 the right place for that (though it could be done there as a hack). The
 closest we can get to emulating right USB behavior is to map the
 SET/CLEAR PORT_FEATURE of the root hub port to the regulator that powers
 the LAN95xx.

 This way, we still don't fall in the dynamically probed space, so we
 could still provide the regulator information to the ehci hub via
 platform data and handle the regulator in ehci_hub_control
 (Set/ClearPortFeature). I'm looking at this as a software workaround for
 all platforms not implementing the EHCI set port power feature
 correctly. The same could be repeated for other HCDs as well if required.

 IMHO it's not a matter of platforms not implementing EHCI set port
 power feature correctly, we should think about onboard devices
 connected to onboard non-root hubs. Setups like LAN9514 on Panda (HSIC
 too ?) that don't run on VBUS of USB, would like their local supply to
 be treated as if it came from the parent hub's port  i.e, tie together
 the USB's set port power control with their OOB regulated power supply
 (U9 on PandaBoard).


 On Pandaboards we are still talking about root hub ports.

 Do you know any of such platforms which power their USB devices out of
 band for _non_ root hub ports?

I don't know of any. But I do believe we shouldn't discount that scenario.
IIANW lan9514 doesn't take in VBUS because it wants to avoid 5V-3.3V
regulating. What if someone designs an omap4 platform with 3
high-speed devices and the same concern in mind ?

 Assuming they do exist, the only solution is to match platform data for
 dynamically probed devices and deal with the regulators in the hub/port
 driver. Something like Andy already proposed.

Yes, but I doubt if that is the only implementation.
One USB specific solution could be to abstract out OOB port power
control in, say, port-power.c which constructs a regulator topology
mapped directly onto onboard devices', from a generic DT binding,
platform_data, ACPI whatever. And then catch any
set/clear_port_feature(POWER) call to enable/disable the regulator
corresponding to that port. Where each regulator could be a
board-specific virtual one, that does all that is necessary (like
clock/reg hierarchy setup) to power up the device.

Regards.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Jassi Brar]

On Wed, Dec 5, 2012 at 7:39 PM, Roger Quadros rog...@ti.com wrote:
 Hi Jassi,

 On 12/01/2012 09:49 AM, Jassi Brar wrote:
 On Tue, Nov 27, 2012 at 10:32 PM, Greg KH gre...@linuxfoundation.org wrote:
 On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote:

 We should have a more generic solution in a more central location, like
 the device core.

 For example, each struct platform_device could have a list of resources
 to be enabled when the device is bound to a driver and disabled when
 the device is unbound.  Such a list could include regulators, clocks,
 and whatever else people can invent.

 In this case, when it is created the ehci-omap.0 platform device could
 get an entry pointing to the LAN95xx's regulator.  Then the regulator
 would automatically be turned on when the platform device is bound to
 the ehci-omap driver.

 How does this sound?

 That sounds much better, and it might come in handy for other types of
 devices than platform devices, so feel free to put this on the core
 'struct device' instead if needed.

 Isn't enabling/disabling of clocks and regulators what
 dev.probe()/remove() of any driver already does?
 If we mean only board specific setup/teardown, still it would mean
 having the device core to do an extra 'probe' call when the current
 probe() is already meant to do whatever it takes to bring the device
 up. To my untrained eyes, it seem like messing with the
 probe()/remove()'s semantics.
  IMHO, if we really must implement something like that, may be we
 should employ some 'broadcast mechanism' so that anything anywhere in
 kernel knows which new device has been probed()/removed()
 successfully. I haven't thought exactly how because I am not sure even
 that would be the right way to approach PandaBoard's problem.

 Looking at Figure 15 – Panda USBB1 Interface Block Diagram of
 http://pandaboard.org/sites/default/files/board_reference/pandaboard-es-b/panda-es-b-manual.pdf
 gives me visions ...

  1) OMAP doesn't provide the USB root-hub, but only ULPIPHY. It is
 USB3320C chip that employs OMAP's ULPIPHY to provide the root-hub. So
 we should have a platform device for USB3320C, the probe() of which
 should simply

 Actually the EHCI controller within OMAP provides the root hub with 3
 ports but no PHY. One of them is connected to the USB3320 which is just
 a ULPI PHY.

a) Enable REFCLK (FREF_CLK3_OUT)
b) Reset itself by cycling RESETB (GPIO_62)
c) Create one ehci-omap platform device

 c) is not appropriate. ehci-omap must be created before usb3320.

  (or in appropriate order if the above isn't)
 That way insmod/rmmod'ing the USB3320C driver would power-up/down the
 whole subsystem.

 Yes, this is where we can think of a generic PHY driver to make sure thy
 PHY has necessary resources enabled. In the Panda case it would be the
 PHY clock and RESET gpio.

I wonder if ehci-omap should assume, besides regulator, a clock might
also be need to setup for the transceiver chip.
Maybe usbhs_bdata in board-omap4panda.c should have
.reset_gpio_port[0]  = GPIO_HUB_NRESET ?


 The LAN95xx chip still needs to be powered up and the PHY driver isn't
 the right place for that (though it could be done there as a hack). The
 closest we can get to emulating right USB behavior is to map the
 SET/CLEAR PORT_FEATURE of the root hub port to the regulator that powers
 the LAN95xx.

 This way, we still don't fall in the dynamically probed space, so we
 could still provide the regulator information to the ehci hub via
 platform data and handle the regulator in ehci_hub_control
 (Set/ClearPortFeature). I'm looking at this as a software workaround for
 all platforms not implementing the EHCI set port power feature
 correctly. The same could be repeated for other HCDs as well if required.

IMHO it's not a matter of platforms not implementing EHCI set port
power feature correctly, we should think about onboard devices
connected to onboard non-root hubs. Setups like LAN9514 on Panda (HSIC
too ?) that don't run on VBUS of USB, would like their local supply to
be treated as if it came from the parent hub's port  i.e, tie together
the USB's set port power control with their OOB regulated power supply
(U9 on PandaBoard).

cheers.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] Device Power: introduce power controller

[Jassi Brar]

On 6 December 2012 18:48, Ming Lei tom.leim...@gmail.com wrote:
 On Thu, Dec 6, 2012 at 11:46 AM, Jassi Brar jaswinder.si...@linaro.org 
 wrote:
 The patch can make usb port deal with the 'power controller' only, and make 
 it
 avoid to deal with regulators/clocks/... directly.

 I am curious too, except for clocks and voltage supplies (regulators),
 what other external resources does a chip need ?

 For example, one indicator LED which doesn't connect to the same power
 domain might need to be triggered after the power switch state is changed.

Hmm.. I am not sure if we could call an indicator LED a resource that
a chip needs to be functional. Isn't how mmc core manages the
indicator led, a better way?

cheers.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Random MAC address from smsc75xx: How to permanently set?

[Jassi Brar]

On Fri, Dec 7, 2012 at 12:21 AM, Dan Williams d...@redhat.com wrote:
 On Thu, 2012-12-06 at 12:44 -0600, Dan Williams wrote:
 On Thu, 2012-12-06 at 18:35 +, Cunningham, Robert wrote:
  I'm trying to bring up an OMAP4 system based on Variscite's OM44 module 
  running Linaro's Ubuntu Precise in a headless configuration.  The system 
  contains two SMSC LAN7500 USB-GigE chips (not dongles), both of which are 
  fully functional.
 
  The GigE chips don't have EEPROMS, so no permanent MAC addresses can be 
  assigned in hardware.  As expected, the smsc75xx driver assigns a random 
  MAC address when the interface is discovered and initialized.  However, we 
  need to provide consistent MAC addresses on these interfaces.  (Yes, we 
  could respin the board to add EEPROMS, but that's a last, and expensive, 
  resort.)
 
  After the system boots, I'd like to change the MAC addresses to specific 
  values.  While there are multiple ways to do this (using commands such as 
  ifconfig, ip, macchanger, and others), it seems the updated MAC address is 
  always overridden when I do ifconfig ethX up, which causes yet another 
  different random MAC address to be created and assigned.  Simply repeating 
  ifconfig up/down causes an endless list of random MAC addresses to be 
  generated.
 
  I created a udev rule that I hoped would handle the situation, but it is 
  also overridden:
  /etc/udev/rules.d/99-mac-address.rules:
  SUBSYSTEM==net, KERNEL==eth0, RUN+=/sbin/ip link set dev %k 
  address XX:XX:XX:XX:XX:00
  SUBSYSTEM==net, KERNEL==eth1, RUN+=/sbin/ip link set dev %k 
  address XX:XX:XX:XX:XX:01
 
  For a single interface, I can use u-boot variables or kernel boot 
  arguments, but they seem to work only for the first interface, and I have 
  two.

 Just matching on interface name won't guarantee that you get the same
 MAC assigned to the same physical interface each time you boot.  You
 can't rely on bus probing order.  What you really want to do is enhance
 the udev rule to match the network interfaces based on stuff like PCI
 bus address, SDIO details, or whatever bus type the interfaces are
 hanging off.  If the device is a virtual one because it's on-chip or
 something like that, then you're at the mercy of the driver's probing
 code because it probably doesn't expose any unique details of the
 device.  This all doesn't have anything to do with the MAC potentially
 being overwritten by something later, but just beware that device
 probing order is *not* generally reliable, which means that interface
 names can and do get reordered.

 Just noticed this was sent only to linux-usb, so I'll assume we're
 talking about USB here.  Unfortunately, USB probing order is even *less*
 reliable than PCI and other types.  So the only thing you can do here is
 hope that the manufacturer set a unique serial number on the network
 device (use lsusb -v to find it) and then you can use that to lock the
 udev rules to that specific device.  If not, you're hosed and you'll
 never get completely reliable mapping between the network interface and
 the MAC address you're trying to set.

Or the usb device path of lan7500 chips onboard, which would remain
same across reboots ?
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] Device Power: introduce power controller

[Jassi Brar]

On 6 December 2012 06:57, Ming Lei tom.leim...@gmail.com wrote:
 On Thu, Dec 6, 2012 at 12:49 AM, Roger Quadros rog...@ti.com wrote:
 On 12/03/2012 05:00 AM, Ming Lei wrote:
 On Mon, Dec 3, 2012 at 12:02 AM, Andy Green andy.gr...@linaro.org wrote:
 On 02/12/12 23:01, the mail apparently from Ming Lei included:

 Power controller is an abstract on simple power on/off switch.

 One power controller can bind to more than one device, which
 provides power logically, for example, we can think one usb port
 in hub provides power to the usb device attached to the port, even
 though the power is supplied actually by other ways, eg. the usb
 hub is a self-power device. From hardware view, more than one
 device can share one power domain, and power controller can power
 on if one of these devices need to provide power, and power off if
 all these devices don't need to provide power.


 What stops us using struct regulator here?  If you have child regulators
 supplied by a parent supply, isn't that the right semantic already without
 introducing a whole new thing?  Apologies if I missed the point.

 There are two purposes:

 One is to hide the implementation details of the power controller because
 the user doesn't care how it is implemented, maybe clock, regulator, gpio
 and other platform dependent stuffs involved, so the patch simplify the 
 usage
 from the view of users.


 Which user are you talking about?

 Here it is the usb port device.

 At least, there are many boards which have hardwired and self-powered usb
 devices, so in theory they can benefits from the power controller.  Maybe
 only regulator and clock can't be covered completely for other boards.

 The patch can make usb port deal with the 'power controller' only, and make it
 avoid to deal with regulators/clocks/... directly.

I am curious too, except for clocks and voltage supplies (regulators),
what other external resources does a chip need ?
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Jassi Brar]

On Tue, Nov 27, 2012 at 10:32 PM, Greg KH gre...@linuxfoundation.org wrote:
 On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote:

 We should have a more generic solution in a more central location, like
 the device core.

 For example, each struct platform_device could have a list of resources
 to be enabled when the device is bound to a driver and disabled when
 the device is unbound.  Such a list could include regulators, clocks,
 and whatever else people can invent.

 In this case, when it is created the ehci-omap.0 platform device could
 get an entry pointing to the LAN95xx's regulator.  Then the regulator
 would automatically be turned on when the platform device is bound to
 the ehci-omap driver.

 How does this sound?

 That sounds much better, and it might come in handy for other types of
 devices than platform devices, so feel free to put this on the core
 'struct device' instead if needed.

Isn't enabling/disabling of clocks and regulators what
dev.probe()/remove() of any driver already does?
If we mean only board specific setup/teardown, still it would mean
having the device core to do an extra 'probe' call when the current
probe() is already meant to do whatever it takes to bring the device
up. To my untrained eyes, it seem like messing with the
probe()/remove()'s semantics.
 IMHO, if we really must implement something like that, may be we
should employ some 'broadcast mechanism' so that anything anywhere in
kernel knows which new device has been probed()/removed()
successfully. I haven't thought exactly how because I am not sure even
that would be the right way to approach PandaBoard's problem.

Looking at Figure 15 – Panda USBB1 Interface Block Diagram of
http://pandaboard.org/sites/default/files/board_reference/pandaboard-es-b/panda-es-b-manual.pdf
gives me visions ...

 1) OMAP doesn't provide the USB root-hub, but only ULPIPHY. It is
USB3320C chip that employs OMAP's ULPIPHY to provide the root-hub. So
we should have a platform device for USB3320C, the probe() of which
should simply
   a) Enable REFCLK (FREF_CLK3_OUT)
   b) Reset itself by cycling RESETB (GPIO_62)
   c) Create one ehci-omap platform device
 (or in appropriate order if the above isn't)
That way insmod/rmmod'ing the USB3320C driver would power-up/down the
whole subsystem.

2) Just like the user has to manually power-on/off any externally
powered hub connected to a PC, maybe we should implement a user
controlled 'soft' switch (reacting to UDEV events from ehci-omap?) to
emulate LAN9514 power-on/off. I do realize it would be cool to have it
automatically toggle in kernel when we (de)power the hub but isn't
that outside of scope of Linux USB implementation?

The above solution isn't most optimal for Panda but it seems a design
more consistent with what we already have. Or so do I think.

Cheers.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB-OTG enabled phone mount desktop partitions via USB port

[Jassi Brar]

On Tue, Oct 9, 2012 at 2:06 AM, 杨苏立 Yang Su Li yangs...@gmail.com wrote:
 Hi,

 I have noticed this mailing list has been intensively used to discuss
 patch development discussion. So please let me know if I should move
 my question to more appropriate places.

 In order to experiment something I need to pretend that my phone has a
 super fast SD card (faster than what is available in the market). So I
 want to simulate it through desktop ramdisk (which is basically disk
 in memory). And here is basically my plan:

 1. Get an USB-OTG enabled phone (say Samsung galaxy nexus)

 2. Connect this phone to a desktop via USB.

 3. Naturally this phone will have some kind of USB driver running.

 4. In the desktop I run the File-backed Storage Gadget (FSG), which
 make an USB host to be an USB slave, and presents a block device
 interface to the host which is backed by a ramdisk.

 5. In the phone, mount the FSG mass storage just as an ordinary disk.

Don't you need to initialize the ramdisk with contents from phone's
storage ? Otherwise it all boils down as unidirectional transfer of
files from desktop to phone. For which you don't really need the OTG
feature.
If your ramdisk does need to reflect the phone's storage, you'll pay
during the gadget connect (mount) and disconnect (unmount) times.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB-OTG enabled phone mount desktop partitions via USB port

[Jassi Brar]

On Wed, Oct 10, 2012 at 3:57 AM, 杨苏立 Yang Su Li yangs...@gmail.com wrote:
 On Tue, Oct 9, 2012 at 1:22 AM, Jassi Brar jassisinghb...@gmail.com wrote:
 On Tue, Oct 9, 2012 at 2:06 AM, 杨苏立 Yang Su Li yangs...@gmail.com wrote:
 Hi,

 I have noticed this mailing list has been intensively used to discuss
 patch development discussion. So please let me know if I should move
 my question to more appropriate places.

 In order to experiment something I need to pretend that my phone has a
 super fast SD card (faster than what is available in the market). So I
 want to simulate it through desktop ramdisk (which is basically disk
 in memory). And here is basically my plan:

 1. Get an USB-OTG enabled phone (say Samsung galaxy nexus)

 2. Connect this phone to a desktop via USB.

 3. Naturally this phone will have some kind of USB driver running.

 4. In the desktop I run the File-backed Storage Gadget (FSG), which
 make an USB host to be an USB slave, and presents a block device
 interface to the host which is backed by a ramdisk.

 5. In the phone, mount the FSG mass storage just as an ordinary disk.

 Don't you need to initialize the ramdisk with contents from phone's
 storage ? Otherwise it all boils down as unidirectional transfer of
 files from desktop to phone. For which you don't really need the OTG
 feature.
 If your ramdisk does need to reflect the phone's storage, you'll pay
 during the gadget connect (mount) and disconnect (unmount) times.

 I guess there might be some misunderstanding here. The ramdisk does
 not need to reflect
 the phone's storage. The ramdisk IS the phone's storage. Which means
 the phone will mount this ramdisk as one of its data partition, and
 read/write on that partition at runtime.

Yes I misunderstood. Somehow I got the impression that you want to
make the desktop see the phone as a super fast storage. Now it all
makes sense to me.
Cheers.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: f_uac2: why do you save two u8 in one u16?

[Jassi Brar]

On Fri, Sep 14, 2012 at 4:44 PM, Sebastian Andrzej Siewior
sebast...@breakpoint.cc wrote:
 On Fri, Sep 14, 2012 at 12:53:56PM +0200, Sebastian Andrzej Siewior wrote:
 looking at f_uac2.c I found this interresting piece of code:

 |struct audio_dev {
 | /* Currently active {Interface[15:8] | AltSettings[7:0]} */
 | __u16 ac_alt, as_out_alt, as_in_alt;
 | 
 |};
 |
 | 
 |#define ALT_SET(x, a)   do {(x) = ~0xff; (x) |= (a); } while (0)
 |#define ALT_GET(x)  ((x)  0xff)
 |#define INTF_SET(x, i)  do {(x) = 0xff; (x) |= ((i)  8); } while (0)
 |#define INTF_GET(x) ((x  8)  0xff)
 | 
 |ALT_SET(agdev-as_out_alt, 0);
 |INTF_SET(agdev-as_out_alt, ret);
 | 

 Could one of you two explain to me why smashing two different things (an
 interface number and an alternative setting number) into one variable
 and using a Macro to seperate them again was a good idea?

I don't see Interface and its AltSetting as different as you do.

 Another thing. alsa_uac2_init() calling platform_device_register() and
 platform_driver_register()? Why is that helpful can't you just call
 snd_uac2_probe() right away?

That is a common practice for virtual drivers (see
sound/drivers/dummy.c) and it helps assign the snd_card's struct
device *.

 You have poor (read as non-existing) error recovery in case something
 goes wrong during the allocation endpoint/interface/request.

Please be more specific.

 What do I need to test this gadget?

insmod g_audio.ko   on gadget
connect usb cable
A new sound card will appear on each the Host and the Gadget. Use is
just like any other sound card. Playback on side could be captured on
the other or routed to speakers if your h/w has them.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] How to handle delays in isochronous transfers?

[Jassi Brar]

On Tue, Sep 11, 2012 at 11:46 PM, Clemens Ladisch clem...@ladisch.de wrote:
 Alan Stern wrote:
 I have tried some initial testing of my updates, using data-OUT
 transfers with URB_ISO_ASAP turned off for the data URBs in
 sound/usb/endpoint.c.  When interrupts are delayed so long that
 synchronization is lost and a data URB submission fails, the sound
 stops playing and doesn't restart.  But the synch URBs continue to be
 submitted and ogg123 doesn't end for quite some time.

 When URB submission fails, the stream should be stopped.  In theory.

Since ISOCH transfers are not expected to be lossless (i.e, not
retried), perhaps we should simply lump all kinds of failures together
and ignore them and simply keep moving on?  Stream stopping should
only be done in response to standard USB state changes that warrant it
(ex switching to AltSetting-0) ?
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] How to handle delays in isochronous transfers?

[Jassi Brar]

On Wed, Sep 5, 2012 at 5:37 PM, Clemens Ladisch clem...@ladisch.de wrote:
 On Tue, 4 Sep 2012, Jassi Brar wrote:

If we progress the h/w pointer of ALSA ring buffer at URB completion (and not 
at URB submission) this shouldn't affect the latency.

 How would this make any difference?  The time between the application
 writing samples to the buffer and the samples actually being played by
 the device would not change.

If we advance the pointer immediately after submission, the total time
a sample spends in flight is ring-buffer + queue length.
If we advance the pointer upon completion, the total time a sample
spends in flight is ring-buffer.

Or I am doped.

Cheers
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] How to handle delays in isochronous transfers?

[Jassi Brar]

On Fri, Aug 31, 2012 at 11:56 PM, Alan Stern st...@rowland.harvard.edu wrote:
 Clemens and Laurent (and anyone else who's interested):

 How should the lower USB layers handle delays in transferring
 isochronous data?  I'm asking you because the most common usages of
 isochronous transfers are for audio and video.

 Here's an example to illustrate what I mean.  Typically an audio or
 video driver will keep a queue of around 10 ms of data submitted to an
 isochronous endpoint.  I have seen reports from users where URB
 completion interrupts were delayed by as much as 50 ms.  In one case
 the delay was caused by a bug in a wireless drivers that left
 interrupts disabled; in another case the cause was unknown -- it might
 have been a hardware problem.  At any rate, when this happens the
 endpoint's queue drains completely.

 Clearly this will cause a glitch in the data stream.  The question is:
 What should we do to recover and re-synchronize?

How about effectively increasing the queue length from 10ms to 50ms
(max anticipated latency) ?
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] How to handle delays in isochronous transfers?

[Jassi Brar]

[CC'ing an USB audio heavyweight - Daniel Mack]

On Tue, Sep 4, 2012 at 12:39 AM, Alan Stern st...@rowland.harvard.edu wrote:
 On Mon, 3 Sep 2012, Jassi Brar wrote:

 On Fri, Aug 31, 2012 at 11:56 PM, Alan Stern st...@rowland.harvard.edu 
 wrote:
  Clemens and Laurent (and anyone else who's interested):
 
  How should the lower USB layers handle delays in transferring
  isochronous data?  I'm asking you because the most common usages of
  isochronous transfers are for audio and video.
 
  Here's an example to illustrate what I mean.  Typically an audio or
  video driver will keep a queue of around 10 ms of data submitted to an
  isochronous endpoint.  I have seen reports from users where URB
  completion interrupts were delayed by as much as 50 ms.  In one case
  the delay was caused by a bug in a wireless drivers that left
  interrupts disabled; in another case the cause was unknown -- it might
  have been a hardware problem.  At any rate, when this happens the
  endpoint's queue drains completely.
 
  Clearly this will cause a glitch in the data stream.  The question is:
  What should we do to recover and re-synchronize?
 
 How about effectively increasing the queue length from 10ms to 50ms
 (max anticipated latency) ?

 There are two problems with that approach.  First, 50 ms isn't really
 the max anticipated latency; it's merely the largest that I've seen so
 far.  (In fact, the max anticipated latency is probably  10 ms; these
 50-ms delays were definitely exceptional.)

It doesn't have to be hardcoded - maybe USB audio/video code could get
the hint via some module parameter?

 Second, people involved in real-time programming (such as audio or
 video) generally want to keep latency to a minimum.

If we progress the h/w pointer of ALSA ring buffer at URB completion
(and not at URB submission), this shouldn't affect the latency. And
IIRC, USB isn't anyway recommended for real-time usage.

cheers.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: bisected regression, v3.5 - next-20120724: PCI PM causes USB hotplug failure

[Jassi Brar]

On Sun, Jul 29, 2012 at 8:25 PM, Alan Stern st...@rowland.harvard.edu wrote:

 For me there's also an issue of style: If you do a synchronous get then it
 looks odd not to do a synchronous put.  My feeling has always been that
 the async routines are for use in non-process contexts, where the sync
 routines can't be used.  Using them just to return a little more
 quickly is a foreign idea.

Another way of looking at it is - I need h/w to be active in order to
proceed so I call get_sync but I don't care if the h/w is not put down
immediately after I am done using it. So the put could be
relaxed/async - At best, we could avoid an unnecessary suspend-resume
cycle which could be expensive power and time wise. At worst, we
return a bit quicker. Or so do I think.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html