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 <[email protected]>
---
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;
out_error_clock_enabled:
--
2.7.4
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 <[email protected]>
> ---
> 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?
--
Jarkko
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> >>> ---
> >>> 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").
--
Jarkko
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 <[email protected]>
>>> ---
>>> 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?
--
Jarkko
> 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
Add an info message for the PXA2xx device driver start-up,
with the indication of:
- mode (slave device or master controller)
- transfer mode (DMA or GPIO)
Signed-off-by: Flavio Suligoi <[email protected]>
---
v1: - first version
v2: - remove warning message "no DMA channels available, using PIO"
- add "slave device"/"master controller" indication message
drivers/spi/spi-pxa2xx.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index f7068cc..b9e04e2 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1696,7 +1696,6 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
if (platform_info->enable_dma) {
status = pxa2xx_spi_dma_setup(drv_data);
if (status) {
- dev_warn(dev, "no DMA channels available, using PIO\n");
platform_info->enable_dma = false;
} else {
controller->can_dma = pxa2xx_spi_can_dma;
@@ -1818,6 +1817,10 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
+ dev_info(dev, "PXA2xx SPI %s (%s mode)\n",
+ platform_info->is_slave ? "slave device" : "master controller",
+ platform_info->enable_dma ? "DMA" : "PIO");
+
/* Register with the SPI framework */
platform_set_drvdata(pdev, drv_data);
status = devm_spi_register_controller(&pdev->dev, controller);
--
2.7.4
On 4/10/19 12:47 PM, Flavio Suligoi wrote:
> Add an info message for the PXA2xx device driver start-up,
> with the indication of:
>
> - mode (slave device or master controller)
> - transfer mode (DMA or GPIO)
>
> Signed-off-by: Flavio Suligoi <[email protected]>
> ---
>
> v1: - first version
> v2: - remove warning message "no DMA channels available, using PIO"
> - add "slave device"/"master controller" indication message
>
> drivers/spi/spi-pxa2xx.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
> index f7068cc..b9e04e2 100644
> --- a/drivers/spi/spi-pxa2xx.c
> +++ b/drivers/spi/spi-pxa2xx.c
> @@ -1696,7 +1696,6 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
> if (platform_info->enable_dma) {
> status = pxa2xx_spi_dma_setup(drv_data);
> if (status) {
> - dev_warn(dev, "no DMA channels available, using PIO\n");
> platform_info->enable_dma = false;
> } else {
> controller->can_dma = pxa2xx_spi_can_dma;
> @@ -1818,6 +1817,10 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
> pm_runtime_set_active(&pdev->dev);
> pm_runtime_enable(&pdev->dev);
>
> + dev_info(dev, "PXA2xx SPI %s (%s mode)\n",
> + platform_info->is_slave ? "slave device" : "master controller",
> + platform_info->enable_dma ? "DMA" : "PIO");
> +
"slave device" is ambiguous. Please use "controller" with both, i.e.
"PXA2xx SPI %s controller (%s mode)\n", ... ? "slave" : "master", ...
With that changed you may add:
Reviewed-by: Jarkko Nikula <[email protected]>
> On 4/10/19 12:47 PM, Flavio Suligoi wrote:
> > Add an info message for the PXA2xx device driver start-up,
> > with the indication of:
> >
> > - mode (slave device or master controller)
> > - transfer mode (DMA or GPIO)
> >
> > Signed-off-by: Flavio Suligoi <[email protected]>
> > ---
> >
> > v1: - first version
> > v2: - remove warning message "no DMA channels available, using PIO"
> > - add "slave device"/"master controller" indication message
> >
> > drivers/spi/spi-pxa2xx.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
> > index f7068cc..b9e04e2 100644
> > --- a/drivers/spi/spi-pxa2xx.c
> > +++ b/drivers/spi/spi-pxa2xx.c
> > @@ -1696,7 +1696,6 @@ static int pxa2xx_spi_probe(struct platform_device
> *pdev)
> > if (platform_info->enable_dma) {
> > status = pxa2xx_spi_dma_setup(drv_data);
> > if (status) {
> > - dev_warn(dev, "no DMA channels available, using PIO\n");
> > platform_info->enable_dma = false;
> > } else {
> > controller->can_dma = pxa2xx_spi_can_dma;
> > @@ -1818,6 +1817,10 @@ static int pxa2xx_spi_probe(struct
> platform_device *pdev)
> > pm_runtime_set_active(&pdev->dev);
> > pm_runtime_enable(&pdev->dev);
> >
> > + dev_info(dev, "PXA2xx SPI %s (%s mode)\n",
> > + platform_info->is_slave ? "slave device" : "master
> controller",
> > + platform_info->enable_dma ? "DMA" : "PIO");
> > +
>
> "slave device" is ambiguous. Please use "controller" with both, i.e.
> "PXA2xx SPI %s controller (%s mode)\n", ... ? "slave" : "master", ...
>
> With that changed you may add:
>
> Reviewed-by: Jarkko Nikula <[email protected]>
Ok
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.
> 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
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.