Message-ID: <4D0FD747.9020700@bluewatersys.com>
Date:	Tue, 21 Dec 2010 11:23:03 +1300
From:	Ryan Mallon <ryan@...ewatersys.com>
To:	Alexey Charkov <alchark@...il.com>
CC:	Russell King - ARM Linux <linux@....linux.org.uk>,
	linux-arm-kernel@...ts.infradead.org,
	vt8500-wm8505-linux-kernel@...glegroups.com,
	Eric Miao <eric.y.miao@...il.com>,
	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>,
	Albin Tonnerre <albin.tonnerre@...e-electrons.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6 v9] ARM: Add basic architecture support for VIA/WonderMedia
 85xx SoC's

On 12/21/2010 10:48 AM, Alexey Charkov wrote:
> 2010/12/20 Ryan Mallon <ryan@...ewatersys.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@...il.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.

>>> +
>>> +void __init vt8500_map_io(void)
>>> +{
>>> +     wmt_current_regs = &wmt_regmaps[VT8500_INDEX];
>>> +     wmt_current_irqs = &wmt_irqs[VT8500_INDEX];
>>> +
>>> +     vt8500_io_desc[0].virtual = wmt_current_regs->mmio_regs_virt;
>>> +     vt8500_io_desc[0].pfn =
>>> +                     __phys_to_pfn(wmt_current_regs->mmio_regs_start);
>>> +     vt8500_io_desc[0].length = wmt_current_regs->mmio_regs_length;
>>> +
>>> +     iotable_init(vt8500_io_desc, ARRAY_SIZE(vt8500_io_desc));
>>> +}
>>> +
>>> +void __init wm8505_map_io(void)
>>> +{
>>> +     wmt_current_regs = &wmt_regmaps[WM8505_INDEX];
>>> +     wmt_current_irqs = &wmt_irqs[WM8505_INDEX];
>>> +
>>> +     vt8500_io_desc[0].virtual = wmt_current_regs->mmio_regs_virt;
>>> +     vt8500_io_desc[0].pfn =
>>> +                     __phys_to_pfn(wmt_current_regs->mmio_regs_start);
>>> +     vt8500_io_desc[0].length = wmt_current_regs->mmio_regs_length;
>>> +
>>> +     iotable_init(vt8500_io_desc, ARRAY_SIZE(vt8500_io_desc));
>>> +}
>>
>> Separate files. If more variants get added, this file will become
>> unwieldy very quickly.
>>
> 
> Is it really worthwhile to create separate files for single 12-line
> functions? After all, WonderMedia does not release new chips every now
> and then :)

I meant if you have the full peripheral initialisation for each variant
in its own file.

>>> +
>>> +void __init vt8500_reserve_mem(void)
>>> +{
>>> +#ifdef CONFIG_FB_VT8500
>>> +     panels[current_panel_idx].bpp = 16; /* Always use RGB565 */
>>> +     preallocate_fb(&panels[current_panel_idx], SZ_4M);
>>> +     vt8500_device_lcdc.dev.platform_data = &panels[current_panel_idx];
>>> +#endif
>>> +}
>>
>> Not sure if this should exist in the platform code or the framebuffer
>> driver. In the latter case it would automatically be CONFIG_FB_VT8500
>> and the platform_data can still be set in the platform code. Is there a
>> reason for this not to be in the framebuffer driver?
>>
> 
> I can't reserve memory via memblock from the driver, and usual runtime
> allocation functions can't handle it (need alignment to 4 megabytes in
> 8500, framebuffer sizes exceed 4 megabytes in 8505).

Can you use one of the initcalls from a driver to to the memblock
reserve? I don't know much about how memblock works. There are also the
various large page allocators in the works, but I don't think anything
has hit mainline yet.

>>> +#ifndef __ASM_ARCH_MEMORY_H
>>> +#define __ASM_ARCH_MEMORY_H
>>> +
>>> +/*
>>> + * Physical DRAM offset.
>>> + */
>>> +#define PHYS_OFFSET  UL(0x00000000)
>>
>> If you renamed this to PHYS_DRAM_OFFSET you wouldn't need the comment :-).
>>
> 
> I'm not the one who chooses :)

Oops, missed that :-).

>>> +void __init vt8500_init_irq(void)
>>> +{
>>> +     unsigned int i;
>>> +
>>> +     ic_regbase = ioremap(wmt_current_regs->ic0, SZ_64K);
>>> +
>>> +     if (ic_regbase) {
>>> +             /* Enable rotating priority for IRQ */
>>> +             writel((1 << 6), ic_regbase + 0x20);
>>> +             writel(0, ic_regbase + 0x24);
>>> +
>>> +             for (i = 0; i < wmt_current_irqs->nr_irqs; i++) {
>>> +                     /* Disable all interrupts and route them to IRQ */
>>> +                     writeb(0x00, ic_regbase + VT8500_IC_DCTR + i);
>>> +
>>> +                     set_irq_chip(i, &vt8500_irq_chip);
>>> +                     set_irq_handler(i, handle_level_irq);
>>> +                     set_irq_flags(i, IRQF_VALID);
>>> +             }
>>> +     } else {
>>> +             printk(KERN_ERR "Unable to remap the Interrupt Controller "
>>> +                             "registers, not enabling IRQs!\n");
>>
>> printk strings should be on a single line (can be > 80 columns) to make
>> grepping easier. You could also use the pr_ macros with pr_fmt set.
>>
> 
> Well, checkpatch.pl complained about that in the first place, so I
> split the line. Should I merge them back in all instances?

Yes. I think checkpatch has been changed to warn about spitting printk
strings across lines now.

> <snip>
> 
>>> diff --git a/arch/arm/mach-vt8500/pwm.c b/arch/arm/mach-vt8500/pwm.c
>>> new file mode 100644
>>> index 0000000..d1356a1
>>> --- /dev/null
>>> +++ b/arch/arm/mach-vt8500/pwm.c
>>
>> I'm not sure what the state of the various efforts to provide a common
>> pwm framework are, but you may want to check.
>>
> 
> I did before starting to write this code, found nothing.

Fair enough.

>>> +
>>> +static inline void pwm_busy_wait(void __iomem *reg, u8 bitmask)
>>> +{
>>> +     int loops = 1000;
>>> +     while ((readb(reg) & bitmask) && --loops)
>>> +             cpu_relax();
>>
>> Ugh. If you are going to busy wait, can't you delay for a known amount
>> of time? Even better, can this be replaced with wait_event or some
>> equivalent?
>>
> 
> The delay should be on the order of several bus cycles, where udelay
> actually busy-waits, too. wait_event would be longer than that to set
> up, and there is no associated interrupt.

I meant if the hardware has some specific maximum wait time then you
could just delay that long. If there is no interrupt then wait_event and
friends probably aren't going to work.

Maybe convert this to a timed loop (i.e. 1 second timeout) using
jiffies. That way you are never dependent on cpu speed. You should
probably also emit a warning if the timeout is reached and the device
still claims to be busy.

>>> +}
>>> +
>>> +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.

>>> +struct pwm_device *pwm_request(int pwm_id, const char *label)
>>> +{
>>> +     struct pwm_device *pwm;
>>> +     int found = 0;
>>> +
>>> +     mutex_lock(&pwm_lock);
>>> +
>>> +     list_for_each_entry(pwm, &pwm_list, node) {
>>> +             if (pwm->pwm_id == pwm_id) {
>>> +                     found = 1;
>>> +                     break;
>>> +             }
>>> +     }
>>> +
>>> +     if (found) {
>>> +             if (pwm->use_count == 0) {
>>> +                     pwm->use_count++;
>>> +                     pwm->label = label;
>>> +             } else
>>> +                     pwm = ERR_PTR(-EBUSY);
>>> +     } else
>>> +             pwm = ERR_PTR(-ENOENT);
>>> +
>>> +     mutex_unlock(&pwm_lock);
>>> +     return pwm;
>>> +}
>>
>> Maybe a bit clearer and more concise like this? Also replaces -ENOENT
>> (No such file or directory) with -ENODEV (No such device):
>>
>>        pwm = ERR_PTR(-ENODEV);
>>        mutex_lock(&pwm_lock);
>>
>>        list_for_each_entry(pwm, &pwm_list, node) {
>>                if (pwm->pwm_id == pwm_id) {
>>                        if (pwm->use_count != 0) {
>>                                pwm = ERR_PTR(-EBUSY);
>>                                break;
>>                        }
>>
>>                        pwm->use_count++;
>>                        pwm->label = label;
>>                        break;
>>                }
>>        }
>>
>>        mutex_unlock(&pwm_lock);
>>        return pwm;
>>
> 
> Isn't pwm overwritten inside the loop? -ENODEV will then be lost with
> this layout.

Oops, yes. You would need a second temporary pwm for the list iterator.
It's not a big deal anyway, just though it could be made more concise by
having all the code inside the loop.

>>> +static int __devinit pwm_probe(struct platform_device *pdev)
>>> +{
>>> +     struct pwm_device *pwms;
>>> +     struct resource *r;
>>> +     int ret = 0;
>>> +     int i;
>>> +
>>> +     pwms = kzalloc(sizeof(struct pwm_device) * VT8500_NR_PWMS, GFP_KERNEL);
>>> +     if (pwms == NULL) {
>>> +             dev_err(&pdev->dev, "failed to allocate memory\n");
>>> +             return -ENOMEM;
>>> +     }
>>
>> Devices should ideally be a single entity, so one platform device per pwm.
>>
> 
> We have 4 pwm outputs that share status registers, so they are not
> really separable.

Okay.

<snip>

>>> +static void vt8500_power_off(void)
>>> +{
>>> +     local_irq_disable();
>>
>> Is this necessary?
>>
> 
> Vendor's code disables interrupts. I believe my device refused to
> actually switch off without this.

Okay, fair enough.

> Thanks for the comments, Ryan!

No problem.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@...ewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists