[PATCH 1/1] spi: pxa2xx: add driver enabling message

[PATCH 1/1] spi: pxa2xx: add driver enabling message

[Flavio Suligoi]

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;
 
 out_error_clock_enabled:
-- 
2.7.4

Re: [PATCH 1/1] spi: pxa2xx: add driver enabling message

[Jarkko Nikula]

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?

-- 
Jarkko

RE: [PATCH 1/1] spi: pxa2xx: add driver enabling message

[Flavio Suligoi]

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

Re: [PATCH 1/1] spi: pxa2xx: add driver enabling message

[Jarkko Nikula]

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?

-- 
Jarkko

RE: [PATCH 1/1] spi: pxa2xx: add driver enabling message

[Flavio Suligoi]

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

Re: [PATCH 1/1] spi: pxa2xx: add driver enabling message

[Jarkko Nikula]

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

RE: [PATCH 1/1] spi: pxa2xx: add driver enabling message

[Flavio Suligoi]

> 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

[PATCH v2 1/1] spi: pxa2xx: add driver enabling message

[Flavio Suligoi]

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 <f.suligoi@asem.it>
---

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

Re: [PATCH v2 1/1] spi: pxa2xx: add driver enabling message

[Jarkko Nikula]

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 <f.suligoi@asem.it>
> ---
> 
> 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 <jarkko.nikula@linux.intel.com>

RE: [PATCH v2 1/1] spi: pxa2xx: add driver enabling message

[Flavio Suligoi]

> 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 <f.suligoi@asem.it>
> > ---
> >
> > 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 <jarkko.nikula@linux.intel.com>

Ok

Flavio

Re: [PATCH 1/1] spi: pxa2xx: add driver enabling message

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH 1/1] spi: pxa2xx: add driver enabling message

[Flavio Suligoi]

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

Re: [PATCH 1/1] spi: pxa2xx: add driver enabling message

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH 1/1] spi: pxa2xx: add driver enabling message

[Flavio Suligoi]

> 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