[06/32] spi: dw: Add support for the Kendryte K210 SoC

Commit Message

Damien Le Moal Nov. 7, 2020, 8:13 a.m. UTC

The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits
ctrlr0 register format. This SoC is also quite slow and gets significant
SD card performance improvements from using no-delay polled transfers.
Add the dw_spi_k210_init() function tied to the
"canaan,kendryte-k210-spi" compatible string to set the
DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields
for this SoC.

Signed-off-by: Damien Le Moal <[e-mail]>
---
 drivers/spi/spi-dw-mmio.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Sean Anderson Nov. 7, 2020, 1:31 p.m. UTC | #1

On 11/7/20 3:13 AM, Damien Le Moal wrote:
> The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits
> ctrlr0 register format. This SoC is also quite slow and gets significant
> SD card performance improvements from using no-delay polled transfers.
> Add the dw_spi_k210_init() function tied to the
> "canaan,kendryte-k210-spi" compatible string to set the
> DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields
> for this SoC.
> 
> Signed-off-by: Damien Le Moal <[e-mail]>
> ---
>  drivers/spi/spi-dw-mmio.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> index 3f1bc384cb45..a00def6c5b39 100644
> --- a/drivers/spi/spi-dw-mmio.c
> +++ b/drivers/spi/spi-dw-mmio.c
> @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev,
>  	return 0;
>  }
>  
> +static int dw_spi_k210_init(struct platform_device *pdev,
> +			    struct dw_spi_mmio *dwsmmio)
> +{
> +	dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY;

Can't you do runtime detection of DFS_32 in probe?

--Sean

> +
> +	return 0;
> +}
> +
>  static int dw_spi_mmio_probe(struct platform_device *pdev)
>  {
>  	int (*init_func)(struct platform_device *pdev,
> @@ -340,6 +348,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
>  	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
>  	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
>  	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
> +	{ .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init},
>  	{ /* end of table */}
>  };
>  MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
>

Damien Le Moal Nov. 7, 2020, 1:42 p.m. UTC | #2

On 2020/11/07 22:31, Sean Anderson wrote:
> On 11/7/20 3:13 AM, Damien Le Moal wrote:
>> The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits
>> ctrlr0 register format. This SoC is also quite slow and gets significant
>> SD card performance improvements from using no-delay polled transfers.
>> Add the dw_spi_k210_init() function tied to the
>> "canaan,kendryte-k210-spi" compatible string to set the
>> DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields
>> for this SoC.
>>
>> Signed-off-by: Damien Le Moal <[e-mail]>
>> ---
>>  drivers/spi/spi-dw-mmio.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
>> index 3f1bc384cb45..a00def6c5b39 100644
>> --- a/drivers/spi/spi-dw-mmio.c
>> +++ b/drivers/spi/spi-dw-mmio.c
>> @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev,
>>  	return 0;
>>  }
>>  
>> +static int dw_spi_k210_init(struct platform_device *pdev,
>> +			    struct dw_spi_mmio *dwsmmio)
>> +{
>> +	dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY;
> 
> Can't you do runtime detection of DFS_32 in probe?

I think it is possible, but it was much easier this way given that it seems that
only the K210 uses the DFS_32.

> 
> --Sean
> 
>> +
>> +	return 0;
>> +}
>> +
>>  static int dw_spi_mmio_probe(struct platform_device *pdev)
>>  {
>>  	int (*init_func)(struct platform_device *pdev,
>> @@ -340,6 +348,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
>>  	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
>>  	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
>>  	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
>> +	{ .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init},
>>  	{ /* end of table */}
>>  };
>>  MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
>>
> 
>

Sean Anderson Nov. 7, 2020, 1:52 p.m. UTC | #3

On 11/7/20 8:42 AM, Damien Le Moal wrote:
> On 2020/11/07 22:31, Sean Anderson wrote:
>> On 11/7/20 3:13 AM, Damien Le Moal wrote:
>>> The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits
>>> ctrlr0 register format. This SoC is also quite slow and gets significant
>>> SD card performance improvements from using no-delay polled transfers.
>>> Add the dw_spi_k210_init() function tied to the
>>> "canaan,kendryte-k210-spi" compatible string to set the
>>> DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields
>>> for this SoC.
>>>
>>> Signed-off-by: Damien Le Moal <[e-mail]>
>>> ---
>>>  drivers/spi/spi-dw-mmio.c | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
>>> index 3f1bc384cb45..a00def6c5b39 100644
>>> --- a/drivers/spi/spi-dw-mmio.c
>>> +++ b/drivers/spi/spi-dw-mmio.c
>>> @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev,
>>>  	return 0;
>>>  }
>>>  
>>> +static int dw_spi_k210_init(struct platform_device *pdev,
>>> +			    struct dw_spi_mmio *dwsmmio)
>>> +{
>>> +	dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY;
>>
>> Can't you do runtime detection of DFS_32 in probe?
> 
> I think it is possible, but it was much easier this way given that it seems that
> only the K210 uses the DFS_32.

I think if it is detectable at runtime it should be, instead of relying
on compatible strings. That way causes less future grief to anyone
porting a device possibly using DFS_32.

 --Sean

>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static int dw_spi_mmio_probe(struct platform_device *pdev)
>>>  {
>>>  	int (*init_func)(struct platform_device *pdev,
>>> @@ -340,6 +348,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
>>>  	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
>>>  	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
>>>  	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
>>> +	{ .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init},
>>>  	{ /* end of table */}
>>>  };
>>>  MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
>>>
>>
>>
> 
>

Mark Brown Nov. 9, 2020, 2:15 p.m. UTC | #4

On Sat, Nov 07, 2020 at 08:52:24AM -0500, Sean Anderson wrote:

> I think if it is detectable at runtime it should be, instead of relying
> on compatible strings. That way causes less future grief to anyone
> porting a device possibly using DFS_32.

Yes, runtime enumeration is generally preferred.  Having the compatible
string is nice in case some quirks are discoverd but for things that
can be enumerated there's less that can go wrong if we do so.

Serge Semin Nov. 9, 2020, 9:21 p.m. UTC | #5

On Sat, Nov 07, 2020 at 05:13:54PM +0900, Damien Le Moal wrote:
> The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits
> ctrlr0 register format. This SoC is also quite slow and gets significant
> SD card performance improvements from using no-delay polled transfers.
> Add the dw_spi_k210_init() function tied to the
> "canaan,kendryte-k210-spi" compatible string to set the
> DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields
> for this SoC.
> 
> Signed-off-by: Damien Le Moal <[e-mail]>
> ---
>  drivers/spi/spi-dw-mmio.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> index 3f1bc384cb45..a00def6c5b39 100644
> --- a/drivers/spi/spi-dw-mmio.c
> +++ b/drivers/spi/spi-dw-mmio.c
> @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev,
>  	return 0;
>  }
>  
> +static int dw_spi_k210_init(struct platform_device *pdev,
> +			    struct dw_spi_mmio *dwsmmio)
> +{
> +	dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY;
> +
> +	return 0;
> +}
> +
>  static int dw_spi_mmio_probe(struct platform_device *pdev)
>  {
>  	int (*init_func)(struct platform_device *pdev,
> @@ -340,6 +348,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
>  	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
>  	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
>  	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},

> +	{ .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init},

Other than the comments from Sean and Mark regarding the DFS_32
feature runtime detectability, I couldn't find a patch with adding the
new new compatible string into the DW APB SSI DT schema. Have I missed
it? If I haven't could you add one to the next version of the series?

-Sergey

>  	{ /* end of table */}
>  };
>  MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
> -- 
> 2.28.0
>

Damien Le Moal Nov. 9, 2020, 9:39 p.m. UTC | #6

On 2020/11/10 6:22, Serge Semin wrote:
> On Sat, Nov 07, 2020 at 05:13:54PM +0900, Damien Le Moal wrote:
>> The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits
>> ctrlr0 register format. This SoC is also quite slow and gets significant
>> SD card performance improvements from using no-delay polled transfers.
>> Add the dw_spi_k210_init() function tied to the
>> "canaan,kendryte-k210-spi" compatible string to set the
>> DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields
>> for this SoC.
>>
>> Signed-off-by: Damien Le Moal <[e-mail]>
>> ---
>>  drivers/spi/spi-dw-mmio.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
>> index 3f1bc384cb45..a00def6c5b39 100644
>> --- a/drivers/spi/spi-dw-mmio.c
>> +++ b/drivers/spi/spi-dw-mmio.c
>> @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev,
>>  	return 0;
>>  }
>>  
>> +static int dw_spi_k210_init(struct platform_device *pdev,
>> +			    struct dw_spi_mmio *dwsmmio)
>> +{
>> +	dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY;
>> +
>> +	return 0;
>> +}
>> +
>>  static int dw_spi_mmio_probe(struct platform_device *pdev)
>>  {
>>  	int (*init_func)(struct platform_device *pdev,
>> @@ -340,6 +348,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
>>  	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
>>  	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
>>  	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
> 
>> +	{ .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init},
> 
> Other than the comments from Sean and Mark regarding the DFS_32
> feature runtime detectability, I couldn't find a patch with adding the
> new new compatible string into the DW APB SSI DT schema. Have I missed
> it? If I haven't could you add one to the next version of the series?

Yes, I will. I forgot to change the DW DT binding doc for this. I did add a
patch for the "polling" property but forgot the compatible string.

In any case, I think that this new compatible string change can be dropped by
switching to automatically detecting the DFS32 and using a different solution
than the polling property change I sent for the RX fifo overflow problem.

I am still going through all the emails trying to understand what to try next to
avoid the polling "hack".

Thanks !

> 
> -Sergey
> 
>>  	{ /* end of table */}
>>  };
>>  MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
>> -- 
>> 2.28.0
>>
>

Rob Herring Nov. 9, 2020, 9:55 p.m. UTC | #7

On Mon, Nov 09, 2020 at 09:39:19PM +0000, Damien Le Moal wrote:
> On 2020/11/10 6:22, Serge Semin wrote:
> > On Sat, Nov 07, 2020 at 05:13:54PM +0900, Damien Le Moal wrote:
> >> The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits
> >> ctrlr0 register format. This SoC is also quite slow and gets significant
> >> SD card performance improvements from using no-delay polled transfers.
> >> Add the dw_spi_k210_init() function tied to the
> >> "canaan,kendryte-k210-spi" compatible string to set the
> >> DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields
> >> for this SoC.
> >>
> >> Signed-off-by: Damien Le Moal <[e-mail]>
> >> ---
> >>  drivers/spi/spi-dw-mmio.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> >> index 3f1bc384cb45..a00def6c5b39 100644
> >> --- a/drivers/spi/spi-dw-mmio.c
> >> +++ b/drivers/spi/spi-dw-mmio.c
> >> @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev,
> >>  	return 0;
> >>  }
> >>  
> >> +static int dw_spi_k210_init(struct platform_device *pdev,
> >> +			    struct dw_spi_mmio *dwsmmio)
> >> +{
> >> +	dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static int dw_spi_mmio_probe(struct platform_device *pdev)
> >>  {
> >>  	int (*init_func)(struct platform_device *pdev,
> >> @@ -340,6 +348,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
> >>  	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
> >>  	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
> >>  	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
> > 
> >> +	{ .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init},
> > 
> > Other than the comments from Sean and Mark regarding the DFS_32
> > feature runtime detectability, I couldn't find a patch with adding the
> > new new compatible string into the DW APB SSI DT schema. Have I missed
> > it? If I haven't could you add one to the next version of the series?
> 
> Yes, I will. I forgot to change the DW DT binding doc for this. I did add a
> patch for the "polling" property but forgot the compatible string.
> 
> In any case, I think that this new compatible string change can be dropped by
> switching to automatically detecting the DFS32 and using a different solution
> than the polling property change I sent for the RX fifo overflow problem.

No, new SoC needs new compatible string. Especially if a new vendor. 

> 
> I am still going through all the emails trying to understand what to try next to
> avoid the polling "hack".

Use compatible.

Rob

Damien Le Moal Nov. 9, 2020, 10 p.m. UTC | #8

On 2020/11/10 6:55, Rob Herring wrote:
> On Mon, Nov 09, 2020 at 09:39:19PM +0000, Damien Le Moal wrote:
>> On 2020/11/10 6:22, Serge Semin wrote:
>>> On Sat, Nov 07, 2020 at 05:13:54PM +0900, Damien Le Moal wrote:
>>>> The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits
>>>> ctrlr0 register format. This SoC is also quite slow and gets significant
>>>> SD card performance improvements from using no-delay polled transfers.
>>>> Add the dw_spi_k210_init() function tied to the
>>>> "canaan,kendryte-k210-spi" compatible string to set the
>>>> DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields
>>>> for this SoC.
>>>>
>>>> Signed-off-by: Damien Le Moal <[e-mail]>
>>>> ---
>>>>  drivers/spi/spi-dw-mmio.c | 9 +++++++++
>>>>  1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
>>>> index 3f1bc384cb45..a00def6c5b39 100644
>>>> --- a/drivers/spi/spi-dw-mmio.c
>>>> +++ b/drivers/spi/spi-dw-mmio.c
>>>> @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev,
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static int dw_spi_k210_init(struct platform_device *pdev,
>>>> +			    struct dw_spi_mmio *dwsmmio)
>>>> +{
>>>> +	dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  static int dw_spi_mmio_probe(struct platform_device *pdev)
>>>>  {
>>>>  	int (*init_func)(struct platform_device *pdev,
>>>> @@ -340,6 +348,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
>>>>  	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
>>>>  	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
>>>>  	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
>>>
>>>> +	{ .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init},
>>>
>>> Other than the comments from Sean and Mark regarding the DFS_32
>>> feature runtime detectability, I couldn't find a patch with adding the
>>> new new compatible string into the DW APB SSI DT schema. Have I missed
>>> it? If I haven't could you add one to the next version of the series?
>>
>> Yes, I will. I forgot to change the DW DT binding doc for this. I did add a
>> patch for the "polling" property but forgot the compatible string.
>>
>> In any case, I think that this new compatible string change can be dropped by
>> switching to automatically detecting the DFS32 and using a different solution
>> than the polling property change I sent for the RX fifo overflow problem.
> 
> No, new SoC needs new compatible string. Especially if a new vendor. 

My apologies for the bad wording: I meant to say the change to the list of
compatible strings that the DW SPI support would not be needed. So from the DW
SPI point of view, there would be no new compatible string to add/document.

> 
>>
>> I am still going through all the emails trying to understand what to try next to
>> avoid the polling "hack".
> 
> Use compatible.

Yes, that is what this patch used. Again, I think there is a chance this change
can be dropped.

> 
> Rob
>

Rob Herring Nov. 9, 2020, 11:07 p.m. UTC | #9

On Mon, Nov 9, 2020 at 4:00 PM Damien Le Moal <[e-mail]> wrote:
>
> On 2020/11/10 6:55, Rob Herring wrote:
> > On Mon, Nov 09, 2020 at 09:39:19PM +0000, Damien Le Moal wrote:
> >> On 2020/11/10 6:22, Serge Semin wrote:
> >>> On Sat, Nov 07, 2020 at 05:13:54PM +0900, Damien Le Moal wrote:
> >>>> The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits
> >>>> ctrlr0 register format. This SoC is also quite slow and gets significant
> >>>> SD card performance improvements from using no-delay polled transfers.
> >>>> Add the dw_spi_k210_init() function tied to the
> >>>> "canaan,kendryte-k210-spi" compatible string to set the
> >>>> DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields
> >>>> for this SoC.
> >>>>
> >>>> Signed-off-by: Damien Le Moal <[e-mail]>
> >>>> ---
> >>>>  drivers/spi/spi-dw-mmio.c | 9 +++++++++
> >>>>  1 file changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> >>>> index 3f1bc384cb45..a00def6c5b39 100644
> >>>> --- a/drivers/spi/spi-dw-mmio.c
> >>>> +++ b/drivers/spi/spi-dw-mmio.c
> >>>> @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev,
> >>>>    return 0;
> >>>>  }
> >>>>
> >>>> +static int dw_spi_k210_init(struct platform_device *pdev,
> >>>> +                      struct dw_spi_mmio *dwsmmio)
> >>>> +{
> >>>> +  dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY;
> >>>> +
> >>>> +  return 0;
> >>>> +}
> >>>> +
> >>>>  static int dw_spi_mmio_probe(struct platform_device *pdev)
> >>>>  {
> >>>>    int (*init_func)(struct platform_device *pdev,
> >>>> @@ -340,6 +348,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
> >>>>    { .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
> >>>>    { .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
> >>>>    { .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
> >>>
> >>>> +  { .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init},
> >>>
> >>> Other than the comments from Sean and Mark regarding the DFS_32
> >>> feature runtime detectability, I couldn't find a patch with adding the
> >>> new new compatible string into the DW APB SSI DT schema. Have I missed
> >>> it? If I haven't could you add one to the next version of the series?
> >>
> >> Yes, I will. I forgot to change the DW DT binding doc for this. I did add a
> >> patch for the "polling" property but forgot the compatible string.
> >>
> >> In any case, I think that this new compatible string change can be dropped by
> >> switching to automatically detecting the DFS32 and using a different solution
> >> than the polling property change I sent for the RX fifo overflow problem.
> >
> > No, new SoC needs new compatible string. Especially if a new vendor.
>
> My apologies for the bad wording: I meant to say the change to the list of
> compatible strings that the DW SPI support would not be needed. So from the DW
> SPI point of view, there would be no new compatible string to add/document.

No, there is a need for a new compatible string to add/document. You
might not need it in the driver if you have a fallback.

Compatible strings should be SoC specific so you can handle quirks
without a DT change. Otherwise, it's a never ending stream of new
properties and DT updates.

> >> I am still going through all the emails trying to understand what to try next to
> >> avoid the polling "hack".
> >
> > Use compatible.
>
> Yes, that is what this patch used. Again, I think there is a chance this change
> can be dropped.

Looks to me like it used a 'polling' property...

Rob

Damien Le Moal Nov. 10, 2020, 12:35 a.m. UTC | #10

On 2020/11/10 8:08, Rob Herring wrote:
> On Mon, Nov 9, 2020 at 4:00 PM Damien Le Moal <[e-mail]> wrote:
>>
>> On 2020/11/10 6:55, Rob Herring wrote:
>>> On Mon, Nov 09, 2020 at 09:39:19PM +0000, Damien Le Moal wrote:
>>>> On 2020/11/10 6:22, Serge Semin wrote:
>>>>> On Sat, Nov 07, 2020 at 05:13:54PM +0900, Damien Le Moal wrote:
>>>>>> The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits
>>>>>> ctrlr0 register format. This SoC is also quite slow and gets significant
>>>>>> SD card performance improvements from using no-delay polled transfers.
>>>>>> Add the dw_spi_k210_init() function tied to the
>>>>>> "canaan,kendryte-k210-spi" compatible string to set the
>>>>>> DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields
>>>>>> for this SoC.
>>>>>>
>>>>>> Signed-off-by: Damien Le Moal <[e-mail]>
>>>>>> ---
>>>>>>  drivers/spi/spi-dw-mmio.c | 9 +++++++++
>>>>>>  1 file changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
>>>>>> index 3f1bc384cb45..a00def6c5b39 100644
>>>>>> --- a/drivers/spi/spi-dw-mmio.c
>>>>>> +++ b/drivers/spi/spi-dw-mmio.c
>>>>>> @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev,
>>>>>>    return 0;
>>>>>>  }
>>>>>>
>>>>>> +static int dw_spi_k210_init(struct platform_device *pdev,
>>>>>> +                      struct dw_spi_mmio *dwsmmio)
>>>>>> +{
>>>>>> +  dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY;
>>>>>> +
>>>>>> +  return 0;
>>>>>> +}
>>>>>> +
>>>>>>  static int dw_spi_mmio_probe(struct platform_device *pdev)
>>>>>>  {
>>>>>>    int (*init_func)(struct platform_device *pdev,
>>>>>> @@ -340,6 +348,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
>>>>>>    { .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
>>>>>>    { .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
>>>>>>    { .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
>>>>>
>>>>>> +  { .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init},
>>>>>
>>>>> Other than the comments from Sean and Mark regarding the DFS_32
>>>>> feature runtime detectability, I couldn't find a patch with adding the
>>>>> new new compatible string into the DW APB SSI DT schema. Have I missed
>>>>> it? If I haven't could you add one to the next version of the series?
>>>>
>>>> Yes, I will. I forgot to change the DW DT binding doc for this. I did add a
>>>> patch for the "polling" property but forgot the compatible string.
>>>>
>>>> In any case, I think that this new compatible string change can be dropped by
>>>> switching to automatically detecting the DFS32 and using a different solution
>>>> than the polling property change I sent for the RX fifo overflow problem.
>>>
>>> No, new SoC needs new compatible string. Especially if a new vendor.
>>
>> My apologies for the bad wording: I meant to say the change to the list of
>> compatible strings that the DW SPI support would not be needed. So from the DW
>> SPI point of view, there would be no new compatible string to add/document.
> 
> No, there is a need for a new compatible string to add/document. You
> might not need it in the driver if you have a fallback.
> 
> Compatible strings should be SoC specific so you can handle quirks
> without a DT change. Otherwise, it's a never ending stream of new
> properties and DT updates.

Ah. OK. I get it now. Thanks for clarifying. So I will keep the new compatible
string (renamed with proper vendor name instead of brand) and document it.

> 
>>>> I am still going through all the emails trying to understand what to try next to
>>>> avoid the polling "hack".
>>>
>>> Use compatible.
>>
>> Yes, that is what this patch used. Again, I think there is a chance this change
>> can be dropped.
> 
> Looks to me like it used a 'polling' property...

I hope to be able to get rid of this change if a proper solution can be found to
the transfer speed problem I am seeing.

> 
> Rob
>

Damien Le Moal Nov. 13, 2020, 8 a.m. UTC | #11

On Sat, 2020-11-07 at 08:31 -0500, Sean Anderson wrote:
> On 11/7/20 3:13 AM, Damien Le Moal wrote:
> > The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits
> > ctrlr0 register format. This SoC is also quite slow and gets significant
> > SD card performance improvements from using no-delay polled transfers.
> > Add the dw_spi_k210_init() function tied to the
> > "canaan,kendryte-k210-spi" compatible string to set the
> > DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields
> > for this SoC.
> > 
> > Signed-off-by: Damien Le Moal <[e-mail]>
> > ---
> >  drivers/spi/spi-dw-mmio.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> > index 3f1bc384cb45..a00def6c5b39 100644
> > --- a/drivers/spi/spi-dw-mmio.c
> > +++ b/drivers/spi/spi-dw-mmio.c
> > @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev,
> >  	return 0;
> >  }
> >  
> > 
> > 
> > 
> > +static int dw_spi_k210_init(struct platform_device *pdev,
> > +			    struct dw_spi_mmio *dwsmmio)
> > +{
> > +	dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY;
> 
> Can't you do runtime detection of DFS_32 in probe?

Done ! Sending that in V2 (I will split the spi dw from the rest of the
series).

Patch

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index 3f1bc384cb45..a00def6c5b39 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -223,6 +223,14 @@  static int dw_spi_keembay_init(struct platform_device *pdev,
 	return 0;
 }
 
+static int dw_spi_k210_init(struct platform_device *pdev,
+			    struct dw_spi_mmio *dwsmmio)
+{
+	dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY;
+
+	return 0;
+}
+
 static int dw_spi_mmio_probe(struct platform_device *pdev)
 {
 	int (*init_func)(struct platform_device *pdev,
@@ -340,6 +348,7 @@  static const struct of_device_id dw_spi_mmio_of_match[] = {
 	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
 	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
 	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
+	{ .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init},
 	{ /* end of table */}
 };
 MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);