Message ID | 1390212438-6855-1-git-send-email-s.hauer@pengutronix.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi, Sascha Hauer wrote: > When a firmware cannot be found for the SDMA engine then we can > continue with the inernal ROM firmware. > > The meaning of this message is frequently asked for, so make clear > that the driver still works with the internal ROM firmware and reduce > the loglevel from err to info. > In case user space firmware loading > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Vinod Koul <vinod.koul@intel.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: Shawn Guo <shawn.guo@linaro.org> > --- > > changes since v1: > - instead of removing the message make it more clear > > drivers/dma/imx-sdma.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > index c75679d..d79eaad 100644 > --- a/drivers/dma/imx-sdma.c > +++ b/drivers/dma/imx-sdma.c > @@ -1259,7 +1259,10 @@ static void sdma_load_firmware(const struct firmware *fw, void *context) > unsigned short *ram_code; > > if (!fw) { > - dev_err(sdma->dev, "firmware not found\n"); > + dev_info(sdma->dev, "external firmware not found, using ROM firmware\n"); > + /* > + * In this case we just use the ROM firmware. > + */ > return; > } >
Hi, Lothar Waßmann wrote: > Hi, > erroneously pressed the 'Send' button... > Sascha Hauer wrote: > > When a firmware cannot be found for the SDMA engine then we can > > continue with the inernal ROM firmware. > > > > The meaning of this message is frequently asked for, so make clear > > that the driver still works with the internal ROM firmware and reduce > > the loglevel from err to info. > > In case user space firmware loading support (CONFIG_FW_LOADER) is enabled, this message may still be inadequate, since the firmware may very well be loaded lateron. Lothar Waßmann
On Mon, Jan 20, 2014 at 12:38:50PM +0000, Russell King - ARM Linux wrote: > On Mon, Jan 20, 2014 at 12:36:27PM +0100, Lothar Waßmann wrote: > > In case user space firmware loading support (CONFIG_FW_LOADER) is > > enabled, this message may still be inadequate, since the firmware may > > very well be loaded lateron. > > I haven't worked out whether that's actually possible - I saw no way to > re-trigger the firmware request, and once the firmware request expires, > there seems to be no way for userspace to say "okay, the firmware is now > available, please load it". In that case why don't we use async version and wait till the userspace is availble and firmware is loaded. So request_firmware_nowait() seems to do the job, I have used in a device where I call it in probe() so that not to slow booting and make firmware bloc availble userpsace is availble. Sounds too good, did I miss anything obvious for this case here?
On Mon, Jan 20, 2014 at 12:36:27PM +0100, Lothar Waßmann wrote: > In case user space firmware loading support (CONFIG_FW_LOADER) is > enabled, this message may still be inadequate, since the firmware may > very well be loaded lateron. I haven't worked out whether that's actually possible - I saw no way to re-trigger the firmware request, and once the firmware request expires, there seems to be no way for userspace to say "okay, the firmware is now available, please load it".
On Mon, Jan 20, 2014 at 05:29:20PM +0530, Vinod Koul wrote: > On Mon, Jan 20, 2014 at 12:38:50PM +0000, Russell King - ARM Linux wrote: > > On Mon, Jan 20, 2014 at 12:36:27PM +0100, Lothar Waßmann wrote: > > > In case user space firmware loading support (CONFIG_FW_LOADER) is > > > enabled, this message may still be inadequate, since the firmware may > > > very well be loaded lateron. > > > > I haven't worked out whether that's actually possible - I saw no way to > > re-trigger the firmware request, and once the firmware request expires, > > there seems to be no way for userspace to say "okay, the firmware is now > > available, please load it". > In that case why don't we use async version and wait till the userspace is > availble and firmware is loaded. > > So request_firmware_nowait() seems to do the job, I have used in a device where > I call it in probe() so that not to slow booting and make firmware bloc availble > userpsace is availble. > > Sounds too good, did I miss anything obvious for this case here? Been there, done that. This would make the RAM Firmware mandatory. The Linux firmware loading mechanism is not very suitable for the SDMA engine. The firmware loading mechanism expects that a driver cannot work without a firmware. Normally this is true, but The SDMA engine can run with the ROM firmware or a RAM firmware loaded during runtime. In fact the firmware consists of Program code, so the SDMA engine could even run with a combination of the ROM firmware and 0..n dynamically loaded firmware modules. Maybe using the firmware loading mechanism for the SDMA engine was the wrong approach from the start. Sascha
Hi, Russell King - ARM Linux wrote: > On Mon, Jan 20, 2014 at 12:36:27PM +0100, Lothar Waßmann wrote: > > In case user space firmware loading support (CONFIG_FW_LOADER) is > > enabled, this message may still be inadequate, since the firmware may > > very well be loaded lateron. > > I haven't worked out whether that's actually possible - I saw no way to > re-trigger the firmware request, and once the firmware request expires, > there seems to be no way for userspace to say "okay, the firmware is now > available, please load it". > I can confirm that it does work, since I'm using it. I have the following in my /etc/init.d/mdev.sh script: | [ "$VERBOSE" = no ] || echo -n "Triggering firmware load" | for dir in $(find /sys/class/firmware/ -type l);do | echo add > "$dir/uevent" | done Lothar
On Mon, Jan 20, 2014 at 03:09:02PM +0100, Lothar Waßmann wrote: > Hi, > > Russell King - ARM Linux wrote: > > On Mon, Jan 20, 2014 at 12:36:27PM +0100, Lothar Waßmann wrote: > > > In case user space firmware loading support (CONFIG_FW_LOADER) is > > > enabled, this message may still be inadequate, since the firmware may > > > very well be loaded lateron. > > > > I haven't worked out whether that's actually possible - I saw no way to > > re-trigger the firmware request, and once the firmware request expires, > > there seems to be no way for userspace to say "okay, the firmware is now > > available, please load it". > > > I can confirm that it does work, since I'm using it. > I have the following in my /etc/init.d/mdev.sh script: > | [ "$VERBOSE" = no ] || echo -n "Triggering firmware load" > | for dir in $(find /sys/class/firmware/ -type l);do > | echo add > "$dir/uevent" > | done I can't check on iMX6 at the moment, but I did look in that directory after the iMX6 SDMA reported that it hadn't found any firmware, and it was empty apart from the "timeout" file.
Hi, Russell King - ARM Linux wrote: > On Mon, Jan 20, 2014 at 03:09:02PM +0100, Lothar Waßmann wrote: > > Hi, > > > > Russell King - ARM Linux wrote: > > > On Mon, Jan 20, 2014 at 12:36:27PM +0100, Lothar Waßmann wrote: > > > > In case user space firmware loading support (CONFIG_FW_LOADER) is > > > > enabled, this message may still be inadequate, since the firmware may > > > > very well be loaded lateron. > > > > > > I haven't worked out whether that's actually possible - I saw no way to > > > re-trigger the firmware request, and once the firmware request expires, > > > there seems to be no way for userspace to say "okay, the firmware is now > > > available, please load it". > > > > > I can confirm that it does work, since I'm using it. > > I have the following in my /etc/init.d/mdev.sh script: > > | [ "$VERBOSE" = no ] || echo -n "Triggering firmware load" > > | for dir in $(find /sys/class/firmware/ -type l);do > > | echo add > "$dir/uevent" > > | done > > I can't check on iMX6 at the moment, but I did look in that directory > after the iMX6 SDMA reported that it hadn't found any firmware, and > it was empty apart from the "timeout" file. > The file is automatically removed after the timeout has expired (or the frimware has been loaded). Thus you must check for it within the timeout period during boot. Lothar Waßmann
Hi Lothar, On Mon, Jan 20, 2014 at 12:36:27PM +0100, Lothar Waßmann wrote: > Hi, > > Lothar Waßmann wrote: > > Hi, > > > erroneously pressed the 'Send' button... > > > Sascha Hauer wrote: > > > When a firmware cannot be found for the SDMA engine then we can > > > continue with the inernal ROM firmware. > > > > > > The meaning of this message is frequently asked for, so make clear > > > that the driver still works with the internal ROM firmware and reduce > > > the loglevel from err to info. > > > > In case user space firmware loading support (CONFIG_FW_LOADER) is > enabled, this message may still be inadequate, since the firmware may > very well be loaded lateron. Then at least until the RAM firmware is loaded the message is still adequate, no? We could add an addional message when the firmware is loaded if you like. Sascha
Hi, Sascha Hauer wrote: > Hi Lothar, > > On Mon, Jan 20, 2014 at 12:36:27PM +0100, Lothar Waßmann wrote: > > Hi, > > > > Lothar Waßmann wrote: > > > Hi, > > > > > erroneously pressed the 'Send' button... > > > > > Sascha Hauer wrote: > > > > When a firmware cannot be found for the SDMA engine then we can > > > > continue with the inernal ROM firmware. > > > > > > > > The meaning of this message is frequently asked for, so make clear > > > > that the driver still works with the internal ROM firmware and reduce > > > > the loglevel from err to info. > > > > > > In case user space firmware loading support (CONFIG_FW_LOADER) is > > enabled, this message may still be inadequate, since the firmware may > > very well be loaded lateron. > > Then at least until the RAM firmware is loaded the message is still adequate, > no? We could add an addional message when the firmware is loaded if you > like. > There is already a message printed upon successful firmware load: |imx-sdma 63fb0000.sdma: loaded firmware 1.1 But one message saying 'firmware load failed' and another saying the opposite is a bit confusing. There actually is also a message saying 'firmware not found' when the timeout expires without any firmware being loaded. Thus at least in case CONFIG_FW_LOADER is enabled the message 'Direct firmware load failed' could be omitted. Lothar Waßmann
On Tue, Jan 21, 2014 at 07:59:22AM +0100, Lothar Waßmann wrote: > The file is automatically removed after the timeout has expired (or > the frimware has been loaded). > Thus you must check for it within the timeout period during boot. ... which is impossible if imx-sdma is built into the kernel - it expires while the initramfs has only just started running, giving a very narrow window for userspace to load firmware.
Hi, Russell King - ARM Linux wrote: > On Tue, Jan 21, 2014 at 07:59:22AM +0100, Lothar Waßmann wrote: > > The file is automatically removed after the timeout has expired (or > > the frimware has been loaded). > > Thus you must check for it within the timeout period during boot. > > ... which is impossible if imx-sdma is built into the kernel - it > expires while the initramfs has only just started running, giving a > very narrow window for userspace to load firmware. > It works for me, but I'm not using initramfs. Lothar Waßmann
On Tue, Jan 21, 2014 at 12:52:00PM +0100, Lothar Waßmann wrote: > Hi, > > Russell King - ARM Linux wrote: > > On Tue, Jan 21, 2014 at 07:59:22AM +0100, Lothar Waßmann wrote: > > > The file is automatically removed after the timeout has expired (or > > > the frimware has been loaded). > > > Thus you must check for it within the timeout period during boot. > > > > ... which is impossible if imx-sdma is built into the kernel - it > > expires while the initramfs has only just started running, giving a > > very narrow window for userspace to load firmware. > > > It works for me, but I'm not using initramfs. You can take my email as a report of "it doesn't work for everyone." The *only* way I can get firmware to load is to build imx-sdma as a module. Building it in is a hopeless case here.
Hi, Russell King - ARM Linux wrote: > On Tue, Jan 21, 2014 at 12:52:00PM +0100, Lothar Waßmann wrote: > > Hi, > > > > Russell King - ARM Linux wrote: > > > On Tue, Jan 21, 2014 at 07:59:22AM +0100, Lothar Waßmann wrote: > > > > The file is automatically removed after the timeout has expired (or > > > > the frimware has been loaded). > > > > Thus you must check for it within the timeout period during boot. > > > > > > ... which is impossible if imx-sdma is built into the kernel - it > > > expires while the initramfs has only just started running, giving a > > > very narrow window for userspace to load firmware. > > > > > It works for me, but I'm not using initramfs. > > You can take my email as a report of "it doesn't work for everyone." The > *only* way I can get firmware to load is to build imx-sdma as a module. > Building it in is a hopeless case here. > You could change the timeout from your initramfs as soon as sysfs has been mounted by writing the number of seconds to /sys/class/firmware/timeout. A timeout of '0' means 'wait forever' which should be long enough for any boot case. ;) Lothar Waßmann
On Tue, Jan 21, 2014 at 02:36:56PM +0100, Lothar Waßmann wrote: > Hi, > > Russell King - ARM Linux wrote: > > On Tue, Jan 21, 2014 at 12:52:00PM +0100, Lothar Waßmann wrote: > > > Hi, > > > > > > Russell King - ARM Linux wrote: > > > > On Tue, Jan 21, 2014 at 07:59:22AM +0100, Lothar Waßmann wrote: > > > > > The file is automatically removed after the timeout has expired (or > > > > > the frimware has been loaded). > > > > > Thus you must check for it within the timeout period during boot. > > > > > > > > ... which is impossible if imx-sdma is built into the kernel - it > > > > expires while the initramfs has only just started running, giving a > > > > very narrow window for userspace to load firmware. > > > > > > > It works for me, but I'm not using initramfs. > > > > You can take my email as a report of "it doesn't work for everyone." The > > *only* way I can get firmware to load is to build imx-sdma as a module. > > Building it in is a hopeless case here. > > > You could change the timeout from your initramfs as soon as sysfs has > been mounted by writing the number of seconds to > /sys/class/firmware/timeout. A timeout of '0' means 'wait forever' which > should be long enough for any boot case. ;) Err, no. Let's walk through the code, shall we? imx-sdma calls request_firmware_nowait() for the firmware. request_firmware_nowait() allocates a work structure, saves all the details, puts it on a queue, and triggers a workqueue to process this. When the work structure is dequeued, request_firmware_work_func() is called. This calls _request_firmware() with the appropriate parameters, including nowait = true. _request_firmware() reads the timeout, and then calls usermodehelper_read_lock_wait() with it. (Once this has happened, changing the timeout has no effect.) This waits for the requisit amount of time for usermodehelper_disabled. Once that happens, fw_get_filesystem_firmware() is called by _request_firmware(), which attempts to read the file from the mounted filesystem. If that fails, the kernel falls back to fw_load_from_user_helper() to invoke a usermode helper to load the firmware. This creates a file in /sys/class/firmware, and issues a uevent, sets a timeout using the timeout value read above, and waits either for the request to be satisfied or a timeout to happen. So... increasing the timeout can _only_ have an effect if it is done before the request is created - which, if imx-sdma is built into the kernel will be impossible to achieve. What I suspect is happening is that ubuntu's initramfs doesn't contain the firmware, but it unblocks the user helper (since it contains and runs udev) which also fails to find the firmware - and there's no provision for the firmware to be included in ubuntu's initramfs. So, let's go back to the point I've been making. If you build imx-sdma into the kernel, there are situations where it is impossible to load replacement firmware, because the attempt to load firmware expires long before there is any chance for it to be loaded. Maybe you don't personally care about ubuntu. Good for you, that's your personal choice. That has absolutely no bearing on this issue though.
On Mon, Jan 20, 2014 at 8:07 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > When a firmware cannot be found for the SDMA engine then we can > continue with the inernal ROM firmware. > > The meaning of this message is frequently asked for, so make clear > that the driver still works with the internal ROM firmware and reduce > the loglevel from err to info. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Vinod Koul <vinod.koul@intel.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: Shawn Guo <shawn.guo@linaro.org> > --- > > changes since v1: > - instead of removing the message make it more clear I know this has been posted a long time ago and agree that it would be useful to change this dev_err code into dev_info: Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com> > > drivers/dma/imx-sdma.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > index c75679d..d79eaad 100644 > --- a/drivers/dma/imx-sdma.c > +++ b/drivers/dma/imx-sdma.c > @@ -1259,7 +1259,10 @@ static void sdma_load_firmware(const struct firmware *fw, void *context) > unsigned short *ram_code; > > if (!fw) { > - dev_err(sdma->dev, "firmware not found\n"); > + dev_info(sdma->dev, "external firmware not found, using ROM firmware\n"); > + /* > + * In this case we just use the ROM firmware. > + */ > return; > } > > -- > 1.8.5.2
On Sat, Oct 25, 2014 at 10:28:15AM -0200, Fabio Estevam wrote: > On Mon, Jan 20, 2014 at 8:07 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > > When a firmware cannot be found for the SDMA engine then we can > > continue with the inernal ROM firmware. > > > > The meaning of this message is frequently asked for, so make clear > > that the driver still works with the internal ROM firmware and reduce > > the loglevel from err to info. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > Cc: Vinod Koul <vinod.koul@intel.com> > > Cc: linux-arm-kernel@lists.infradead.org > > Cc: Shawn Guo <shawn.guo@linaro.org> > > --- > > > > changes since v1: > > - instead of removing the message make it more clear > > I know this has been posted a long time ago and agree that it would be > useful to change this dev_err code into dev_info: In that case cn someone pls repost this...
On Wed, Nov 12, 2014 at 9:13 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> In that case cn someone pls repost this...
Sure, just resent it. Thanks
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index c75679d..d79eaad 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -1259,7 +1259,10 @@ static void sdma_load_firmware(const struct firmware *fw, void *context) unsigned short *ram_code; if (!fw) { - dev_err(sdma->dev, "firmware not found\n"); + dev_info(sdma->dev, "external firmware not found, using ROM firmware\n"); + /* + * In this case we just use the ROM firmware. + */ return; }
When a firmware cannot be found for the SDMA engine then we can continue with the inernal ROM firmware. The meaning of this message is frequently asked for, so make clear that the driver still works with the internal ROM firmware and reduce the loglevel from err to info. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> Cc: Vinod Koul <vinod.koul@intel.com> Cc: linux-arm-kernel@lists.infradead.org Cc: Shawn Guo <shawn.guo@linaro.org> --- changes since v1: - instead of removing the message make it more clear drivers/dma/imx-sdma.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)