diff mbox

[1/3] DaVinci: move MUSB platform device to devices.c

Message ID 200908282229.54352.sshtylyov@ru.mvista.com (mailing list archive)
State Changes Requested
Headers show

Commit Message

Sergei Shtylyov Aug. 28, 2009, 6:29 p.m. UTC
There's absolutely no reason why the DaVinci USB platfrom device should
be in its own module; moreover, it will stand in the way of DA8xx's USB
platfrom device which occupies different region and IRQ -- so, move it
into devices.c and get rid of usb.c...

While at it, add 'davinci_' prefix to setup_usb(), remove its duplicate
declaration in common.h, and rename 'usb_dev' to 'davinci_usb_device' to
match the naming pattern established for devices.c...

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

---
The patch is against the recent DaVinci tree...

 arch/arm/mach-davinci/usb.c                 |  110 ----------------------------
 arch/arm/mach-davinci/Makefile              |    2 
 arch/arm/mach-davinci/board-dm355-evm.c     |    2 
 arch/arm/mach-davinci/board-dm355-leopard.c |    2 
 arch/arm/mach-davinci/board-dm644x-evm.c    |    2 
 arch/arm/mach-davinci/board-sffsdr.c        |    2 
 arch/arm/mach-davinci/devices.c             |   94 +++++++++++++++++++++++
 arch/arm/mach-davinci/include/mach/common.h |    5 -
 8 files changed, 100 insertions(+), 119 deletions(-)

Comments

Kevin Hilman Sept. 14, 2009, 8:06 p.m. UTC | #1
Sergei Shtylyov <sshtylyov@ru.mvista.com> writes:

> There's absolutely no reason why the DaVinci USB platfrom device should
> be in its own module; 

Other than devices*.c getting very cluttered, difficult to maintain
and the root cause of merge conflicts because of so many unrleated
patches wanting to touch the same files.

For this reason, as well as to share more between DMx and DA8xx, I
would like to move to a model where devices.c is split up into usb.c,
mmc.c, etc., so I don't like this change.

> moreover, it will stand in the way of DA8xx's USB
> platfrom device which occupies different region and IRQ -- so, move it
> into devices.c and get rid of usb.c...

I'd rather see both DMx and da8xx consolidated into usb.c.  In your
patches, there is lots of duplication between those files.

Then usb.c would have all the resource and platform_data for all devices
and the interface would be:

  davinci_register_usb()   -- renamed from setup_usb()
  da8xx_register_usb11()
  da8xx_register_usb20()

Kevin

> While at it, add 'davinci_' prefix to setup_usb(), remove its duplicate
> declaration in common.h, and rename 'usb_dev' to 'davinci_usb_device' to
> match the naming pattern established for devices.c...
>
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
>
> ---
> The patch is against the recent DaVinci tree...
>
>  arch/arm/mach-davinci/usb.c                 |  110 ----------------------------
>  arch/arm/mach-davinci/Makefile              |    2 
>  arch/arm/mach-davinci/board-dm355-evm.c     |    2 
>  arch/arm/mach-davinci/board-dm355-leopard.c |    2 
>  arch/arm/mach-davinci/board-dm644x-evm.c    |    2 
>  arch/arm/mach-davinci/board-sffsdr.c        |    2 
>  arch/arm/mach-davinci/devices.c             |   94 +++++++++++++++++++++++
>  arch/arm/mach-davinci/include/mach/common.h |    5 -
>  8 files changed, 100 insertions(+), 119 deletions(-)
>
> Index: linux-davinci/arch/arm/mach-davinci/Makefile
> ===================================================================
> --- linux-davinci.orig/arch/arm/mach-davinci/Makefile
> +++ linux-davinci/arch/arm/mach-davinci/Makefile
> @@ -5,7 +5,7 @@
>  
>  # Common objects
>  obj-y 			:= time.o clock.o serial.o io.o psc.o \
> -			   gpio.o dma.o usb.o common.o sram.o
> +			   gpio.o dma.o common.o sram.o
>  
>  obj-$(CONFIG_DAVINCI_MUX)		+= mux.o
>  
> Index: linux-davinci/arch/arm/mach-davinci/board-dm355-evm.c
> ===================================================================
> --- linux-davinci.orig/arch/arm/mach-davinci/board-dm355-evm.c
> +++ linux-davinci/arch/arm/mach-davinci/board-dm355-evm.c
> @@ -275,7 +275,7 @@ static __init void dm355_evm_init(void)
>  	gpio_request(2, "usb_id_toggle");
>  	gpio_direction_output(2, USB_ID_VALUE);
>  	/* irlml6401 switches over 1A in under 8 msec */
> -	setup_usb(500, 8);
> +	davinci_setup_usb(500, 8);
>  
>  	davinci_setup_mmc(0, &dm355evm_mmc_config);
>  	davinci_setup_mmc(1, &dm355evm_mmc_config);
> Index: linux-davinci/arch/arm/mach-davinci/board-dm355-leopard.c
> ===================================================================
> --- linux-davinci.orig/arch/arm/mach-davinci/board-dm355-leopard.c
> +++ linux-davinci/arch/arm/mach-davinci/board-dm355-leopard.c
> @@ -271,7 +271,7 @@ static __init void dm355_leopard_init(vo
>  	gpio_request(2, "usb_id_toggle");
>  	gpio_direction_output(2, USB_ID_VALUE);
>  	/* irlml6401 switches over 1A in under 8 msec */
> -	setup_usb(500, 8);
> +	davinci_setup_usb(500, 8);
>  
>  	davinci_setup_mmc(0, &dm355leopard_mmc_config);
>  	davinci_setup_mmc(1, &dm355leopard_mmc_config);
> Index: linux-davinci/arch/arm/mach-davinci/board-dm644x-evm.c
> ===================================================================
> --- linux-davinci.orig/arch/arm/mach-davinci/board-dm644x-evm.c
> +++ linux-davinci/arch/arm/mach-davinci/board-dm644x-evm.c
> @@ -409,7 +409,7 @@ evm_u35_setup(struct i2c_client *client,
>  	/* irlml6401 switches over 1A, in under 8 msec;
>  	 * now it can be managed by nDRV_VBUS ...
>  	 */
> -	setup_usb(500, 8);
> +	davinci_setup_usb(500, 8);
>  
>  	return 0;
>  }
> Index: linux-davinci/arch/arm/mach-davinci/board-sffsdr.c
> ===================================================================
> --- linux-davinci.orig/arch/arm/mach-davinci/board-sffsdr.c
> +++ linux-davinci/arch/arm/mach-davinci/board-sffsdr.c
> @@ -165,7 +165,7 @@ static __init void davinci_sffsdr_init(v
>  	davinci_serial_init(&uart_config);
>  	soc_info->emac_pdata->phy_mask = SFFSDR_PHY_MASK;
>  	soc_info->emac_pdata->mdio_max_freq = SFFSDR_MDIO_FREQUENCY;
> -	setup_usb(0, 0); /* We support only peripheral mode. */
> +	davinci_setup_usb(0, 0); /* We support only peripheral mode. */
>  
>  	/* mux VLYNQ pins */
>  	davinci_cfg_reg(DM644X_VLYNQEN);
> Index: linux-davinci/arch/arm/mach-davinci/devices.c
> ===================================================================
> --- linux-davinci.orig/arch/arm/mach-davinci/devices.c
> +++ linux-davinci/arch/arm/mach-davinci/devices.c
> @@ -15,6 +15,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/io.h>
> +#include <linux/usb/musb.h>
>  
>  #include <asm/mach/map.h>
>  
> @@ -28,6 +29,7 @@
>  #include <mach/time.h>
>  
>  #define DAVINCI_I2C_BASE	     0x01C21000
> +#define DAVINCI_USB_OTG_BASE	     0x01C64000
>  #define DAVINCI_MMCSD0_BASE	     0x01E10000
>  #define DM355_MMCSD0_BASE	     0x01E11000
>  #define DM355_MMCSD1_BASE	     0x01E00000
> @@ -62,6 +64,98 @@ void __init davinci_init_i2c(struct davi
>  	(void) platform_device_register(&davinci_i2c_device);
>  }
>  
> +#if defined(CONFIG_USB_MUSB_HDRC) || defined(CONFIG_USB_MUSB_HDRC_MODULE)
> +
> +static struct musb_hdrc_eps_bits musb_eps[] = {
> +	{ "ep1_tx", 8, },
> +	{ "ep1_rx", 8, },
> +	{ "ep2_tx", 8, },
> +	{ "ep2_rx", 8, },
> +	{ "ep3_tx", 5, },
> +	{ "ep3_rx", 5, },
> +	{ "ep4_tx", 5, },
> +	{ "ep4_rx", 5, },
> +};
> +
> +static struct musb_hdrc_config musb_config = {
> +	.multipoint	= true,
> +	.dyn_fifo	= true,
> +	.soft_con	= true,
> +	.dma		= true,
> +
> +	.num_eps	= 5,
> +	.dma_channels	= 8,
> +	.ram_bits	= 10,
> +	.eps_bits	= musb_eps,
> +};
> +
> +static struct musb_hdrc_platform_data usb_data = {
> +#if defined(CONFIG_USB_MUSB_OTG)
> +	/* OTG requires a Mini-AB connector */
> +	.mode           = MUSB_OTG,
> +#elif defined(CONFIG_USB_MUSB_PERIPHERAL)
> +	.mode           = MUSB_PERIPHERAL,
> +#elif defined(CONFIG_USB_MUSB_HOST)
> +	.mode           = MUSB_HOST,
> +#endif
> +	.clock		= "usb",
> +	.config		= &musb_config,
> +};
> +
> +static struct resource usb_resources[] = {
> +	{
> +		/* physical address */
> +		.start          = DAVINCI_USB_OTG_BASE,
> +		.end            = DAVINCI_USB_OTG_BASE + 0x5ff,
> +		.flags          = IORESOURCE_MEM,
> +	},
> +	{
> +		.start          = IRQ_USBINT,
> +		.flags          = IORESOURCE_IRQ,
> +	},
> +	{
> +		/* placeholder for the dedicated CPPI IRQ */
> +		.flags          = IORESOURCE_IRQ,
> +	},
> +};
> +
> +static u64 usb_dmamask = DMA_BIT_MASK(32);
> +
> +static struct platform_device davinci_usb_device = {
> +	.name           = "musb_hdrc",
> +	.id             = -1,
> +	.dev = {
> +		.platform_data		= &usb_data,
> +		.dma_mask		= &usb_dmamask,
> +		.coherent_dma_mask      = DMA_BIT_MASK(32),
> +	},
> +	.resource       = usb_resources,
> +	.num_resources  = ARRAY_SIZE(usb_resources),
> +};
> +
> +void __init davinci_setup_usb(unsigned mA, unsigned potpgt_msec)
> +{
> +	usb_data.power = mA / 2;
> +	usb_data.potpgt = potpgt_msec / 2;
> +
> +	if (cpu_is_davinci_dm646x()) {
> +		/* Override the defaults as DM6467 uses different IRQs. */
> +		davinci_usb_device.resource[1].start = IRQ_DM646X_USBINT;
> +		davinci_usb_device.resource[2].start = IRQ_DM646X_USBDMAINT;
> +	} else	/* other devices don't have dedicated CPPI IRQ */
> +		davinci_usb_device.num_resources = 2;
> +
> +	platform_device_register(&davinci_usb_device);
> +}
> +
> +#else
> +
> +void __init davinci_setup_usb(unsigned mA, unsigned potpgt_msec)
> +{
> +}
> +
> +#endif	/* CONFIG_USB_MUSB_HDRC */
> +
>  #if	defined(CONFIG_MMC_DAVINCI) || defined(CONFIG_MMC_DAVINCI_MODULE)
>  
>  static u64 mmcsd0_dma_mask = DMA_BIT_MASK(32);
> Index: linux-davinci/arch/arm/mach-davinci/include/mach/common.h
> ===================================================================
> --- linux-davinci.orig/arch/arm/mach-davinci/include/mach/common.h
> +++ linux-davinci/arch/arm/mach-davinci/include/mach/common.h
> @@ -21,10 +21,7 @@ extern void __iomem *davinci_intc_base;
>  extern int davinci_intc_type;
>  
>  /* parameters describe VBUS sourcing for host mode */
> -extern void setup_usb(unsigned mA, unsigned potpgt_msec);
> -
> -/* parameters describe VBUS sourcing for host mode */
> -extern void setup_usb(unsigned mA, unsigned potpgt_msec);
> +void davinci_setup_usb(unsigned mA, unsigned potpgt_msec);
>  
>  struct davinci_timer_instance {
>  	void __iomem	*base;
> Index: linux-davinci/arch/arm/mach-davinci/usb.c
> ===================================================================
> --- linux-davinci.orig/arch/arm/mach-davinci/usb.c
> +++ /dev/null
> @@ -1,110 +0,0 @@
> -/*
> - * USB
> - */
> -#include <linux/kernel.h>
> -#include <linux/module.h>
> -#include <linux/init.h>
> -#include <linux/platform_device.h>
> -#include <linux/dma-mapping.h>
> -
> -#include <linux/usb/musb.h>
> -#include <linux/usb/otg.h>
> -
> -#include <mach/common.h>
> -#include <mach/hardware.h>
> -#include <mach/irqs.h>
> -#include <mach/cputype.h>
> -
> -#define DAVINCI_USB_OTG_BASE 0x01C64000
> -
> -#if defined(CONFIG_USB_MUSB_HDRC) || defined(CONFIG_USB_MUSB_HDRC_MODULE)
> -static struct musb_hdrc_eps_bits musb_eps[] = {
> -	{ "ep1_tx", 8, },
> -	{ "ep1_rx", 8, },
> -	{ "ep2_tx", 8, },
> -	{ "ep2_rx", 8, },
> -	{ "ep3_tx", 5, },
> -	{ "ep3_rx", 5, },
> -	{ "ep4_tx", 5, },
> -	{ "ep4_rx", 5, },
> -};
> -
> -static struct musb_hdrc_config musb_config = {
> -	.multipoint	= true,
> -	.dyn_fifo	= true,
> -	.soft_con	= true,
> -	.dma		= true,
> -
> -	.num_eps	= 5,
> -	.dma_channels	= 8,
> -	.ram_bits	= 10,
> -	.eps_bits	= musb_eps,
> -};
> -
> -static struct musb_hdrc_platform_data usb_data = {
> -#if defined(CONFIG_USB_MUSB_OTG)
> -	/* OTG requires a Mini-AB connector */
> -	.mode           = MUSB_OTG,
> -#elif defined(CONFIG_USB_MUSB_PERIPHERAL)
> -	.mode           = MUSB_PERIPHERAL,
> -#elif defined(CONFIG_USB_MUSB_HOST)
> -	.mode           = MUSB_HOST,
> -#endif
> -	.clock		= "usb",
> -	.config		= &musb_config,
> -};
> -
> -static struct resource usb_resources[] = {
> -	{
> -		/* physical address */
> -		.start          = DAVINCI_USB_OTG_BASE,
> -		.end            = DAVINCI_USB_OTG_BASE + 0x5ff,
> -		.flags          = IORESOURCE_MEM,
> -	},
> -	{
> -		.start          = IRQ_USBINT,
> -		.flags          = IORESOURCE_IRQ,
> -	},
> -	{
> -		/* placeholder for the dedicated CPPI IRQ */
> -		.flags          = IORESOURCE_IRQ,
> -	},
> -};
> -
> -static u64 usb_dmamask = DMA_BIT_MASK(32);
> -
> -static struct platform_device usb_dev = {
> -	.name           = "musb_hdrc",
> -	.id             = -1,
> -	.dev = {
> -		.platform_data		= &usb_data,
> -		.dma_mask		= &usb_dmamask,
> -		.coherent_dma_mask      = DMA_BIT_MASK(32),
> -	},
> -	.resource       = usb_resources,
> -	.num_resources  = ARRAY_SIZE(usb_resources),
> -};
> -
> -void __init setup_usb(unsigned mA, unsigned potpgt_msec)
> -{
> -	usb_data.power = mA / 2;
> -	usb_data.potpgt = potpgt_msec / 2;
> -
> -	if (cpu_is_davinci_dm646x()) {
> -		/* Override the defaults as DM6467 uses different IRQs. */
> -		usb_dev.resource[1].start = IRQ_DM646X_USBINT;
> -		usb_dev.resource[2].start = IRQ_DM646X_USBDMAINT;
> -	} else	/* other devices don't have dedicated CPPI IRQ */
> -		usb_dev.num_resources = 2;
> -
> -	platform_device_register(&usb_dev);
> -}
> -
> -#else
> -
> -void __init setup_usb(unsigned mA, unsigned potpgt_msec)
> -{
> -}
> -
> -#endif  /* CONFIG_USB_MUSB_HDRC */
> -
>
>
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source@linux.davincidsp.com
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Subbrathnam, Swaminathan Sept. 15, 2009, 2:38 a.m. UTC | #2
Kevin,

My re-structuring patch would be along similar lines.  There are cople of references in the usb.c that need to be re-done (platform specific memorymaps for ex) to be generic across platforms.

I am delayed on this patch due to other priorities currently.  Will put in an RFC by this week.

regards
swami
Sergei Shtylyov Sept. 15, 2009, 10:21 a.m. UTC | #3
Hello.

Kevin Hilman wrote:

>> There's absolutely no reason why the DaVinci USB platfrom device should
>> be in its own module; 
>>     
>
> Other than devices*.c getting very cluttered, difficult to maintain
> and the root cause of merge conflicts because of so many unrleated
> patches wanting to touch the same files.
>
> For this reason, as well as to share more between DMx and DA8xx, I
> would like to move to a model where devices.c is split up into usb.c,
> mmc.c, etc., so I don't like this change.
>   

   And I should say that don't like the split up idea. Because for me it 
would mean that we're going to live in cpu_is_*() hell for all the 
platform devices as every device detail is different between DaVincis 
and DA8xx -- there is not much common, contrary to what you seem to 
believe; and I started to hope that we'd avoid that kind of cpu_is_*() 
hell... That said, I've never been fond of your idea of having both 
subarchs in the single directory too. Now, with this split-up, this 
approach is going to be even more messy...

>> moreover, it will stand in the way of DA8xx's USB
>> platfrom device which occupies different region and IRQ -- so, move it
>> into devices.c and get rid of usb.c...
>>     
>
> I'd rather see both DMx and da8xx consolidated into usb.c.  In your
> patches, there is lots of duplication between those files.
>   

   I wouldn't like to live in cpu_is_*() hell, hence I had no choice but 
duplicate...

> Then usb.c would have all the resource and platform_data for all devices
> and the interface would be:
>
>   davinci_register_usb()   -- renamed from setup_usb()
>   da8xx_register_usb11()
>   da8xx_register_usb20()
>   

   Oh... if that means I can still have different MUSB platform devices 
(with the same platform data probably) for DaVinci and DA8xx and get 
around using cpu_is_*(), then I can consider that... But anyway, that 
would mean that the e.g. the DaVinci code would be burdened with e.g 
DA8xx devices when DA8xx is *not* configured (and vice versa), unless we 
introduce some #ifdef'ery into this file -- so I still don't quite like 
your idea...

> Kevin

WBR, Sergei
Sergei Shtylyov Sept. 15, 2009, 10:47 a.m. UTC | #4
Hello.

Subbrathnam, Swaminathan wrote:

> Kevin,
>
> My re-structuring patch would be along similar lines.  There are cople of references in the usb.c that need to be re-done (platform specific memorymaps for ex) to be generic across platforms.
>   

   I'm not sure I understand what you mean. Across which platfroms?

> I am delayed on this patch due to other priorities currently.  Will put in an RFC by this week.
>   

   And I'm going to recast my stuff ASAP...

> regards
> swami
>
> ________________________________________
> From: davinci-linux-open-source-bounces@linux.davincidsp.com [davinci-linux-open-source-bounces@linux.davincidsp.com] On Behalf Of Kevin Hilman [khilman@deeprootsystems.com]
> Sent: Tuesday, September 15, 2009 1:36 AM
> To: Sergei Shtylyov
> Cc: davinci-linux-open-source@linux.davincidsp.com; linux-usb@vger.kernel.org
> Subject: Re: [PATCH 1/3] DaVinci: move MUSB platform device to devices.c
>
> Sergei Shtylyov <sshtylyov@ru.mvista.com> writes:
>
>   
>> There's absolutely no reason why the DaVinci USB platfrom device should
>> be in its own module;
>>     
>
> Other than devices*.c getting very cluttered, difficult to maintain
> and the root cause of merge conflicts because of so many unrleated
> patches wanting to touch the same files.
>
> For this reason, as well as to share more between DMx and DA8xx, I
> would like to move to a model where devices.c is split up into usb.c,
> mmc.c, etc., so I don't like this change.
>
>   
>> moreover, it will stand in the way of DA8xx's USB
>> platfrom device which occupies different region and IRQ -- so, move it
>> into devices.c and get rid of usb.c...
>>     
>
> I'd rather see both DMx and da8xx consolidated into usb.c.  In your
> patches, there is lots of duplication between those files.
>
> Then usb.c would have all the resource and platform_data for all devices
> and the interface would be:
>
>   davinci_register_usb()   -- renamed from setup_usb()
>   da8xx_register_usb11()
>   da8xx_register_usb20()
>
> Kevin
>   

WBR, Sergei
Subbrathnam, Swaminathan Sept. 15, 2009, 10:50 a.m. UTC | #5
Primus, Freon platforms to start with and other platforms that utilize the same controller in future.

-----Original Message-----
From: Sergei Shtylyov [mailto:sshtylyov@ru.mvista.com] 
Sent: Tuesday, September 15, 2009 4:17 PM
To: Subbrathnam, Swaminathan
Cc: Kevin Hilman; davinci-linux-open-source@linux.davincidsp.com; linux-usb@vger.kernel.org
Subject: Re: [PATCH 1/3] DaVinci: move MUSB platform device to devices.c

Hello.

Subbrathnam, Swaminathan wrote:

> Kevin,
>
> My re-structuring patch would be along similar lines.  There are cople of references in the usb.c that need to be re-done (platform specific memorymaps for ex) to be generic across platforms.
>   

   I'm not sure I understand what you mean. Across which platfroms?

> I am delayed on this patch due to other priorities currently.  Will put in an RFC by this week.
>   

   And I'm going to recast my stuff ASAP...

> regards
> swami
>
> ________________________________________
> From: davinci-linux-open-source-bounces@linux.davincidsp.com [davinci-linux-open-source-bounces@linux.davincidsp.com] On Behalf Of Kevin Hilman [khilman@deeprootsystems.com]
> Sent: Tuesday, September 15, 2009 1:36 AM
> To: Sergei Shtylyov
> Cc: davinci-linux-open-source@linux.davincidsp.com; linux-usb@vger.kernel.org
> Subject: Re: [PATCH 1/3] DaVinci: move MUSB platform device to devices.c
>
> Sergei Shtylyov <sshtylyov@ru.mvista.com> writes:
>
>   
>> There's absolutely no reason why the DaVinci USB platfrom device should
>> be in its own module;
>>     
>
> Other than devices*.c getting very cluttered, difficult to maintain
> and the root cause of merge conflicts because of so many unrleated
> patches wanting to touch the same files.
>
> For this reason, as well as to share more between DMx and DA8xx, I
> would like to move to a model where devices.c is split up into usb.c,
> mmc.c, etc., so I don't like this change.
>
>   
>> moreover, it will stand in the way of DA8xx's USB
>> platfrom device which occupies different region and IRQ -- so, move it
>> into devices.c and get rid of usb.c...
>>     
>
> I'd rather see both DMx and da8xx consolidated into usb.c.  In your
> patches, there is lots of duplication between those files.
>
> Then usb.c would have all the resource and platform_data for all devices
> and the interface would be:
>
>   davinci_register_usb()   -- renamed from setup_usb()
>   da8xx_register_usb11()
>   da8xx_register_usb20()
>
> Kevin
>   

WBR, Sergei
Sergei Shtylyov Sept. 15, 2009, 11:01 a.m. UTC | #6
Hello.

Subbrathnam, Swaminathan wrote:

> Primus, Freon platforms to start with and other platforms that utilize the same controller in future.
>   

   How there can be a generic memory map between OMAP-L1x and DaVincis 
(if that's what you mean)?

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sshtylyov@ru.mvista.com] 
> Sent: Tuesday, September 15, 2009 4:17 PM
> To: Subbrathnam, Swaminathan
> Cc: Kevin Hilman; davinci-linux-open-source@linux.davincidsp.com; linux-usb@vger.kernel.org
> Subject: Re: [PATCH 1/3] DaVinci: move MUSB platform device to devices.c
>
> Hello.
>
> Subbrathnam, Swaminathan wrote:
>
>   
>> Kevin,
>>
>> My re-structuring patch would be along similar lines.  There are cople of references in the usb.c that need to be re-done (platform specific memorymaps for ex) to be generic across platforms.
>>   
>>     
>
>    I'm not sure I understand what you mean. Across which platfroms?

WBR, Sergei
Kevin Hilman Sept. 16, 2009, 3:27 p.m. UTC | #7
Sergei Shtylyov <sshtylyov@ru.mvista.com> writes:

> Hello.
>
> Kevin Hilman wrote:
>
>>> There's absolutely no reason why the DaVinci USB platfrom device should
>>> be in its own module;     
>>
>> Other than devices*.c getting very cluttered, difficult to maintain
>> and the root cause of merge conflicts because of so many unrleated
>> patches wanting to touch the same files.
>>
>> For this reason, as well as to share more between DMx and DA8xx, I
>> would like to move to a model where devices.c is split up into usb.c,
>> mmc.c, etc., so I don't like this change.
>>   
>
>   And I should say that don't like the split up idea. Because for me
> it would mean that we're going to live in cpu_is_*() hell for all the
> platform devices as every device detail is different between DaVincis
> and DA8xx -- there is not much common, contrary to what you seem to
> believe; and I started to hope that we'd avoid that kind of cpu_is_*()
> hell... That said, I've never been fond of your idea of having both
> subarchs in the single directory too. Now, with this split-up, this
> approach is going to be even more messy...
>
>>> moreover, it will stand in the way of DA8xx's USB
>>> platfrom device which occupies different region and IRQ -- so, move it
>>> into devices.c and get rid of usb.c...
>>>     
>>
>> I'd rather see both DMx and da8xx consolidated into usb.c.  In your
>> patches, there is lots of duplication between those files.
>>   
>
>   I wouldn't like to live in cpu_is_*() hell, hence I had no choice
> but duplicate...

I'll choose cpu_is_*() over code duplication.

>> Then usb.c would have all the resource and platform_data for all devices
>> and the interface would be:
>>
>>   davinci_register_usb()   -- renamed from setup_usb()
>>   da8xx_register_usb11()
>>   da8xx_register_usb20()
>>   
>
>   Oh... if that means I can still have different MUSB platform devices
> (with the same platform data probably) for DaVinci and DA8xx and get
> around using cpu_is_*(), then I can consider that... 

That would be fine with me.

> But anyway, that would mean that the e.g. the DaVinci code would be
> burdened with e.g DA8xx devices when DA8xx is *not* configured (and
> vice versa), unless we introduce some #ifdef'ery into this file --
> so I still don't quite like your idea...

Yes, burdened by a little memory bloat at the expense of a
maintainable kernel.  That's the trade-off, and I've chosen
maintainability.

The memory bloat is easy enough to solve by using #ifdefs if you like.
I do not object to #ifdefs in these cases.

Kevin
diff mbox

Patch

Index: linux-davinci/arch/arm/mach-davinci/Makefile
===================================================================
--- linux-davinci.orig/arch/arm/mach-davinci/Makefile
+++ linux-davinci/arch/arm/mach-davinci/Makefile
@@ -5,7 +5,7 @@ 
 
 # Common objects
 obj-y 			:= time.o clock.o serial.o io.o psc.o \
-			   gpio.o dma.o usb.o common.o sram.o
+			   gpio.o dma.o common.o sram.o
 
 obj-$(CONFIG_DAVINCI_MUX)		+= mux.o
 
Index: linux-davinci/arch/arm/mach-davinci/board-dm355-evm.c
===================================================================
--- linux-davinci.orig/arch/arm/mach-davinci/board-dm355-evm.c
+++ linux-davinci/arch/arm/mach-davinci/board-dm355-evm.c
@@ -275,7 +275,7 @@  static __init void dm355_evm_init(void)
 	gpio_request(2, "usb_id_toggle");
 	gpio_direction_output(2, USB_ID_VALUE);
 	/* irlml6401 switches over 1A in under 8 msec */
-	setup_usb(500, 8);
+	davinci_setup_usb(500, 8);
 
 	davinci_setup_mmc(0, &dm355evm_mmc_config);
 	davinci_setup_mmc(1, &dm355evm_mmc_config);
Index: linux-davinci/arch/arm/mach-davinci/board-dm355-leopard.c
===================================================================
--- linux-davinci.orig/arch/arm/mach-davinci/board-dm355-leopard.c
+++ linux-davinci/arch/arm/mach-davinci/board-dm355-leopard.c
@@ -271,7 +271,7 @@  static __init void dm355_leopard_init(vo
 	gpio_request(2, "usb_id_toggle");
 	gpio_direction_output(2, USB_ID_VALUE);
 	/* irlml6401 switches over 1A in under 8 msec */
-	setup_usb(500, 8);
+	davinci_setup_usb(500, 8);
 
 	davinci_setup_mmc(0, &dm355leopard_mmc_config);
 	davinci_setup_mmc(1, &dm355leopard_mmc_config);
Index: linux-davinci/arch/arm/mach-davinci/board-dm644x-evm.c
===================================================================
--- linux-davinci.orig/arch/arm/mach-davinci/board-dm644x-evm.c
+++ linux-davinci/arch/arm/mach-davinci/board-dm644x-evm.c
@@ -409,7 +409,7 @@  evm_u35_setup(struct i2c_client *client,
 	/* irlml6401 switches over 1A, in under 8 msec;
 	 * now it can be managed by nDRV_VBUS ...
 	 */
-	setup_usb(500, 8);
+	davinci_setup_usb(500, 8);
 
 	return 0;
 }
Index: linux-davinci/arch/arm/mach-davinci/board-sffsdr.c
===================================================================
--- linux-davinci.orig/arch/arm/mach-davinci/board-sffsdr.c
+++ linux-davinci/arch/arm/mach-davinci/board-sffsdr.c
@@ -165,7 +165,7 @@  static __init void davinci_sffsdr_init(v
 	davinci_serial_init(&uart_config);
 	soc_info->emac_pdata->phy_mask = SFFSDR_PHY_MASK;
 	soc_info->emac_pdata->mdio_max_freq = SFFSDR_MDIO_FREQUENCY;
-	setup_usb(0, 0); /* We support only peripheral mode. */
+	davinci_setup_usb(0, 0); /* We support only peripheral mode. */
 
 	/* mux VLYNQ pins */
 	davinci_cfg_reg(DM644X_VLYNQEN);
Index: linux-davinci/arch/arm/mach-davinci/devices.c
===================================================================
--- linux-davinci.orig/arch/arm/mach-davinci/devices.c
+++ linux-davinci/arch/arm/mach-davinci/devices.c
@@ -15,6 +15,7 @@ 
 #include <linux/platform_device.h>
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
+#include <linux/usb/musb.h>
 
 #include <asm/mach/map.h>
 
@@ -28,6 +29,7 @@ 
 #include <mach/time.h>
 
 #define DAVINCI_I2C_BASE	     0x01C21000
+#define DAVINCI_USB_OTG_BASE	     0x01C64000
 #define DAVINCI_MMCSD0_BASE	     0x01E10000
 #define DM355_MMCSD0_BASE	     0x01E11000
 #define DM355_MMCSD1_BASE	     0x01E00000
@@ -62,6 +64,98 @@  void __init davinci_init_i2c(struct davi
 	(void) platform_device_register(&davinci_i2c_device);
 }
 
+#if defined(CONFIG_USB_MUSB_HDRC) || defined(CONFIG_USB_MUSB_HDRC_MODULE)
+
+static struct musb_hdrc_eps_bits musb_eps[] = {
+	{ "ep1_tx", 8, },
+	{ "ep1_rx", 8, },
+	{ "ep2_tx", 8, },
+	{ "ep2_rx", 8, },
+	{ "ep3_tx", 5, },
+	{ "ep3_rx", 5, },
+	{ "ep4_tx", 5, },
+	{ "ep4_rx", 5, },
+};
+
+static struct musb_hdrc_config musb_config = {
+	.multipoint	= true,
+	.dyn_fifo	= true,
+	.soft_con	= true,
+	.dma		= true,
+
+	.num_eps	= 5,
+	.dma_channels	= 8,
+	.ram_bits	= 10,
+	.eps_bits	= musb_eps,
+};
+
+static struct musb_hdrc_platform_data usb_data = {
+#if defined(CONFIG_USB_MUSB_OTG)
+	/* OTG requires a Mini-AB connector */
+	.mode           = MUSB_OTG,
+#elif defined(CONFIG_USB_MUSB_PERIPHERAL)
+	.mode           = MUSB_PERIPHERAL,
+#elif defined(CONFIG_USB_MUSB_HOST)
+	.mode           = MUSB_HOST,
+#endif
+	.clock		= "usb",
+	.config		= &musb_config,
+};
+
+static struct resource usb_resources[] = {
+	{
+		/* physical address */
+		.start          = DAVINCI_USB_OTG_BASE,
+		.end            = DAVINCI_USB_OTG_BASE + 0x5ff,
+		.flags          = IORESOURCE_MEM,
+	},
+	{
+		.start          = IRQ_USBINT,
+		.flags          = IORESOURCE_IRQ,
+	},
+	{
+		/* placeholder for the dedicated CPPI IRQ */
+		.flags          = IORESOURCE_IRQ,
+	},
+};
+
+static u64 usb_dmamask = DMA_BIT_MASK(32);
+
+static struct platform_device davinci_usb_device = {
+	.name           = "musb_hdrc",
+	.id             = -1,
+	.dev = {
+		.platform_data		= &usb_data,
+		.dma_mask		= &usb_dmamask,
+		.coherent_dma_mask      = DMA_BIT_MASK(32),
+	},
+	.resource       = usb_resources,
+	.num_resources  = ARRAY_SIZE(usb_resources),
+};
+
+void __init davinci_setup_usb(unsigned mA, unsigned potpgt_msec)
+{
+	usb_data.power = mA / 2;
+	usb_data.potpgt = potpgt_msec / 2;
+
+	if (cpu_is_davinci_dm646x()) {
+		/* Override the defaults as DM6467 uses different IRQs. */
+		davinci_usb_device.resource[1].start = IRQ_DM646X_USBINT;
+		davinci_usb_device.resource[2].start = IRQ_DM646X_USBDMAINT;
+	} else	/* other devices don't have dedicated CPPI IRQ */
+		davinci_usb_device.num_resources = 2;
+
+	platform_device_register(&davinci_usb_device);
+}
+
+#else
+
+void __init davinci_setup_usb(unsigned mA, unsigned potpgt_msec)
+{
+}
+
+#endif	/* CONFIG_USB_MUSB_HDRC */
+
 #if	defined(CONFIG_MMC_DAVINCI) || defined(CONFIG_MMC_DAVINCI_MODULE)
 
 static u64 mmcsd0_dma_mask = DMA_BIT_MASK(32);
Index: linux-davinci/arch/arm/mach-davinci/include/mach/common.h
===================================================================
--- linux-davinci.orig/arch/arm/mach-davinci/include/mach/common.h
+++ linux-davinci/arch/arm/mach-davinci/include/mach/common.h
@@ -21,10 +21,7 @@  extern void __iomem *davinci_intc_base;
 extern int davinci_intc_type;
 
 /* parameters describe VBUS sourcing for host mode */
-extern void setup_usb(unsigned mA, unsigned potpgt_msec);
-
-/* parameters describe VBUS sourcing for host mode */
-extern void setup_usb(unsigned mA, unsigned potpgt_msec);
+void davinci_setup_usb(unsigned mA, unsigned potpgt_msec);
 
 struct davinci_timer_instance {
 	void __iomem	*base;
Index: linux-davinci/arch/arm/mach-davinci/usb.c
===================================================================
--- linux-davinci.orig/arch/arm/mach-davinci/usb.c
+++ /dev/null
@@ -1,110 +0,0 @@ 
-/*
- * USB
- */
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/init.h>
-#include <linux/platform_device.h>
-#include <linux/dma-mapping.h>
-
-#include <linux/usb/musb.h>
-#include <linux/usb/otg.h>
-
-#include <mach/common.h>
-#include <mach/hardware.h>
-#include <mach/irqs.h>
-#include <mach/cputype.h>
-
-#define DAVINCI_USB_OTG_BASE 0x01C64000
-
-#if defined(CONFIG_USB_MUSB_HDRC) || defined(CONFIG_USB_MUSB_HDRC_MODULE)
-static struct musb_hdrc_eps_bits musb_eps[] = {
-	{ "ep1_tx", 8, },
-	{ "ep1_rx", 8, },
-	{ "ep2_tx", 8, },
-	{ "ep2_rx", 8, },
-	{ "ep3_tx", 5, },
-	{ "ep3_rx", 5, },
-	{ "ep4_tx", 5, },
-	{ "ep4_rx", 5, },
-};
-
-static struct musb_hdrc_config musb_config = {
-	.multipoint	= true,
-	.dyn_fifo	= true,
-	.soft_con	= true,
-	.dma		= true,
-
-	.num_eps	= 5,
-	.dma_channels	= 8,
-	.ram_bits	= 10,
-	.eps_bits	= musb_eps,
-};
-
-static struct musb_hdrc_platform_data usb_data = {
-#if defined(CONFIG_USB_MUSB_OTG)
-	/* OTG requires a Mini-AB connector */
-	.mode           = MUSB_OTG,
-#elif defined(CONFIG_USB_MUSB_PERIPHERAL)
-	.mode           = MUSB_PERIPHERAL,
-#elif defined(CONFIG_USB_MUSB_HOST)
-	.mode           = MUSB_HOST,
-#endif
-	.clock		= "usb",
-	.config		= &musb_config,
-};
-
-static struct resource usb_resources[] = {
-	{
-		/* physical address */
-		.start          = DAVINCI_USB_OTG_BASE,
-		.end            = DAVINCI_USB_OTG_BASE + 0x5ff,
-		.flags          = IORESOURCE_MEM,
-	},
-	{
-		.start          = IRQ_USBINT,
-		.flags          = IORESOURCE_IRQ,
-	},
-	{
-		/* placeholder for the dedicated CPPI IRQ */
-		.flags          = IORESOURCE_IRQ,
-	},
-};
-
-static u64 usb_dmamask = DMA_BIT_MASK(32);
-
-static struct platform_device usb_dev = {
-	.name           = "musb_hdrc",
-	.id             = -1,
-	.dev = {
-		.platform_data		= &usb_data,
-		.dma_mask		= &usb_dmamask,
-		.coherent_dma_mask      = DMA_BIT_MASK(32),
-	},
-	.resource       = usb_resources,
-	.num_resources  = ARRAY_SIZE(usb_resources),
-};
-
-void __init setup_usb(unsigned mA, unsigned potpgt_msec)
-{
-	usb_data.power = mA / 2;
-	usb_data.potpgt = potpgt_msec / 2;
-
-	if (cpu_is_davinci_dm646x()) {
-		/* Override the defaults as DM6467 uses different IRQs. */
-		usb_dev.resource[1].start = IRQ_DM646X_USBINT;
-		usb_dev.resource[2].start = IRQ_DM646X_USBDMAINT;
-	} else	/* other devices don't have dedicated CPPI IRQ */
-		usb_dev.num_resources = 2;
-
-	platform_device_register(&usb_dev);
-}
-
-#else
-
-void __init setup_usb(unsigned mA, unsigned potpgt_msec)
-{
-}
-
-#endif  /* CONFIG_USB_MUSB_HDRC */
-