Re: [PATCH] i.MX: vf610: Add support for ZII VF610 Dev Family

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Tue, Jan 24, 2017 at 12:09 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> Hi Andrey,
>
> On Sun, Jan 22, 2017 at 09:57:34PM -0800, Andrey Smirnov wrote:
>> Add support for ZII VF610 Dev based designs such as:
>>
>>     - VF610 Dev, revision B
>>     - VF610 Dev, revision C
>>     - CFU1, revision A
>>     - SPU3, revision A
>>     - SCU4 AIB, revision C
>>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>
>> Sascha, this patch is rebased on 'next' instead of 'master' so that
>> you won't have to resolve conflicts with RDU2 patches in 'next'. Let
>> me know if you'd rather have it rebased on 'master'.
>
> It's fine to base on next in this case.
>
>> +struct named_signal {
>> +     unsigned int gpio;
>> +     const char *name;
>> +     int value;
>> +};
>> +
>> +static int expose_signals(const struct named_signal *signals,
>> +                           size_t signal_num)
>> +{
>> +     int ret, i;
>> +
>> +     for (i = 0; i < signal_num; i++) {
>> +             const struct named_signal *signal = &signals[i];
>> +
>> +             if (signal->value < 0)
>> +                     ret = gpio_direction_input(signal->gpio);
>> +             else
>> +                     ret = gpio_direction_output(signal->gpio, signal->value);
>> +
>> +             if (ret) {
>> +                     pr_err("Failed to configure \"%s\"\n", signal->name);
>> +                     return ret;
>> +             }
>
> This looks like gpio_request_array(). Could you use this instead?

Almost. Unfortunately, gpio_request_array doesn't do much with "label'
portion of a descriptor, except to use it when displaying information
about GPIOs.

What I am doing here as well is exposing those GPIO in a very
primitive way by calling

export_env_ull(signal->name, signal->gpio);

and so it becomes possible to do things like

gpio_set_value ${soc_sw_rstn} 0

instead of having to use numeric value to designate desired GPIO. If
there's a way this can be done better or if there a good change we can
make to gpio_request_array(), I am more than happy to change this
code.

>
>> index 0000000..4465632
>> --- /dev/null
>> +++ b/arch/arm/boards/zii-vf610-dev/lowlevel.c
>> @@ -0,0 +1,132 @@
>> +#include <common.h>
>> +#include <linux/sizes.h>
>> +#include <mach/generic.h>
>> +#include <asm/barebox-arm-head.h>
>> +#include <asm/barebox-arm.h>
>> +#include <mach/vf610-regs.h>
>> +#include <mach/clock-vf610.h>
>> +#include <mach/iomux-vf610.h>
>> +#include <debug_ll.h>
>> +
>> +#if defined(CONFIG_DEBUG_LL)
>> +#    if !defined(CONFIG_DEBUG_VF610_UART)
>> +#            warning "CONFIG_DEBUG_LL is enabled but CONFIG_DEBUG_VF610_UART is not"
>> +#    else
>> +#            if CONFIG_DEBUG_IMX_UART_PORT != 1
>> +#                    warning "CONFIG_DEBUG_LL output is not drirected to port 1"
>> +#            endif
>> +#    endif
>> +#endif
>
> Just because this board is enabled doesn't mean the user is actually
> currently interested in this board. He might want to enable
> CONFIG_DEBUG_LL for another board, still he is warned about an invalid
> config here. This is not good.
>

Heh, I added this portion of the code because I was having exactly the
opposite issue, but I see your point.

> You could drop the DEBUG_LL support and instead go to PBL console
> support. See arch/arm/boards/freescale-mx6-sabrelite/lowlevel.c
> for a good example. Basically it means you call this right after entry:
>
>         arm_early_mmu_cache_invalidate();
>         relocate_to_current_adr();
>         setup_c();
>         barrier();
>
> From here on you have a valid C environment (And thus don't need to work
> around switch/case with wrong LUTs) and can also call:
>
>         pbl_set_putc(xxx_uart_putc, uart);
>
> after that you can use printf like functions to print messages.
>

Thank you for the suggestion, if it is all the same to you I think
I'll just drop the preprocessor check, though. DEBUG_LL is more of a
debugging feature that very few people would use and switching to PBL
console would mean a bunch of additional work.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox