[rpi bsp] add cmdline options

[QIAO YANG]

On Jul 29, 2015, at 07:56 AM, Pavel Pisa <pisa at cmp.felk.cvut.cz> wrote:

> Hello Qiao Yang,
>
> On Tuesday 28 of July 2015 01:35:45 Pavel Pisa wrote:
>> Hello Qiao Yang,
>>
>> On Friday 24 of July 2015 18:04:47 QIAO YANG wrote:
>> > Some updates for you. I've found out that my previous problem with
>> > cmdline due to my minicom didn't wrap up the lines.... I've chosen the
>> > interface of vc to retrieve cmdline so that we don't need to parse the
>> > atag nor devicetree. Now we can use --video=... to choose resolution and
>> > --console=... to choose console.
>
> I have got to little more testing. Ticker runs correctly
> with console on serial port and even with graphics console.
> But option --console=fbcons works only if it is the last option
> on the line in cmdline.txt. Problem is in the comparison because
> if there are more options then string provided by rpi_cmdline_arg()
> continues after "fbcons" by blank character. I think that
> copy of the string to the user provided buffer is not necessary
> in rpi_cmdline_arg(). It would bring up issues with space enough
> to fit data etc. I have use simple strncmp to resolve issue.
> But something more elegant can be provided.
>
> diff --git a/c/src/lib/libbsp/arm/raspberrypi/console/console_select.c b/c/src/lib/libbsp/arm/raspberrypi
> index 74cf78e..ad24d33 100644
> --- a/c/src/lib/libbsp/arm/raspberrypi/console/console_select.c
> +++ b/c/src/lib/libbsp/arm/raspberrypi/console/console_select.c
> @@ -91,7 +91,7 @@ void bsp_console_select(void)
> if (opt)
> {
> opt += sizeof("--console=")-1;
> - if (strcmp(opt,"fbcons") == 0)
> + if (strncmp(opt,"fbcons", sizeof("fbcons"-1)) == 0)
> {
> Console_Port_Minor = BSP_CONSOLE_FB;
> BSPPrintkPort = BSP_CONSOLE_FB;
> @@ -99,8 +99,8 @@ void bsp_console_select(void)
> }
> else
> {
> - Console_Port_Minor = BSP_CONSOLE_FB;
> - BSPPrintkPort = BSP_CONSOLE_FB;
> + Console_Port_Minor = BSP_CONSOLE_UART0;
> + BSPPrintkPort = BSP_CONSOLE_UART0;
> }
>
>
> /*
>
>> As for the interface to command line through VC, I expect that
>> this interface allows access only to value set in config.txt
>> but it is not changed by U-boot if used. If that is true
>> then use of ATAG or device tree would be better for flexibility.
>> It would be reusable on other ARM targets as well.
>> But that this can be tried in some follow up project.
>
> It seems to me that behavior is as expected, VideoCore
> returns what it parsed from cmdline.txt but arguments
> set by U-boot are different. So long term term solution
> is to implement shared ARM BSP atags and devicetree parser.
> It can be minimal for start only to locate kernel command
> line. It would help to other ARM BSPs in general.
>
> MMU setup seems to be OK for me, but what I really
> dislike is duplicate of arm-cp15-start.h to RPi.
> Shared one should be used.
>
> Les problem is to use different name for
> arm_cp15_start_mmu_config_table to not clash
> with the definition in header.
>
> I think that the name arm_cp15_start_mmu_config_table
> is not part of RTEMS API so it can be changed freely
> and if original arm_cp15_start_mmu_config_table
> is not defined then incorrect use is catch
> easily.
>
> Please, try to resolve this first. Then the suggested
> change for strncmp and do more testing.
>
> I suggest to adapt rpi_init_cmdline() a little.
>
> void rpi_init_cmdline(void)
> {
> bcm2835_get_cmdline_entries get_cmdline_entries;
> bcm2835_mailbox_get_cmdline(&get_cmdline_entries);
> int i; /*I suggest to be C89 compatible, do not
> mix declaration with code in single block */
> for (i = 0; i < MAX_CMDLINE_LENGTH; ++i)
> {
> _rpi_cmdline[i] = get_cmdline_entries.cmdline[i];
> }
> }
>
> It is not necessary to place 512 bytes onto stack.
> If you declare get_cmdline_entries static global
>
> static bcm2835_get_cmdline_entries get_cmdline_entries;
>
> Then you can read the line to this global.
>
> If you declare then _rpi_cmdline as
>
> char *_rpi_cmdline;
 
This is reasonable. I've updated it.
>
>
> you only point it into right place and do not need
> copy. You should add code, which ensures that command
> line is zero terminated if you use strstr() later.
>
> You can use something like
>
>
> bcm2835_mailbox_get_cmdline(&get_cmdline_entries);
> get_cmdline_entries.cmdline[sizeof(get_cmdline_entries.cmdline) - 1] = 0;
After calling mailbox, I know exactly the length of cmdline and when I return the value to the entries, I've added a zero terminated character at the end. 

>
>
> Generally code division to the functions
> and files looks good.
>
> Best wishes,
>
> Pavel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20150803/7c19b449/attachment.html>