| Message ID | 1554736964-6058-1-git-send-email-f.suligoi@asem.it (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] spi: pxa2xx: add driver enabling message | expand |
Hi On 4/8/19 6:22 PM, Flavio Suligoi wrote: > Add an info message for the PXA2xx device driver start-up, > with the indication of the transfer mode used (DMA or GPIO). > > This info is useful to individuate the timing when > the module starts. > > Signed-off-by: Flavio Suligoi <f.suligoi@asem.it> > --- > drivers/spi/spi-pxa2xx.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c > index f7068cc..d449501 100644 > --- a/drivers/spi/spi-pxa2xx.c > +++ b/drivers/spi/spi-pxa2xx.c > @@ -1826,6 +1826,9 @@ static int pxa2xx_spi_probe(struct platform_device *pdev) > goto out_error_clock_enabled; > } > > + dev_info(dev, "PXA2xx SPI master controller (%s mode)\n", > + platform_info->enable_dma ? "DMA" : "PIO"); > + > return status; > Would this look better if moved before devm_spi_register_controller() call?
Hi Jarkko, > Hi > > On 4/8/19 6:22 PM, Flavio Suligoi wrote: > > Add an info message for the PXA2xx device driver start-up, > > with the indication of the transfer mode used (DMA or GPIO). > > > > This info is useful to individuate the timing when > > the module starts. > > > > Signed-off-by: Flavio Suligoi <f.suligoi@asem.it> > > --- > > drivers/spi/spi-pxa2xx.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c > > index f7068cc..d449501 100644 > > --- a/drivers/spi/spi-pxa2xx.c > > +++ b/drivers/spi/spi-pxa2xx.c > > @@ -1826,6 +1826,9 @@ static int pxa2xx_spi_probe(struct platform_device > *pdev) > > goto out_error_clock_enabled; > > } > > > > + dev_info(dev, "PXA2xx SPI master controller (%s mode)\n", > > + platform_info->enable_dma ? "DMA" : "PIO"); > > + > > return status; > > > Would this look better if moved before devm_spi_register_controller() > call? Ok, so in case of SPI registering failure, we have two messages, as: pxa2xx-spi 80860F0E:00: PXA2xx SPI master controller (DMA mode) pxa2xx-spi 80860F0E:00: problem registering spi controller Do you think that it is more explicative? Flavio
On 4/9/19 5:11 PM, Flavio Suligoi wrote: > Hi Jarkko, > >> Hi >> >> On 4/8/19 6:22 PM, Flavio Suligoi wrote: >>> Add an info message for the PXA2xx device driver start-up, >>> with the indication of the transfer mode used (DMA or GPIO). >>> >>> This info is useful to individuate the timing when >>> the module starts. >>> >>> Signed-off-by: Flavio Suligoi <f.suligoi@asem.it> >>> --- >>> drivers/spi/spi-pxa2xx.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c >>> index f7068cc..d449501 100644 >>> --- a/drivers/spi/spi-pxa2xx.c >>> +++ b/drivers/spi/spi-pxa2xx.c >>> @@ -1826,6 +1826,9 @@ static int pxa2xx_spi_probe(struct platform_device >> *pdev) >>> goto out_error_clock_enabled; >>> } >>> >>> + dev_info(dev, "PXA2xx SPI master controller (%s mode)\n", >>> + platform_info->enable_dma ? "DMA" : "PIO"); >>> + >>> return status; >>> >> Would this look better if moved before devm_spi_register_controller() >> call? > > Ok, so in case of SPI registering failure, we have two messages, as: > > pxa2xx-spi 80860F0E:00: PXA2xx SPI master controller (DMA mode) > pxa2xx-spi 80860F0E:00: problem registering spi controller > > Do you think that it is more explicative? > I think yes and it should not cause confusion since the error message is the last one. I was thinking the successful registration case when DMA is not available. First there is a warning, followed by a debug message from SPI core (if CONFIG_SPI_DEBUG) and then info message. [ 9.506895] pxa2xx-spi pxa2xx-spi.13: no DMA channels available, using PIO [ 9.516770] pxa2xx-spi pxa2xx-spi.13: registered master spi2 [ 9.518527] pxa2xx-spi pxa2xx-spi.13: PXA2xx SPI master controller (PIO mode) Actually this info message doesn't necessarily tell will the driver end up using DMA for transfers. See pxa2xx_spi_can_dma() and pxa2xx_spi_transfer_one(). How about replacing "no DMA channels available, using PIO" and have instead single info message telling is the DMA available or does the driver use PIO only?
Hi Jarkko, > > > >> Hi > >> > >> On 4/8/19 6:22 PM, Flavio Suligoi wrote: > >>> Add an info message for the PXA2xx device driver start-up, > >>> with the indication of the transfer mode used (DMA or GPIO). > >>> > >>> This info is useful to individuate the timing when > >>> the module starts. > >>> > >>> Signed-off-by: Flavio Suligoi <f.suligoi@asem.it> > >>> --- > >>> drivers/spi/spi-pxa2xx.c | 3 +++ > >>> 1 file changed, 3 insertions(+) > >>> > >>> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c > >>> index f7068cc..d449501 100644 > >>> --- a/drivers/spi/spi-pxa2xx.c > >>> +++ b/drivers/spi/spi-pxa2xx.c > >>> @@ -1826,6 +1826,9 @@ static int pxa2xx_spi_probe(struct > platform_device > >> *pdev) > >>> goto out_error_clock_enabled; > >>> } > >>> > >>> + dev_info(dev, "PXA2xx SPI master controller (%s mode)\n", > >>> + platform_info->enable_dma ? "DMA" : "PIO"); > >>> + > >>> return status; > >>> > >> Would this look better if moved before devm_spi_register_controller() > >> call? > > > > Ok, so in case of SPI registering failure, we have two messages, as: > > > > pxa2xx-spi 80860F0E:00: PXA2xx SPI master controller (DMA mode) > > pxa2xx-spi 80860F0E:00: problem registering spi controller > > > > Do you think that it is more explicative? > > > I think yes and it should not cause confusion since the error message is > the last one. > > I was thinking the successful registration case when DMA is not > available. First there is a warning, followed by a debug message from > SPI core (if CONFIG_SPI_DEBUG) and then info message. > > [ 9.506895] pxa2xx-spi pxa2xx-spi.13: no DMA channels available, > using PIO > [ 9.516770] pxa2xx-spi pxa2xx-spi.13: registered master spi2 > [ 9.518527] pxa2xx-spi pxa2xx-spi.13: PXA2xx SPI master controller > (PIO mode) I have added this message because, using an x86 machine, the message: "pxa2xx-spi pxa2xx-spi.13: registered master spi2" doesn't appear in the kernel messages! > Actually this info message doesn't necessarily tell will the driver end > up using DMA for transfers. See pxa2xx_spi_can_dma() and > pxa2xx_spi_transfer_one(). > > How about replacing "no DMA channels available, using PIO" and have > instead single info message telling is the DMA available or does the > driver use PIO only? Ok, it's a good idea, to avoid to many similar messages. So I can simply remove the: "no DMA channels available, using PIO" and leave only the new: " pxa2xx-spi pxa2xx-spi.13: PXA2xx SPI master controller (PIO mode)" Flavio
On 4/10/19 11:13 AM, Flavio Suligoi wrote: >> [ 9.506895] pxa2xx-spi pxa2xx-spi.13: no DMA channels available, >> using PIO >> [ 9.516770] pxa2xx-spi pxa2xx-spi.13: registered master spi2 >> [ 9.518527] pxa2xx-spi pxa2xx-spi.13: PXA2xx SPI master controller >> (PIO mode) > > I have added this message because, using an x86 machine, the message: > > "pxa2xx-spi pxa2xx-spi.13: registered master spi2" > > doesn't appear in the kernel messages! > Yeah, it needs CONFIG_SPI_DEBUG. >> Actually this info message doesn't necessarily tell will the driver end >> up using DMA for transfers. See pxa2xx_spi_can_dma() and >> pxa2xx_spi_transfer_one(). >> >> How about replacing "no DMA channels available, using PIO" and have >> instead single info message telling is the DMA available or does the >> driver use PIO only? > > Ok, it's a good idea, to avoid to many similar messages. > So I can simply remove the: > > "no DMA channels available, using PIO" > > and leave only the new: > > " pxa2xx-spi pxa2xx-spi.13: PXA2xx SPI master controller (PIO mode)" > Yes, something like that. Now I remember, please also take into account driver is dual role since commit ec93cb6f827b ("spi: pxa2xx: Add slave mode support").
> On 4/10/19 11:13 AM, Flavio Suligoi wrote: > >> [ 9.506895] pxa2xx-spi pxa2xx-spi.13: no DMA channels available, > >> using PIO > >> [ 9.516770] pxa2xx-spi pxa2xx-spi.13: registered master spi2 > >> [ 9.518527] pxa2xx-spi pxa2xx-spi.13: PXA2xx SPI master controller > >> (PIO mode) > > > > I have added this message because, using an x86 machine, the message: > > > > "pxa2xx-spi pxa2xx-spi.13: registered master spi2" > > > > doesn't appear in the kernel messages! > > > Yeah, it needs CONFIG_SPI_DEBUG. > > >> Actually this info message doesn't necessarily tell will the driver end > >> up using DMA for transfers. See pxa2xx_spi_can_dma() and > >> pxa2xx_spi_transfer_one(). > >> > >> How about replacing "no DMA channels available, using PIO" and have > >> instead single info message telling is the DMA available or does the > >> driver use PIO only? > > > > Ok, it's a good idea, to avoid to many similar messages. > > So I can simply remove the: > > > > "no DMA channels available, using PIO" > > > > and leave only the new: > > > > " pxa2xx-spi pxa2xx-spi.13: PXA2xx SPI master controller (PIO mode)" > > > Yes, something like that. > > Now I remember, please also take into account driver is dual role since > commit ec93cb6f827b ("spi: pxa2xx: Add slave mode support"). Ok, right, I have to consider the slave mode, too. Thanks Jarkko! Flavio
On Mon, Apr 08, 2019 at 05:22:44PM +0200, Flavio Suligoi wrote: > Add an info message for the PXA2xx device driver start-up, > with the indication of the transfer mode used (DMA or GPIO). > > This info is useful to individuate the timing when > the module starts. Adding this sort of message to every driver is going to make boot far too noisy, it's one thing if we actually enumerate information about the physical device but this isn't really that. There are already prints in the driver core for when things get probed which can be enabled if ordering issues need to be debugged.
Hi Mark, > On Mon, Apr 08, 2019 at 05:22:44PM +0200, Flavio Suligoi wrote: > > Add an info message for the PXA2xx device driver start-up, > > with the indication of the transfer mode used (DMA or GPIO). > > > > This info is useful to individuate the timing when > > the module starts. > > Adding this sort of message to every driver is going to make boot far > too noisy, it's one thing if we actually enumerate information about the > physical device but this isn't really that. There are already prints in > the driver core for when things get probed which can be enabled if > ordering issues need to be debugged. You have right about to avoid too many boot messages, but in this case, using an x86 machine and with the spi-pxa2xx in DMA mode, so without the message: "no DMA channels available, using PIO", there is absolutely no indication about the existence of the SPI master controller. This is the first reason for this patch. The second reason is about the DMA/PIO mode indication. With the board I'm using, sometimes the spi-pxa2xx driver can't allocate a DMA channel and works in PIO mode. So, with the advice of Jarkko, I think that a valid solution could be: 1) remove the "no DMA channels available, using PIO" message 2) add a new message with the indications of: - controller mode (slave or master) - transfer mode (DMA or PIO) What do you think about this? Flavio
On Wed, Apr 10, 2019 at 11:47:43AM +0000, Flavio Suligoi wrote: > You have right about to avoid too many boot messages, > but in this case, using an x86 machine and with > the spi-pxa2xx in DMA mode, so without the message: > "no DMA channels available, using PIO", > there is absolutely no indication about the existence > of the SPI master controller. It's totally fine to not have a boot print for the device, the best way to find devices if you need them is to look in sysfs anyway. > The second reason is about the DMA/PIO mode indication. > With the board I'm using, sometimes the spi-pxa2xx driver can't allocate > a DMA channel and works in PIO mode. > So, with the advice of Jarkko, I think that a valid solution could be: > 1) remove the "no DMA channels available, using PIO" message > 2) add a new message with the indications of: > - controller mode (slave or master) > - transfer mode (DMA or PIO) > What do you think about this? If the system is randomly failing to assign a DMA channel when it should then shouldn't we just fix that? A print which is presumably intended to prompt the user to reboot to try to get things working doesn't seem like a good solution.
> On Wed, Apr 10, 2019 at 11:47:43AM +0000, Flavio Suligoi wrote: > > > You have right about to avoid too many boot messages, > > but in this case, using an x86 machine and with > > the spi-pxa2xx in DMA mode, so without the message: > > > "no DMA channels available, using PIO", > > > there is absolutely no indication about the existence > > of the SPI master controller. > > It's totally fine to not have a boot print for the device, the best way > to find devices if you need them is to look in sysfs anyway. Ok > > The second reason is about the DMA/PIO mode indication. > > With the board I'm using, sometimes the spi-pxa2xx driver can't allocate > > a DMA channel and works in PIO mode. > > > So, with the advice of Jarkko, I think that a valid solution could be: > > > 1) remove the "no DMA channels available, using PIO" message > > 2) add a new message with the indications of: > > - controller mode (slave or master) > > - transfer mode (DMA or PIO) > > > What do you think about this? > > If the system is randomly failing to assign a DMA channel when it should > then shouldn't we just fix that? A print which is presumably intended > to prompt the user to reboot to try to get things working doesn't seem > like a good solution. Right, I'm just fix the DMA problem (I'm preparing a patch about this) Flavio
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c index f7068cc..d449501 100644 --- a/drivers/spi/spi-pxa2xx.c +++ b/drivers/spi/spi-pxa2xx.c @@ -1826,6 +1826,9 @@ static int pxa2xx_spi_probe(struct platform_device *pdev) goto out_error_clock_enabled; } + dev_info(dev, "PXA2xx SPI master controller (%s mode)\n", + platform_info->enable_dma ? "DMA" : "PIO"); + return status; out_error_clock_enabled:
Add an info message for the PXA2xx device driver start-up, with the indication of the transfer mode used (DMA or GPIO). This info is useful to individuate the timing when the module starts. Signed-off-by: Flavio Suligoi <f.suligoi@asem.it> --- drivers/spi/spi-pxa2xx.c | 3 +++ 1 file changed, 3 insertions(+)