mbox series

[v2,0/2] spi: loongson: add bus driver for the loongson spi

Message ID 20230317082950.12738-1-zhuyinbo@loongson.cn
Headers show
Series spi: loongson: add bus driver for the loongson spi | expand

Message

Yinbo Zhu March 17, 2023, 8:29 a.m. UTC
Loongson platform support spi hardware controller and this series patch
was to add spi driver and binding support.

Change in v2:
		1. This [PATCH v2 1/2] dt-bindings patch need depend on clk patch:
	 	   https://
		   lore.kernel.org/all/20230307115022.12846-1-zhuyinbo@loongson.cn/
		2. Remove the clock-names in spi yaml file.
		3. Add "loongson,ls7a-spi" compatible in spi yaml file.
		4. Add an || COMPILE_TEST and drop && PCI then add some CONFIG_PCI
		   macro to limit some pci code.
		5. Make the spi driver top code comment block that use C++ style.
		6. Drop spi->max_speed_hz.
		7. Add a spin_lock for loongson_spi_setup.
		8. Add a timeout and cpu_relax() in loongson_spi_write_read_8bit.
		9. Add spi_transfer_one and drop transfer and rework entire spi
		   driver that include some necessary changes.
		10. Use module_init replace subsys_initcall.
		11. About PM interface that I don't find any issue so I don't add
		    any changes.

Yinbo Zhu (2):
  dt-bindings: spi: add loongson spi
  spi: loongson: add bus driver for the loongson spi controller

 .../bindings/spi/loongson,ls-spi.yaml         |  44 ++
 MAINTAINERS                                   |   7 +
 drivers/spi/Kconfig                           |  10 +
 drivers/spi/Makefile                          |   1 +
 drivers/spi/spi-loongson.c                    | 459 ++++++++++++++++++
 5 files changed, 521 insertions(+)

Comments

Krzysztof Kozlowski March 17, 2023, 9:15 a.m. UTC | #1
On 17/03/2023 09:29, Yinbo Zhu wrote:
> Add the Loongson platform spi binding with DT schema format using
> json-schema.
> 
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---
>  .../bindings/spi/loongson,ls-spi.yaml         | 44 +++++++++++++++++++
>  MAINTAINERS                                   |  6 +++
>  2 files changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
> new file mode 100644
> index 000000000000..936b8dc82ce8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +

Drop blank line above.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/loongson,ls-spi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Loongson SPI controller
> +
> +maintainers:
> +  - Yinbo Zhu <zhuyinbo@loongson.cn>
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - loongson,ls2k-spi
> +      - loongson,ls7a-spi
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1

I don't understand why did you change it. I did not ask for it.

Best regards,
Krzysztof
Rob Herring March 17, 2023, 2:55 p.m. UTC | #2
On Fri, 17 Mar 2023 16:29:49 +0800, Yinbo Zhu wrote:
> Add the Loongson platform spi binding with DT schema format using
> json-schema.
> 
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---
>  .../bindings/spi/loongson,ls-spi.yaml         | 44 +++++++++++++++++++
>  MAINTAINERS                                   |  6 +++
>  2 files changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/spi/loongson,ls-spi.example.dts:22.28-29 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/spi/loongson,ls-spi.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1512: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230317082950.12738-2-zhuyinbo@loongson.cn

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Krzysztof Kozlowski March 17, 2023, 3:51 p.m. UTC | #3
On 17/03/2023 16:51, Krzysztof Kozlowski wrote:
> On 17/03/2023 11:00, zhuyinbo wrote:
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - loongson,ls2k-spi
>>>> +      - loongson,ls7a-spi
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  clocks:
>>>> +    minItems: 1
>>> I don't understand why did you change it. I did not ask for it.
>>>
>>> Best regards,
>>> Krzysztof
>> Add clocks "minItems: 1" description is for fix yaml file compile issue.
> 
> minItems: 1 is not correct, so you cannot use incorrect code to suppress
> some warning. This should be list the clocks or use maxItems: 1, if you
> have only one clock.

BTW, as Rob's bot reports, this wasn't even tested... Please test the
patches before sending them.

Best regards,
Krzysztof
Mark Brown March 17, 2023, 4:26 p.m. UTC | #4
On Fri, Mar 17, 2023 at 04:29:50PM +0800, Yinbo Zhu wrote:

> +static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
> +				     struct spi_device *spi, struct spi_transfer *t)
> +{

...

> +		loongson_spi->hz = hz;
> +		loongson_spi->spcr = div_tmp & 3;
> +		loongson_spi->sper = (div_tmp >> 2) & 3;
> +		val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);

This is writing to general chip registers, apparently not per chip
select ones.

> +static int loongson_spi_setup(struct spi_device *spi)
> +{
> +	struct loongson_spi *loongson_spi;

> +	spin_lock(&loongson_spi->lock);
> +	loongson_spi_update_state(loongson_spi, spi, NULL);

As IIRC I mentioned last time setup() might be called while other
transfers are happening and therefore shouldn't affect parallel
operations on other devices.

> +static const struct of_device_id loongson_spi_id_table[] = {
> +	{ .compatible = "loongson,ls2k-spi", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, loongson_spi_id_table);
> +
> +static struct platform_driver loongson_spi_driver = {
> +	.probe = loongson_spi_platform_probe,
> +	.driver	= {
> +		.name	= "loongson-spi",
> +		.owner	= THIS_MODULE,
> +		.bus = &platform_bus_type,
> +		.pm = &loongson_spi_dev_pm_ops,
> +		.of_match_table = loongson_spi_id_table,
> +	},
> +};
> +
> +#ifdef CONFIG_PCI
> +static int loongson_spi_pci_register(struct pci_dev *pdev,
> +			const struct pci_device_id *ent)

Again as I said last time the two buses should probably be separate
modules.

Otherwise this looks fine.
Yinbo Zhu March 18, 2023, 1:16 a.m. UTC | #5
在 2023/3/17 下午10:55, Rob Herring 写道:
> On Fri, 17 Mar 2023 16:29:49 +0800, Yinbo Zhu wrote:
>> Add the Loongson platform spi binding with DT schema format using
>> json-schema.
>>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>> ---
>>   .../bindings/spi/loongson,ls-spi.yaml         | 44 +++++++++++++++++++
>>   MAINTAINERS                                   |  6 +++
>>   2 files changed, 50 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls-spi.yaml
>>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> Error: Documentation/devicetree/bindings/spi/loongson,ls-spi.example.dts:22.28-29 syntax error
> FATAL ERROR: Unable to parse input tree
> make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/spi/loongson,ls-spi.example.dtb] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1512: dt_binding_check] Error 2

Hi  Rob Herring,


this error happen on 22 line, this need depend on 
https://lore.kernel.org/all/20230307115022.12846-1-zhuyinbo@loongson.cn/

22             clocks = <&clk LOONGSON2_BOOT_CLK>;  //22 line yaml 
code's  dtb

I had add change log in cover letter patch [PATCH v2 0/2], as follows,  
but robot still report error

What should I do next time to ensure that your robot relies on other patches before testing ?

Change in v2:
		1. This [PATCH v2 1/2] dt-bindings patch need depend on clk patch:
	 	   https://
		lore.kernel.org/all/20230307115022.12846-1-zhuyinbo@loongson.cn/

Thanks,

>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230317082950.12738-2-zhuyinbo@loongson.cn
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
Yinbo Zhu March 18, 2023, 1:18 a.m. UTC | #6
在 2023/3/17 下午11:51, Krzysztof Kozlowski 写道:
> On 17/03/2023 11:00, zhuyinbo wrote:
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - loongson,ls2k-spi
>>>> +      - loongson,ls7a-spi
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  clocks:
>>>> +    minItems: 1
>>> I don't understand why did you change it. I did not ask for it.
>>>
>>> Best regards,
>>> Krzysztof
>> Add clocks "minItems: 1" description is for fix yaml file compile issue.
> minItems: 1 is not correct, so you cannot use incorrect code to suppress
> some warning. This should be list the clocks or use maxItems: 1, if you
> have only one clock.

okay, I got it.

thanks.

>
> Best regards,
> Krzysztof
Yinbo Zhu March 18, 2023, 1:38 a.m. UTC | #7
在 2023/3/18 上午12:14, Mark Brown 写道:
> On Fri, Mar 17, 2023 at 04:51:48PM +0100, Krzysztof Kozlowski wrote:
>> On 17/03/2023 16:51, Krzysztof Kozlowski wrote:
>>> minItems: 1 is not correct, so you cannot use incorrect code to suppress
>>> some warning. This should be list the clocks or use maxItems: 1, if you
>>> have only one clock.
>> BTW, as Rob's bot reports, this wasn't even tested... Please test the
>> patches before sending them.
> If they're managing to see and try to fix warnings they're doing some
> kinds of testing, obviously they've missed something you wanted doing
> but there's clearly been some testing done.

Thanks your understanding !    I had test it  and this patch need depend 
on a clock patch, and I had added

this depend on description on changelog,  but I don't know how do make 
the robot can depend on my clock patch after testing.


Thanks!
Yinbo Zhu March 18, 2023, 6:07 a.m. UTC | #8
在 2023/3/18 上午12:26, Mark Brown 写道:
> On Fri, Mar 17, 2023 at 04:29:50PM +0800, Yinbo Zhu wrote:
>
>> +static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
>> +				     struct spi_device *spi, struct spi_transfer *t)
>> +{
> ...
>
>> +		loongson_spi->hz = hz;
>> +		loongson_spi->spcr = div_tmp & 3;
>> +		loongson_spi->sper = (div_tmp >> 2) & 3;
>> +		val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);
> This is writing to general chip registers, apparently not per chip
> select ones.

The loongson_spi_update_state was only be called in setup or transfer_one, and I will also
add a spin lock in tranfser_one.

>
>> +static int loongson_spi_setup(struct spi_device *spi)
>> +{
>> +	struct loongson_spi *loongson_spi;
>> +	spin_lock(&loongson_spi->lock);
>> +	loongson_spi_update_state(loongson_spi, spi, NULL);
> As IIRC I mentioned last time setup() might be called while other
> transfers are happening and therefore shouldn't affect parallel
> operations on other devices.
I think add spin_lock in  transfer_one interface that should be to fix 
this issue, Do you think so?
loongson_spi_transfer_one(struct spi_controller *ctrl, struct spi_dev
  {
         struct loongson_spi *loongson_spi = 
spi_master_get_devdata(spi->master);

+       spin_lock(&loongson_spi->lock);
         loongson_spi_update_state(loongson_spi, spi, xfer);
+       spin_unlock(&loongson_spi->lock);

>
>> +static const struct of_device_id loongson_spi_id_table[] = {
>> +	{ .compatible = "loongson,ls2k-spi", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, loongson_spi_id_table);
>> +
>> +static struct platform_driver loongson_spi_driver = {
>> +	.probe = loongson_spi_platform_probe,
>> +	.driver	= {
>> +		.name	= "loongson-spi",
>> +		.owner	= THIS_MODULE,
>> +		.bus = &platform_bus_type,
>> +		.pm = &loongson_spi_dev_pm_ops,
>> +		.of_match_table = loongson_spi_id_table,
>> +	},
>> +};
>> +
>> +#ifdef CONFIG_PCI
>> +static int loongson_spi_pci_register(struct pci_dev *pdev,
>> +			const struct pci_device_id *ent)
> Again as I said last time the two buses should probably be separate
> modules.
>
> Otherwise this looks fine.
okay, I will do it.
Mark Brown March 20, 2023, 12:52 p.m. UTC | #9
On Sat, Mar 18, 2023 at 02:07:16PM +0800, zhuyinbo wrote:
> 在 2023/3/18 上午12:26, Mark Brown 写道:
> > On Fri, Mar 17, 2023 at 04:29:50PM +0800, Yinbo Zhu wrote:

> > As IIRC I mentioned last time setup() might be called while other
> > transfers are happening and therefore shouldn't affect parallel
> > operations on other devices.

> I think add spin_lock in  transfer_one interface that should be to fix this
> issue, Do you think so?

No, that doesn't help if setup() reconfigures the controller while it's
doing a transfer.  The issue is that the controller might be put into
the wrong mode or run at the wrong speed.
Yinbo Zhu March 21, 2023, 2:54 a.m. UTC | #10
在 2023/3/20 下午8:52, Mark Brown 写道:
> On Sat, Mar 18, 2023 at 02:07:16PM +0800, zhuyinbo wrote:
>> 在 2023/3/18 上午12:26, Mark Brown 写道:
>>> On Fri, Mar 17, 2023 at 04:29:50PM +0800, Yinbo Zhu wrote:
>>> As IIRC I mentioned last time setup() might be called while other
>>> transfers are happening and therefore shouldn't affect parallel
>>> operations on other devices.
>> I think add spin_lock in  transfer_one interface that should be to fix this
>> issue, Do you think so?
> No, that doesn't help if setup() reconfigures the controller while it's
> doing a transfer.  The issue is that the controller might be put into
> the wrong mode or run at the wrong speed.

sorry, I don't got that why cpu still can call setup's critical region 
when cpu call transfer_one to  transfer spi data.

when I added a spin_lock for setup and transfer_one then setup and 
transfer_one's critical region cann't be called

simultaneously as I know, because the their lock was same lock.
Mark Brown March 21, 2023, 12:08 p.m. UTC | #11
On Tue, Mar 21, 2023 at 10:54:32AM +0800, zhuyinbo wrote:
> 在 2023/3/20 下午8:52, Mark Brown 写道:

> > No, that doesn't help if setup() reconfigures the controller while it's
> > doing a transfer.  The issue is that the controller might be put into
> > the wrong mode or run at the wrong speed.

> sorry, I don't got that why cpu still can call setup's critical region when
> cpu call transfer_one to  transfer spi data.

> when I added a spin_lock for setup and transfer_one then setup and
> transfer_one's critical region cann't be called

> simultaneously as I know, because the their lock was same lock.

Think what happens if the two SPI devices have different configurations
- for example, a different SPI mode.  The register state won't be
corrupted but the devices will still end up seeing misconfigured SPI
transfers.
Yinbo Zhu March 23, 2023, 12:46 p.m. UTC | #12
在 2023/3/21 下午8:08, Mark Brown 写道:
> On Tue, Mar 21, 2023 at 10:54:32AM +0800, zhuyinbo wrote:
>> 在 2023/3/20 下午8:52, Mark Brown 写道:
>>> No, that doesn't help if setup() reconfigures the controller while it's
>>> doing a transfer.  The issue is that the controller might be put into
>>> the wrong mode or run at the wrong speed.
>> sorry, I don't got that why cpu still can call setup's critical region when
>> cpu call transfer_one to  transfer spi data.
>> when I added a spin_lock for setup and transfer_one then setup and
>> transfer_one's critical region cann't be called
>> simultaneously as I know, because the their lock was same lock.
> Think what happens if the two SPI devices have different configurations
> - for example, a different SPI mode.  The register state won't be
> corrupted but the devices will still end up seeing misconfigured SPI
> transfers.

I think add following change and that issue what you said will can be 
fixed,   in addition, the spin_lock

was also not needed.   Do you think so?

@@ -101,8 +101,10 @@ static int loongson_spi_setup(struct spi_device *spi)
         if (spi->chip_select >= spi->master->num_chipselect)
                 return -EINVAL;

+       loongson_spi->hz = 0;
+       loongson_spi->mode &= SPI_NO_CS;
+
         spin_lock(&loongson_spi->lock);
-       loongson_spi_update_state(loongson_spi, spi, NULL);
         loongson_spi_set_cs(spi, 1);
Mark Brown March 23, 2023, 1:59 p.m. UTC | #13
On Thu, Mar 23, 2023 at 08:46:19PM +0800, zhuyinbo wrote:

> I think add following change and that issue what you said will can be
> fixed,   in addition, the spin_lock

> was also not needed.   Do you think so?

> @@ -101,8 +101,10 @@ static int loongson_spi_setup(struct spi_device *spi)
>         if (spi->chip_select >= spi->master->num_chipselect)
>                 return -EINVAL;
> 
> +       loongson_spi->hz = 0;
> +       loongson_spi->mode &= SPI_NO_CS;
> +
>         spin_lock(&loongson_spi->lock);
> -       loongson_spi_update_state(loongson_spi, spi, NULL);

Looks plausible, yes - I'd need to see the full thing to verify.
Yinbo Zhu March 24, 2023, 3:32 a.m. UTC | #14
在 2023/3/23 下午9:59, Mark Brown 写道:
> On Thu, Mar 23, 2023 at 08:46:19PM +0800, zhuyinbo wrote:
>
>> I think add following change and that issue what you said will can be
>> fixed,   in addition, the spin_lock
>> was also not needed.   Do you think so?
>> @@ -101,8 +101,10 @@ static int loongson_spi_setup(struct spi_device *spi)
>>          if (spi->chip_select >= spi->master->num_chipselect)
>>                  return -EINVAL;
>>
>> +       loongson_spi->hz = 0;
>> +       loongson_spi->mode &= SPI_NO_CS;
>> +
>>          spin_lock(&loongson_spi->lock);
>> -       loongson_spi_update_state(loongson_spi, spi, NULL);
> Looks plausible, yes - I'd need to see the full thing to verify.

okay, I will send v3.