Message-ID: <20150513153609.GB11677@kroah.com>
Date:	Wed, 13 May 2015 08:36:09 -0700
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	Pantelis Antoniou <pantelis.antoniou@...sulko.com>
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

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.

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


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

Can't you put these in a dt file?

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

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

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

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

thanks,

greg k-h
--
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