On Fri, May 11, 2018 at 01:38:21PM +0300, Radu Pirea wrote:

> +config SPI_AT91_USART
> + tristate "Atmel USART Controller as SPI"
> + depends on HAS_DMA
> + depends on (ARCH_AT91 || COMPILE_TEST)
> + select MFD_AT91_USART
> + help
> + This selects a driver for the AT91 USART Controller as SPI Master,
> + present on AT91 and SAMA5 SoC series.
> +

This looks like there's some tab/space mixing going on here.

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for AT91 USART Controllers as SPI
> + *
> + * Copyright (C) 2018 Microchip Technology Inc.

Make the entire block a C++ comment so it looks more intentional rather
tha mixing C and C++.

> +static inline void at91_usart_spi_tx(struct at91_usart_spi *aus)
> +{
> + unsigned int len = aus->current_transfer->len;
> + unsigned int remaining = aus->current_tx_remaining_bytes;
> + const u8 *tx_buf = aus->current_transfer->tx_buf;
> +
> + if (tx_buf && remaining) {
> + if (at91_usart_spi_tx_ready(aus))
> + spi_writel(aus, THR, tx_buf[len - remaining]);
> + aus->current_tx_remaining_bytes--;

Missing braces here - we only write to the FIFO if there's space but we
unconditionally decrement the counter.

> + } else {
> + if (at91_usart_spi_tx_ready(aus))
> + spi_writel(aus, THR, US_DUMMY_TX);
> + }
> +}

This looks like you're open coding SPI_CONTROLLER_MUST_TX

> + int len = aus->current_transfer->len;
> + int remaining = aus->current_rx_remaining_bytes;
> + u8 *rx_buf = aus->current_transfer->rx_buf;
> +
> + if (aus->current_rx_remaining_bytes) {
> + rx_buf[len - remaining] = spi_readb(aus, RHR);
> + aus->current_rx_remaining_bytes--;
> + } else {
> + spi_readb(aus, RHR);
> + }

Similarly for _MUST_RX.

> + controller->flags = SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX;

You're actually setting both flags... this means that the handling for
cases with missing TX or RX buffers can't happen.

Attachments:

(No filename) (1.92 kB)
signature.asc (499.00 B)
Download all attachments

2018-05-17 16:48:08

On Wed, May 23, 2018 at 11:10:28AM +0300, Radu Pirea wrote:
> On 05/17/2018 08:04 AM, Mark Brown wrote:

> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Driver for AT91 USART Controllers as SPI
> > > + *
> > > + * Copyright (C) 2018 Microchip Technology Inc.

> > Make the entire block a C++ comment so it looks more intentional rather
> > tha mixing C and C++.

> I know it's ugly, but SPDX license identifier must be in a separate comment
> block.

No, it doesn't - it just needs to be the first line of the file.

Attachments:

(No filename) (547.00 B)
signature.asc (499.00 B)
Download all attachments

2018-05-25 02:25:45

by Radu Pirea

[permalink] [raw]

Subject: Re: [PATCH v3 5/6] spi: at91-usart: add driver for at91-usart as spi

On 05/17/2018 07:54 AM, Mark Brown wrote:
> On Tue, May 15, 2018 at 12:22:24PM +0300, Radu Pirea wrote:
>> On Mon, 2018-05-14 at 20:38 +0300, Andy Shevchenko wrote:
>
>>> So, what is not going as expected in "SPI core takes care of CSs"
>>> case?
>>> Did you use oscilloscope for that?
>
>> Yes, I used and CSs was not asserted. Anyway, I will will try again.
>
> If the core chip select handling is not working properly for some reason
> then the core chip select handling should be fixed rather than just open
> coding in your driver - probably it's also broken for other users.
>

Hi Mark,

I found the fix for cs-gpios. If I change spi_add_device function like
this(see below) everything is ok.

int spi_add_device(struct spi_device *spi)

...

if (ctlr->cs_gpios){
spi->cs_gpio = ctlr->cs_gpios[spi->chip_select];
if(gpio_is_valid(spi->cs_gpio))
gpio_direction_output(spi->cs_gpio, !(spi->mode &
SPI_CS_HIGH));

}

...

return status;
}

In the subsystem gpio direction of pins is never set and
gpio_set_value() don't set the direction.
In my opinion gpio_direction_output() set direction should be called in
spi_add_device. What do you think? Is ok?

2018-05-25 02:36:09

by Alexandre Belloni

[permalink] [raw]

Subject: Re: [PATCH v3 5/6] spi: at91-usart: add driver for at91-usart as spi

Hi,

On 24/05/2018 19:04:11+0300, Radu Pirea wrote:
>
>
> On 05/17/2018 07:54 AM, Mark Brown wrote:
> > On Tue, May 15, 2018 at 12:22:24PM +0300, Radu Pirea wrote:
> > > On Mon, 2018-05-14 at 20:38 +0300, Andy Shevchenko wrote:
> >
> > > > So, what is not going as expected in "SPI core takes care of CSs"
> > > > case?
> > > > Did you use oscilloscope for that?
> >
> > > Yes, I used and CSs was not asserted. Anyway, I will will try again.
> >
> > If the core chip select handling is not working properly for some reason
> > then the core chip select handling should be fixed rather than just open
> > coding in your driver - probably it's also broken for other users.
> >
>
> Hi Mark,
>
> I found the fix for cs-gpios. If I change spi_add_device function like
> this(see below) everything is ok.
>
> int spi_add_device(struct spi_device *spi)
>
> ...
>
> if (ctlr->cs_gpios){
> spi->cs_gpio = ctlr->cs_gpios[spi->chip_select];
> if(gpio_is_valid(spi->cs_gpio))
> gpio_direction_output(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
>
> }
>
> ...
>
> return status;
> }
>
> In the subsystem gpio direction of pins is never set and gpio_set_value()
> don't set the direction.
> In my opinion gpio_direction_output() set direction should be called in
> spi_add_device. What do you think? Is ok?

Back in 2014, I was suggesting using devm_gpio_request_one() in
of_spi_register_master(). That would take care of setting the direction
of the GPIO:

https://www.spinics.net/lists/arm-kernel/msg351251.html

I never took the time to create the patch and test though.

--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

2018-05-25 02:36:53

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v3 5/6] spi: at91-usart: add driver for at91-usart as spi

On Thu, May 24, 2018 at 07:04:11PM +0300, Radu Pirea wrote:

> if (ctlr->cs_gpios){
> spi->cs_gpio = ctlr->cs_gpios[spi->chip_select];
> if(gpio_is_valid(spi->cs_gpio))
> gpio_direction_output(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
>
> }

You're expected to request the GPIOs in your driver using one of the
modern request functions like gpio_request_one() (or ideally the GPIO
descriptor APIs now) which combine the direction setting and request
into a single operation.

Attachments:

(No filename) (526.00 B)
signature.asc (499.00 B)
Download all attachments

2018-05-25 02:38:01

On 05/25/2018 04:35 PM, Richard Genoud wrote:
> On 25/05/2018 14:17, Radu Pirea wrote:
>>
>>
>> On 05/15/2018 04:14 PM, Richard Genoud wrote:
>>> On 15/05/2018 14:47, Radu Pirea wrote:
>>>> On Mon, 2018-05-14 at 12:57 +0200, Richard Genoud wrote:
>>>>> After your patch, the DMA is not selected anymore:
>>>>> atmel_usart_serial atmel_usart_serial.0.auto: TX channel not
>>>>> available, switch to pio
>>>>> instead of:
>>>>> atmel_usart fffff200.serial: using dma1chan2 for tx DMA transfers
>>>>>
>>>> Fixed.
>>>>> And the kernel doesn't log anymore on the serial console, despite the
>>>>> loglevel=8
>>>>> (after reverting this series, the kernel logs reappears on the serial
>>>>> console)
>>>>>
>>>> Which serial are you using as console?
>>> fffff200.serial (sam9g35-cm)
>>> ( stdout-path = "serial0:115200n8"; in the DTS )
>>>
>>> With this series applied, all the kernel log goes on the screen.
>>> Without, it goes on the serial debug.
>>>
>> I tested again with archlinux arm and poky-linux4sam release as distros
>> and kernel log goes on the serial debug. Can you give me more details
>> like cmdline?
> I used kernel 4.17-rc6
> at91_dt_defconfig
> at91sam9g35ek.dtb
>
> Kernel command line: root=/dev/mtdblock1 rw rootfstype=ubifs ubi.mtd=1 root=ubi0:rootfs
I reproduced your issue.
If in cmdline console is not set like this "console=ttyS0,115200", the
console goes to the lcd.
> ( the one from the DTS )
>
> Detailed instructions:
>
> git checkout v4.17-rc6
> ARCH=arm CROSS_COMPILE=path_to_my_Xchain/arm-linux- LOADADDR=0x20008000 make -j 12 at91_dt_defconfig
> ARCH=arm CROSS_COMPILE=path_to_my_Xchain/arm-linux- LOADADDR=0x20008000 make -j 12 uImage at91sam9g35ek.dtb
> cp arch/arm/boot/uImage arch/arm/boot/dts/at91sam9g35ek.dtb /tftpboot/
>
> From uboot:
> tftpboot 0x20007FC0 uImage
> tftpboot 0x26400000 at91sam9g35ek.dtb
> bootm 0x20007FC0 - 0x26400000
>
> [ I see the logs on the serial debug ]
>
> git am \[PATCH\ v3\ [1-6]*
>
> ARCH=arm CROSS_COMPILE=path_to_my_Xchain/arm-linux- LOADADDR=0x20008000 make -j 12 uImage at91sam9g35ek.dtb
> cp arch/arm/boot/uImage arch/arm/boot/dts/at91sam9g35ek.dtb /tftpboot/
>
> From uboot:
> tftpboot 0x20007FC0 uImage
> tftpboot 0x26400000 at91sam9g35ek.dtb
> bootm 0x20007FC0 - 0x26400000
>
> [ I don't see the logs on the serial debug anymore ]
>
>
>>>>> (tests done on sam9g35)
>>>>>
>>>> I will consider the rest of suggestions.
>>>>> regards,
>>>>> Richard
>>>
>