On 02/23/2017 08:22 PM, Rush, Jason A. wrote:
> Marek Vasut wrote:
>> On 02/22/2017 06:37 PM, Rush, Jason A. wrote:
>>> Marek Vasut wrote:
>>>> On 02/21/2017 05:50 PM, Rush, Jason A. wrote:
>>>>> The socfpga arch uses a different value for the indaddrtrig reg than
>>>>> the ahbbase address. Adopting the Linux DT bindings separates the
>>>>> ahbbase and trigger-base addresses, allowing the trigger-base to be+
>>>>> set correctly on the socfpga arch.
>>>>>
>>>>> Tested on Terasic SoCkit dev board (Altera Cyclone V)
>>>>>
>>>>> Signed-off-by: Jason A. Rush <jason.r...@gd-ms.com>
>>>>> ---
>>>>> arch/arm/dts/socfpga.dtsi | 1 +
>>>>> drivers/spi/cadence_qspi.c | 2 ++
>>>>> drivers/spi/cadence_qspi.h | 1 +
>>>>> drivers/spi/cadence_qspi_apb.c | 4 ++--
>>>>> 4 files changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
>>>>> index 8588221e57..2aff0c2419 100644
>>>>> --- a/arch/arm/dts/socfpga.dtsi
>>>>> +++ b/arch/arm/dts/socfpga.dtsi
>>>>> @@ -644,6 +644,7 @@
>>>>> clocks = <&qspi_clk>;
>>>>> ext-decoder = <0>; /* external decoder */
>>>>> num-cs = <4>;
>>>>> + trigger-base = <0x00000000>;
>>>>
>>>> Can you separate the DT patch from the driver patch ? Also, can you check
>>>> the other users of the CQSPI driver to see if they define the
>>>> trigger base ?
>>>>
>>>
>>> Yes, I will separate into two patches.
>>
>> Thanks
>>
>>> I default the trigger_base to the same value as the ahbbase if the
>>> trigger-base
>>> was not defined in the DT. That way, the driver code works as before for
>>> architectures that expect the trigger_base to equal the value of the
>>> ahbbase.
>>> (e.g. TI K2G SoC). I updated only the Altera SoC dtsi file since that
>>> architecture
>>> needs a different value for the trigger_base.
>>
>> In fact, the Linux DT bindings have the following and no AHB base, so
>> please stick to that:
>>
>> - cdns,trigger-address : 32-bit indirect AHB trigger address.
>>
>> For details, see
>> Documentation/devicetree/bindings/mtd/cadence-quadspi.txt in Linux 4.8
>> or so and newer.
>>
>>> Should I change this behavior to default the value to 0x0, and patch the 3
>>> dts/dtsi
>>> files that use the cadence driver to explicitly include the trigger-base?
>>
>> Yeah, looks sensible.
>>
>>>>
>>>>> fifo-depth = <128>;
>>>>> sram-size = <128>;
>>>>> bus-num = <2>;
>>>>> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
>>>>> index 9a6e41f330..a18b331f6c 100644
>>>>> --- a/drivers/spi/cadence_qspi.c
>>>>> +++ b/drivers/spi/cadence_qspi.c
>>>>> @@ -296,6 +296,8 @@ static int cadence_spi_ofdata_to_platdata(struct
>>>>> udevice *bus)
>>>>>
>>>>> plat->regbase = (void *)data[0];
>>>>> plat->ahbbase = (void *)data[2];
>>>>> + plat->trigger_base = (void *)fdtdec_get_int(blob, node, "trigger-base",
>>>>> + (int)plat->ahbbase);
>>>>
>>>> Probably get u32 , but what about 64-bit systems ? Don't we have some
>>>> fdtdec_get.*addr ?
>>>
>>> You're right, this should be a u32. I don't think I should have made
>>> trigger_base
>>> a void* in the first place, but instead it should be a u32. Looking at the
>>> Linux
>>> kernel, which I just realized they call it trigger_address not
>>> trigger_base, it is just
>>> a 32-bit value that is written into a 32-bit wide register, not an iomem
>>> memory
>>> mapped pointer.
>>
>> Ah right, the reg is 32bit . Is it possible that on aargh64, someone
>> will pass 64bit trigger base in ?
>>
>>> What if I change it to a u32 and rename it to trigger_address (which I
>>> should
>>> have done the first time)? That would align us correctly with the Linux
>>> kernel.
>>
>> See above /wrt the naming. void __iomem * works on both 32 and 64bit
>> systems, so I'd prefer to see that.
>>
>>>>
>>>>> plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
>>>>>
>>>>> /* All other paramters are embedded in the child node */ diff --git
>>>>> a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h index
>>>>> d1927a4003..394820f308 100644
>>>>> --- a/drivers/spi/cadence_qspi.h
>>>>> +++ b/drivers/spi/cadence_qspi.h
>>>>> @@ -18,6 +18,7 @@ struct cadence_spi_platdata {
>>>>> unsigned int max_hz;
>>>>> void *regbase;
>>>>> void *ahbbase;
>>>>
>>>> Can you remove the AHB base ? I think it's no longer used.
>>>
>>> ahbbase is still used in cadence_qspi_apb.c, it's the register that the
>>> QSPI data
>>> is read from, so it's still needed.
>>
>> Aaaah, and looking at the Linux bindings, there it comes from the reg
>> property. OK, so much for the bloody confusing naming. If you feel like
>> cleaning this up, separate patch is welcome, if not ... oh well.
>>
>>>> Also, I think this should be void __iomem * here , also for regbase .
>>>>
>>>
>>> This is probably true, regbase and ahbbase should both be __iomem *, but
>>> that feels like a different clean-up patch. If you'd like me to, I could
>>> update
>>> both of these as part of this patch though.
>>
>> Yeah, that'd be brilliant if you could clean this bit too. Thanks!
>>
>>>>
>>>>> + void *trigger_base;
>>>>>
>>>>> u32 page_size;
>>>>> u32 block_size;
>>>>> diff --git a/drivers/spi/cadence_qspi_apb.c
>>>>> b/drivers/spi/cadence_qspi_apb.c index e02f2217f4..0e66d5fd82 100644
>>>>> --- a/drivers/spi/cadence_qspi_apb.c
>>>>> +++ b/drivers/spi/cadence_qspi_apb.c
>>>>> @@ -560,7 +560,7 @@ int cadence_qspi_apb_indirect_read_setup(struct
>>>>> cadence_spi_platdata *plat,
>>>>> addr_bytes = cmdlen - 1;
>>>>>
>>>>> /* Setup the indirect trigger address */
>>>>> - writel((u32)plat->ahbbase,
>>>>> + writel((u32)plat->trigger_base,
>>>>> plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
>>>>>
>>>>> /* Configure the opcode */
>>>>> @@ -710,7 +710,7 @@ int cadence_qspi_apb_indirect_write_setup(struct
>>>>> cadence_spi_platdata *plat,
>>>>> return -EINVAL;
>>>>> }
>>>>> /* Setup the indirect trigger address */
>>>>> - writel((u32)plat->ahbbase,
>>>>> + writel((u32)plat->trigger_base,
>>>>> plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
>>>>>
>>>>> /* Configure the opcode */
>>>>>
>>>>
>>>
>
> While I was debugging some of my changes, I noticed that the data being read
> from the
> QSPI flash device appears to be random. The CPU no longer resets while
> performing a
> read when the indirect trigger address is setup correctly for the Altrera
> SoC, but there
> appears to be a larger problem with reading data in general.
>
> When I apply my patch to the v2016.11 release, reads appear correct.
> However, when I
> apply my patch to the v2017.01 release, the data read from the QSPI device
> appear to be
> random/corrupt. I noticed the cadence_spi_apb.c file changed significantly
> between
> v2016.11 and v2017.01, possibly a change in this file is causing the problem
> on the Altera
> SoC.
>
> I'm not really that familiar with the cadence device, so this issue is
> getting a little beyond a
> simple patch to setup the indirect trigger address correctly for the Altrera
> SoC. Is there
> anyone more familiar with the cadence device on the Altera SoC that could
> take a look
> into this?
Vignesh did those changes, so I think he can assist you. In the
meantime, you can try git bisect . Another thing you can try is
disabling the dcache and see if that fixes things (dcache off),
I recall seeing some caching issues with CQSPI.
--
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
