"Mark A. Greer" <[email protected]> writes:
> 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).
The <soc>.c file itself was intended to have all these things since
they are static, and specific to each SoC.
> 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.
Except the current <soc>.c files contain the data for several of the
pieces you moved into devices-da830.c (EDMA, UARTs, EMAC, ...) The
other complaint is that there are still several devices that are
identical except for base addresses and IRQs (I2C, MMC, MUSB, etc.) I
would like to find a more common way to handle these than duplicating
code across devices*.c (and/or musb.c, serial.c, etc.)
But in any case, my whining is about relatively minor organization
issues which we can address after getting the basics in place.
So I propose you submit a new version with the other minor fixups I
mentioned in the first review and keep the da830.c, da830-devices.c
split for now and I'll merge that. Then, I may make some proposals to
you in things that I think should be slightly different.
> 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.
My preference is to only add the parts as they are required, which
allows more targeted review, an eliminates compiler warnings
etc. about unused code.
> 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.
OK.
>> > +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?
Both.
> 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.
Probably true, but since they are common across the DMx chips, they
were put here rather than duplicating them in each <soc>.h file.
Now that we have this CONFIG_ARCH_DAVINCI_DMx naming, some of the
things we've prefixed with DAVINCI_ should probably be renamed as DMx_
to be a little more clear.
Kevin
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
