| Message ID | 20230704090453.83980-1-william.qiu@starfivetech.com |
|---|---|
| Headers | show |
| Series | Add initialization of clock for StarFive JH7110 SoC | expand |
On 04/07/2023 11:04, William Qiu wrote: > Add the quad spi controller node for the StarFive JH7110 SoC. > > Co-developed-by: Ziv Xu <ziv.xu@starfivetech.com> > Signed-off-by: Ziv Xu <ziv.xu@starfivetech.com> > Signed-off-by: William Qiu <william.qiu@starfivetech.com> > Reviewed-by: Hal Feng <hal.feng@starfivetech.com> ... > + qspi: spi@13010000 { > + compatible = "starfive,jh7110-qspi", "cdns,qspi-nor"; > + reg = <0x0 0x13010000 0x0 0x10000>, > + <0x0 0x21000000 0x0 0x400000>; > + interrupts = <25>; > + clocks = <&syscrg JH7110_SYSCLK_QSPI_REF>, > + <&syscrg JH7110_SYSCLK_QSPI_AHB>, > + <&syscrg JH7110_SYSCLK_QSPI_APB>; > + clock-names = "ref", "ahb", "apb"; > + resets = <&syscrg JH7110_SYSRST_QSPI_APB>, > + <&syscrg JH7110_SYSRST_QSPI_AHB>, > + <&syscrg JH7110_SYSRST_QSPI_REF>; > + reset-names = "qspi", "qspi-ocp", "rstc_ref"; > + cdns,fifo-depth = <256>; > + cdns,fifo-width = <4>; > + cdns,trigger-address = <0x0>; Bus nodes are usually disabled by default and enabled when needed for specific boards. Best regards, Krzysztof
Hey William, On Tue, Jul 04, 2023 at 05:04:52PM +0800, William Qiu wrote: > Add QSPI clock operation in device probe. > > Signed-off-by: William Qiu <william.qiu@starfivetech.com> > Reviewed-by: Hal Feng <hal.feng@starfivetech.com> > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/ These Reported-by tags don't seem correct, given they were reports about this patch, not the reason for it - but did you actually check that you fixed the errors that the patch produces? This particular one seems to complain about a hunk that is still in the patch & the CI running on the RISC-V patchwork is complaining about it. Cheers, Conor. > @@ -1840,6 +1858,8 @@ static int cqspi_resume(struct device *dev) > struct spi_master *master = dev_get_drvdata(dev); > > clk_prepare_enable(cqspi->clk); > + if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi")) > + clk_bulk_prepare_enable(cqspi->num_clks, cqspi->clks); > cqspi_wait_idle(cqspi); > cqspi_controller_init(cqspi); > > -- > 2.34.1 >
On Tue, Jul 04, 2023 at 05:36:03PM +0100, Conor Dooley wrote: > On Tue, Jul 04, 2023 at 05:04:52PM +0800, William Qiu wrote: > > Add QSPI clock operation in device probe. > > > > Signed-off-by: William Qiu <william.qiu@starfivetech.com> > > Reviewed-by: Hal Feng <hal.feng@starfivetech.com> > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/ > These Reported-by tags don't seem correct, given they were reports about > this patch, not the reason for it - but did you actually check that you > fixed the errors that the patch produces? Yeah, the Reported-bys that LKP sends in response to on list patches are a menace, they just generate noise. > This particular one seems to complain about a hunk that is still in the > patch & the CI running on the RISC-V patchwork is complaining about it. I'm surprised that builds cleanly anywhere...
On 04/07/2023 11:04, William Qiu wrote: > Add QSPI clock operation in device probe. > > Signed-off-by: William Qiu <william.qiu@starfivetech.com> > Reviewed-by: Hal Feng <hal.feng@starfivetech.com> > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/ > Reported-by: Julia Lawall <julia.lawall@inria.fr> > Closes: https://lore.kernel.org/r/202306040644.6ZHs55x4-lkp@intel.com/ > > @@ -1840,6 +1858,8 @@ static int cqspi_resume(struct device *dev) > struct spi_master *master = dev_get_drvdata(dev); > > clk_prepare_enable(cqspi->clk); > + if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi")) Don't add compatible checks inside the code. It does not scale. We expect compatibles to be listed only in one place - of_device_id - and customize driver with match data / quirks / flags. Comment applies to all your diff hunks. > + clk_bulk_prepare_enable(cqspi->num_clks, cqspi->clks); > cqspi_wait_idle(cqspi); > cqspi_controller_init(cqspi); > Best regards, Krzysztof
On 2023/7/5 0:36, Conor Dooley wrote: > Hey William, > > On Tue, Jul 04, 2023 at 05:04:52PM +0800, William Qiu wrote: >> Add QSPI clock operation in device probe. >> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com> >> Reviewed-by: Hal Feng <hal.feng@starfivetech.com> >> Reported-by: kernel test robot <lkp@intel.com> >> Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/ > > These Reported-by tags don't seem correct, given they were reports about > this patch, not the reason for it - but did you actually check that you > fixed the errors that the patch produces? > > This particular one seems to complain about a hunk that is still in the > patch & the CI running on the RISC-V patchwork is complaining about it. > > Cheers, > Conor. > I checked and found that I had only partially fixed it. I'll fix it in next version. Thanks for your comments. Best regards, William >> @@ -1840,6 +1858,8 @@ static int cqspi_resume(struct device *dev) >> struct spi_master *master = dev_get_drvdata(dev); >> >> clk_prepare_enable(cqspi->clk); >> + if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi")) >> + clk_bulk_prepare_enable(cqspi->num_clks, cqspi->clks); >> cqspi_wait_idle(cqspi); >> cqspi_controller_init(cqspi); >> >> -- >> 2.34.1 >>
On 2023/7/5 14:21, Krzysztof Kozlowski wrote: > On 04/07/2023 11:04, William Qiu wrote: >> Add QSPI clock operation in device probe. >> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com> >> Reviewed-by: Hal Feng <hal.feng@starfivetech.com> >> Reported-by: kernel test robot <lkp@intel.com> >> Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/ >> Reported-by: Julia Lawall <julia.lawall@inria.fr> >> Closes: https://lore.kernel.org/r/202306040644.6ZHs55x4-lkp@intel.com/ > > >> >> @@ -1840,6 +1858,8 @@ static int cqspi_resume(struct device *dev) >> struct spi_master *master = dev_get_drvdata(dev); >> >> clk_prepare_enable(cqspi->clk); >> + if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi")) > > Don't add compatible checks inside the code. It does not scale. We > expect compatibles to be listed only in one place - of_device_id - and > customize driver with match data / quirks / flags. > > Comment applies to all your diff hunks. > I'll use "of_device_get_match_data" to replace it. But the way I added reset before is also by compatible checks. Should I change this place to "of_device_get_match_data" as well? >> + clk_bulk_prepare_enable(cqspi->num_clks, cqspi->clks); >> cqspi_wait_idle(cqspi); >> cqspi_controller_init(cqspi); >> > > Best regards, > Krzysztof >
On 2023/7/4 17:43, Krzysztof Kozlowski wrote: > On 04/07/2023 11:04, William Qiu wrote: >> Add the quad spi controller node for the StarFive JH7110 SoC. >> >> Co-developed-by: Ziv Xu <ziv.xu@starfivetech.com> >> Signed-off-by: Ziv Xu <ziv.xu@starfivetech.com> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com> >> Reviewed-by: Hal Feng <hal.feng@starfivetech.com> > > ... > >> + qspi: spi@13010000 { >> + compatible = "starfive,jh7110-qspi", "cdns,qspi-nor"; >> + reg = <0x0 0x13010000 0x0 0x10000>, >> + <0x0 0x21000000 0x0 0x400000>; >> + interrupts = <25>; >> + clocks = <&syscrg JH7110_SYSCLK_QSPI_REF>, >> + <&syscrg JH7110_SYSCLK_QSPI_AHB>, >> + <&syscrg JH7110_SYSCLK_QSPI_APB>; >> + clock-names = "ref", "ahb", "apb"; >> + resets = <&syscrg JH7110_SYSRST_QSPI_APB>, >> + <&syscrg JH7110_SYSRST_QSPI_AHB>, >> + <&syscrg JH7110_SYSRST_QSPI_REF>; >> + reset-names = "qspi", "qspi-ocp", "rstc_ref"; >> + cdns,fifo-depth = <256>; >> + cdns,fifo-width = <4>; >> + cdns,trigger-address = <0x0>; > > Bus nodes are usually disabled by default and enabled when needed for > specific boards. > Will add status. Thanks for your comments. Best regards, William > Best regards, > Krzysztof >
On 05/07/2023 09:04, William Qiu wrote: > > > On 2023/7/5 14:21, Krzysztof Kozlowski wrote: >> On 04/07/2023 11:04, William Qiu wrote: >>> Add QSPI clock operation in device probe. >>> >>> Signed-off-by: William Qiu <william.qiu@starfivetech.com> >>> Reviewed-by: Hal Feng <hal.feng@starfivetech.com> >>> Reported-by: kernel test robot <lkp@intel.com> >>> Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/ >>> Reported-by: Julia Lawall <julia.lawall@inria.fr> >>> Closes: https://lore.kernel.org/r/202306040644.6ZHs55x4-lkp@intel.com/ >> >> >>> >>> @@ -1840,6 +1858,8 @@ static int cqspi_resume(struct device *dev) >>> struct spi_master *master = dev_get_drvdata(dev); >>> >>> clk_prepare_enable(cqspi->clk); >>> + if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi")) >> >> Don't add compatible checks inside the code. It does not scale. We >> expect compatibles to be listed only in one place - of_device_id - and >> customize driver with match data / quirks / flags. >> >> Comment applies to all your diff hunks. >> > I'll use "of_device_get_match_data" to replace it. But the way I added > reset before is also by compatible checks. Should I change this place to > "of_device_get_match_data" as well? I don't know what's there, but in general driver should be written in a consistent style. Best regards, Krzysztof
On 2023/7/5 15:23, Krzysztof Kozlowski wrote: > On 05/07/2023 09:04, William Qiu wrote: >> >> >> On 2023/7/5 14:21, Krzysztof Kozlowski wrote: >>> On 04/07/2023 11:04, William Qiu wrote: >>>> Add QSPI clock operation in device probe. >>>> >>>> Signed-off-by: William Qiu <william.qiu@starfivetech.com> >>>> Reviewed-by: Hal Feng <hal.feng@starfivetech.com> >>>> Reported-by: kernel test robot <lkp@intel.com> >>>> Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/ >>>> Reported-by: Julia Lawall <julia.lawall@inria.fr> >>>> Closes: https://lore.kernel.org/r/202306040644.6ZHs55x4-lkp@intel.com/ >>> >>> >>>> >>>> @@ -1840,6 +1858,8 @@ static int cqspi_resume(struct device *dev) >>>> struct spi_master *master = dev_get_drvdata(dev); >>>> >>>> clk_prepare_enable(cqspi->clk); >>>> + if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi")) >>> >>> Don't add compatible checks inside the code. It does not scale. We >>> expect compatibles to be listed only in one place - of_device_id - and >>> customize driver with match data / quirks / flags. >>> >>> Comment applies to all your diff hunks. >>> >> I'll use "of_device_get_match_data" to replace it. But the way I added >> reset before is also by compatible checks. Should I change this place to >> "of_device_get_match_data" as well? > > I don't know what's there, but in general driver should be written in a > consistent style. >It's in line 1719, inside the "cqspi_probe", but this part of the code is already merged in the main line. Should I keep it in a consistent style? Best regards, William > Best regards, > Krzysztof >
Hi, On 2023-07-04 17:04, William Qiu wrote: > Add the quad spi controller node for the StarFive JH7110 SoC. > > Co-developed-by: Ziv Xu <ziv.xu@starfivetech.com> > Signed-off-by: Ziv Xu <ziv.xu@starfivetech.com> > Signed-off-by: William Qiu <william.qiu@starfivetech.com> > Reviewed-by: Hal Feng <hal.feng@starfivetech.com> > --- > .../jh7110-starfive-visionfive-2.dtsi | 32 +++++++++++++++++++ > arch/riscv/boot/dts/starfive/jh7110.dtsi | 18 +++++++++++ > 2 files changed, 50 insertions(+) > > diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi > index 2a6d81609284..983b683e2f27 100644 > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi > @@ -126,6 +126,38 @@ &i2c6 { > status = "okay"; > }; > > +&qspi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + nor_flash: flash@0 { > + compatible = "jedec,spi-nor"; > + reg = <0>; > + cdns,read-delay = <5>; > + spi-max-frequency = <12000000>; > + cdns,tshsl-ns = <1>; > + cdns,tsd2d-ns = <1>; > + cdns,tchsh-ns = <1>; > + cdns,tslch-ns = <1>; > + > + partitions { > + compatible = "fixed-partitions"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + spl@0 { > + reg = <0x0 0x20000>; > + }; > + uboot@100000 { > + reg = <0x100000 0x300000>; > + }; > + data@f00000 { > + reg = <0xf00000 0x100000>; > + }; It appears that this uses the old layout for the SPI flash. The new layout is described there: https://doc-en.rvspace.org/VisionFive2/Boot_UG/JH7110_SDK/boot_address_allocation.html Regards Aurelien
On 2023/7/18 1:13, Aurelien Jarno wrote: > Hi, > > On 2023-07-04 17:04, William Qiu wrote: >> Add the quad spi controller node for the StarFive JH7110 SoC. >> >> Co-developed-by: Ziv Xu <ziv.xu@starfivetech.com> >> Signed-off-by: Ziv Xu <ziv.xu@starfivetech.com> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com> >> Reviewed-by: Hal Feng <hal.feng@starfivetech.com> >> --- >> .../jh7110-starfive-visionfive-2.dtsi | 32 +++++++++++++++++++ >> arch/riscv/boot/dts/starfive/jh7110.dtsi | 18 +++++++++++ >> 2 files changed, 50 insertions(+) >> >> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi >> index 2a6d81609284..983b683e2f27 100644 >> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi >> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi >> @@ -126,6 +126,38 @@ &i2c6 { >> status = "okay"; >> }; >> >> +&qspi { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + nor_flash: flash@0 { >> + compatible = "jedec,spi-nor"; >> + reg = <0>; >> + cdns,read-delay = <5>; >> + spi-max-frequency = <12000000>; >> + cdns,tshsl-ns = <1>; >> + cdns,tsd2d-ns = <1>; >> + cdns,tchsh-ns = <1>; >> + cdns,tslch-ns = <1>; >> + >> + partitions { >> + compatible = "fixed-partitions"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + >> + spl@0 { >> + reg = <0x0 0x20000>; >> + }; >> + uboot@100000 { >> + reg = <0x100000 0x300000>; >> + }; >> + data@f00000 { >> + reg = <0xf00000 0x100000>; >> + }; > > It appears that this uses the old layout for the SPI flash. The new > layout is described there: > > https://doc-en.rvspace.org/VisionFive2/Boot_UG/JH7110_SDK/boot_address_allocation.html > > Regards > Aurelien > I'll take a look, and use it then. Thanks for your comments. Best regards, William
On Tue, 04 Jul 2023 17:04:50 +0800, William Qiu wrote: > This patchset adds initial rudimentary support for the StarFive > Quad SPI controller driver. And this driver will be used in > StarFive's VisionFive 2 board. In 6.4, the QSPI_AHB and QSPI_APB > clocks changed from the default ON state to the default OFF state, > so these clocks need to be enabled in the driver.At the same time, > dts patch is added to this series. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/3] dt-bindings: qspi: cdns,qspi-nor: Add clocks for StarFive JH7110 SoC commit: 0d2b6a1b8515204924b9174ae0135e1f4ff29b21 [2/3] spi: cadence-quadspi: Add clock configuration for StarFive JH7110 QSPI commit: 33f1ef6d4eb6bca726608ed939c9fd94d96ceefd All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark