On Thu, Apr 30, 2009 at 11:04:07AM -0700, Kevin Hilman wrote:
> Hi Mark,
>
> Sorry for the review lag and the lack of attention to the da830 stuff.
> I've been focusing on getting DaVinci core code reworked and submitted
> upstream for the next merge window.
No problem.
> > These areas have (or will have) compile errors
> > intentionally inserted if both CONFIG_ARCH_DAVINCI_DMx
> > and CONFIG_ARCH_DAVINCI_DA830 are enabled at the same
> > time. There is also a check in mach-davinci/Kconfig
> > to prevent enabling both architectures concurrently.
>
> This comment should be updated as the Kconfig requirement has been
> removed as I requested in favor of just compile-time errors.
Oops :)
> In general, I'm not sure I follow the rationale for a new
> devices-da830.c. Things that are common should be done in devices.c
> using soc_info base abstractions. Things that are SoC specific could
> be done in <soc>.c
devices-da830.c contains all of the SoC-defined platform_device,
resource, and platform_data. They're defined by the h/w and should
never change (without a h/w change).
I broke it out into a separate file to keep da830.c at a reasonable
size, keep all the platform/resource data in one easy to find place,
and it made da830.c look more-or-less like dm355.c & friends.
No other reasons than those.
> > diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c
> > new file mode 100644
> > +#define DA830_KICK0_MAGIC 0x83e70b13
> > +#define DA830_KICK1_MAGIC 0x95a4f1e0
> > +#define DA830_KICK0 IO_ADDRESS(DA830_BOOT_CFG_BASE + 0x38)
> > +#define DA830_KICK1 IO_ADDRESS(DA830_BOOT_CFG_BASE + 0x3c)
> > +
> > +void da830_unlock_cfg_regs(void)
> > +{
> > + __raw_writel(DA830_KICK0_MAGIC, DA830_KICK0);
> > + __raw_writel(DA830_KICK1_MAGIC, DA830_KICK1);
> > +}
>
> IIRC, you mention earlier that this is no longer required? I don't
> see any users of it. If not, please drop from hear and the header.
It isn't required but I left it there in case some other da830 board
came along that required it. I'll remove it.
> > diff --git a/arch/arm/mach-davinci/devices-da830.c
> > b/arch/arm/mach-davinci/devices-da830.c
> > +struct platform_device da830_serial_device = {
> > + .name = "serial8250",
> > + .id = PLAT8250_DEV_PLATFORM,
> > + .dev = {
> > + .platform_data = da830_serial_pdata,
> > + },
> > +};
>
> This UART stuff should be done in da830.c just like the other <soc>.c
> files.
Its platform_data that won't ever change (without a hardware change) so,
as per my logic above, I put it there.
Ditto for the other instances you point out.
> > +int da830_register_edma(void)
> > +{
> > + return platform_device_register(&da830_edma_device);
> > +}
>
> Then this could be dropped and the platform_device_register just added
> to da830.c:da830_init().
Using the da830_register_xxx() style was recommended to me by David
(assuming I understood him correctly). I see the advantage of it
being limited global data coming out of devices-da830.c. Its also
clear what's being registered; although, platform_device_register()
calls make it pretty clear too.
Personally, I [slightly] prefer having the board code call
platform_device_register() unless we're going to make a bunch of
common "register_device" routines (see below). da830_init() probably
isn't the right place because it doesn't know what devices should
be registered. Only the board code knows for sure.
> > +int da830_register_i2c(int instance, struct davinci_i2c_platform_data
> > *pdata)
> > +{
> > + struct platform_device *pdev;
> > +
> > + if (instance == 0)
> > + pdev = &da830_i2c_device0;
> > + else if (instance == 1)
> > + pdev = &da830_i2c_device1;
> > + else
> > + return -EINVAL;
> > +
> > + pdev->dev.platform_data = pdata;
> > + return platform_device_register(pdev);
> > +}
>
> How about adding i2c_bases to soc_info, then re-using devices.c for
> i2c setup. And for handling multiple instances, you can use the 'id'
> field of platform_device instead of an extra argument.
I don't think soc_info is a good place for this.
The board code is the one that decides what needs to be
registered so it makes sense that it calls some sort of:
- register_device(<type>, <instance>, <pdata>) or
- register_device_<type>(<instance>, <pdata>) or
- platform_device_register(<platform_device ptr>)
routine. Using soc_info to pass base addrs for this purpose is
unnecessary. Plus struct davinci_soc_info will get huge as more
SoCs with different numbers and types of devices are added to
mach-davinci.
Note that board code calling platform_device_register() would
be all that's needed if we implement the "enable/disable" hooks
in drivers.
As far as reusing devices.c, this raises a good question,
"Put da830 data/code in devices.c or make a parallel da830
devices file?"
There is basically nothing in common between devices.c and
devices-da830.c other than some platform_device structs and
platform_device_register() calls. All of the resource and
platform_data are different and there are some extra platform_device
structs in devices-da830.c for devices that don't exist on davincis.
The platform_device_register() calls are in __init routines and
will disappear so I don't see that as a big issue. So the only
real issue with not combining is the duplication of some
platform_device structs.
If we do combine, the davinci kernel images will be bloated with
da830 data and da830 kernel images will be bloated with davinci
data--we can't support both types in the same kernel image yet.
Even if/when both types are supported in the same kernel image,
there's no harm in having the data in separate files. It makes
it a whole lot easier to know where the data is (e.g., for da830,
all the SoC-determined data is in devices-da830.c). devices.c
will become pretty cluttered with da830 data & cpu_is_xxx() calls.
It also makes it easier to add support for a new SoCs--just put all
its data in one file and forget it (unless it shares the same data
as an existing SoC).
We could separate the registration code from the data and put davinci
data in one file da830 data in another. That would eliminate the code
duplication.
Thoughts?
> > +static struct resource da830_watchdog_resources[] = {
> > + {
> > + .start = DA830_WDOG_BASE,
> > + .end = DA830_WDOG_BASE + SZ_4K - 1,
> > + .flags = IORESOURCE_MEM,
> > + },
> > +};
> > +
> > +static struct platform_device da830_watchdog_device = {
> > + .name = "watchdog",
> > + .id = -1,
> > + .num_resources = ARRAY_SIZE(da830_watchdog_resources),
> > + .resource = da830_watchdog_resources,
> > +};
> > +
> > +int da830_register_watchdog(void)
> > +{
> > + return platform_device_register(&da830_watchdog_device);
> > +}
>
> This is duplicating setup already available in soc_info + devices.c.
Actually, this is probably wrong in that the '.name' will likely
change to "omap_rtc" so the commonality with devices.c will be small.
I haven't got serious about the rtc yet.
> > +static struct resource da830_usb20_resources[] = {
> > + {
> > + .start = DA830_USB0_CFG_BASE,
> > + .end = DA830_USB0_CFG_BASE + SZ_64K - 1,
> > + .flags = IORESOURCE_MEM,
> > + },
> > + {
> > + .start = IRQ_DA830_USB_INT,
> > + .end = IRQ_DA830_USB_INT,
> > + .flags = IORESOURCE_IRQ,
> > + }
> > +};
> > +
> > +static u64 da830_usb20_dma_mask = DMA_32BIT_MASK;
> > +
> > +static struct platform_device da830_usb20_device = {
> > + .name = "musb_hdrc",
> > + .id = -1,
> > + .dev = {
> > + .dma_mask = &da830_usb20_dma_mask,
> > + .coherent_dma_mask = DMA_32BIT_MASK,
> > + },
> > + .num_resources = ARRAY_SIZE(da830_usb20_resources),
> > + .resource = da830_usb20_resources,
> > +};
> > +
> > +int da830_register_usb20(struct musb_hdrc_platform_data *pdata)
> > +{
> > + da830_usb20_device.dev.platform_data = pdata;
> > + return platform_device_register(&da830_usb20_device);
> > +}
>
> Any reason usb.c cannot be re-used for MUSB on da830? At first
> glance, adding an usb_otg_base and usb_usb_irq to soc_info would be
> needed then usb.c could be re-used.
I thought about that but then figured there wouldn't be enough
common data (plus I wanted to keep all the SoC-defined da830
platform_data in one place, devices-da830.c). Its an easy change
but I'll await the outcome of the discussion above before changing
anything.
> > +static struct platform_device da830_usb11_device = {
> > + .name = "ohci",
> > + .id = 0,
> > + .dev = {
> > + .dma_mask = &da830_usb11_dma_mask,
> > + .coherent_dma_mask = 0xffffffff,
> > + },
> > + .num_resources = ARRAY_SIZE(da830_usb11_resources),
> > + .resource = da830_usb11_resources,
> > +};
>
> This OHCI init data should move to da830.c, but since it is never
> registered it should either be removed or registered in da830.c to
> avoid complier warnings about 'defined but not used'.
It will be used once the initial da830 evm & ohci patches to go in
and I submit a patch to change board-da830.c to use it.
My intention with devices-da830.c was to add everything that is on the
SoC at once so it can be reviewed/debugged once and never changed again.
I can always add the pieces as they're required, if you would prefer.
Ditto for eqep & other instances you idenfied.
> Just curious... Is this known to work with the existing upstream
> ohci driver? Does it use ohci-omap?
No, it requires Sergei's patch which adds ohci-da830.c.
> > +struct emac_platform_data da830_emac_pdata = {
> > + .ctrl_reg_offset = DA830_EMAC_CTRL_REG_OFFSET,
> > + .ctrl_mod_reg_offset = DA830_EMAC_MOD_REG_OFFSET,
> > + .ctrl_ram_offset = DA830_EMAC_RAM_OFFSET,
> > + .mdio_reg_offset = DA830_MDIO_REG_OFFSET,
> > + .ctrl_ram_size = DA830_EMAC_CTRL_RAM_SIZE,
> > + .version = EMAC_VERSION_2,
> > +};
> > +
> > +static struct platform_device da830_emac_device = {
> > + .name = "davinci_emac",
> > + .id = 1,
> > + .dev = {
> > + .platform_data = &da830_emac_pdata,
> > + },
> > + .num_resources = ARRAY_SIZE(da830_emac_resources),
> > + .resource = da830_emac_resources,
> > +};
>
> This should be moved to da830.c and the pdata made static.
Re: moving it to da830.c, same comment as above.
Re: static, good catch.
> > +static u64 da830_mmc_dma_mask = DMA_32BIT_MASK;
>
> This is now DMA_BIT_MASK(32) upstream. I've been fixing this in the
> existing code as it goes upstream.
Okay, thank you.
> > +static struct platform_device da830_mmc_device = {
> > + .name = "davinci_mmc",
> > + .id = 0,
> > + .dev = {
> > + .dma_mask = &da830_mmc_dma_mask,
> > + .coherent_dma_mask = DMA_32BIT_MASK,
> > + },
> > + .num_resources = ARRAY_SIZE(da830_mmc_resources),
> > + .resource = da830_mmc_resources,
> > +};
> > +
> > +int da830_register_mmc(struct davinci_mmc_config *pdata)
> > +{
> > + da830_mmc_device.dev.platform_data = pdata;
> > + return platform_device_register(&da830_mmc_device);
> > +}
>
> Again for MMC, we're re-using the same driver with differences in
> bases and interrupts. Another good case for extending soc_info.
Same discussion as the i2c one above.
> > diff --git a/arch/arm/mach-davinci/include/mach/da830.h
> > b/arch/arm/mach-davinci/include/mach/da830.h
> > +/*
> > + * The cp_intc interrupt controller for the da830 isn't in the same
> > + * chunk of physical memory space as the other registers (like it is
> > + * on the davincis) so it needs to be mapped separately. It will be
> > + * mapped early on when the I/O space is mapped and we'll put it just
> > + * before the I/O space in the processor's virtual memory space.
> > + */
> > +#define DA830_ARM_INTC_BASE 0xfffee000
>
> Minor nit: should this be named ..CPINTC_BASE?
I think the original motivation with this name was to keep it
consistent with the davinci counterpart, DAVINCI_ARM_INTC_BASE.
I have no preference so I'm happy to change it.
> > +#define DA830_CP_INTC_SIZE SZ_8K
> > +#define DA830_CP_INTC_VIRT (IO_VIRT - DA830_CP_INTC_SIZE - SZ_4K)
> > diff --git a/arch/arm/mach-davinci/include/mach/serial.h
> > b/arch/arm/mach-davinci/include/mach/serial.h
> > index a59b7de..d5da19e 100644
> > --- a/arch/arm/mach-davinci/include/mach/serial.h
> > +++ b/arch/arm/mach-davinci/include/mach/serial.h
> > @@ -20,6 +20,10 @@
> >
> > #define DM355_UART2_BASE (IO_PHYS + 0x206000)
> >
> > +#define DA830_UART0_BASE (IO_PHYS + 0x42000)
> > +#define DA830_UART1_BASE (IO_PHYS + 0x10c000)
> > +#define DA830_UART2_BASE (IO_PHYS + 0x10d000)
>
> You can put these directly into da830.c. I will do the same for the
> dm355 one which should be in dm355.c since these base addreses really
> do not need to be globally visible.
Okay.
> > diff --git a/arch/arm/mach-davinci/include/mach/time.h
> > b/arch/arm/mach-davinci/include/mach/time.h
> > --- a/arch/arm/mach-davinci/include/mach/time.h
> > +++ b/arch/arm/mach-davinci/include/mach/time.h
> > @@ -15,6 +15,20 @@
> > #define DAVINCI_TIMER1_BASE (IO_PHYS + 0x21800)
> > #define DAVINCI_WDOG_BASE (IO_PHYS + 0x21C00)
> >
> > +#define DA830_TIMER64P0_BASE (IO_PHYS + 0x20000)
> > +#define DA830_TIMER64P1_BASE (IO_PHYS + 0x21000)
> > +#define DA830_WDOG_BASE DA830_TIMER64P1_BASE
> > +
> > +/* Offsets of the 8 compare registers on the da830 */
> > +#define DA830_CMP12_0 0x60
> > +#define DA830_CMP12_1 0x64
> > +#define DA830_CMP12_2 0x68
> > +#define DA830_CMP12_3 0x6c
> > +#define DA830_CMP12_4 0x70
> > +#define DA830_CMP12_5 0x74
> > +#define DA830_CMP12_6 0x78
> > +#define DA830_CMP12_7 0x7c
>
> These also can go directly into da830.c.
Do you mean just the CMP definitions or the base addr definitions too?
If you mean the base addr definitions too, that's fine (actually, I
prefer that) but we should move the davinci base addr definitions too.
Mark
--
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
