Date:	Wed, 13 May 2015 18:51:57 +0300
From:	Pantelis Antoniou <pantelis.antoniou@...sulko.com>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	Matt Porter <mporter@...sulko.com>,
	Koen Kooi <koen@...inion.thruhere.net>,
	Robert Nelson <robertcnelson@...il.com>,
	Rob Herring <robherring2@...il.com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Jonathan Corbet <corbet@....net>,
	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
	Guenter Roeck <linux@...ck-us.net>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Benoît Cousso <bcousson@...libre.com>,
	linux-api@...r.kernel.org, linux-doc@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] misc: Beaglebone capemanager

Hi Greg,

> On May 13, 2015, at 18:36 , Greg Kroah-Hartman <gregkh@...uxfoundation.org> wrote:
> 
> On Wed, May 13, 2015 at 03:10:25PM +0300, Pantelis Antoniou wrote:
>> Hi Greg,
>> 
>>> On May 13, 2015, at 14:55 , Greg Kroah-Hartman <gregkh@...uxfoundation.org> wrote:
>>> 
>>> On Wed, May 13, 2015 at 10:59:41AM +0300, Pantelis Antoniou wrote:
>>>> A cape loader based on DT overlays and DT objects.
>>>> 
>>>> This is the beaglebone cape manager which allows capes to be automatically
>>>> probed and instantiated via means of a device tree overlay deduced from
>>>> the part-number and version contained on the cape's EEPROM.
>>>> 
>>>> The reference manual contains information about the specification
>>>> and the contents of the EEPROM.
>>>> 
>>>> http://beagleboard.org/static/beaglebone/latest/Docs/Hardware/BONE_SRM.pdf
>>>> 
>>>> Documentation about the workings of the cape manager is located
>>>> in Documentation/misc-devices/bone_capemgr.txt
>>>> 
>>>> This driver is using the EEPROM framework interface to retrieve
>>>> the data stored on the baseboard and cape EEPROMs.
>>>> 
>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@...sulko.com>
>>>> ---
>>>> drivers/misc/Kconfig        |   10 +
>>>> drivers/misc/Makefile       |    1 +
>>>> drivers/misc/bone_capemgr.c | 1926 +++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 1937 insertions(+)
>>>> create mode 100644 drivers/misc/bone_capemgr.c
>>>> 
>>>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>>>> index 006242c..f9e09e1 100644
>>>> --- a/drivers/misc/Kconfig
>>>> +++ b/drivers/misc/Kconfig
>>>> @@ -515,6 +515,16 @@ config VEXPRESS_SYSCFG
>>>> 	  bus. System Configuration interface is one of the possible means
>>>> 	  of generating transactions on this bus.
>>>> 
>>>> +config BONE_CAPEMGR
>>>> +	tristate "Beaglebone cape manager"
>>>> +	depends on ARCH_OMAP2PLUS && OF
>>>> +	select EEPROM
>>>> +	select OF_OVERLAY
>>>> +	default n
>>> 
>>> N is always the default, please remove.
>>> 
>> 
>> OK
>> 
>>>> +	help
>>>> +	  Say Y here to include support for automatic loading of
>>>> +	  beaglebone capes.
>>>> +
>>> 
>>> What about if it's a module?
>>> 
>>> 
>> 
>> It should work but it’s going to be weird (i.e. no-one has used it as such).
>> I’ll update the blurb.
> 
> You are saying it is a valid config option, so please test it as such,
> or if it's not a valid config option, don't allow it.
> 

It is a valid option, just no-one has a use for it. Who knows what kind of
crazy setup the end-users will use.

I’ll test it as a module to make sure.

>>>> source "drivers/misc/c2port/Kconfig"
>>>> source "drivers/misc/eeprom/Kconfig"
>>>> source "drivers/misc/cb710/Kconfig"
>>>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>>>> index 7d5c4cd..659b78b 100644
>>>> --- a/drivers/misc/Makefile
>>>> +++ b/drivers/misc/Makefile
>>>> @@ -56,3 +56,4 @@ obj-$(CONFIG_GENWQE)		+= genwqe/
>>>> obj-$(CONFIG_ECHO)		+= echo/
>>>> obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
>>>> obj-$(CONFIG_CXL_BASE)		+= cxl/
>>>> +obj-$(CONFIG_BONE_CAPEMGR)	+= bone_capemgr.o
>>>> diff --git a/drivers/misc/bone_capemgr.c b/drivers/misc/bone_capemgr.c
>>>> new file mode 100644
>>>> index 0000000..423719c
>>>> --- /dev/null
>>>> +++ b/drivers/misc/bone_capemgr.c
>>>> @@ -0,0 +1,1926 @@
>>>> +/*
>>>> + * TI Beaglebone cape manager
>>>> + *
>>>> + * Copyright (C) 2012 Texas Instruments Inc.
>>>> + * Copyright (C) 2012-2015 Konsulko Group.
>>>> + * Author: Pantelis Antoniou <pantelis.antoniou@...sulko.com>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License as published by
>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>> + * (at your option) any later version.
>>> 
>>> I have to ask, do you really mean, "or any later version”?
>>> 
>> 
>> Yes, this is purely a community thing. No evil vendor at play at all.
> 
> I have no idea what that means.
> 
> 

That this is purely work for the community. No-one pays for it.

>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>> + * GNU General Public License for more details.
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/completion.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/clk.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/of_fdt.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/pm_runtime.h>
>>>> +#include <linux/pinctrl/consumer.h>
>>>> +#include <linux/firmware.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/ctype.h>
>>>> +#include <linux/string.h>
>>>> +#include <linux/memory.h>
>>>> +#include <linux/kthread.h>
>>>> +#include <linux/wait.h>
>>>> +#include <linux/file.h>
>>>> +#include <linux/fs.h>
>>>> +#include <linux/eeprom-consumer.h>
>>>> +
>>>> +/* disabled capes */
>>>> +static char *disable_partno;
>>>> +module_param(disable_partno, charp, 0444);
>>>> +MODULE_PARM_DESC(disable_partno,
>>>> +		"Comma delimited list of PART-NUMBER[:REV] of disabled capes");
>>>> +
>>>> +/* enable capes */
>>>> +static char *enable_partno;
>>>> +module_param(enable_partno, charp, 0444);
>>>> +MODULE_PARM_DESC(enable_partno,
>>>> +		"Comma delimited list of PART-NUMBER[:REV] of enabled capes");
>>>> +
>>>> +/* delay to scan on boot until rootfs appears */
>>>> +static int boot_scan_period = 1000;
>>>> +module_param(boot_scan_period, int, 0444);
>>>> +MODULE_PARM_DESC(boot_scan_period,
>>>> +		"boot scan period until rootfs firmware is available");
>>> 
>>> Ick, no module parameters please, can't we drop these?
>>> 
>> 
>> In a nutshell, no :)
>> 
>> These has to be way to control whether a cape is disabled (if found) and enabled (if the manufacturer in it’s infinite wisdom removed the EEPROM to save $0.01 per cape).
> 
> Please line-wrap your responses.
> 
>> The easiest way to achieve this is with the kernel command line.
> 
> No it is not.  You can't easily change the kernel command line for an
> embedded system (or so everyone keeps telling me when I say things like
> "just update your kernel command line!")  So embedded people can't have
> it both ways, sorry.
> 

Depends on the embedded system. On the ones I’m working with a sane bootloader
(for example u-boot) the command line is easily modified.

> Can't you put these in a dt file?
> 

Do you really want to start this argument? Didn’t we used to have great flamewars
about putting linux specific things in the DT?

Does enabling & disabling capes fall into ‘hardware description’?

I guess we’ll have to ask the DT maintainers for that.

Rob, Grant? 

>>> And we have a rootfs delay option already for the whole system, this
>>> module shouldn't need a special one.
>>> 
>> 
>> No, that won’t work.
>> 
>> You see the problem is as follows.
>> 
>> The capemanager is not built as a module, it is builtin.
> 
> Not according to your Kconfig file :)
> 

Touché

>> By the time the probe method is called the rootfs has no been mounted yet.
>> This is actually what we want for the cases where the rootfs resides on a cape and
>> the cape’s dtbo firmware file is built-in the kernel image.
>> 
>> This does not work when the firmware file is located in the rootfs filesystem, since
>> the call to request_firmware will fail at that time.
> 
> Then you let it happen later.  Or do it async.  Don't create new command
> line options for when we already have the same command line option!
> 

Err, the two options are nothing alike no? One deals is defined as:

rootdelay: "Delay (in seconds) to pause before attempting to mount the root filesystem"

The other is the period with which to perform period firmware requests.

>> That option controls the polling delay until the system boot state indicates that the
>> rootfs is available and the call will succeed.
> 
> If this is such an issue, just sit and spin and wait for it to show up.
> 

I can do that; that would get rid of the option too.

>> There are lots of warts in our firmware loader.
> 
> You can fix them, don't make the kernel work around the warts because
> you don't want to :)
> 

Yes, but that’s not the case here. I have in mind fixing some problems with the
firmware loader, but http://www.imdb.com/title/tt0070355/quotes?item=qt1629888


> thanks,
> 
> greg k-h


Regards

— Pantelis

--
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