| Message ID | 20230317082950.12738-1-zhuyinbo@loongson.cn |
|---|---|
| Headers | show |
| Series | spi: loongson: add bus driver for the loongson spi | expand |
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
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.
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
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.
在 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.
在 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
在 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!
在 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.
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.
在 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.
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.
在 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);
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.
在 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.