| Message ID | 20230815154412.713846-1-hsinyi@chromium.org |
|---|---|
| Headers | show |
| Series | Add a property to override the quad mode | expand |
Hi, >On gigadevice gd25lq64c, the quad mode is enabled after BFPT is parsed. >According to datasheet[1], Quad enable (QE) bit needs to be set to 0 to >use write protection (WP) pin. It also recommends setting default value of >QE to 0 to avoid a potential short issue. So you are using either dual or single io mode. Why can't you use the device tree property spi-{tx,rx}-bus-width? >Add a disable-quad-mode property in devicetree that platform can use it to >override the quad mode status parsed from BFPT to use write protection. > >[1] >https://www.elm-tech.com/ja/products/spi-flash-memory/gd25lq64/gd25lq64.pdf should be a link on the vendor Homepage if possible. -michael
On Tue, Aug 15, 2023 at 11:59 PM Michael Walle <michael@walle.cc> wrote: > > Hi, > > >On gigadevice gd25lq64c, the quad mode is enabled after BFPT is parsed. > >According to datasheet[1], Quad enable (QE) bit needs to be set to 0 to > >use write protection (WP) pin. It also recommends setting default value of > >QE to 0 to avoid a potential short issue. > > So you are using either dual or single io mode. Why can't you use the device tree property spi-{tx,rx}-bus-width? I tried setting spi-tx-bus-width and spi-rx-bus-width to either 0 or 1 and WP still doesn't work. For this chip, quad_enable will be set to spi_nor_sr2_bit1_quad_enable (QER flag is BFPT_DWORD15_QER_SR2_BIT1_BUGGY)[1] spi_nor_write_sr_and_check() calls spi_nor_write_16bit_sr_and_check()[2] and the function sets QE bit if quad_enable is not NULL. [1] https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/sfdp.c#L575 [2] https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/core.c#L879 Setting spi-{tx,rx}-bus-width still falls to this function and cases. > > >Add a disable-quad-mode property in devicetree that platform can use it to > >override the quad mode status parsed from BFPT to use write protection. > > > >[1] > >https://www.elm-tech.com/ja/products/spi-flash-memory/gd25lq64/gd25lq64.pdf > > should be a link on the vendor Homepage if possible. Right, https://www.gigadevice.com.cn/Public/Uploads/uploadfile/files/20220714/DS-00012-GD25LQ64C-Rev3.4.pdf is a more proper datasheet source. The QE description is also in page 13. > > -michael >
On 15/08/2023 19:21, Hsin-Yi Wang wrote: > On Tue, Aug 15, 2023 at 11:59 PM Michael Walle <michael@walle.cc> wrote: >> >> Hi, >> >>> On gigadevice gd25lq64c, the quad mode is enabled after BFPT is parsed. >>> According to datasheet[1], Quad enable (QE) bit needs to be set to 0 to >>> use write protection (WP) pin. It also recommends setting default value of >>> QE to 0 to avoid a potential short issue. >> >> So you are using either dual or single io mode. Why can't you use the device tree property spi-{tx,rx}-bus-width? > > I tried setting spi-tx-bus-width and spi-rx-bus-width to either 0 or 1 > and WP still doesn't work. > For this chip, quad_enable will be set to spi_nor_sr2_bit1_quad_enable > (QER flag is BFPT_DWORD15_QER_SR2_BIT1_BUGGY)[1] > > spi_nor_write_sr_and_check() calls > spi_nor_write_16bit_sr_and_check()[2] and the function sets QE bit if > quad_enable is not NULL. > > [1] https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/sfdp.c#L575 > [2] https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/core.c#L879 > > Setting spi-{tx,rx}-bus-width still falls to this function and cases. with tx/rx bus width = 2, how quad mode is still possible? IOW, why do you need new property? You wrote here about driver, but I ask about bindings. Best regards, Krzysztof
Hi, On Tue, Aug 15, 2023 at 8:45 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > Some of the SKUs are using gigadevice gd25lq64c flash chip. The chip > default enables quad mode, which results in the write protect pin set to > IO pin. In trogdor platforms, we won't use quad enable for all SKUs, so > apply the property to disable spi nor's quad mode. > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > --- > arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi > index 5a33e16a8b677..0806ce8e86bea 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi > @@ -436,6 +436,7 @@ flash@0 { > spi-max-frequency = <37500000>; > spi-tx-bus-width = <2>; > spi-rx-bus-width = <2>; > + disable-quad-mode; This seems unnecessary. Unless "tx-bus-width" or "rx-bus-width" is 4 then Quad SPI isn't enabled. You don't need an explicit property since this can just be inferred from the tx and rx bus width. -Doug
On Wed, Aug 16, 2023 at 3:19 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 15/08/2023 19:21, Hsin-Yi Wang wrote: > > On Tue, Aug 15, 2023 at 11:59 PM Michael Walle <michael@walle.cc> wrote: > >> > >> Hi, > >> > >>> On gigadevice gd25lq64c, the quad mode is enabled after BFPT is parsed. > >>> According to datasheet[1], Quad enable (QE) bit needs to be set to 0 to > >>> use write protection (WP) pin. It also recommends setting default value of > >>> QE to 0 to avoid a potential short issue. > >> > >> So you are using either dual or single io mode. Why can't you use the device tree property spi-{tx,rx}-bus-width? > > > > I tried setting spi-tx-bus-width and spi-rx-bus-width to either 0 or 1 > > and WP still doesn't work. > > For this chip, quad_enable will be set to spi_nor_sr2_bit1_quad_enable > > (QER flag is BFPT_DWORD15_QER_SR2_BIT1_BUGGY)[1] > > > > spi_nor_write_sr_and_check() calls > > spi_nor_write_16bit_sr_and_check()[2] and the function sets QE bit if > > quad_enable is not NULL. > > > > [1] https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/sfdp.c#L575 > > [2] https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/core.c#L879 > > > > Setting spi-{tx,rx}-bus-width still falls to this function and cases. > > with tx/rx bus width = 2, how quad mode is still possible? IOW, why do > you need new property? You wrote here about driver, but I ask about > bindings. > This may be a bug in the driver that setting spi-{tx,rx}-bus-width still enables the QE bit. I proposed another method in the chip's fixup to deal with this issue instead of creating a new binding property. v2: https://lore.kernel.org/lkml/20230816104245.2676965-1-hsinyi@chromium.org/ > Best regards, > Krzysztof >
On 8/15/23 18:31, Hsin-Yi Wang wrote: > Some of the SKUs are using gigadevice gd25lq64c flash chip. The chip > default enables quad mode, which results in the write protect pin set to > IO pin. In mt8183 kukui, we won't use quad enable for all SKUs, so apply > the property to disable spi nor's quad mode. Hi Hsin-Yi, To me this property, and the way you 'apply' it, makes me think that you are using the devicetree as a configuration and not a description of the hardware itself. I think the driver should decide whether to use quad or not depending on the situation or the pinout of the device (as in your case quad mode overlaps with the WP pin) Eugen > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > --- > arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi > index 6ce16a265e053..8e4761e2b8ff4 100644 > --- a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi > +++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi > @@ -877,6 +877,7 @@ w25q64dw: flash@0 { > compatible = "winbond,w25q64dw", "jedec,spi-nor"; > reg = <0>; > spi-max-frequency = <25000000>; > + disable-quad-mode; > }; > }; >