Re: [U-Boot] [PATCH] spi: cadence_qspi_apb: Add trigger-base DT bindings from Linux

[R, Vignesh]

On 2/28/2017 8:38 PM, Rush, Jason A. wrote:
[...]
> 
> This also works.
> 
> Marek - how do you feel about a patch series with the following:
> 
> 1. revert commit 57897c13de03ac0136d64641a3eab526c6810387
>  spi: cadence_qspi_apb: Use 32 bit indirect write transaction when 
> possible
> 2. revert commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f
>  spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible
> 3. Apply my slightly modified version of 
> https://patchwork.ozlabs.org/patch/693069/
>  (should I keep Vignesh as the signed-off?)

Depends on how much you have changed the code. If the change is
significant then change the authorship to you and drop my signed-off.
Else, keep the authorship and signed-off.
If you are adding something new to the patch like adding code to make
sure that only 32bit data reads are issued, then I suggest you to submit
that change as separate patch.

> 4. Clean-up loading the reg variables to use fdtdec_get_addr_xxx(...) and mark
>  them all as __iomem
> 5. Load the indirect-trigger property from the DT as a u32.  I think this 
> still
>  needs to be a u32 because it's used in a writel(...) 32-bit write call in
>  cadence_qspi_apb.c
> 6. Add indirect-trigger to dts files that use cadence driver
> 
> I think that captures all the changes needed.
> 
> Also, for the naming of the DT property, Linux has uses a 'cdns,' prefix
> (e.g. cdns,trigger-address) for a number of their properties (cdns,tshsl-ns,
> cdns,tsd2d-ns, etc.), but u-boot does not currently use the 'cdns,' prefix for
> the same properties.  Do you want the 'cdns,' prefix on trigger-address? 
> If so, do you want me to rename all the other properties to align them with
> the Linux DT as an additional patch?
> 
> --
> Regards,
> Jason
> 

-- 
Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] spi: cadence_qspi_apb: Add trigger-base DT bindings from Linux

[R, Vignesh]

On 2/25/2017 1:25 AM, Rush, Jason A. wrote:
> R, Vignesh wrote:
>> On 2/24/2017 12:55 AM, Marek Vasut wrote:
>>> 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:
>> [...]
>>>>
>>
>> You could try reverting my commits:
>> commit 57897c13de03ac0136d64641a3eab526c6810387 spi: cadence_qspi_apb:
>> Use 32 bit indirect write transaction when possible
>> commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f spi: cadence_qspi_apb:
>> Use 32 bit indirect read transaction when possible
>>
> 
> When I reverted these two commits and added my patch for the indirect
> trigger_address, it works correctly.
>

Oops, these patches are required as Cadence QSPI controller(I am not
sure if all versions of IP are newer versions only) has a limitation
that the external master is only permitted to issue 32-bit data
interface reads until the last word of an indirect transfer.


> Also, when I disabled the dcache (dcache off) as Marek suggested, it works
> correctly when running from the master branch (again with my indirect
> trigger_address patch).
> 

Just that I understand correctly, with latest master(with no patches
reverted) + your patch for indirect trigger_address + dcache off, you
don't see any problem?

>>
>> But there were other patches by others in v2017.01-rc1, like
>> spi: cadence_qspi: Fix CS timings which may have impact.
>>
> 
> I left all other commits in except the two Vignesh suggested to revert, so
> it seems to be related to those two commits and caching.  As another data
> point, I can load and boot linux with caching on from another source (MMC).
> So I don't think it's a problem with memory/caching in general.
> 
> Any suggestions on how to proceed from here?
> 

My patches use common bounce_buffer implementation which does dcache
flush/invalidate and if dcache has issues then I guess those operations
may be causing data corruption. Could you do a bit more research for me?

1. As a hack, could you just disable dcache operations in bounce_buffer
implementation? Here is the diff for the same:


diff --git a/common/bouncebuf.c b/common/bouncebuf.c
index 054d9e0302cc..2878b9eed1ae 100644
--- a/common/bouncebuf.c
+++ b/common/bouncebuf.c
@@ -55,21 +55,21 @@ int bounce_buffer_start(struct bounce_buffer *state, void 
*data,
 * Flush data to RAM so DMA reads can pick it up,
 * and any CPU writebacks don't race with DMA writes
 */
-   flush_dcache_range((unsigned long)state->bounce_buffer,
-   (unsigned long)(state->bounce_buffer) +
-   state->len_aligned);
+// flush_dcache_range((unsigned long)state->bounce_buffer,
+// (unsigned long)(state->bounce_buffer) +
+// state->len_aligned);

return 0;
 }

 int bounce_buffer_stop(struct bounce_buffer *state)
 {
-   if (state->flags & GEN_BB_WRITE) {
-   /* Invalidate cache so that CPU can see any newly DMA'd data */
-   invalidate_dcache_range((unsigned long)state->bounce_buffer,
-   (unsigned long)(state->bounce_buffer) +
-   state->len_aligned);
-   }
+// if (state->flags & GEN_BB_WRITE) {
+// /* Invalidate cache so that CPU can see any newly DMA'd data */
+// invalidate_dcache_range((unsigned long)state->bounce_buffer,
+// (unsigned long)(state->bounce_buffer) +
+// state->len_aligned);
+// }

if (state->bounce_buffer == state->user_buffer)
return 0;
>
>
>

2. If that works, I guess there is some issue wrt CQSPI and dcache on your 
platform,
I suggest you to revert my above two patches and try non bounce buffer version 
of 
my changes here: https://patchwork.ozlabs.org/patch/693069/.
This patch takes care of indirect write. I don't have similar patch for
indirect read but that wasn't required.

-- 
Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] spi: cadence_qspi_apb: Add trigger-base DT bindings from Linux

[R, Vignesh]

On 2/24/2017 12:55 AM, Marek Vasut wrote:
> 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:
[...]
>>
>> 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.
>>

How random is it? Is the problem seen only when unaligned read/write are
done or when length of transfer is not a multiple of word(4 byte)?
If the data is really random in all cases, then I suspect timing issues.
Please see if delay values are populated correctly in DT.

>> 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.
> 

You could try reverting my commits:
commit 57897c13de03ac0136d64641a3eab526c6810387 spi: cadence_qspi_apb:
Use 32 bit indirect write transaction when possible
commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f spi: cadence_qspi_apb:
Use 32 bit indirect read transaction when possible

But there were other patches by others in v2017.01-rc1, like
spi: cadence_qspi: Fix CS timings which may have impact.


-- 
Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH RESEND v2 1/2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

[R, Vignesh]

On 12/21/2016 10:42 AM, Vignesh R wrote:
> According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC
> TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit
> data interface writes until the last word of an indirect transfer
> otherwise indirect writes is known to fails sometimes. So, make sure
> that QSPI indirect writes are 32 bit sized except for the last write. If
> the txbuf is unaligned then use bounce buffer to avoid data aborts.
> 
> So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER
> for all boards that use Cadence QSPI driver.
> 
> [1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf
> 
> Signed-off-by: Vignesh R 
> Reviewed-by: Marek Vasut 

Gentle ping on the series...

> ---
> 
> Resend v2: Rebased on latest rc.
> Link to v2:https://patchwork.ozlabs.org/patch/698614/
> 
>  drivers/spi/cadence_qspi_apb.c   | 26 --
>  include/configs/k2g_evm.h|  1 +
>  include/configs/socfpga_common.h |  1 +
>  include/configs/stv0991.h|  1 +
>  4 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
> index df6a91fc9f7b..5d5b6f0d350b 100644
> --- a/drivers/spi/cadence_qspi_apb.c
> +++ b/drivers/spi/cadence_qspi_apb.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "cadence_qspi.h"
>  
>  #define CQSPI_REG_POLL_US1 /* 1us */
> @@ -724,6 +725,17 @@ int cadence_qspi_apb_indirect_write_execute(struct 
> cadence_spi_platdata *plat,
>   unsigned int remaining = n_tx;
>   unsigned int write_bytes;
>   int ret;
> + struct bounce_buffer bb;
> + u8 *bb_txbuf;
> +
> + /*
> +  * Handle non-4-byte aligned accesses via bounce buffer to
> +  * avoid data abort.
> +  */
> + ret = bounce_buffer_start(, (void *)txbuf, n_tx, GEN_BB_READ);
> + if (ret)
> + return ret;
> + bb_txbuf = bb.bounce_buffer;
>  
>   /* Configure the indirect read transfer bytes */
>   writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
> @@ -734,11 +746,11 @@ int cadence_qspi_apb_indirect_write_execute(struct 
> cadence_spi_platdata *plat,
>  
>   while (remaining > 0) {
>   write_bytes = remaining > page_size ? page_size : remaining;
> - /* Handle non-4-byte aligned access to avoid data abort. */
> - if (((uintptr_t)txbuf % 4) || (write_bytes % 4))
> - writesb(plat->ahbbase, txbuf, write_bytes);
> - else
> - writesl(plat->ahbbase, txbuf, write_bytes >> 2);
> + writesl(plat->ahbbase, bb_txbuf, write_bytes >> 2);
> + if (write_bytes % 4)
> + writesb(plat->ahbbase,
> + bb_txbuf + rounddown(write_bytes, 4),
> + write_bytes % 4);
>  
>   ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
>  CQSPI_REG_SDRAMLEVEL_WR_MASK <<
> @@ -748,7 +760,7 @@ int cadence_qspi_apb_indirect_write_execute(struct 
> cadence_spi_platdata *plat,
>   goto failwr;
>   }
>  
> - txbuf += write_bytes;
> + bb_txbuf += write_bytes;
>   remaining -= write_bytes;
>   }
>  
> @@ -759,6 +771,7 @@ int cadence_qspi_apb_indirect_write_execute(struct 
> cadence_spi_platdata *plat,
>   printf("Indirect write completion error (%i)\n", ret);
>   goto failwr;
>   }
> + bounce_buffer_stop();
>  
>   /* Clear indirect completion status */
>   writel(CQSPI_REG_INDIRECTWR_DONE,
> @@ -769,6 +782,7 @@ failwr:
>   /* Cancel the indirect write */
>   writel(CQSPI_REG_INDIRECTWR_CANCEL,
>  plat->regbase + CQSPI_REG_INDIRECTWR);
> + bounce_buffer_stop();
>   return ret;
>  }
>  
> diff --git a/include/configs/k2g_evm.h b/include/configs/k2g_evm.h
> index 2da0d8dd7f00..4f9c42abac7e 100644
> --- a/include/configs/k2g_evm.h
> +++ b/include/configs/k2g_evm.h
> @@ -78,6 +78,7 @@
>  #define CONFIG_CADENCE_QSPI
>  #define CONFIG_CQSPI_REF_CLK 38400
>  #define CONFIG_CQSPI_DECODER 0x0
> +#define CONFIG_BOUNCE_BUFFER
>  #endif
>  
>  #endif /* __CONFIG_K2G_EVM_H */
> diff --git a/include/configs/socfpga_common.h 
> b/include/configs/socfpga_common.h
> index 58a655084491..e50c2fac8d99 100644
> --- a/include/configs/socfpga_common.h
> +++ b/include/configs/socfpga_common.h
> @@ -208,6 +208,7 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
>  #define CONFIG_CQSPI_REF_CLK cm_get_qspi_controller_clk_hz()
>  #endif
>  #define CONFIG_CQSPI_DECODER 0
> +#define CONFIG_BOUNCE_BUFFER
>  
>  /*
>   * Designware SPI support
> diff --git a/include/configs/stv0991.h b/include/configs/stv0991.h
> index bfd1bd719285..09a3064bd6d6 100644
> --- a/include/configs/stv0991.h
> +++ b/include/configs/stv0991.h
> @@ 

Re: [U-Boot] [PATCH RESEND v2 2/2] spi: ti_qspi: Fix baudrate divider calculation

[R, Vignesh]

[...]

On 11/4/2016 4:31 PM, Jagan Teki wrote:
>>> >> diff --git a/drivers/spi/ti_qspi.c b/drivers/spi/ti_qspi.c
>>> >> index 52520dff6325..b5de70bf40e3 100644
>>> >> --- a/drivers/spi/ti_qspi.c
>>> >> +++ b/drivers/spi/ti_qspi.c
>>> >> @@ -16,6 +16,7 @@
>>> >>  #include 
>>> >>  #include 
>>> >>  #include 
>>> >> +#include 
>>> >>
>>> >>  DECLARE_GLOBAL_DATA_PTR;
>>> >>
>>> >> @@ -118,21 +119,19 @@ static void ti_spi_set_speed(struct ti_qspi_priv 
>>> >> *priv, uint hz)
>>> >> if (!hz)
>>> >> clk_div = 0;
>>> >> else
>>> >> -   clk_div = (priv->fclk / hz) - 1;
>>> >> -
>>> >> -   debug("ti_spi_set_speed: hz: %d, clock divider %d\n", hz, 
>>> >> clk_div);
>>> >> +   clk_div = DIV_ROUND_UP(priv->fclk, hz) - 1;
>>> >>
>>> >> /* disable SCLK */
>>> >> writel(readl(>base->clk_ctrl) & ~QSPI_CLK_EN,
>>> >>>base->clk_ctrl);
>> >
>> > Move this before enable SCLK.

Ok...

> Do send the updated v3 or discusses further, I need to send the release PR?

Sorry for the delay.. Posted v3 with above change.

-- 
Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH RESEND v2 1/2] ARM: dts: dra7xx: Update spi-max-frequency for qspi slave node

[R, Vignesh]

On 10/31/2016 5:24 PM, Tom Rini wrote:
> On Mon, Oct 31, 2016 at 09:40:34AM +0530, Vignesh R wrote:
> 
>> Update the spi-max-frequency property of m25p80 flash slave to match
>> that of TI QSPI controller node, so that QSPI operations happen at
>> maximum supported frequency of 76.8MHz.
>>
>> Signed-off-by: Vignesh R 
>> Reviewed-by: Jagan Teki 
> 
> And this is also done in the kernel right?  Thanks!
> 

Yes, the kernel patch is merged.

-- 
Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/2] spi: ti_qspi: Fix baudrate divider calculation

[R, Vignesh]

On 10/14/2016 12:27 PM, Jagan Teki wrote:
> On Fri, Oct 14, 2016 at 10:54 AM, Vignesh R  wrote:
...
  DECLARE_GLOBAL_DATA_PTR;

 @@ -118,7 +119,7 @@ static void ti_spi_set_speed(struct ti_qspi_priv 
 *priv, uint hz)
 if (!hz)
 clk_div = 0;
 else
 -   clk_div = (priv->fclk / hz) - 1;
 +   clk_div = DIV_ROUND_UP(priv->fclk, hz) - 1;
>>>
>>> Better to have a checks for min and max divider values or mask.
>>
>> That code already exists in this function.
> 
> True but it's unnecessary to print the wrong baud prior to checking.
> Do the check, then print/debug and finally write reg.
> 

Posted a v2 in reply to the patch. Thanks for the review!

Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 3/4] net: cpsw: Add support to drive gpios for ethernet to be functional

[R, Vignesh]

On 7/26/2016 12:26 PM, Wolfgang Denk wrote:
> Dear "R, Vignesh",
> 
> In message <a210f61e-bf51-a946-013c-dd98fd9a1...@ti.com> you wrote:
>>
>>>> @@ -1203,6 +1206,16 @@ static int cpsw_eth_ofdata_to_platdata(struct 
>>>> udevice *dev)
>>>>return -ENOENT;
>>>>}
>>>>  
>>>> +
>>>> +  num_mode_gpios = gpio_get_list_count(dev, "mode-gpios");
>>>
>>> Extra blank line added.
>>
>> I added blank line for readability. So, that code block handling
>> toggling of "mode-gpios" is separated from the rest. I can remove that
>> if that seems unnecessary.
> 
> There was already a blank line which provides enough separation. No
> need to have multiple consecutive blank lines.

Sorry, will clean that up in v2. Thanks!

Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 3/4] net: cpsw: Add support to drive gpios for ethernet to be functional

[R, Vignesh]

On 7/25/2016 7:08 PM, Tom Rini wrote:
> On Mon, Jul 25, 2016 at 06:40:22PM +0530, Vignesh R wrote:
> 
>> On DRA72 EVM, cpsw slaves may be muxed with other modules. This
>> selection is controlled by a pcf gpio line. Add support for cpsw driver
>> to acquire mode-gpios and select the appropriate slave using gpio APIs.
>>
>> Signed-off-by: Vignesh R 
> 
> Reviewed-by: Tom Rini 
> 
> Minor nit below:
> 
>> @@ -1203,6 +1206,16 @@ static int cpsw_eth_ofdata_to_platdata(struct udevice 
>> *dev)
>>  return -ENOENT;
>>  }
>>  
>> +
>> +num_mode_gpios = gpio_get_list_count(dev, "mode-gpios");
> 
> Extra blank line added.

I added blank line for readability. So, that code block handling
toggling of "mode-gpios" is separated from the rest. I can remove that
if that seems unnecessary.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/2] spi: ti_qspi: Fix failure on multiple READ_ID cmd

[R, Vignesh]

On 7/11/2016 12:05 PM, Jagan Teki wrote:
> On 11 July 2016 at 11:00, Vignesh R  wrote:
>> Populating QSPI_RD_SNGL bit(0x1) in priv->cmd means that value
>> QSPI_INVAL (0x4) is not written to CMD field of QSPI_SPI_CMD_REG in
>> ti_qspi_cs_deactivate(). Therefore CS is never deactivated between
>> successive READ ID which results in sf probe to fail.
>> Fix this by not populating priv->cmd with QSPI_RD_SNGL and OR it wih
>> priv->cmd as required (similar to the convention followed in the
>> driver).
>>
>> Signed-off-by: Vignesh R 
>> ---
>>  drivers/spi/ti_qspi.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/ti_qspi.c b/drivers/spi/ti_qspi.c
>> index 9a372ad31dae..376fe378ed63 100644
>> --- a/drivers/spi/ti_qspi.c
>> +++ b/drivers/spi/ti_qspi.c
>> @@ -247,13 +247,12 @@ static int __ti_qspi_xfer(struct ti_qspi_priv *priv, 
>> unsigned int bitlen,
>> debug("tx done, status %08x\n", status);
>> }
>> if (rxp) {
>> -   priv->cmd |= QSPI_RD_SNGL;
>> debug("rx cmd %08x dc %08x\n",
>>   priv->cmd, priv->dc);
> 
> | QSPI_RD_SNGL on debug statement as well.

Thanks, will fix this in v2.

> 
>> #ifdef CONFIG_DRA7XX
>> udelay(500);
>> #endif
> 
> Can't we fix this delay, still need?

The patch that added this delay ( b545a98f5dc563 spi: ti_qspi: Add delay
for successful bulk erase) says its added to meet bulk erase timing
constraints (AFAIK, I believe bulk erase is same as chip erase which is
not supported by sf erase but may be supported in future). I will
experiment a bit and convince myself whether this delay is really
applicable or not.

Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v4] dm: core: implement dev_map_phsymem()

[R, Vignesh]

On 5/6/2016 9:00 PM, Jagan Teki wrote:
> On 6 May 2016 at 09:28, Vignesh R  wrote:
>> This API helps to map physical register addresss pace of device to
>> virtual address space easily. Its just a wrapper around map_physmem()
>> with MAP_NOCACHE flag.
>>
>> Signed-off-by: Vignesh R 
>> Suggested-by: Simon Glass 
>> Reviewed-by: Jagan Teki 
>>
>> ---
>>
>> v4: Reorder include files to avoid build warning on dra7xx.
>>
>>  drivers/core/device.c | 6 ++
>>  include/dm/device.h   | 9 +
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>> index 1322991d6c7b..6b19b4b8c7a0 100644
>> --- a/drivers/core/device.c
>> +++ b/drivers/core/device.c
>> @@ -10,6 +10,7 @@
>>   */
>>
>>  #include 
>> +#include 
> 
> I think this look same as v3 [1] please check?

Yeah, v3 has the include files in correct. Please ignore this patch.
Sorry for the spam.

Regards
Vignesh.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] QSPI XIP boot on am437x

[R, Vignesh]

Hi Albert,

Thanks for the response!

On 11/10/2015 5:44 PM, Albert ARIBAUD wrote:
> Hello Vignesh,
> 
> On Tue, 10 Nov 2015 14:29:54 +0530, Vignesh R  wrote:
>> Hi,
>>
>> With commit 7ae8350f67eea("ti: armv7: Move SPL SDRAM init to the right
>> place, drop unused CONFIG_SPL_STACK") QSPI XIP boot appears to be broken
>> on AM437x SK EVM.
>>
>> Following UART initialization code (as indicated by TODO) causes the XIP
>> boot failure.
>>
>>
>> In arch/arm/cpu/armv7/am33xx/board.c:
>> @@ -275,9 +275,9 @@ void s_init(void)
>>  #if defined(CONFIG_NOR_BOOT) || defined(CONFIG_QSPI_BOOT)
>> /* TODO: This does not work, gd is not available yet */
>>gd->baudrate = CONFIG_BAUDRATE;
>>serial_init();
>>gd->have_console = 1;
>>  #endif
>>
>>
>> I was able to boot successfully from QSPI by commenting out the above code.
>> But, could you suggest me what needs to be done as part of TODO in order
>> to get QSPI XIP boot working?
> 
> Can't answer specifically on am437x, but basically, the problem you
> have here may be that the code is running in a C runtime environment in
> which only the global data is writable. This global data is a struct
> global_data (see include/asm-generic/global_data.h) which is supposed
> to be pointed to by the variable GD.
> 
> Can you detail the failure you are encountering?
> 
> Typically, GD is set up from within function board_init_f_mem(), before
> calling board_init_f(), or from arch/arm/lib/crt0.S.
> 
> So all depends on whether s_init() is executed before or after
> board_init_f_mem().
> 
> If s_init() runs before board_init_f_mem(), then you must move it to
> run after board_init_f_mem(). :)
> 
> If s_init() runs after board_init_f_mem() and you still have the issue,
> then your problem would be that gd is badly initialized. Is your board
> built for Thumb with a recent compiler, by any chance? I any case, can
> you test the value of gd when reaching the gd->baudrate line above?

Yes, s_init() is being called before call to _main(in
arch/arm/lib/crt0.S that sets up GD) but all these calls are from arm
generic files and nothing specific to am437x:
reset (arch/arm/cpu/armv7/start.S)
-> cpu_init_crit(arch/arm/cpu/armv7/start.S)
 -> lowlevel_init(arch/arm/cpu/armv7/lowlevel_init.S)
-> s_init(arch/arm/cpu/armv7/board/am33xx.c)

The failure appears to be in serial_init(), it tries to access gd->flags
which is not allocated yet and reads wrong value.

I was wondering whether entire UART initialization code in s_init() in
arch/arm/cpu/armv7/board/am33xx.c can be moved to the end of
board_init_f() where GD is accessible.

Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [U-Boot RESEND v2 09/10] spi: ti_qspi: Use DMA to read from qspi flash

[R, Vignesh]

On 8/17/2015 1:48 PM, Jagan Teki wrote:
 On 17 August 2015 at 13:29, Vignesh R vigne...@ti.com wrote:
 ti_qspi uses memory map mode for faster read. Enabling DMA will increase
 read speed by 3x @48MHz on DRA74 EVM.

 Signed-off-by: Vignesh R vigne...@ti.com
 Reviewed-by: Jagan Teki jt...@openedev.com
 ---
  drivers/spi/ti_qspi.c | 23 +++
  1 file changed, 23 insertions(+)

 diff --git a/drivers/spi/ti_qspi.c b/drivers/spi/ti_qspi.c
 index 3356c0f072e5..753d68980bd6 100644
 --- a/drivers/spi/ti_qspi.c
 +++ b/drivers/spi/ti_qspi.c
 @@ -13,6 +13,8 @@
  #include spi.h
  #include asm/gpio.h
  #include asm/omap_gpio.h
 +#include asm/omap_common.h
 +#include asm/ti-common/ti-edma3.h

  /* ti qpsi register bit masks */
  #define QSPI_TIMEOUT200
 @@ -347,3 +349,24 @@ int spi_xfer(struct spi_slave *slave, unsigned int 
 bitlen, const void *dout,

 return 0;
  }
 Please add below comment here, I have asked the same on previous version patch
 this will track us to the work future.
 
 /* TODO: control from sf layer to here through dm-spi */

Oops.. Sorry, I overlooked it.. Will add the comment and send it soon.
Thanks!

 +#ifdef CONFIG_TI_EDMA3
 +void spi_flash_copy_mmap(void *data, void *offset, size_t len)
 +{
 +   unsigned intaddr = (unsigned int) (data);
 +   unsigned intedma_slot_num = 1;
 +
 +   /* Invalidate the area, so no writeback into the RAM races with DMA 
 */
 +   invalidate_dcache_range(addr, addr + roundup(len, 
 ARCH_DMA_MINALIGN));
 +
 +   /* enable edma3 clocks */
 +   enable_edma3_clocks();
 +
 +   /* Call edma3 api to do actual DMA transfer */
 +   edma3_transfer(EDMA3_BASE, edma_slot_num, data, offset, len);
 +
 +   /* disable edma3 clocks */
 +   disable_edma3_clocks();
 +
 +   *((unsigned int *)offset) += len;
 +}
 +#endif
 --
 2.5.0
 
 thanks!
 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 08/11] spi: ti_qspi: Use DMA to read from qspi flash

[R, Vignesh]

On 7/15/2015 12:32 AM, Tom Rini wrote:
 On Thu, Jul 09, 2015 at 12:10:03PM +0530, Vignesh R wrote:


 On 07/03/2015 05:12 PM, Tom Rini wrote:
 On Fri, Jul 03, 2015 at 04:46:10PM +0530, Vignesh R wrote:

 ti_qspi uses memory map mode for faster read. Enabling DMA will increase
 read speed by 3x @48MHz on DRA74 EVM.

 Signed-off-by: Vignesh R vigne...@ti.com

 This ignores the feedback from
 http://lists.denx.de/pipermail/u-boot/2014-July/183715.html where we
 need to model the DMA changes on how it's done for mxs_spi.c

 Is the following patch an acceptable solution?

 
 Jagan, are you OK with the SPI side of this?  Thanks!

Gentle ping... Any comments? I will send a v2 for this series if the
below patch is acceptable.

 
 8---

 Move DMA related initialization code to helper function in ti-edma3
 driver. Use this function for scheduling DMA transfer from ti_qspi driver.

 diff --git a/arch/arm/include/asm/ti-common/ti-edma3.h
 b/arch/arm/include/asm/ti-common/ti-edma3.h
 index 5adc1dac0e65..6a7a321c1bdf 100644
 --- a/arch/arm/include/asm/ti-common/ti-edma3.h
 +++ b/arch/arm/include/asm/ti-common/ti-edma3.h
 @@ -117,5 +117,7 @@ void edma3_set_src_addr(u32 base, int slot, u32 src);
  void edma3_set_transfer_params(u32 base, int slot, int acnt,
 int bcnt, int ccnt, u16 bcnt_rld,
 enum edma3_sync_dimension sync_mode);
 +void edma3_transfer(unsigned long edma3_base_addr, unsigned int
 +edma_slot_num, void *dst, void *src, size_t len);

  #endif
 diff --git a/drivers/dma/ti-edma3.c b/drivers/dma/ti-edma3.c
 index 8184ded9fa81..d6a427f2e21d 100644
 --- a/drivers/dma/ti-edma3.c
 +++ b/drivers/dma/ti-edma3.c
 @@ -382,3 +382,81 @@ void qedma3_stop(u32 base, struct
 edma3_channel_config *cfg)
  /* Clear the channel map */
  __raw_writel(0, base + EDMA3_QCHMAP(cfg-chnum));
  }
 +
 +void edma3_transfer(unsigned long edma3_base_addr, unsigned int
 +edma_slot_num, void *dst, void *src, size_t len)
 +{
 +struct edma3_slot_configslot;
 +struct edma3_channel_config edma_channel;
 +int b_cnt_value = 1;
 +int rem_bytes  = 0;
 +int a_cnt_value = len;
 +unsigned intaddr = (unsigned int) (dst);
 +unsigned intmax_acnt  = 0x7FFFU;
 +
 +if (len  max_acnt) {
 +b_cnt_value = (len / max_acnt);
 +rem_bytes  = (len % max_acnt);
 +a_cnt_value = max_acnt;
 +}
 +
 +slot.opt= 0;
 +slot.src= ((unsigned int) src);
 +slot.acnt   = a_cnt_value;
 +slot.bcnt   = b_cnt_value;
 +slot.ccnt   = 1;
 +slot.src_bidx   = a_cnt_value;
 +slot.dst_bidx   = a_cnt_value;
 +slot.src_cidx   = 0;
 +slot.dst_cidx   = 0;
 +slot.link   = EDMA3_PARSET_NULL_LINK;
 +slot.bcntrld= 0;
 +slot.opt= EDMA3_SLOPT_TRANS_COMP_INT_ENB |
 +  EDMA3_SLOPT_COMP_CODE(0) |
 +  EDMA3_SLOPT_STATIC | EDMA3_SLOPT_AB_SYNC;
 +
 +edma3_slot_configure(edma3_base_addr, edma_slot_num, slot);
 +edma_channel.slot = edma_slot_num;
 +edma_channel.chnum = 0;
 +edma_channel.complete_code = 0;
 + /* set event trigger to dst update */
 +edma_channel.trigger_slot_word = EDMA3_TWORD(dst);
 +
 +qedma3_start(edma3_base_addr, edma_channel);
 +edma3_set_dest_addr(edma3_base_addr, edma_channel.slot, addr);
 +
 +while (edma3_check_for_transfer(edma3_base_addr, edma_channel))
 +;
 +qedma3_stop(edma3_base_addr, edma_channel);
 +
 +if (rem_bytes != 0) {
 +slot.opt= 0;
 +slot.src=
 +(b_cnt_value * max_acnt) + ((unsigned int) src);
 +slot.acnt   = rem_bytes;
 +slot.bcnt   = 1;
 +slot.ccnt   = 1;
 +slot.src_bidx   = rem_bytes;
 +slot.dst_bidx   = rem_bytes;
 +slot.src_cidx   = 0;
 +slot.dst_cidx   = 0;
 +slot.link   = EDMA3_PARSET_NULL_LINK;
 +slot.bcntrld= 0;
 +slot.opt= EDMA3_SLOPT_TRANS_COMP_INT_ENB |
 +  EDMA3_SLOPT_COMP_CODE(0) |
 +  EDMA3_SLOPT_STATIC | EDMA3_SLOPT_AB_SYNC;
 +edma3_slot_configure(edma3_base_addr, edma_slot_num, slot);
 +edma_channel.slot = edma_slot_num;
 +edma_channel.chnum = 0;
 +edma_channel.complete_code = 0;
 +/* set event trigger to dst update */
 +edma_channel.trigger_slot_word = EDMA3_TWORD(dst);
 +
 +qedma3_start(edma3_base_addr, edma_channel);
 +edma3_set_dest_addr(edma3_base_addr, edma_channel.slot, addr +
 +(max_acnt * b_cnt_value));
 +

Re: [U-Boot] [PATCH 08/11] spi: ti_qspi: Use DMA to read from qspi flash

[R, Vignesh]

On 7/3/2015 5:12 PM, Tom Rini wrote:
 On Fri, Jul 03, 2015 at 04:46:10PM +0530, Vignesh R wrote:
 
 ti_qspi uses memory map mode for faster read. Enabling DMA will increase
 read speed by 3x @48MHz on DRA74 EVM.

 Signed-off-by: Vignesh R vigne...@ti.com
 
 This ignores the feedback from
 http://lists.denx.de/pipermail/u-boot/2014-July/183715.html where we
 need to model the DMA changes on how it's done for mxs_spi.c
 

Sorry.. I didn't look into that before.
mxs_spi uses peripheral DMA to read/write flash. But ti_qspi can use DMA
to read from flash in mmap mode only. In current u-boot, defining
CONFIG_TI_SPI_MMAP will make memory map address available
(spi_flash-memory_map) to sf layer and spi_flash_cmd_read_ops() (in
sf_ops.c) directly calls memcpy() to read data from flash into buffer.
There is no spi_xfer() call to the ti_qspi driver at all.

In order to implement mxs_spi like approach for ti_qspi.c, I can delete
mmap handling in sf_ops.c( I don't think any other spi driver uses this
part of code), so that spi_xfer() is always called. And then, in
spi_xfer() implementation of ti_qspi, I can do DMA transfer similar to
mxs_spi.c. Is this approach ok?

And are you ok with patch 1 and 2 of this series?

Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 09/11] dma: ti-edma3: Add BIT(x) macro definition

[R, Vignesh]

On 7/3/2015 7:27 PM, Andy Pont wrote:
 Vignesh wrote...
 
 [snip]
 
 +#define BIT(x)  (1  (x))
 +
 
 Is this not something that would be better in a global header file somewhere 
 rather than it starting a trend of a per-driver, per-arch, etc. definitions?

I agree there are few per arch defintions.
How about adding it to include/linux/bitops.h as in linux kernel?

Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot