Re: [PATCH v3 0/7] Fix AST2600 quad mode SPI pinmux settings

From: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
To: "Cédric Le Goater" <clg@kaod.org>, "Joel Stanley" <joel@jms.id.au>
Cc: devicetree <devicetree@vger.kernel.org>,
	linux-aspeed <linux-aspeed@lists.ozlabs.org>,
	Arnd Bergmann <arnd@arndb.de>,
	Linus Walleij <linus.walleij@linaro.org>,
	Jamie Iles <quic_jiles@quicinc.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	"Graeme Gregory" <quic_ggregory@quicinc.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v3 0/7] Fix AST2600 quad mode SPI pinmux settings
Date: Fri, 1 Apr 2022 15:25:41 -0700	[thread overview]
Message-ID: <a7902f38-06fc-56e1-c5fb-d224e859eeb6@quicinc.com> (raw)
In-Reply-To: <d652e592-29ce-3920-d1f8-66b3a617033f@kaod.org>

Hello Cédric,

On 4/1/2022 3:07 PM, Cédric Le Goater wrote:
> Hello Jae,
> 
> On 4/1/22 16:10, Jae Hyun Yoo wrote:
>> Hi Cédric,
>>
>> On 3/31/2022 9:06 AM, Jae Hyun Yoo wrote:
>>> Hello Cédric,
>>>
>>> On 3/31/2022 8:56 AM, Cédric Le Goater wrote:
>>>> Hello Jae,
>>>>
>>>> On 3/31/22 17:44, Jae Hyun Yoo wrote:
>>>>> On 3/30/2022 10:50 PM, Joel Stanley wrote:
>>>>>> On Tue, 29 Mar 2022 at 17:40, Jae Hyun Yoo 
>>>>>> <quic_jaehyoo@quicinc.com> wrote:
>>>>>>>
>>>>>>> I’m sending this patch series to fix current issues in AST2600 
>>>>>>> pinmux
>>>>>>> settings while enabling quad mode SPI support.
>>>>>>>
>>>>>>> FWSPI18 pins are basically 1.8v logic pins that are different 
>>>>>>> from the
>>>>>>> dedicated FWSPI pins that provide 3.3v logic level, so FWSPI18 
>>>>>>> pins can’t
>>>>>>> be grouped with FWSPIDQ2 and FWSPIDQ3, so this series fix the issue.
>>>>>>>
>>>>>>> Also, fixes QSPI1 and QSPI2 function settings in AST2600 pinctrl 
>>>>>>> dtsi to
>>>>>>> make it able to enable quad mode on SPI1 and SPI2 interfaces.
>>>>>>>
>>>>>>> With this series, quad mode pinmux can be set like below.
>>>>>>>
>>>>>>> FW SPI:
>>>>>>> &fmc {
>>>>>>>          pinctrl-names = "default";
>>>>>>>          pinctrl-0 = <&pinctrl_fwqspi_default>;
>>>>>>> }
>>>>>>>
>>>>>>> SPI1:
>>>>>>> &spi1 {
>>>>>>>          pinctrl-names = "default";
>>>>>>>          pinctrl-0 = <&pinctrl_qspi1_default>;
>>>>>>> }
>>>>>>>
>>>>>>> SPI2:
>>>>>>> &spi2 {
>>>>>>>          pinctrl-names = "default";
>>>>>>>          pinctrl-0 = <&pinctrl_qspi2_default>;
>>>>>>> }
>>>>>>
>>>>>> Thanks. I hope to see a board from you that uses this soon :)
>>>>>>
>>>>>> I'll send the patches as fixes once -rc1 is out.
>>>>>
>>>>> Thanks Joel!
>>>>>
>>>>> Yes, I would be able to send my BMC board dts soon.
>>>>> Thanks in advance for your review on that too.
>>>>
>>>> Out of curiosity, which driver are you using ? the one from SDK ?
>>>>
>>>> I proposed a new one for upstream supporting all AST2400, AST2500, 
>>>> AST2600
>>>> controllers. I would be glad to have some feedback if you have time.
>>>
>>> Yes, I saw your patch set of the new driver.
>>>
>>> I'm currently using this fix with legacy aspeed-smc driver after
>>> adding some fixes. I'll give it a try with your new driver as well and
>>> will give you some feedback if I find any.
>>
>> I tested this patch series using the new spi-aspeed-smc driver you
>> proposed.
>>
>> https://lore.kernel.org/linux-arm-kernel/20220325100849.2019209-1-clg@kaod.org/ 
>>
>>
>> I modified my BMC's device tree like below to enable quad mode.
>>
>>      &fmc {
>>          status = "okay";
>>          pinctrl-names = "default";
>>          pinctrl-0 = <&pinctrl_fwqspi_default>;
>>
>>          flash@0 {
>>              status = "okay";
>>              m25p,fast-read;
>>              label = "bmc";
>>              spi-rx-bus-width = <4>;
>>              spi-max-frequency = <133000000>;
>>      #include "openbmc-flash-layout-64.dtsi"
>>          };
>>
>>          flash@1 {
>>              status = "okay";
>>              m25p,fast-read;
>>              label = "alt-bmc";
>>              spi-rx-bus-width = <4>;
>>              spi-max-frequency = <133000000>;
>>      #include "openbmc-flash-layout-64-alt.dtsi"
>>          };
>>      };
> 
> Ah ! I have an AST2600 A0 EVB without FWQSPI wires and I could not test.
> I am glad you did. Thanks.
> 
> I did test the SPI1/SPI2 controllers with Quad SPI activated and results
> were OK. I think we could include your changes above in my patchset
> since A0 boards are pretty rare these days and unsupported.

Yes, you could include above device tree changes in your patch set for
AST2600 EVB but please test qemu ast2600-evb model first. It would crash
while booting if we enable quad mode because the machine's fmc spi is
set to w25q256 which doesn't emulate quad mode properly, so you may need
to apply below fix on top of the latest qemu tree.

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index a17b75f4940a..f7eea27f7656 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -753,7 +753,7 @@ static void 
aspeed_machine_ast2500_evb_class_init(ObjectClass *oc, void *data)
      mc->desc       = "Aspeed AST2500 EVB (ARM1176)";
      amc->soc_name  = "ast2500-a1";
      amc->hw_strap1 = AST2500_EVB_HW_STRAP1;
-    amc->fmc_model = "w25q256";
+    amc->fmc_model = "mx25l25635e";
      amc->spi_model = "mx25l25635e";
      amc->num_cs    = 1;
      amc->i2c_init  = ast2500_evb_i2c_init;

>> And I got these kernel boot logs.
>>
>> [    0.720745] spi-nor spi0.0: w25q512nwfm (65536 Kbytes)
>> [    0.837368] spi-aspeed-smc 1e620000.spi: CE0 read buswidth:4 
>> [0x406c0741]
>> [    0.846352] 5 fixed-partitions partitions found on MTD device bmc
>> [    0.853220] Creating 5 MTD partitions on "bmc":
>> [    0.858295] 0x000000000000-0x0000000e0000 : "u-boot"
>> [    0.865014] 0x0000000e0000-0x000000100000 : "u-boot-env"
>> [    0.872229] 0x000000100000-0x000000a00000 : "kernel"
>> [    0.878963] 0x000000a00000-0x000002a00000 : "rofs"
>> [    0.885406] 0x000002a00000-0x000004000000 : "rwfs"
>> [    0.892880] spi-nor spi0.1: w25q512nwfm (65536 Kbytes)
>> [    1.009460] spi-aspeed-smc 1e620000.spi: CE1 read buswidth:4 
>> [0x406c0741]
>> [    1.018334] 5 fixed-partitions partitions found on MTD device alt-bmc
>> [    1.025537] Creating 5 MTD partitions on "alt-bmc":
>> [    1.031027] 0x000000000000-0x0000000e0000 : "u-boot-alt"
>> [    1.038165] 0x0000000e0000-0x000000100000 : "u-boot-env-alt"
>> [    1.045623] 0x000000100000-0x000000a00000 : "kernel-alt"
>> [    1.052807] 0x000000a00000-0x000002a00000 : "rofs-alt"
>> [    1.059800] 0x000002a00000-0x000004000000 : "rwfs-alt"
>>
>> As you can see in the log, FMC10[31:28] and FMC14[31:28] are properly
>> set to 0100b which means 'quad bit read/write, data cycle only'.
> 
> yes and Quad Output SPI opcode (0x6c)

Indeed.

>> I verified that your new driver supports quad mode properly and it has
>> worked well so far without making any issue.
>>
>> Thanks for your making the new driver.
>> I left my comment in your patch proposal thread.
> 
> I will include your 'Tested' tag in v5. I hope to address Pratyush comments
> next week. Thanks a lot for the support.

Sure. Thanks again for the new driver! :)

-Jae
