Re: [PATCH 1/5] misc: Beaglebone capemanager

From: Pantelis Antoniou
Date: Wed May 13 2015 - 11:52:10 EST

Next message: Michal Suchanek: "Re: [PATCH] spi: Force the registration of the spidev devices"
Previous message: Jeff Moyer: "Re: [PATCH] blk: don't account discard request size"
In reply to: Greg Kroah-Hartman: "Re: [PATCH 1/5] misc: Beaglebone capemanager"
Next in thread: Geert Uytterhoeven: "Re: [PATCH 1/5] misc: Beaglebone capemanager"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Greg,

> On May 13, 2015, at 18:36 , Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxxxx>
>>>> + *
>>>> + * 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Michal Suchanek: "Re: [PATCH] spi: Force the registration of the spidev devices"
Previous message: Jeff Moyer: "Re: [PATCH] blk: don't account discard request size"
In reply to: Greg Kroah-Hartman: "Re: [PATCH 1/5] misc: Beaglebone capemanager"
Next in thread: Geert Uytterhoeven: "Re: [PATCH 1/5] misc: Beaglebone capemanager"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]