[RFC][PATCH] ARM: shmobile: kzm9g: add r8a66597_udc support

[RFC][PATCH] ARM: shmobile: kzm9g: add r8a66597_udc support

[Tetsuyuki Kobayashi]

Add r8a66597_udc support for CN6 connector on KZM-A9-GT board.
r8a66597 USB controller can not use host mode and peripheral mode
at the same time. At default r8a66597_udc is disabled.
To use r8a66597_udc define USE_R8A66597_UDC.

Signed-off-by: Tetsuyuki Kobayashi <koba@kmckk.co.jp>
---
Hello,

This is an experimental patch to test if r8a66597_udc works or not.
I think that using r8a66597_udc is very rare because usually r8a66597_hcd is used
and they can not use at the same time. So this patch is really for test and 
I add this patch code is all inside #ifdef USE_R8A66597_UDC.

My question, this kind of test code should not merge to mainline?
Or, the define USE_R8A66597_UDC should be change to CONFIG_R8A66597_UDC or so, 
and it to Kconfig?

 arch/arm/mach-shmobile/board-kzm9g.c |   40 ++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/arm/mach-shmobile/board-kzm9g.c b/arch/arm/mach-shmobile/board-kzm9g.c
index 7d4ee5c..af3e110 100644
--- a/arch/arm/mach-shmobile/board-kzm9g.c
+++ b/arch/arm/mach-shmobile/board-kzm9g.c
@@ -131,6 +131,37 @@ static struct platform_device usb_host_device = {
 	.resource	= usb_resources,
 };
 
+#ifdef USE_R8A66597_UDC
+static struct r8a66597_platdata usb1_gadget_data = {
+	.on_chip = 0,
+	.xtal		= R8A66597_PLATDATA_XTAL_48MHZ,
+};
+
+static struct resource usb1_gadget_resources[] = {
+	[0] = {
+		.start	= 0x10010000,
+		.end	= 0x1001ffff - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= intcs_evt2irq(0x220), /* IRQ1 */
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device usb1_gadget_device = {
+	.name		= "r8a66597_udc",
+	.id		= 1, /* USB1 */
+	.dev = {
+		.dma_mask		= NULL,         /*  not use dma */
+		.coherent_dma_mask	= 0xffffffff,
+		.platform_data		= &usb1_gadget_data,
+	},
+	.num_resources	= ARRAY_SIZE(usb1_gadget_resources),
+	.resource	= usb1_gadget_resources,
+};
+#endif
+
 /* USB Func CN17 */
 struct usbhs_private {
 	unsigned int phy;
@@ -577,8 +608,17 @@ static struct i2c_board_info i2c3_devices[] = {
 
 static struct platform_device *kzm_devices[] __initdata = {
 	&smsc_device,
+#ifdef USE_R8A66597_UDC
+	/*
+	 * Do not enable usb_host_device because r8a66597_udc and r8a66597_hcd
+	 * can not use at the same time.
+	 * Also, do not enable usbhs_device for simplify.
+	 */
+	&usb1_gadget_device,
+#else
 	&usb_host_device,
 	&usbhs_device,
+#endif
 	&lcdc_device,
 	&mmc_device,
 	&sdhi0_device,
-- 
1.7.9.5

Re: [RFC][PATCH] ARM: shmobile: kzm9g: add r8a66597_udc support

[Kuninori Morimoto]

Hi Kobayashi-san

> Add r8a66597_udc support for CN6 connector on KZM-A9-GT board.
> r8a66597 USB controller can not use host mode and peripheral mode
> at the same time. At default r8a66597_udc is disabled.
> To use r8a66597_udc define USE_R8A66597_UDC.
> 
> Signed-off-by: Tetsuyuki Kobayashi <koba@kmckk.co.jp>
> ---
> Hello,
> 
> This is an experimental patch to test if r8a66597_udc works or not.
> I think that using r8a66597_udc is very rare because usually r8a66597_hcd is used
> and they can not use at the same time. So this patch is really for test and 
> I add this patch code is all inside #ifdef USE_R8A66597_UDC.
> 
> My question, this kind of test code should not merge to mainline?
> Or, the define USE_R8A66597_UDC should be change to CONFIG_R8A66597_UDC or so, 
> and it to Kconfig?

I'm not sure the maintainer's opinion,
but if this patch is only for the "test", then #ifdef xx_TEST_xx is better name.

OTOH, if you add "choice" in Kconfig to select UDC/HCD (not for test),
it is better for users.

> +#ifdef USE_R8A66597_UDC
> +	/*
> +	 * Do not enable usb_host_device because r8a66597_udc and r8a66597_hcd
> +	 * can not use at the same time.
> +	 * Also, do not enable usbhs_device for simplify.
> +	 */
> +	&usb1_gadget_device,
> +#else
>  	&usb_host_device,
>  	&usbhs_device,
> +#endif

Why is usbhs_device disabled for simplify ?
what's happen if it was enabled ?

Best regards
---
Kuninori Morimoto

Re: [RFC][PATCH] ARM: shmobile: kzm9g: add r8a66597_udc support

[Tetsuyuki Kobayashi]

Hi Morimoto-san, thank you for comment.

(2012/07/04 11:54), Kuninori Morimoto wrote:

>> Add r8a66597_udc support for CN6 connector on KZM-A9-GT board.
>> r8a66597 USB controller can not use host mode and peripheral mode
>> at the same time. At default r8a66597_udc is disabled.
>> To use r8a66597_udc define USE_R8A66597_UDC.
>>
>> Signed-off-by: Tetsuyuki Kobayashi <koba@kmckk.co.jp>
>> ---
>> Hello,
>>
>> This is an experimental patch to test if r8a66597_udc works or not.
>> I think that using r8a66597_udc is very rare because usually r8a66597_hcd is used
>> and they can not use at the same time. So this patch is really for test and
>> I add this patch code is all inside #ifdef USE_R8A66597_UDC.
>>
>> My question, this kind of test code should not merge to mainline?
>> Or, the define USE_R8A66597_UDC should be change to CONFIG_R8A66597_UDC or so,
>> and it to Kconfig?
>
> I'm not sure the maintainer's opinion,
> but if this patch is only for the "test", then #ifdef xx_TEST_xx is better name.
>
> OTOH, if you add "choice" in Kconfig to select UDC/HCD (not for test),
> it is better for users.
>
Now in my mind, I will choose the latter. KZM-A9-GT is an *evaluation* 
board, so someone want to evaluate R8A66597_UDC.


>> +#ifdef USE_R8A66597_UDC
>> +	/*
>> +	 * Do not enable usb_host_device because r8a66597_udc and r8a66597_hcd
>> +	 * can not use at the same time.
>> +	 * Also, do not enable usbhs_device for simplify.
>> +	 */
>> +	&usb1_gadget_device,
>> +#else
>>   	&usb_host_device,
>>   	&usbhs_device,
>> +#endif
>
> Why is usbhs_device disabled for simplify ?
> what's happen if it was enabled ?
>

If the board has only one USB gadget, it is simple and almost
board has only one USB gadget.
But I don't understand yet how it works if the board has 2 or more
USB devices. Is it possible one for mass storage and the other for
ethernet over USB, or so?

Re: [RFC][PATCH] ARM: shmobile: kzm9g: add r8a66597_udc support

[Kuninori Morimoto]

Hi Kobayashi-san

> > I'm not sure the maintainer's opinion,
> > but if this patch is only for the "test", then #ifdef xx_TEST_xx is better name.
> >
> > OTOH, if you add "choice" in Kconfig to select UDC/HCD (not for test),
> > it is better for users.
> >
> Now in my mind, I will choose the latter. KZM-A9-GT is an *evaluation* 
> board, so someone want to evaluate R8A66597_UDC.

cool !

> >> +#ifdef USE_R8A66597_UDC
> >> +	/*
> >> +	 * Do not enable usb_host_device because r8a66597_udc and r8a66597_hcd
> >> +	 * can not use at the same time.
> >> +	 * Also, do not enable usbhs_device for simplify.
> >> +	 */
> >> +	&usb1_gadget_device,
> >> +#else
> >>   	&usb_host_device,
> >>   	&usbhs_device,
> >> +#endif
> >
> > Why is usbhs_device disabled for simplify ?
> > what's happen if it was enabled ?

> If the board has only one USB gadget, it is simple and almost
> board has only one USB gadget.
> But I don't understand yet how it works if the board has 2 or more
> USB devices. Is it possible one for mass storage and the other for
> ethernet over USB, or so?

I think so on latest kernel. (old kernel didn't support it)
I forgot detail of it, but 2usb-gadget worked correctly on other board (= mackerel ?)
 > insmod g_mass_strage file=xxx
 > insmod g_ether
If it doesn't work correctly, driver seems broken

# Current kzm9g board is using r8a66597_hcd/udc driver, since renesas_usbhs still doesn't support
# "external" chip yet.
# But in the future, it should be switched to use renesas_usbhs instead of r8a66597_xxx.

I don't care if you disabled renesas_usbhs or not.
(I don't know maintainer's opinion)
But when you use that style, please care compile warning.
I got this

/opt/usr/src/WORK/morimoto/gitlinux/linux-2.6/arch/arm/mach-shmobile/board-kzm9g.c:123:31: warning: 'usb_host_device' defined but not used [-Wunused-variable]
/opt/usr/src/WORK/morimoto/gitlinux/linux-2.6/arch/arm/mach-shmobile/board-kzm9g.c:298:31: warning: 'usbhs_device' defined but not used [-Wunused-variable]


Best regards
---
Kuninori Morimoto

Re: [RFC][PATCH] ARM: shmobile: kzm9g: add r8a66597_udc support

[Tetsuyuki Kobayashi]

Hi Morimoto-san

(2012/07/05 9:53), Kuninori Morimoto wrote:
>
> Hi Kobayashi-san
>
>>> I'm not sure the maintainer's opinion,
>>> but if this patch is only for the "test", then #ifdef xx_TEST_xx is better name.
>>>
>>> OTOH, if you add "choice" in Kconfig to select UDC/HCD (not for test),
>>> it is better for users.
>>>
>> Now in my mind, I will choose the latter. KZM-A9-GT is an *evaluation*
>> board, so someone want to evaluate R8A66597_UDC.
>
> cool !
>
>>>> +#ifdef USE_R8A66597_UDC
>>>> +	/*
>>>> +	 * Do not enable usb_host_device because r8a66597_udc and r8a66597_hcd
>>>> +	 * can not use at the same time.
>>>> +	 * Also, do not enable usbhs_device for simplify.
>>>> +	 */
>>>> +	&usb1_gadget_device,
>>>> +#else
>>>>    	&usb_host_device,
>>>>    	&usbhs_device,
>>>> +#endif
>>>
>>> Why is usbhs_device disabled for simplify ?
>>> what's happen if it was enabled ?
>
>> If the board has only one USB gadget, it is simple and almost
>> board has only one USB gadget.
>> But I don't understand yet how it works if the board has 2 or more
>> USB devices. Is it possible one for mass storage and the other for
>> ethernet over USB, or so?
>
> I think so on latest kernel. (old kernel didn't support it)
> I forgot detail of it, but 2usb-gadget worked correctly on other board (= mackerel ?)
>   > insmod g_mass_strage file=xxx
>   > insmod g_ether
> If it doesn't work correctly, driver seems broken
>
Thank you. I wanted this kind of information! I will study more.

> # Current kzm9g board is using r8a66597_hcd/udc driver, since renesas_usbhs still doesn't support
> # "external" chip yet.
> # But in the future, it should be switched to use renesas_usbhs instead of r8a66597_xxx.
>
> I don't care if you disabled renesas_usbhs or not.
> (I don't know maintainer's opinion)
> But when you use that style, please care compile warning.
> I got this
>
> /opt/usr/src/WORK/morimoto/gitlinux/linux-2.6/arch/arm/mach-shmobile/board-kzm9g.c:123:31: warning: 'usb_host_device' defined but not used [-Wunused-variable]
> /opt/usr/src/WORK/morimoto/gitlinux/linux-2.6/arch/arm/mach-shmobile/board-kzm9g.c:298:31: warning: 'usbhs_device' defined but not used [-Wunused-variable]
>
Thank you for trying my patch.
Yes. I will take care of them.

Re: [RFC][PATCH] ARM: shmobile: kzm9g: add r8a66597_udc support

[Simon Horman]

On Tue, Jul 03, 2012 at 03:32:58PM +0900, Tetsuyuki Kobayashi wrote:
> Add r8a66597_udc support for CN6 connector on KZM-A9-GT board.
> r8a66597 USB controller can not use host mode and peripheral mode
> at the same time. At default r8a66597_udc is disabled.
> To use r8a66597_udc define USE_R8A66597_UDC.
> 
> Signed-off-by: Tetsuyuki Kobayashi <koba@kmckk.co.jp>
> ---
> Hello,
> 
> This is an experimental patch to test if r8a66597_udc works or not.
> I think that using r8a66597_udc is very rare because usually r8a66597_hcd is used
> and they can not use at the same time. So this patch is really for test and 
> I add this patch code is all inside #ifdef USE_R8A66597_UDC.
> 
> My question, this kind of test code should not merge to mainline?
> Or, the define USE_R8A66597_UDC should be change to CONFIG_R8A66597_UDC or so, 
> and it to Kconfig?

Hi Kobayashi-san,

my opinion is that this kind of code should be merged into mainline and
that a run-time switch or or more likely Kconfig options should be in place
to ensure that hdc and udc can't run at the same time.

Paul, do you have an opinion on this?

> 
>  arch/arm/mach-shmobile/board-kzm9g.c |   40 ++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/arch/arm/mach-shmobile/board-kzm9g.c b/arch/arm/mach-shmobile/board-kzm9g.c
> index 7d4ee5c..af3e110 100644
> --- a/arch/arm/mach-shmobile/board-kzm9g.c
> +++ b/arch/arm/mach-shmobile/board-kzm9g.c
> @@ -131,6 +131,37 @@ static struct platform_device usb_host_device = {
>  	.resource	= usb_resources,
>  };
>  
> +#ifdef USE_R8A66597_UDC
> +static struct r8a66597_platdata usb1_gadget_data = {
> +	.on_chip = 0,
> +	.xtal		= R8A66597_PLATDATA_XTAL_48MHZ,
> +};
> +
> +static struct resource usb1_gadget_resources[] = {
> +	[0] = {
> +		.start	= 0x10010000,
> +		.end	= 0x1001ffff - 1,
> +		.flags	= IORESOURCE_MEM,
> +	},
> +	[1] = {
> +		.start	= intcs_evt2irq(0x220), /* IRQ1 */
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct platform_device usb1_gadget_device = {
> +	.name		= "r8a66597_udc",
> +	.id		= 1, /* USB1 */
> +	.dev = {
> +		.dma_mask		= NULL,         /*  not use dma */
> +		.coherent_dma_mask	= 0xffffffff,
> +		.platform_data		= &usb1_gadget_data,
> +	},
> +	.num_resources	= ARRAY_SIZE(usb1_gadget_resources),
> +	.resource	= usb1_gadget_resources,
> +};
> +#endif
> +
>  /* USB Func CN17 */
>  struct usbhs_private {
>  	unsigned int phy;
> @@ -577,8 +608,17 @@ static struct i2c_board_info i2c3_devices[] = {
>  
>  static struct platform_device *kzm_devices[] __initdata = {
>  	&smsc_device,
> +#ifdef USE_R8A66597_UDC
> +	/*
> +	 * Do not enable usb_host_device because r8a66597_udc and r8a66597_hcd
> +	 * can not use at the same time.
> +	 * Also, do not enable usbhs_device for simplify.
> +	 */
> +	&usb1_gadget_device,
> +#else
>  	&usb_host_device,
>  	&usbhs_device,
> +#endif
>  	&lcdc_device,
>  	&mmc_device,
>  	&sdhi0_device,
> -- 
> 1.7.9.5
> 

Re: [RFC][PATCH] ARM: shmobile: kzm9g: add r8a66597_udc support

[Paul Mundt]

(Resending due to mail server silliness, apologies for any duplicates)

On Sat, Aug 25, 2012 at 11:01:10AM +0900, Simon Horman wrote:
> On Tue, Jul 03, 2012 at 03:32:58PM +0900, Tetsuyuki Kobayashi wrote:
> > Add r8a66597_udc support for CN6 connector on KZM-A9-GT board.
> > r8a66597 USB controller can not use host mode and peripheral mode
> > at the same time. At default r8a66597_udc is disabled.
> > To use r8a66597_udc define USE_R8A66597_UDC.
> > 
> > Signed-off-by: Tetsuyuki Kobayashi <koba@kmckk.co.jp>
> > ---
> > Hello,
> > 
> > This is an experimental patch to test if r8a66597_udc works or not.
> > I think that using r8a66597_udc is very rare because usually r8a66597_hcd is used
> > and they can not use at the same time. So this patch is really for test and 
> > I add this patch code is all inside #ifdef USE_R8A66597_UDC.
> > 
> > My question, this kind of test code should not merge to mainline?
> > Or, the define USE_R8A66597_UDC should be change to CONFIG_R8A66597_UDC or so, 
> > and it to Kconfig?
> 
> Hi Kobayashi-san,
> 
> my opinion is that this kind of code should be merged into mainline and
> that a run-time switch or or more likely Kconfig options should be in place
> to ensure that hdc and udc can't run at the same time.
> 
> Paul, do you have an opinion on this?
> 
In general the ifdeffery for this should be avoided anyways, whether this
means we need to register a dummy gpio or whatever else that can be
consumed by both the udc/hcd drivers, ensuring that whoever gets there
first succeeds in its probe path.

If you need to put ifdefs in your platform device array, something is
horribly wrong in the drivers. ie, there's nothing stopping you from
having both built as modules, and you should have no problems switching
from one to the other.

Re: [RFC][PATCH] ARM: shmobile: kzm9g: add r8a66597_udc support

[Tetsuyuki Kobayashi]

Paul, Simon, Thank you for comments.
I will refine this patch using Kconfig option.

(2012/08/30 15:22), Paul Mundt wrote:
> (Resending due to mail server silliness, apologies for any duplicates)
>
> On Sat, Aug 25, 2012 at 11:01:10AM +0900, Simon Horman wrote:
>> On Tue, Jul 03, 2012 at 03:32:58PM +0900, Tetsuyuki Kobayashi wrote:
>>> Add r8a66597_udc support for CN6 connector on KZM-A9-GT board.
>>> r8a66597 USB controller can not use host mode and peripheral mode
>>> at the same time. At default r8a66597_udc is disabled.
>>> To use r8a66597_udc define USE_R8A66597_UDC.
>>>
>>> Signed-off-by: Tetsuyuki Kobayashi <koba@kmckk.co.jp>
>>> ---
>>> Hello,
>>>
>>> This is an experimental patch to test if r8a66597_udc works or not.
>>> I think that using r8a66597_udc is very rare because usually r8a66597_hcd is used
>>> and they can not use at the same time. So this patch is really for test and
>>> I add this patch code is all inside #ifdef USE_R8A66597_UDC.
>>>
>>> My question, this kind of test code should not merge to mainline?
>>> Or, the define USE_R8A66597_UDC should be change to CONFIG_R8A66597_UDC or so,
>>> and it to Kconfig?
>>
>> Hi Kobayashi-san,
>>
>> my opinion is that this kind of code should be merged into mainline and
>> that a run-time switch or or more likely Kconfig options should be in place
>> to ensure that hdc and udc can't run at the same time.
>>
>> Paul, do you have an opinion on this?
>>
> In general the ifdeffery for this should be avoided anyways, whether this
> means we need to register a dummy gpio or whatever else that can be
> consumed by both the udc/hcd drivers, ensuring that whoever gets there
> first succeeds in its probe path.
>
> If you need to put ifdefs in your platform device array, something is
> horribly wrong in the drivers. ie, there's nothing stopping you from
> having both built as modules, and you should have no problems switching
> from one to the other.
>