qemu: aspeed_smc: Calculate checksum on normal DMA

Message ID 20181112142231.162425-1-bluecmd@google.com
State New
Headers show
Series
  • qemu: aspeed_smc: Calculate checksum on normal DMA
Related show

Commit Message

Christian Svensson Nov. 12, 2018, 2:22 p.m.
This patch adds the missing checksum calculation on normal DMA transfer.
According to the datasheet this is how the SMC should behave.

Verified on AST1250 that the hardware matches the behaviour.

Signed-off-by: Christian Svensson <bluecmd@google.com>
---
 hw/ssi/aspeed_smc.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Cédric Le Goater Nov. 12, 2018, 2:39 p.m. | #1
Hello Christian,

On 11/12/18 3:22 PM, Christian Svensson wrote:
> This patch adds the missing checksum calculation on normal DMA transfer.
> According to the datasheet this is how the SMC should behave.

The datasheet is not very clear on the topic. 

Bit 2 of FMC80 says :

	CheckSum Calculation Only
	0: Normal DMA operation
	1: Checksum accumulation only
	This bit is valid only when the DMA direction = 0
 
and one could understand that under Normal DMA operation, there is 
no Checksum accumulation. 

Anyhow, I am ok for adding the change. 

> Verified on AST1250 that the hardware matches the behaviour.

Did you check under u-boot, did you add DMA support to the Linux driver ? 
do you have a firmware doing so you could share ? I would like to check
for the ast2500.

Thanks,

C. 


> Signed-off-by: Christian Svensson <bluecmd@google.com>
> ---
>  hw/ssi/aspeed_smc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 55ceffe100..0dc22e44cf 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -871,6 +871,7 @@ static void aspeed_smc_dma_rw(AspeedSMCState *s)
>          s->regs[R_DMA_FLASH_ADDR] += 4;
>          s->regs[R_DMA_DRAM_ADDR] += 4;
>          s->regs[R_DMA_LEN] -= 4;
> +        s->regs[R_DMA_CHECKSUM] += data;
>      }
>  }
>  
>
Christian Svensson Nov. 12, 2018, 2:52 p.m. | #2
Hi,

Thanks for the review.

On Mon, Nov 12, 2018 at 3:39 PM Cédric Le Goater <clg@kaod.org> wrote:
> The datasheet is not very clear on the topic.

I won't argue there.

> and one could understand that under Normal DMA operation, there is
> no Checksum accumulation.

Indeed, that's what I thought as well. Then my question to myself was that
since that means that you can either 1) transfer data or 2) calculate the
checksum it seems like a pretty useless feature.

If you look at 8.6.2 DMA CheckSum Calculation Mode (for ast2400 at least)
you
see the sentence:
> 5. FMC80[2] is set when only checksum calculation is necessary, no data
movement

Which I take to read that you can execute the whole section and you will
get a checksum + data transfer, but if you set FMC80[2] then you get the
checksum, but no data movement.

> do you have a firmware doing so you could share ? I would like to check
> for the ast2500.

I have my own firmware here:
https://github.com/u-root/u-bmc/blob/07979a696df4351ec6dbb424ca80a5ebcc2eda1d/platform/ast2400/boot/main.S

Cheers,
<div dir="ltr"><div dir="ltr"><div dir="ltr">Hi,</div><div dir="ltr"><br></div><div dir="ltr">Thanks for the review.</div><div dir="ltr"><br>On Mon, Nov 12, 2018 at 3:39 PM Cédric Le Goater &lt;<a href="mailto:clg@kaod.org">clg@kaod.org</a>&gt; wrote:<br>&gt; The datasheet is not very clear on the topic.<br><br>I won&#39;t argue there.<br><br>&gt; and one could understand that under Normal DMA operation, there is<br>&gt; no Checksum accumulation.<div><br></div><div>Indeed, that&#39;s what I thought as well. Then my question to myself was that</div><div>since that means that you can either 1) transfer data or 2) calculate the</div><div>checksum it seems like a pretty useless feature.</div><div><br></div><div>If you look at 8.6.2 DMA CheckSum Calculation Mode (for ast2400 at least) you</div><div>see the sentence:</div><div>&gt; 5. FMC80[2] is set when only checksum calculation is necessary, no data movement</div><div><br></div><div>Which I take to read that you can execute the whole section and you will</div><div>get a checksum + data transfer, but if you set FMC80[2] then you get the</div><div>checksum, but no data movement.</div><div><br>&gt; do you have a firmware doing so you could share ? I would like to check<br>&gt; for the ast2500.</div><div><br></div><div>I have my own firmware here:</div><div><a href="https://github.com/u-root/u-bmc/blob/07979a696df4351ec6dbb424ca80a5ebcc2eda1d/platform/ast2400/boot/main.S">https://github.com/u-root/u-bmc/blob/07979a696df4351ec6dbb424ca80a5ebcc2eda1d/platform/ast2400/boot/main.S</a><br></div><div><br></div><div>Cheers,</div></div></div></div>
Christian Svensson Jan. 11, 2019, 12:30 p.m. | #3
Ping. What's left to be done here?


On Mon, Nov 12, 2018 at 3:52 PM Christian Svensson <blue@cmd.nu> wrote:

> Hi,
>
> Thanks for the review.
>
> On Mon, Nov 12, 2018 at 3:39 PM Cédric Le Goater <clg@kaod.org> wrote:
> > The datasheet is not very clear on the topic.
>
> I won't argue there.
>
> > and one could understand that under Normal DMA operation, there is
> > no Checksum accumulation.
>
> Indeed, that's what I thought as well. Then my question to myself was that
> since that means that you can either 1) transfer data or 2) calculate the
> checksum it seems like a pretty useless feature.
>
> If you look at 8.6.2 DMA CheckSum Calculation Mode (for ast2400 at least)
> you
> see the sentence:
> > 5. FMC80[2] is set when only checksum calculation is necessary, no data
> movement
>
> Which I take to read that you can execute the whole section and you will
> get a checksum + data transfer, but if you set FMC80[2] then you get the
> checksum, but no data movement.
>
> > do you have a firmware doing so you could share ? I would like to check
> > for the ast2500.
>
> I have my own firmware here:
>
> https://github.com/u-root/u-bmc/blob/07979a696df4351ec6dbb424ca80a5ebcc2eda1d/platform/ast2400/boot/main.S
>
> Cheers,
>
<div dir="ltr">Ping. What&#39;s left to be done here?<div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 12, 2018 at 3:52 PM Christian Svensson &lt;<a href="mailto:blue@cmd.nu" target="_blank">blue@cmd.nu</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr">Hi,</div><div dir="ltr"><br></div><div dir="ltr">Thanks for the review.</div><div dir="ltr"><br>On Mon, Nov 12, 2018 at 3:39 PM Cédric Le Goater &lt;<a href="mailto:clg@kaod.org" target="_blank">clg@kaod.org</a>&gt; wrote:<br>&gt; The datasheet is not very clear on the topic.<br><br>I won&#39;t argue there.<br><br>&gt; and one could understand that under Normal DMA operation, there is<br>&gt; no Checksum accumulation.<div><br></div><div>Indeed, that&#39;s what I thought as well. Then my question to myself was that</div><div>since that means that you can either 1) transfer data or 2) calculate the</div><div>checksum it seems like a pretty useless feature.</div><div><br></div><div>If you look at 8.6.2 DMA CheckSum Calculation Mode (for ast2400 at least) you</div><div>see the sentence:</div><div>&gt; 5. FMC80[2] is set when only checksum calculation is necessary, no data movement</div><div><br></div><div>Which I take to read that you can execute the whole section and you will</div><div>get a checksum + data transfer, but if you set FMC80[2] then you get the</div><div>checksum, but no data movement.</div><div><br>&gt; do you have a firmware doing so you could share ? I would like to check<br>&gt; for the ast2500.</div><div><br></div><div>I have my own firmware here:</div><div><a href="https://github.com/u-root/u-bmc/blob/07979a696df4351ec6dbb424ca80a5ebcc2eda1d/platform/ast2400/boot/main.S" target="_blank">https://github.com/u-root/u-bmc/blob/07979a696df4351ec6dbb424ca80a5ebcc2eda1d/platform/ast2400/boot/main.S</a><br></div><div><br></div><div>Cheers,</div></div></div></div>
</blockquote></div>
Cédric Le Goater Jan. 11, 2019, 12:55 p.m. | #4
On 1/11/19 1:30 PM, Christian Svensson wrote:
> Ping. What's left to be done here?

Nothing. I have merged the update in my patch.

Thanks,

C.

> On Mon, Nov 12, 2018 at 3:52 PM Christian Svensson <blue@cmd.nu <mailto:blue@cmd.nu>> wrote:
> 
>     Hi,
> 
>     Thanks for the review.
> 
>     On Mon, Nov 12, 2018 at 3:39 PM Cédric Le Goater <clg@kaod.org <mailto:clg@kaod.org>> wrote:
>     > The datasheet is not very clear on the topic.
> 
>     I won't argue there.
> 
>     > and one could understand that under Normal DMA operation, there is
>     > no Checksum accumulation.
> 
>     Indeed, that's what I thought as well. Then my question to myself was that
>     since that means that you can either 1) transfer data or 2) calculate the
>     checksum it seems like a pretty useless feature.
> 
>     If you look at 8.6.2 DMA CheckSum Calculation Mode (for ast2400 at least) you
>     see the sentence:
>     > 5. FMC80[2] is set when only checksum calculation is necessary, no data movement
> 
>     Which I take to read that you can execute the whole section and you will
>     get a checksum + data transfer, but if you set FMC80[2] then you get the
>     checksum, but no data movement.
> 
>     > do you have a firmware doing so you could share ? I would like to check
>     > for the ast2500.
> 
>     I have my own firmware here:
>     https://github.com/u-root/u-bmc/blob/07979a696df4351ec6dbb424ca80a5ebcc2eda1d/platform/ast2400/boot/main.S
> 
>     Cheers,
>

Patch

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 55ceffe100..0dc22e44cf 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -871,6 +871,7 @@  static void aspeed_smc_dma_rw(AspeedSMCState *s)
         s->regs[R_DMA_FLASH_ADDR] += 4;
         s->regs[R_DMA_DRAM_ADDR] += 4;
         s->regs[R_DMA_LEN] -= 4;
+        s->regs[R_DMA_CHECKSUM] += data;
     }
 }