lkml.org 
[lkml]   [2010]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 1/6 v9] ARM: Add basic architecture support for VIA/WonderMedia 85xx SoC's
    From
    2010/12/21 Ryan Mallon <ryan@bluewatersys.com>:
    > On 12/21/2010 12:49 PM, Alexey Charkov wrote:
    >> 2010/12/21 Ryan Mallon <ryan@bluewatersys.com>:
    >>> On 12/21/2010 12:00 PM, Alexey Charkov wrote:
    >>>> 2010/12/21 Ryan Mallon <ryan@bluewatersys.com>:
    >>>>> On 12/21/2010 10:48 AM, Alexey Charkov wrote:
    >>>>>> 2010/12/20 Ryan Mallon <ryan@bluewatersys.com>:
    >>>>>>> On 12/21/2010 08:54 AM, Alexey Charkov wrote:
    >>>>>>>> This adds support for the family of Systems-on-Chip produced initially
    >>>>>>>> by VIA and now its subsidiary WonderMedia that have recently become
    >>>>>>>> widespread in lower-end Chinese ARM-based tablets and netbooks.
    >>>>>>>>
    >>>>>>>> Support is included for both VT8500 and WM8505, selectable by a
    >>>>>>>> configuration switch at kernel build time.
    >>>>>>>>
    >>>>>>>> Included are basic machine initialization files, register and
    >>>>>>>> interrupt definitions, support for the on-chip interrupt controller,
    >>>>>>>> high-precision OS timer, GPIO lines, necessary macros for early debug,
    >>>>>>>> pulse-width-modulated outputs control, as well as platform device
    >>>>>>>> configurations for the specific drivers implemented elsewhere.
    >>>>>>>>
    >>>>>>>> Signed-off-by: Alexey Charkov <alchark@gmail.com>
    >>>>>>>
    >>>>>>> Hi Alexey,
    >>>>>>>
    >>>>>>> Quick review below.
    >>>>>
    >>>>>> <snip>
    >>>>>>>> +void __init wmt_set_resources(void)
    >>>>>>>> +{
    >>>>>>>> +     resources_lcdc[0].start = wmt_current_regs->lcdc;
    >>>>>>>> +     resources_lcdc[0].end = wmt_current_regs->lcdc + SZ_1K - 1;
    >>>>>>>> +     resources_lcdc[1].start = wmt_current_irqs->lcdc;
    >>>>>>>> +     resources_lcdc[1].end = wmt_current_irqs->lcdc;
    >>>>>>>
    >>>>>>> Ah, this makes more sense. But why have all the indirection? The
    >>>>>>> wmt_regmaps table could just be replaced with #defines and then have
    >>>>>>> separate device files for the VT8500 and the WM8505. This would also
    >>>>>>> make clearer which variants have which peripherals.
    >>>>>>>
    >>>>>>
    >>>>>> This was the way I implemented it originally. However, Arnd made quite
    >>>>>> a valid suggestion to allow runtime selection of the chip variant,
    >>>>>> thus registers and interrupts need to be held in an indexed data type
    >>>>>> instead of just compile-time macros. In addition, there is now some
    >>>>>> overall movement towards unification of binary kernel images for
    >>>>>> different ARM variants (as far as I can see), so this would be
    >>>>>> required in any case.
    >>>>>>
    >>>>>> Furthermore, as with many unbranded Chinese products, it's somewhat
    >>>>>> difficult to reliably determine the exact chip version used in your
    >>>>>> netbook without disassembling it. Reading a hardware register for
    >>>>>> identification is easier :)
    >>>>>
    >>>>> Okay, that makes sense. I still think there must be a better way than
    >>>>> having a massive indirect table with all the values. Why not detect the
    >>>>> variant in the core code and then have something like:
    >>>>>
    >>>>> int init_devices(void)
    >>>>> {
    >>>>>        int board_type = detect_board_type();
    >>>>>
    >>>>>        switch (board_type) {
    >>>>>        case BOARD_TYPE_VT8500:
    >>>>>                return vt8500_init_devices();
    >>>>>
    >>>>>        case BOARD_TYPE_WM8505:
    >>>>>                return wm8500_init_devices();
    >>>>>        }
    >>>>>
    >>>>>        pr_err("Unknown board type\n");
    >>>>>        BUG(); /* panic()? */
    >>>>>        while (1)
    >>>>>                ;
    >>>>> }
    >>>>>
    >>>>> Then you can have the peripheral setup for each of the variants in their
    >>>>> own files and use #defines. It may get tricky in a couple of places if
    >>>>> you need to be able to access some value directly which is different on
    >>>>> each of the variants, but that can be handled on a case by case basis.
    >>>>> The majority of the numbers will be passed into drivers via the resource
    >>>>> structs.
    >>>>>
    >>>>
    >>>> This is more or less what I'm doing right now - except for the
    >>>> separation between different files. I tried to avoid duplication of
    >>>> similar things here. Is the indirect table really a big issue? I'm a
    >>>> bit reluctant to copy about the whole devices.c for each chip variant,
    >>>> which would be otherwise required. Further, it would add more
    >>>> complexity to the timer, irq, gpio, i8042 and probably some other
    >>>> places.
    >>>
    >>> Yeah, agreed about the duplication. My approach would require the common
    >>> peripherals to be defined for each variant. I'm not entirely against the
    >>> indirect table (and am even starting to think it may be the best overall
    >>> solution), it's just that it can be a bit difficult to follow because to
    >>> add a device you need to do the following:
    >>>
    >>>  - Add a partially empty resource/platform_device struct
    >>>  - Add resource entries to the resource table definition
    >>>  - Add resource values to the resource table
    >>>  - Add assignment of resource values to device init code
    >>>
    >>
    >> That's actually only one step more than what machines with static
    >> resource definitions require (the last one). Flexibility does come at
    >> a cost, so there should be a mathematical limit to optimization of
    >> this thing :)
    >
    > No it isn't. You don't have the massive table, which requires
    > modifications to both the definition and declaration, on machines with
    > static resource definitions.
    >
    > How about using the resource structures directly rather than introducing
    > the table which is effectively holding the same information? Something
    > like this?
    >
    > In vt8500_resources.c (and similarly for wm8505_resources.c):
    >
    > static struct resource vt8500_resources_uart0[] __initdata = {
    >        [0] = {
    >                .flags  = IORESOURCE_MEM,
    >                .start  = VT8500_UART0_PHYS_BASE,
    >                .end    = VT8500_UART0_PHYS_BASE + 0xff,
    >        },
    >        [1] = {
    >                .flags  = IORESOURCE_IRQ,
    >                .start  = VT8500_UART0_IRQ,
    >                .end    = VT8500_UART0_IRQ,
    >        },
    > };
    >
    > struct resource *vt8500_resources[] __initdata = {
    >        [VT8500_UART0] = &vt8500_resources_uart0,
    >        ...
    > };
    >
    > In devices.c:
    >
    > extern struct resource *vt8500_resources;
    > extern struct resource *wm8505_resources;
    >
    > /* Set this pointer according to board variant */
    > static struct resource *resources;
    >
    > void __init wmt_set_resources(void)
    > {
    >        vt8500_device_uart0.resource = resources[VT8500_UART0];
    >        ...
    > }
    >
    > This way we only have a single externed resource structure per
    > board-variant, there is no additional table needed, and the resource
    > definitions can be read clearly. Alternatively the wmt_regmaps/wmt_irqs
    > tables could be modified to use struct resource rather than individual
    > fields which would simplify the assignments later.

    This way we will again duplicate quite much: those files will mostly
    differ in just the macro definitions of specific registers/irqs.

    What if I just move all the initializations inside my runtime helper
    function, and add a macro to save space and improve readability?
    Something along the lines of:

    static struct resource resources_lcdc[2] __initdata;

    #define WMT_MMIO_RES(__start, __size) \
    {\
    .flags = IORESOURCE_MEM,\
    .start = __start,\
    .end = __start + __size - 1,\
    }
    #define WMT_IRQ_RES(__irq) \
    {\
    .flags = IORESOURCE_IRQ,\
    .start = __irq,\
    .end = __irq,\
    }

    void __init wmt_set_resources(void)
    {
    resources_lcdc[0] = WMT_MMIO_RES(wmt_current_regs->lcdc, SZ_1K);
    resources_lcdc[1] = WMT_IRQ_RES(wmt_current_irqs->lcdc);
    ...
    }

    Then there will be no half-empty initializations scattered around
    separate from the other assignments (which is probably the worst thing
    in current configuration).

    Best regards,
    Alexey
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2010-12-21 11:03    [W:0.038 / U:301.812 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site