lkml.org 
[lkml]   [2010]   [Dec]   [20]   [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: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 :)

    > The indirection also makes it harder to quickly determine the IRQ number
    > of memory address of a peripheral.
    >
    > The current solution using the indirect table is okay, but it would be
    > nice to find a solution which reduces some of this effort.
    >
    >>>>>> +}
    >>>>>> +
    >>>>>> +int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
    >>>>>> +{
    >>>>>> +     unsigned long long c;
    >>>>>> +     unsigned long period_cycles, prescale, pv, dc;
    >>>>>> +
    >>>>>> +     if (pwm == NULL || period_ns == 0 || duty_ns > period_ns)
    >>>>>> +             return -EINVAL;
    >>>>>> +
    >>>>>> +     c = 25000000/2; /* wild guess --- need to implement clocks */
    >>>>>> +     c = c * period_ns;
    >>>>>> +     do_div(c, 1000000000);
    >>>>>> +     period_cycles = c;
    >>>>>
    >>>>> This looks like it could be reworked to remove the do_div call.
    >>>>>
    >>>>
    >>>> I just followed PXA implementation in this regard. Are there any
    >>>> specific suggestions? Note that c should not be a complie-time
    >>>> constant eventually, as this is the guessed PWM base frequency (should
    >>>> be read from the hardware, but the code for clocks is not yet in).
    >>>
    >>> I didn't have a particular solution in mind, but often by changing the
    >>> units used and rearranging the math a bit you can often avoid having to
    >>> do horrible multiplies and divides.
    >>>
    >>> For now at least you could avoid the do_div by assigning period_cycles
    >>> directly.
    >>>
    >>
    >> It depends on period_ns, which is passed in as an argument from
    >> whatever uses PWM, so I'm not sure it can be assigned directly.
    >
    > Ah. How big a number is period_ns? Can you do something like this instead?
    >
    >        period_cycles = ((250 / 2) * period_ns) / 10000;
    >
    > and still safely avoid overflows?
    >

    The only current in-kernel user of PWM is the backlight, and that
    currently uses period_ns = 250000. At this value it does not overflow.
    However, in a general case the base frequency will also be returned as
    a large number (like 12500000) as per CLK infrastructure conventions
    (once that part is implemented). Further, I can't see any built-in
    reasons for period_ns to be bounded by anything below sizeof(int). The
    hardware will work with up to 4096*1024*1000000000/base_frequency (see
    the code for constraints), so it can in principle overflow with 32 bit
    arithmetics.

    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 00:51    [W:0.062 / U:31.696 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site