Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Michal Simek]

On 10/13/2015 07:33 AM, Mike Looijmans wrote:
> On 12-10-15 14:38, Michal Simek wrote:
>> Hi Mike,
>>
>> On 10/12/2015 02:22 PM, Mike Looijmans wrote:
>>> On 12-10-15 13:16, Michal Simek wrote:

>>> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>>> +const char *buf, size_t count)
>>> +{
>>> + struct zynq_fpga_priv *priv;
>>> + int err;
>>> + char *kbuf;
>>> + size_t i, in_count;
>>> + dma_addr_t dma_addr;
>>> + u32 transfer_length = 0;
>>> + bool endian_swap = false;
>>> +
>>> + in_count = count;
>>> + priv = mgr->priv;
>>> +
>>> + kbuf = dma_alloc_coherent(priv->dev, count, _addr,
>>> GFP_KERNEL);
>>> + if (!kbuf)
>>> + return -ENOMEM;
>>> +
>>> + memcpy(kbuf, buf, count);
>>> +
>>> + /* look for the sync word */
>>> + for (i = 0; i < count - 4; i++) {
>>> + if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
>>> + dev_dbg(priv->dev, "Found normal sync
>>> word\n");
>>> + endian_swap = false;
>>> + break;
>>> + }

 This is bin format

>>> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
>>> + dev_dbg(priv->dev, "Found swapped sync
>>> word\n");
>>> + endian_swap = true;
>>> + break;
>>> + }

 This is bit format from header

>>> + }
>>
>> How much control do we have over mandating the format of firmware at
>> this point?  It'd be swell if we could just mandate a specific
>> endianness, and leave this munging to usermode.
>
> That's a good question. Personally I do only care about one of both,
> but that's just because I get to decide for my targets...
> Opinions from the Xilinx guys?

 Don't know full history about this but in past bitstream in BIT format
 was used. Which is header (partially decoding in u-boot for example)
 with data.
 On zynq native format is BIN which is format without header and data is
 swapped.
 This code just detects which format is used. If BIT, header is skipped
 and data is swapped to BIN format.

 Back to origin question if this is something what can be handled from
 user space. And answer is - yes it can be handled there.
 But based on my experience it is very useful to be able to handle BIT
 because it is built by tools by default.
 Also with BIN format you are loosing record what this data bitstream
 targets. Header in BIT gives you at least some ideas.
>>>
>>> People should stop using "cat" to program the FPGA and use a userspace
>>> tool instead. I've already released such tools under GPL, so anyone can
>>> pick up on it and extend it as required.
>>
>> Link?
> 
> https://github.com/topic-embedded-products/dyplo-utils/blob/master/dyploprogrammer.cpp
> 
> https://github.com/topic-embedded-products/libdyplo/blob/master/hardware.cpp#L261
> 
> 
> Will need some work to combine into a single tool though.
> 
>> This is fpga manager based driver where "cat" won't be used.
> 
> Haven't looked into it yet, but I guess at some point one will have to
> stream some data from userspace into the device, right?

Currently loading bitstream via firmware interface is used.

> 
>>> The header for the "bit" format is completely ignored (you can't even
>>> use it to determine if the bitstream is compatible with the current
>>> device) so there's no point in carrying it around.
>>
>> up2you what you want to do with it. If you work with different boards
>> with different FPGAs it is at least helpful to know if X.bit target this
>> or that board. Unfortunately I am not aware about any public document
>> which describe what there is written.
>>
>>> On the zynq, doing
>>> the "swap" in userspace was measurably faster than having the driver
>>> handle it, and that was even without using NEON instructions for byte
>>> swapping.
>>>
>>> I admit that being able to do "cat static.bit > /dev/xdevcfg" has had
>>> its uses. But it's not something that belongs in mainline Linux.
>>
>> It is about comfort but I have really not a problem that driver will
>> handle just BIN format.
>>
>>> Probably one of the key reasons that the "bit" format is still popular
>>> is that getting the Vivado tools to create a proper "bin" that will
>>> actually work on the Zynq is about as easy as nailing jelly to a tree.
>>> We've been using a simple Python script to do the bit->bin conversion
>>> for that reason.
>>
>> In vivado it is one tcl cmd. But truth is that I don't really get why
>> BIN is not generated by default.
> 
> If I recall correctly, Vivado strips the "bit" header but doesn't swap
> the bytes, so the resulting bin file won't work.


I have built bitstream from vivado 

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Michal Simek]

On 10/13/2015 07:33 AM, Mike Looijmans wrote:
> On 12-10-15 14:38, Michal Simek wrote:
>> Hi Mike,
>>
>> On 10/12/2015 02:22 PM, Mike Looijmans wrote:
>>> On 12-10-15 13:16, Michal Simek wrote:

>>> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>>> +const char *buf, size_t count)
>>> +{
>>> + struct zynq_fpga_priv *priv;
>>> + int err;
>>> + char *kbuf;
>>> + size_t i, in_count;
>>> + dma_addr_t dma_addr;
>>> + u32 transfer_length = 0;
>>> + bool endian_swap = false;
>>> +
>>> + in_count = count;
>>> + priv = mgr->priv;
>>> +
>>> + kbuf = dma_alloc_coherent(priv->dev, count, _addr,
>>> GFP_KERNEL);
>>> + if (!kbuf)
>>> + return -ENOMEM;
>>> +
>>> + memcpy(kbuf, buf, count);
>>> +
>>> + /* look for the sync word */
>>> + for (i = 0; i < count - 4; i++) {
>>> + if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
>>> + dev_dbg(priv->dev, "Found normal sync
>>> word\n");
>>> + endian_swap = false;
>>> + break;
>>> + }

 This is bin format

>>> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
>>> + dev_dbg(priv->dev, "Found swapped sync
>>> word\n");
>>> + endian_swap = true;
>>> + break;
>>> + }

 This is bit format from header

>>> + }
>>
>> How much control do we have over mandating the format of firmware at
>> this point?  It'd be swell if we could just mandate a specific
>> endianness, and leave this munging to usermode.
>
> That's a good question. Personally I do only care about one of both,
> but that's just because I get to decide for my targets...
> Opinions from the Xilinx guys?

 Don't know full history about this but in past bitstream in BIT format
 was used. Which is header (partially decoding in u-boot for example)
 with data.
 On zynq native format is BIN which is format without header and data is
 swapped.
 This code just detects which format is used. If BIT, header is skipped
 and data is swapped to BIN format.

 Back to origin question if this is something what can be handled from
 user space. And answer is - yes it can be handled there.
 But based on my experience it is very useful to be able to handle BIT
 because it is built by tools by default.
 Also with BIN format you are loosing record what this data bitstream
 targets. Header in BIT gives you at least some ideas.
>>>
>>> People should stop using "cat" to program the FPGA and use a userspace
>>> tool instead. I've already released such tools under GPL, so anyone can
>>> pick up on it and extend it as required.
>>
>> Link?
> 
> https://github.com/topic-embedded-products/dyplo-utils/blob/master/dyploprogrammer.cpp
> 
> https://github.com/topic-embedded-products/libdyplo/blob/master/hardware.cpp#L261
> 
> 
> Will need some work to combine into a single tool though.
> 
>> This is fpga manager based driver where "cat" won't be used.
> 
> Haven't looked into it yet, but I guess at some point one will have to
> stream some data from userspace into the device, right?

Currently loading bitstream via firmware interface is used.

> 
>>> The header for the "bit" format is completely ignored (you can't even
>>> use it to determine if the bitstream is compatible with the current
>>> device) so there's no point in carrying it around.
>>
>> up2you what you want to do with it. If you work with different boards
>> with different FPGAs it is at least helpful to know if X.bit target this
>> or that board. Unfortunately I am not aware about any public document
>> which describe what there is written.
>>
>>> On the zynq, doing
>>> the "swap" in userspace was measurably faster than having the driver
>>> handle it, and that was even without using NEON instructions for byte
>>> swapping.
>>>
>>> I admit that being able to do "cat static.bit > /dev/xdevcfg" has had
>>> its uses. But it's not something that belongs in mainline Linux.
>>
>> It is about comfort but I have really not a problem that driver will
>> handle just BIN format.
>>
>>> Probably one of the key reasons that the "bit" format is still popular
>>> is that getting the Vivado tools to create a proper "bin" that will
>>> actually work on the Zynq is about as easy as nailing jelly to a tree.
>>> We've been using a simple Python script to do the bit->bin conversion
>>> for that reason.
>>
>> In vivado it is one tcl cmd. But truth is that I don't really get why
>> BIN is not generated by default.
> 
> If I recall correctly, Vivado strips the "bit" header but doesn't swap
> the bytes, so the resulting bin file won't work.


I have built bitstream from vivado 

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Mike Looijmans]

On 12-10-15 14:38, Michal Simek wrote:

Hi Mike,

On 10/12/2015 02:22 PM, Mike Looijmans wrote:

On 12-10-15 13:16, Michal Simek wrote:



+static int zynq_fpga_ops_write(struct fpga_manager *mgr,
+const char *buf, size_t count)
+{
+ struct zynq_fpga_priv *priv;
+ int err;
+ char *kbuf;
+ size_t i, in_count;
+ dma_addr_t dma_addr;
+ u32 transfer_length = 0;
+ bool endian_swap = false;
+
+ in_count = count;
+ priv = mgr->priv;
+
+ kbuf = dma_alloc_coherent(priv->dev, count, _addr,
GFP_KERNEL);
+ if (!kbuf)
+ return -ENOMEM;
+
+ memcpy(kbuf, buf, count);
+
+ /* look for the sync word */
+ for (i = 0; i < count - 4; i++) {
+ if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
+ dev_dbg(priv->dev, "Found normal sync word\n");
+ endian_swap = false;
+ break;
+ }


This is bin format


+ if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
+ dev_dbg(priv->dev, "Found swapped sync word\n");
+ endian_swap = true;
+ break;
+ }


This is bit format from header


+ }


How much control do we have over mandating the format of firmware at
this point?  It'd be swell if we could just mandate a specific
endianness, and leave this munging to usermode.


That's a good question. Personally I do only care about one of both,
but that's just because I get to decide for my targets...
Opinions from the Xilinx guys?


Don't know full history about this but in past bitstream in BIT format
was used. Which is header (partially decoding in u-boot for example)
with data.
On zynq native format is BIN which is format without header and data is
swapped.
This code just detects which format is used. If BIT, header is skipped
and data is swapped to BIN format.

Back to origin question if this is something what can be handled from
user space. And answer is - yes it can be handled there.
But based on my experience it is very useful to be able to handle BIT
because it is built by tools by default.
Also with BIN format you are loosing record what this data bitstream
targets. Header in BIT gives you at least some ideas.


People should stop using "cat" to program the FPGA and use a userspace
tool instead. I've already released such tools under GPL, so anyone can
pick up on it and extend it as required.


Link?


https://github.com/topic-embedded-products/dyplo-utils/blob/master/dyploprogrammer.cpp
https://github.com/topic-embedded-products/libdyplo/blob/master/hardware.cpp#L261

Will need some work to combine into a single tool though.


This is fpga manager based driver where "cat" won't be used.


Haven't looked into it yet, but I guess at some point one will have to stream 
some data from userspace into the device, right?



The header for the "bit" format is completely ignored (you can't even
use it to determine if the bitstream is compatible with the current
device) so there's no point in carrying it around.


up2you what you want to do with it. If you work with different boards
with different FPGAs it is at least helpful to know if X.bit target this
or that board. Unfortunately I am not aware about any public document
which describe what there is written.


On the zynq, doing
the "swap" in userspace was measurably faster than having the driver
handle it, and that was even without using NEON instructions for byte
swapping.

I admit that being able to do "cat static.bit > /dev/xdevcfg" has had
its uses. But it's not something that belongs in mainline Linux.


It is about comfort but I have really not a problem that driver will
handle just BIN format.


Probably one of the key reasons that the "bit" format is still popular
is that getting the Vivado tools to create a proper "bin" that will
actually work on the Zynq is about as easy as nailing jelly to a tree.
We've been using a simple Python script to do the bit->bin conversion
for that reason.


In vivado it is one tcl cmd. But truth is that I don't really get why
BIN is not generated by default.


If I recall correctly, Vivado strips the "bit" header but doesn't swap the 
bytes, so the resulting bin file won't work.




Using the "bin" format in the driver keeps it simple and singular.
Userspace tools can add whatever wrappers and headers they feel
appropriate to it, these checks don't belong in the driver since they
will be application specific. For example, some users would want to
verify that a partial bitstream actually matches the static part that's
currently in the FPGA.


agree.

Thanks,
Michal






Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijm...@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this 

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Mike Looijmans]

On 12-10-15 13:16, Michal Simek wrote:



+static int zynq_fpga_ops_write(struct fpga_manager *mgr,
+const char *buf, size_t count)
+{
+ struct zynq_fpga_priv *priv;
+ int err;
+ char *kbuf;
+ size_t i, in_count;
+ dma_addr_t dma_addr;
+ u32 transfer_length = 0;
+ bool endian_swap = false;
+
+ in_count = count;
+ priv = mgr->priv;
+
+ kbuf = dma_alloc_coherent(priv->dev, count, _addr, GFP_KERNEL);
+ if (!kbuf)
+ return -ENOMEM;
+
+ memcpy(kbuf, buf, count);
+
+ /* look for the sync word */
+ for (i = 0; i < count - 4; i++) {
+ if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
+ dev_dbg(priv->dev, "Found normal sync word\n");
+ endian_swap = false;
+ break;
+ }


This is bin format


+ if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
+ dev_dbg(priv->dev, "Found swapped sync word\n");
+ endian_swap = true;
+ break;
+ }


This is bit format from header


+ }


How much control do we have over mandating the format of firmware at
this point?  It'd be swell if we could just mandate a specific
endianness, and leave this munging to usermode.


That's a good question. Personally I do only care about one of both,
but that's just because I get to decide for my targets...
Opinions from the Xilinx guys?


Don't know full history about this but in past bitstream in BIT format
was used. Which is header (partially decoding in u-boot for example)
with data.
On zynq native format is BIN which is format without header and data is
swapped.
This code just detects which format is used. If BIT, header is skipped
and data is swapped to BIN format.

Back to origin question if this is something what can be handled from
user space. And answer is - yes it can be handled there.
But based on my experience it is very useful to be able to handle BIT
because it is built by tools by default.
Also with BIN format you are loosing record what this data bitstream
targets. Header in BIT gives you at least some ideas.


People should stop using "cat" to program the FPGA and use a userspace tool 
instead. I've already released such tools under GPL, so anyone can pick up on 
it and extend it as required.


The header for the "bit" format is completely ignored (you can't even use it 
to determine if the bitstream is compatible with the current device) so 
there's no point in carrying it around. On the zynq, doing the "swap" in 
userspace was measurably faster than having the driver handle it, and that was 
even without using NEON instructions for byte swapping.


I admit that being able to do "cat static.bit > /dev/xdevcfg" has had its 
uses. But it's not something that belongs in mainline Linux.


Probably one of the key reasons that the "bit" format is still popular is that 
getting the Vivado tools to create a proper "bin" that will actually work on 
the Zynq is about as easy as nailing jelly to a tree. We've been using a 
simple Python script to do the bit->bin conversion for that reason.


Using the "bin" format in the driver keeps it simple and singular. Userspace 
tools can add whatever wrappers and headers they feel appropriate to it, these 
checks don't belong in the driver since they will be application specific. For 
example, some users would want to verify that a partial bitstream actually 
matches the static part that's currently in the FPGA.


Mike.


Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijm...@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Michal Simek]

Hi Mike,

On 10/12/2015 02:22 PM, Mike Looijmans wrote:
> On 12-10-15 13:16, Michal Simek wrote:
>>
> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
> +const char *buf, size_t count)
> +{
> + struct zynq_fpga_priv *priv;
> + int err;
> + char *kbuf;
> + size_t i, in_count;
> + dma_addr_t dma_addr;
> + u32 transfer_length = 0;
> + bool endian_swap = false;
> +
> + in_count = count;
> + priv = mgr->priv;
> +
> + kbuf = dma_alloc_coherent(priv->dev, count, _addr,
> GFP_KERNEL);
> + if (!kbuf)
> + return -ENOMEM;
> +
> + memcpy(kbuf, buf, count);
> +
> + /* look for the sync word */
> + for (i = 0; i < count - 4; i++) {
> + if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
> + dev_dbg(priv->dev, "Found normal sync word\n");
> + endian_swap = false;
> + break;
> + }
>>
>> This is bin format
>>
> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
> + dev_dbg(priv->dev, "Found swapped sync word\n");
> + endian_swap = true;
> + break;
> + }
>>
>> This is bit format from header
>>
> + }

 How much control do we have over mandating the format of firmware at
 this point?  It'd be swell if we could just mandate a specific
 endianness, and leave this munging to usermode.
>>>
>>> That's a good question. Personally I do only care about one of both,
>>> but that's just because I get to decide for my targets...
>>> Opinions from the Xilinx guys?
>>
>> Don't know full history about this but in past bitstream in BIT format
>> was used. Which is header (partially decoding in u-boot for example)
>> with data.
>> On zynq native format is BIN which is format without header and data is
>> swapped.
>> This code just detects which format is used. If BIT, header is skipped
>> and data is swapped to BIN format.
>>
>> Back to origin question if this is something what can be handled from
>> user space. And answer is - yes it can be handled there.
>> But based on my experience it is very useful to be able to handle BIT
>> because it is built by tools by default.
>> Also with BIN format you are loosing record what this data bitstream
>> targets. Header in BIT gives you at least some ideas.
> 
> People should stop using "cat" to program the FPGA and use a userspace
> tool instead. I've already released such tools under GPL, so anyone can
> pick up on it and extend it as required.

Link?

This is fpga manager based driver where "cat" won't be used.

> 
> The header for the "bit" format is completely ignored (you can't even
> use it to determine if the bitstream is compatible with the current
> device) so there's no point in carrying it around. 

up2you what you want to do with it. If you work with different boards
with different FPGAs it is at least helpful to know if X.bit target this
or that board. Unfortunately I am not aware about any public document
which describe what there is written.

> On the zynq, doing
> the "swap" in userspace was measurably faster than having the driver
> handle it, and that was even without using NEON instructions for byte
> swapping.
> 
> I admit that being able to do "cat static.bit > /dev/xdevcfg" has had
> its uses. But it's not something that belongs in mainline Linux.

It is about comfort but I have really not a problem that driver will
handle just BIN format.

> Probably one of the key reasons that the "bit" format is still popular
> is that getting the Vivado tools to create a proper "bin" that will
> actually work on the Zynq is about as easy as nailing jelly to a tree.
> We've been using a simple Python script to do the bit->bin conversion
> for that reason.

In vivado it is one tcl cmd. But truth is that I don't really get why
BIN is not generated by default.

> Using the "bin" format in the driver keeps it simple and singular.
> Userspace tools can add whatever wrappers and headers they feel
> appropriate to it, these checks don't belong in the driver since they
> will be application specific. For example, some users would want to
> verify that a partial bitstream actually matches the static part that's
> currently in the FPGA.

agree.

Thanks,
Michal


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Michal Simek]

>>> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>>> +const char *buf, size_t count)
>>> +{
>>> + struct zynq_fpga_priv *priv;
>>> + int err;
>>> + char *kbuf;
>>> + size_t i, in_count;
>>> + dma_addr_t dma_addr;
>>> + u32 transfer_length = 0;
>>> + bool endian_swap = false;
>>> +
>>> + in_count = count;
>>> + priv = mgr->priv;
>>> +
>>> + kbuf = dma_alloc_coherent(priv->dev, count, _addr, GFP_KERNEL);
>>> + if (!kbuf)
>>> + return -ENOMEM;
>>> +
>>> + memcpy(kbuf, buf, count);
>>> +
>>> + /* look for the sync word */
>>> + for (i = 0; i < count - 4; i++) {
>>> + if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
>>> + dev_dbg(priv->dev, "Found normal sync word\n");
>>> + endian_swap = false;
>>> + break;
>>> + }

This is bin format

>>> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
>>> + dev_dbg(priv->dev, "Found swapped sync word\n");
>>> + endian_swap = true;
>>> + break;
>>> + }

This is bit format from header

>>> + }
>>
>> How much control do we have over mandating the format of firmware at
>> this point?  It'd be swell if we could just mandate a specific
>> endianness, and leave this munging to usermode.
> 
> That's a good question. Personally I do only care about one of both,
> but that's just because I get to decide for my targets...
> Opinions from the Xilinx guys?

Don't know full history about this but in past bitstream in BIT format
was used. Which is header (partially decoding in u-boot for example)
with data.
On zynq native format is BIN which is format without header and data is
swapped.
This code just detects which format is used. If BIT, header is skipped
and data is swapped to BIN format.

Back to origin question if this is something what can be handled from
user space. And answer is - yes it can be handled there.
But based on my experience it is very useful to be able to handle BIT
because it is built by tools by default.
Also with BIN format you are loosing record what this data bitstream
targets. Header in BIT gives you at least some ideas.

Thanks,
Michal


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Michal Simek]

Hi Mike,

On 10/12/2015 02:22 PM, Mike Looijmans wrote:
> On 12-10-15 13:16, Michal Simek wrote:
>>
> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
> +const char *buf, size_t count)
> +{
> + struct zynq_fpga_priv *priv;
> + int err;
> + char *kbuf;
> + size_t i, in_count;
> + dma_addr_t dma_addr;
> + u32 transfer_length = 0;
> + bool endian_swap = false;
> +
> + in_count = count;
> + priv = mgr->priv;
> +
> + kbuf = dma_alloc_coherent(priv->dev, count, _addr,
> GFP_KERNEL);
> + if (!kbuf)
> + return -ENOMEM;
> +
> + memcpy(kbuf, buf, count);
> +
> + /* look for the sync word */
> + for (i = 0; i < count - 4; i++) {
> + if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
> + dev_dbg(priv->dev, "Found normal sync word\n");
> + endian_swap = false;
> + break;
> + }
>>
>> This is bin format
>>
> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
> + dev_dbg(priv->dev, "Found swapped sync word\n");
> + endian_swap = true;
> + break;
> + }
>>
>> This is bit format from header
>>
> + }

 How much control do we have over mandating the format of firmware at
 this point?  It'd be swell if we could just mandate a specific
 endianness, and leave this munging to usermode.
>>>
>>> That's a good question. Personally I do only care about one of both,
>>> but that's just because I get to decide for my targets...
>>> Opinions from the Xilinx guys?
>>
>> Don't know full history about this but in past bitstream in BIT format
>> was used. Which is header (partially decoding in u-boot for example)
>> with data.
>> On zynq native format is BIN which is format without header and data is
>> swapped.
>> This code just detects which format is used. If BIT, header is skipped
>> and data is swapped to BIN format.
>>
>> Back to origin question if this is something what can be handled from
>> user space. And answer is - yes it can be handled there.
>> But based on my experience it is very useful to be able to handle BIT
>> because it is built by tools by default.
>> Also with BIN format you are loosing record what this data bitstream
>> targets. Header in BIT gives you at least some ideas.
> 
> People should stop using "cat" to program the FPGA and use a userspace
> tool instead. I've already released such tools under GPL, so anyone can
> pick up on it and extend it as required.

Link?

This is fpga manager based driver where "cat" won't be used.

> 
> The header for the "bit" format is completely ignored (you can't even
> use it to determine if the bitstream is compatible with the current
> device) so there's no point in carrying it around. 

up2you what you want to do with it. If you work with different boards
with different FPGAs it is at least helpful to know if X.bit target this
or that board. Unfortunately I am not aware about any public document
which describe what there is written.

> On the zynq, doing
> the "swap" in userspace was measurably faster than having the driver
> handle it, and that was even without using NEON instructions for byte
> swapping.
> 
> I admit that being able to do "cat static.bit > /dev/xdevcfg" has had
> its uses. But it's not something that belongs in mainline Linux.

It is about comfort but I have really not a problem that driver will
handle just BIN format.

> Probably one of the key reasons that the "bit" format is still popular
> is that getting the Vivado tools to create a proper "bin" that will
> actually work on the Zynq is about as easy as nailing jelly to a tree.
> We've been using a simple Python script to do the bit->bin conversion
> for that reason.

In vivado it is one tcl cmd. But truth is that I don't really get why
BIN is not generated by default.

> Using the "bin" format in the driver keeps it simple and singular.
> Userspace tools can add whatever wrappers and headers they feel
> appropriate to it, these checks don't belong in the driver since they
> will be application specific. For example, some users would want to
> verify that a partial bitstream actually matches the static part that's
> currently in the FPGA.

agree.

Thanks,
Michal


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Michal Simek]

>>> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>>> +const char *buf, size_t count)
>>> +{
>>> + struct zynq_fpga_priv *priv;
>>> + int err;
>>> + char *kbuf;
>>> + size_t i, in_count;
>>> + dma_addr_t dma_addr;
>>> + u32 transfer_length = 0;
>>> + bool endian_swap = false;
>>> +
>>> + in_count = count;
>>> + priv = mgr->priv;
>>> +
>>> + kbuf = dma_alloc_coherent(priv->dev, count, _addr, GFP_KERNEL);
>>> + if (!kbuf)
>>> + return -ENOMEM;
>>> +
>>> + memcpy(kbuf, buf, count);
>>> +
>>> + /* look for the sync word */
>>> + for (i = 0; i < count - 4; i++) {
>>> + if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
>>> + dev_dbg(priv->dev, "Found normal sync word\n");
>>> + endian_swap = false;
>>> + break;
>>> + }

This is bin format

>>> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
>>> + dev_dbg(priv->dev, "Found swapped sync word\n");
>>> + endian_swap = true;
>>> + break;
>>> + }

This is bit format from header

>>> + }
>>
>> How much control do we have over mandating the format of firmware at
>> this point?  It'd be swell if we could just mandate a specific
>> endianness, and leave this munging to usermode.
> 
> That's a good question. Personally I do only care about one of both,
> but that's just because I get to decide for my targets...
> Opinions from the Xilinx guys?

Don't know full history about this but in past bitstream in BIT format
was used. Which is header (partially decoding in u-boot for example)
with data.
On zynq native format is BIN which is format without header and data is
swapped.
This code just detects which format is used. If BIT, header is skipped
and data is swapped to BIN format.

Back to origin question if this is something what can be handled from
user space. And answer is - yes it can be handled there.
But based on my experience it is very useful to be able to handle BIT
because it is built by tools by default.
Also with BIN format you are loosing record what this data bitstream
targets. Header in BIT gives you at least some ideas.

Thanks,
Michal


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Mike Looijmans]

On 12-10-15 13:16, Michal Simek wrote:



+static int zynq_fpga_ops_write(struct fpga_manager *mgr,
+const char *buf, size_t count)
+{
+ struct zynq_fpga_priv *priv;
+ int err;
+ char *kbuf;
+ size_t i, in_count;
+ dma_addr_t dma_addr;
+ u32 transfer_length = 0;
+ bool endian_swap = false;
+
+ in_count = count;
+ priv = mgr->priv;
+
+ kbuf = dma_alloc_coherent(priv->dev, count, _addr, GFP_KERNEL);
+ if (!kbuf)
+ return -ENOMEM;
+
+ memcpy(kbuf, buf, count);
+
+ /* look for the sync word */
+ for (i = 0; i < count - 4; i++) {
+ if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
+ dev_dbg(priv->dev, "Found normal sync word\n");
+ endian_swap = false;
+ break;
+ }


This is bin format


+ if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
+ dev_dbg(priv->dev, "Found swapped sync word\n");
+ endian_swap = true;
+ break;
+ }


This is bit format from header


+ }


How much control do we have over mandating the format of firmware at
this point?  It'd be swell if we could just mandate a specific
endianness, and leave this munging to usermode.


That's a good question. Personally I do only care about one of both,
but that's just because I get to decide for my targets...
Opinions from the Xilinx guys?


Don't know full history about this but in past bitstream in BIT format
was used. Which is header (partially decoding in u-boot for example)
with data.
On zynq native format is BIN which is format without header and data is
swapped.
This code just detects which format is used. If BIT, header is skipped
and data is swapped to BIN format.

Back to origin question if this is something what can be handled from
user space. And answer is - yes it can be handled there.
But based on my experience it is very useful to be able to handle BIT
because it is built by tools by default.
Also with BIN format you are loosing record what this data bitstream
targets. Header in BIT gives you at least some ideas.


People should stop using "cat" to program the FPGA and use a userspace tool 
instead. I've already released such tools under GPL, so anyone can pick up on 
it and extend it as required.


The header for the "bit" format is completely ignored (you can't even use it 
to determine if the bitstream is compatible with the current device) so 
there's no point in carrying it around. On the zynq, doing the "swap" in 
userspace was measurably faster than having the driver handle it, and that was 
even without using NEON instructions for byte swapping.


I admit that being able to do "cat static.bit > /dev/xdevcfg" has had its 
uses. But it's not something that belongs in mainline Linux.


Probably one of the key reasons that the "bit" format is still popular is that 
getting the Vivado tools to create a proper "bin" that will actually work on 
the Zynq is about as easy as nailing jelly to a tree. We've been using a 
simple Python script to do the bit->bin conversion for that reason.


Using the "bin" format in the driver keeps it simple and singular. Userspace 
tools can add whatever wrappers and headers they feel appropriate to it, these 
checks don't belong in the driver since they will be application specific. For 
example, some users would want to verify that a partial bitstream actually 
matches the static part that's currently in the FPGA.


Mike.


Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijm...@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Mike Looijmans]

On 12-10-15 14:38, Michal Simek wrote:

Hi Mike,

On 10/12/2015 02:22 PM, Mike Looijmans wrote:

On 12-10-15 13:16, Michal Simek wrote:



+static int zynq_fpga_ops_write(struct fpga_manager *mgr,
+const char *buf, size_t count)
+{
+ struct zynq_fpga_priv *priv;
+ int err;
+ char *kbuf;
+ size_t i, in_count;
+ dma_addr_t dma_addr;
+ u32 transfer_length = 0;
+ bool endian_swap = false;
+
+ in_count = count;
+ priv = mgr->priv;
+
+ kbuf = dma_alloc_coherent(priv->dev, count, _addr,
GFP_KERNEL);
+ if (!kbuf)
+ return -ENOMEM;
+
+ memcpy(kbuf, buf, count);
+
+ /* look for the sync word */
+ for (i = 0; i < count - 4; i++) {
+ if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
+ dev_dbg(priv->dev, "Found normal sync word\n");
+ endian_swap = false;
+ break;
+ }


This is bin format


+ if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
+ dev_dbg(priv->dev, "Found swapped sync word\n");
+ endian_swap = true;
+ break;
+ }


This is bit format from header


+ }


How much control do we have over mandating the format of firmware at
this point?  It'd be swell if we could just mandate a specific
endianness, and leave this munging to usermode.


That's a good question. Personally I do only care about one of both,
but that's just because I get to decide for my targets...
Opinions from the Xilinx guys?


Don't know full history about this but in past bitstream in BIT format
was used. Which is header (partially decoding in u-boot for example)
with data.
On zynq native format is BIN which is format without header and data is
swapped.
This code just detects which format is used. If BIT, header is skipped
and data is swapped to BIN format.

Back to origin question if this is something what can be handled from
user space. And answer is - yes it can be handled there.
But based on my experience it is very useful to be able to handle BIT
because it is built by tools by default.
Also with BIN format you are loosing record what this data bitstream
targets. Header in BIT gives you at least some ideas.


People should stop using "cat" to program the FPGA and use a userspace
tool instead. I've already released such tools under GPL, so anyone can
pick up on it and extend it as required.


Link?


https://github.com/topic-embedded-products/dyplo-utils/blob/master/dyploprogrammer.cpp
https://github.com/topic-embedded-products/libdyplo/blob/master/hardware.cpp#L261

Will need some work to combine into a single tool though.


This is fpga manager based driver where "cat" won't be used.


Haven't looked into it yet, but I guess at some point one will have to stream 
some data from userspace into the device, right?



The header for the "bit" format is completely ignored (you can't even
use it to determine if the bitstream is compatible with the current
device) so there's no point in carrying it around.


up2you what you want to do with it. If you work with different boards
with different FPGAs it is at least helpful to know if X.bit target this
or that board. Unfortunately I am not aware about any public document
which describe what there is written.


On the zynq, doing
the "swap" in userspace was measurably faster than having the driver
handle it, and that was even without using NEON instructions for byte
swapping.

I admit that being able to do "cat static.bit > /dev/xdevcfg" has had
its uses. But it's not something that belongs in mainline Linux.


It is about comfort but I have really not a problem that driver will
handle just BIN format.


Probably one of the key reasons that the "bit" format is still popular
is that getting the Vivado tools to create a proper "bin" that will
actually work on the Zynq is about as easy as nailing jelly to a tree.
We've been using a simple Python script to do the bit->bin conversion
for that reason.


In vivado it is one tcl cmd. But truth is that I don't really get why
BIN is not generated by default.


If I recall correctly, Vivado strips the "bit" header but doesn't swap the 
bytes, so the resulting bin file won't work.




Using the "bin" format in the driver keeps it simple and singular.
Userspace tools can add whatever wrappers and headers they feel
appropriate to it, these checks don't belong in the driver since they
will be application specific. For example, some users would want to
verify that a partial bitstream actually matches the static part that's
currently in the FPGA.


agree.

Thanks,
Michal






Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijm...@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this 

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Moritz Fischer]

Hi Alan,

thanks for your feedback!

On Fri, Oct 9, 2015 at 8:09 PM, atull  wrote:
> On Thu, 8 Oct 2015, Moritz Fischer wrote:
>
>> --- /dev/null
>> +++ b/drivers/fpga/zynq-fpga.c
>> @@ -0,0 +1,478 @@
>> +/*
>> + * Copyright (c) 2011-2015 Xilinx Inc.
>> + * Copyright (c) 2015, National Instruments Corp.
>> + *
>> + * FPGA Manager Driver for Xilinx Zynq, heavily based on xdevcfg driver
>> + * in their vendor tree.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>
> Hi Moritz,
>
> That was fast!  I just have a couple of very minor comments...
>
> Please alphabetize the #includes.
>
>> +
>> +/* Offsets into SLCR regmap */
>> +#define SLCR_FPGA_RST_CTRL_OFFSET0x240 /* FPGA Software Reset Control */
>> +#define SLCR_LVL_SHFTR_EN_OFFSET 0x900 /* Level Shifters Enable */
>> +
>> +/* Constant Definitions */
>> +#define CTRL_OFFSET  0x00 /* Control Register */
>> +#define LOCK_OFFSET  0x04 /* Lock Register */
>> +#define INT_STS_OFFSET   0x0c /* Interrupt Status Register */
>> +#define INT_MASK_OFFSET  0x10 /* Interrupt Mask Register */
>> +#define STATUS_OFFSET0x14 /* Status Register */
>> +#define DMA_SRC_ADDR_OFFSET  0x18 /* DMA Source Address Register */
>> +#define DMA_DEST_ADDR_OFFSET 0x1c /* DMA Destination Address Reg */
>> +#define DMA_SRC_LEN_OFFSET   0x20 /* DMA Source Transfer Length */
>> +#define DMA_DEST_LEN_OFFSET  0x24 /* DMA Destination Transfer */
>> +#define UNLOCK_OFFSET0x34 /* Unlock Register */
>> +#define MCTRL_OFFSET 0x80 /* Misc. Control Register */
>
> Please fix up the indenting.

Will do.
>
>> +
>> +/* Control Register Bit definitions */
>> +#define CTRL_PCFG_PROG_B_MASKBIT(30) /* Program signal to reset 
>> FPGA */
>> +#define CTRL_PCAP_PR_MASKBIT(27) /* Enable PCAP for PR */
>> +#define CTRL_PCAP_MODE_MASK  BIT(26) /* Enable PCAP */
>> +
>> +/* Miscellaneous Control Register bit definitions */
>> +#define MCTRL_PCAP_LPBK_MASK BIT(4) /* Internal PCAP loopback */
>> +
>> +/* Status register bit definitions */
>> +#define STATUS_PCFG_INIT_MASKBIT(4) /* FPGA init status */
>> +
>> +/* Interrupt Status/Mask Register Bit definitions */
>> +#define IXR_DMA_DONE_MASKBIT(13) /* DMA command done */
>> +#define IXR_D_P_DONE_MASKBIT(12) /* DMA and PCAP cmd done */
>> +#define IXR_PCFG_DONE_MASK   BIT(2)  /* FPGA programmed */
>> +#define IXR_ERROR_FLAGS_MASK 0x00F0F860
>> +#define IXR_ALL_MASK 0xF8F7F87F
>> +
>> +/* Miscellaneous constant values */
>> +#define DMA_INVALID_ADDRESS  GENMASK(31, 0)  /* Invalid DMA address */
>> +#define UNLOCK_MASK  0x757bdf0d /* Used to unlock the device */
>> +
>> +/* Masks for controlling stuff in SLCR */
>> +#define LVL_SHFTR_DISABLE_ALL_MASK   0x0 /* Disable all Level shifters */
>> +#define LVL_SHFTR_ENABLE_PS_TO_PL0xa /* Enable all Level shifters */
>> +#define LVL_SHFTR_ENABLE_PL_TO_PS0xf /* Enable all Level shifters */
>> +#define FPGA_RST_ALL_MASK0xf /* Enable global resets */
>> +#define FPGA_RST_NONE_MASK   0x0 /* Disable global resets */
>> +
>
>> +static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>> + const char *buf, size_t count)
>> +{
>> + struct zynq_fpga_priv *priv;
>> + u32 ctrl, status;
>> + int err;
>> +
>> + priv = mgr->priv;
>> +
>> + err = clk_enable(priv->clk);
>> + if (err)
>> + return err;
>
> You might not even need to enable/disable the clock if not doing PR.

Yeah, you're probably right.
>
>> +
>> + /* only reset if we're not doing partial reconfig */
>> + if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
>> + /* assert AXI interface resets */
>> + regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
>> +  FPGA_RST_ALL_MASK);
>> +
>> + /* disable level shifters */
>> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
>> +  LVL_SHFTR_DISABLE_ALL_MASK);
>> + /* enable output level shifters */
>> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
>> +  LVL_SHFTR_ENABLE_PS_TO_PL);
>> +
>> + /* create a rising edge on PCFG_INIT. PCFG_INIT follows
>> +  * PCFG_PROG_B, so 

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Moritz Fischer]

Hi Alan,

thanks for your feedback!

On Fri, Oct 9, 2015 at 8:09 PM, atull  wrote:
> On Thu, 8 Oct 2015, Moritz Fischer wrote:
>
>> --- /dev/null
>> +++ b/drivers/fpga/zynq-fpga.c
>> @@ -0,0 +1,478 @@
>> +/*
>> + * Copyright (c) 2011-2015 Xilinx Inc.
>> + * Copyright (c) 2015, National Instruments Corp.
>> + *
>> + * FPGA Manager Driver for Xilinx Zynq, heavily based on xdevcfg driver
>> + * in their vendor tree.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>
> Hi Moritz,
>
> That was fast!  I just have a couple of very minor comments...
>
> Please alphabetize the #includes.
>
>> +
>> +/* Offsets into SLCR regmap */
>> +#define SLCR_FPGA_RST_CTRL_OFFSET0x240 /* FPGA Software Reset Control */
>> +#define SLCR_LVL_SHFTR_EN_OFFSET 0x900 /* Level Shifters Enable */
>> +
>> +/* Constant Definitions */
>> +#define CTRL_OFFSET  0x00 /* Control Register */
>> +#define LOCK_OFFSET  0x04 /* Lock Register */
>> +#define INT_STS_OFFSET   0x0c /* Interrupt Status Register */
>> +#define INT_MASK_OFFSET  0x10 /* Interrupt Mask Register */
>> +#define STATUS_OFFSET0x14 /* Status Register */
>> +#define DMA_SRC_ADDR_OFFSET  0x18 /* DMA Source Address Register */
>> +#define DMA_DEST_ADDR_OFFSET 0x1c /* DMA Destination Address Reg */
>> +#define DMA_SRC_LEN_OFFSET   0x20 /* DMA Source Transfer Length */
>> +#define DMA_DEST_LEN_OFFSET  0x24 /* DMA Destination Transfer */
>> +#define UNLOCK_OFFSET0x34 /* Unlock Register */
>> +#define MCTRL_OFFSET 0x80 /* Misc. Control Register */
>
> Please fix up the indenting.

Will do.
>
>> +
>> +/* Control Register Bit definitions */
>> +#define CTRL_PCFG_PROG_B_MASKBIT(30) /* Program signal to reset 
>> FPGA */
>> +#define CTRL_PCAP_PR_MASKBIT(27) /* Enable PCAP for PR */
>> +#define CTRL_PCAP_MODE_MASK  BIT(26) /* Enable PCAP */
>> +
>> +/* Miscellaneous Control Register bit definitions */
>> +#define MCTRL_PCAP_LPBK_MASK BIT(4) /* Internal PCAP loopback */
>> +
>> +/* Status register bit definitions */
>> +#define STATUS_PCFG_INIT_MASKBIT(4) /* FPGA init status */
>> +
>> +/* Interrupt Status/Mask Register Bit definitions */
>> +#define IXR_DMA_DONE_MASKBIT(13) /* DMA command done */
>> +#define IXR_D_P_DONE_MASKBIT(12) /* DMA and PCAP cmd done */
>> +#define IXR_PCFG_DONE_MASK   BIT(2)  /* FPGA programmed */
>> +#define IXR_ERROR_FLAGS_MASK 0x00F0F860
>> +#define IXR_ALL_MASK 0xF8F7F87F
>> +
>> +/* Miscellaneous constant values */
>> +#define DMA_INVALID_ADDRESS  GENMASK(31, 0)  /* Invalid DMA address */
>> +#define UNLOCK_MASK  0x757bdf0d /* Used to unlock the device */
>> +
>> +/* Masks for controlling stuff in SLCR */
>> +#define LVL_SHFTR_DISABLE_ALL_MASK   0x0 /* Disable all Level shifters */
>> +#define LVL_SHFTR_ENABLE_PS_TO_PL0xa /* Enable all Level shifters */
>> +#define LVL_SHFTR_ENABLE_PL_TO_PS0xf /* Enable all Level shifters */
>> +#define FPGA_RST_ALL_MASK0xf /* Enable global resets */
>> +#define FPGA_RST_NONE_MASK   0x0 /* Disable global resets */
>> +
>
>> +static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>> + const char *buf, size_t count)
>> +{
>> + struct zynq_fpga_priv *priv;
>> + u32 ctrl, status;
>> + int err;
>> +
>> + priv = mgr->priv;
>> +
>> + err = clk_enable(priv->clk);
>> + if (err)
>> + return err;
>
> You might not even need to enable/disable the clock if not doing PR.

Yeah, you're probably right.
>
>> +
>> + /* only reset if we're not doing partial reconfig */
>> + if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
>> + /* assert AXI interface resets */
>> + regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
>> +  FPGA_RST_ALL_MASK);
>> +
>> + /* disable level shifters */
>> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
>> +  LVL_SHFTR_DISABLE_ALL_MASK);
>> + /* enable output level shifters */
>> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
>> +  LVL_SHFTR_ENABLE_PS_TO_PL);
>> +
>> + /* create a rising edge on PCFG_INIT. PCFG_INIT follows
>> + 

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[atull]

On Fri, 9 Oct 2015, Greg KH wrote:

> On Fri, Oct 09, 2015 at 07:09:15PM +0100, atull wrote:
> > On Thu, 8 Oct 2015, Moritz Fischer wrote:
> > 
> > > --- /dev/null
> > > +++ b/drivers/fpga/zynq-fpga.c
> > > @@ -0,0 +1,478 @@
> > > +/*
> > > + * Copyright (c) 2011-2015 Xilinx Inc.
> > > + * Copyright (c) 2015, National Instruments Corp.
> > > + *
> > > + * FPGA Manager Driver for Xilinx Zynq, heavily based on xdevcfg driver
> > > + * in their vendor tree.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; version 2 of the License.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > 
> > Hi Moritz,
> > 
> > That was fast!  I just have a couple of very minor comments...
> > 
> > Please alphabetize the #includes.
> 
> Bah, who cares about that, it's not a requirement at all.
> 
> greg k-h
> 

Nice to know!  I get slammed every time for that!

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[atull]

On Thu, 8 Oct 2015, Moritz Fischer wrote:

> --- /dev/null
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -0,0 +1,478 @@
> +/*
> + * Copyright (c) 2011-2015 Xilinx Inc.
> + * Copyright (c) 2015, National Instruments Corp.
> + *
> + * FPGA Manager Driver for Xilinx Zynq, heavily based on xdevcfg driver
> + * in their vendor tree.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Hi Moritz,

That was fast!  I just have a couple of very minor comments...

Please alphabetize the #includes.

> +
> +/* Offsets into SLCR regmap */
> +#define SLCR_FPGA_RST_CTRL_OFFSET0x240 /* FPGA Software Reset Control */
> +#define SLCR_LVL_SHFTR_EN_OFFSET 0x900 /* Level Shifters Enable */
> +
> +/* Constant Definitions */
> +#define CTRL_OFFSET  0x00 /* Control Register */
> +#define LOCK_OFFSET  0x04 /* Lock Register */
> +#define INT_STS_OFFSET   0x0c /* Interrupt Status Register */
> +#define INT_MASK_OFFSET  0x10 /* Interrupt Mask Register */
> +#define STATUS_OFFSET0x14 /* Status Register */
> +#define DMA_SRC_ADDR_OFFSET  0x18 /* DMA Source Address Register */
> +#define DMA_DEST_ADDR_OFFSET 0x1c /* DMA Destination Address Reg */
> +#define DMA_SRC_LEN_OFFSET   0x20 /* DMA Source Transfer Length */
> +#define DMA_DEST_LEN_OFFSET  0x24 /* DMA Destination Transfer */
> +#define UNLOCK_OFFSET0x34 /* Unlock Register */
> +#define MCTRL_OFFSET 0x80 /* Misc. Control Register */

Please fix up the indenting.

> +
> +/* Control Register Bit definitions */
> +#define CTRL_PCFG_PROG_B_MASKBIT(30) /* Program signal to reset FPGA 
> */
> +#define CTRL_PCAP_PR_MASKBIT(27) /* Enable PCAP for PR */
> +#define CTRL_PCAP_MODE_MASK  BIT(26) /* Enable PCAP */
> +
> +/* Miscellaneous Control Register bit definitions */
> +#define MCTRL_PCAP_LPBK_MASK BIT(4) /* Internal PCAP loopback */
> +
> +/* Status register bit definitions */
> +#define STATUS_PCFG_INIT_MASKBIT(4) /* FPGA init status */
> +
> +/* Interrupt Status/Mask Register Bit definitions */
> +#define IXR_DMA_DONE_MASKBIT(13) /* DMA command done */
> +#define IXR_D_P_DONE_MASKBIT(12) /* DMA and PCAP cmd done */
> +#define IXR_PCFG_DONE_MASK   BIT(2)  /* FPGA programmed */
> +#define IXR_ERROR_FLAGS_MASK 0x00F0F860
> +#define IXR_ALL_MASK 0xF8F7F87F
> +
> +/* Miscellaneous constant values */
> +#define DMA_INVALID_ADDRESS  GENMASK(31, 0)  /* Invalid DMA address */
> +#define UNLOCK_MASK  0x757bdf0d /* Used to unlock the device */
> +
> +/* Masks for controlling stuff in SLCR */
> +#define LVL_SHFTR_DISABLE_ALL_MASK   0x0 /* Disable all Level shifters */
> +#define LVL_SHFTR_ENABLE_PS_TO_PL0xa /* Enable all Level shifters */
> +#define LVL_SHFTR_ENABLE_PL_TO_PS0xf /* Enable all Level shifters */
> +#define FPGA_RST_ALL_MASK0xf /* Enable global resets */
> +#define FPGA_RST_NONE_MASK   0x0 /* Disable global resets */
> +

> +static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> + const char *buf, size_t count)
> +{
> + struct zynq_fpga_priv *priv;
> + u32 ctrl, status;
> + int err;
> +
> + priv = mgr->priv;
> +
> + err = clk_enable(priv->clk);
> + if (err)
> + return err;

You might not even need to enable/disable the clock if not doing PR.
Does this driver support both full reconfig and partial reconfig?

> +
> + /* only reset if we're not doing partial reconfig */
> + if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> + /* assert AXI interface resets */
> + regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
> +  FPGA_RST_ALL_MASK);
> +
> + /* disable level shifters */
> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +  LVL_SHFTR_DISABLE_ALL_MASK);
> + /* enable output level shifters */
> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +  LVL_SHFTR_ENABLE_PS_TO_PL);
> +
> + /* create a rising edge on PCFG_INIT. PCFG_INIT follows
> +  * PCFG_PROG_B, so we need to poll it after setting PCFG_PROG_B
> +  * to make sure the rising edge actually happens
> +  */
> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> + 

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Greg KH]

On Fri, Oct 09, 2015 at 07:09:15PM +0100, atull wrote:
> On Thu, 8 Oct 2015, Moritz Fischer wrote:
> 
> > --- /dev/null
> > +++ b/drivers/fpga/zynq-fpga.c
> > @@ -0,0 +1,478 @@
> > +/*
> > + * Copyright (c) 2011-2015 Xilinx Inc.
> > + * Copyright (c) 2015, National Instruments Corp.
> > + *
> > + * FPGA Manager Driver for Xilinx Zynq, heavily based on xdevcfg driver
> > + * in their vendor tree.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; version 2 of the License.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> 
> Hi Moritz,
> 
> That was fast!  I just have a couple of very minor comments...
> 
> Please alphabetize the #includes.

Bah, who cares about that, it's not a requirement at all.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Moritz Fischer]

Hi Josh,

thanks for the review!

On Fri, Oct 9, 2015 at 6:33 PM, Josh Cartwright  wrote:
> Hey Moritz-
>
> On Fri, Oct 09, 2015 at 12:45:07AM +0200, Moritz Fischer wrote:
>> This commit adds FPGA Manager support for the Xilinx Zynq chip.
>> The code heavily borrows from the xdevcfg driver in Xilinx'
>> vendor tree.
>>
>> Signed-off-by: Moritz Fischer 
> [..]
>> +++ b/drivers/fpga/zynq-fpga.c
> [..]
>> +static irqreturn_t zynq_fpga_isr(int irq, void *data)
>> +{
>> + u32 intr_status;
>> + struct zynq_fpga_priv *priv = data;
>> +
>> + spin_lock(>lock);
>
> I'm confused about the locking here.  You have this lock, but it's only
> used in the isr.  It's claimed purpose is to protect 'error', but that
> clearly isn't happening.

Ouch, yes ...
>
>> + intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
>> +
>> + if (!intr_status) {
>> + spin_unlock(>lock);
>> + return IRQ_NONE;
>> + }
>> +
>> + zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
>> +
>> + if ((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)
>> + complete(>dma_done);
>
> Just so I understand, wouldn't you also want to complete() in the error
> case, too?

Ehrm ... yes. Definitely.
>
>> + if ((intr_status & IXR_ERROR_FLAGS_MASK) ==
>> + IXR_ERROR_FLAGS_MASK) {
>> + priv->error = true;
>> + dev_err(priv->dev, "%s dma error\n", __func__);
>> + }
>> + spin_unlock(>lock);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>> + const char *buf, size_t count)
>> +{
>> + struct zynq_fpga_priv *priv;
>> + u32 ctrl, status;
>> + int err;
>> +
>> + priv = mgr->priv;
>> +
>> + err = clk_enable(priv->clk);
>> + if (err)
>> + return err;
>> +
>> + /* only reset if we're not doing partial reconfig */
>> + if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
>> + /* assert AXI interface resets */
>> + regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
>> +  FPGA_RST_ALL_MASK);
>> +
>> + /* disable level shifters */
>> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
>> +  LVL_SHFTR_DISABLE_ALL_MASK);
>> + /* enable output level shifters */
>> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
>> +  LVL_SHFTR_ENABLE_PS_TO_PL);
>> +
>> + /* create a rising edge on PCFG_INIT. PCFG_INIT follows
>> +  * PCFG_PROG_B, so we need to poll it after setting PCFG_PROG_B
>> +  * to make sure the rising edge actually happens
>> +  */
>> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
>> + ctrl |= CTRL_PCFG_PROG_B_MASK;
>> +
>> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
>> +
>> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
>> +STATUS_PCFG_INIT_MASK, 20, 0);
>
> And if we timeout?

Ehrm ... then we should cleanup & return ...
>
>> +
>> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
>> + ctrl &= ~CTRL_PCFG_PROG_B_MASK;
>> +
>> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
>> +
>> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, !(status &
>> +STATUS_PCFG_INIT_MASK), 20, 0);
>
> And if we timeout?

See above ...
>
>> +
>> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
>> + ctrl |= CTRL_PCFG_PROG_B_MASK;
>> +
>> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
>> +
>> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
>> +STATUS_PCFG_INIT_MASK, 20, 0);
>
> And if we timeout?

Ok ok ... got it...
>
>> + }
>> +
>> + clk_disable(priv->clk);
>> +
>> + return 0;
>> +}
>> +
>> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>> +const char *buf, size_t count)
>> +{
>> + struct zynq_fpga_priv *priv;
>> + int err;
>> + char *kbuf;
>> + size_t i, in_count;
>> + dma_addr_t dma_addr;
>> + u32 transfer_length = 0;
>> + bool endian_swap = false;
>> +
>> + in_count = count;
>> + priv = mgr->priv;
>> +
>> + kbuf = dma_alloc_coherent(priv->dev, count, _addr, GFP_KERNEL);
>> + if (!kbuf)
>> + return -ENOMEM;
>> +
>> + memcpy(kbuf, buf, count);
>> +
>> + /* look for the sync word */
>> + for (i = 0; i < count - 4; i++) {
>> + if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
>> + dev_dbg(priv->dev, "Found normal sync word\n");
>> + endian_swap = false;
>> + break;
>> + }
>> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
>> + 

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Josh Cartwright]

Hey Moritz-

On Fri, Oct 09, 2015 at 12:45:07AM +0200, Moritz Fischer wrote:
> This commit adds FPGA Manager support for the Xilinx Zynq chip.
> The code heavily borrows from the xdevcfg driver in Xilinx'
> vendor tree.
> 
> Signed-off-by: Moritz Fischer 
[..]
> +++ b/drivers/fpga/zynq-fpga.c
[..]
> +static irqreturn_t zynq_fpga_isr(int irq, void *data)
> +{
> + u32 intr_status;
> + struct zynq_fpga_priv *priv = data;
> +
> + spin_lock(>lock);

I'm confused about the locking here.  You have this lock, but it's only
used in the isr.  It's claimed purpose is to protect 'error', but that
clearly isn't happening.

> + intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
> +
> + if (!intr_status) {
> + spin_unlock(>lock);
> + return IRQ_NONE;
> + }
> +
> + zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
> +
> + if ((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)
> + complete(>dma_done);

Just so I understand, wouldn't you also want to complete() in the error
case, too?

> + if ((intr_status & IXR_ERROR_FLAGS_MASK) ==
> + IXR_ERROR_FLAGS_MASK) {
> + priv->error = true;
> + dev_err(priv->dev, "%s dma error\n", __func__);
> + }
> + spin_unlock(>lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> + const char *buf, size_t count)
> +{
> + struct zynq_fpga_priv *priv;
> + u32 ctrl, status;
> + int err;
> +
> + priv = mgr->priv;
> +
> + err = clk_enable(priv->clk);
> + if (err)
> + return err;
> +
> + /* only reset if we're not doing partial reconfig */
> + if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> + /* assert AXI interface resets */
> + regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
> +  FPGA_RST_ALL_MASK);
> +
> + /* disable level shifters */
> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +  LVL_SHFTR_DISABLE_ALL_MASK);
> + /* enable output level shifters */
> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +  LVL_SHFTR_ENABLE_PS_TO_PL);
> +
> + /* create a rising edge on PCFG_INIT. PCFG_INIT follows
> +  * PCFG_PROG_B, so we need to poll it after setting PCFG_PROG_B
> +  * to make sure the rising edge actually happens
> +  */
> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> + ctrl |= CTRL_PCFG_PROG_B_MASK;
> +
> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
> +STATUS_PCFG_INIT_MASK, 20, 0);

And if we timeout?

> +
> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> + ctrl &= ~CTRL_PCFG_PROG_B_MASK;
> +
> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, !(status &
> +STATUS_PCFG_INIT_MASK), 20, 0);

And if we timeout?

> +
> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> + ctrl |= CTRL_PCFG_PROG_B_MASK;
> +
> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
> +STATUS_PCFG_INIT_MASK, 20, 0);

And if we timeout?

> + }
> +
> + clk_disable(priv->clk);
> +
> + return 0;
> +}
> +
> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
> +const char *buf, size_t count)
> +{
> + struct zynq_fpga_priv *priv;
> + int err;
> + char *kbuf;
> + size_t i, in_count;
> + dma_addr_t dma_addr;
> + u32 transfer_length = 0;
> + bool endian_swap = false;
> +
> + in_count = count;
> + priv = mgr->priv;
> +
> + kbuf = dma_alloc_coherent(priv->dev, count, _addr, GFP_KERNEL);
> + if (!kbuf)
> + return -ENOMEM;
> +
> + memcpy(kbuf, buf, count);
> +
> + /* look for the sync word */
> + for (i = 0; i < count - 4; i++) {
> + if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
> + dev_dbg(priv->dev, "Found normal sync word\n");
> + endian_swap = false;
> + break;
> + }
> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
> + dev_dbg(priv->dev, "Found swapped sync word\n");
> + endian_swap = true;
> + break;
> + }
> + }

How much control do we have over mandating the format of firmware at
this point?  It'd be swell if we could just mandate a specific
endianness, and leave this munging to usermode.

> + /* remove the header, align the data on 

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Josh Cartwright]

Hey Moritz-

On Fri, Oct 09, 2015 at 12:45:07AM +0200, Moritz Fischer wrote:
> This commit adds FPGA Manager support for the Xilinx Zynq chip.
> The code heavily borrows from the xdevcfg driver in Xilinx'
> vendor tree.
> 
> Signed-off-by: Moritz Fischer 
[..]
> +++ b/drivers/fpga/zynq-fpga.c
[..]
> +static irqreturn_t zynq_fpga_isr(int irq, void *data)
> +{
> + u32 intr_status;
> + struct zynq_fpga_priv *priv = data;
> +
> + spin_lock(>lock);

I'm confused about the locking here.  You have this lock, but it's only
used in the isr.  It's claimed purpose is to protect 'error', but that
clearly isn't happening.

> + intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
> +
> + if (!intr_status) {
> + spin_unlock(>lock);
> + return IRQ_NONE;
> + }
> +
> + zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
> +
> + if ((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)
> + complete(>dma_done);

Just so I understand, wouldn't you also want to complete() in the error
case, too?

> + if ((intr_status & IXR_ERROR_FLAGS_MASK) ==
> + IXR_ERROR_FLAGS_MASK) {
> + priv->error = true;
> + dev_err(priv->dev, "%s dma error\n", __func__);
> + }
> + spin_unlock(>lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> + const char *buf, size_t count)
> +{
> + struct zynq_fpga_priv *priv;
> + u32 ctrl, status;
> + int err;
> +
> + priv = mgr->priv;
> +
> + err = clk_enable(priv->clk);
> + if (err)
> + return err;
> +
> + /* only reset if we're not doing partial reconfig */
> + if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> + /* assert AXI interface resets */
> + regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
> +  FPGA_RST_ALL_MASK);
> +
> + /* disable level shifters */
> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +  LVL_SHFTR_DISABLE_ALL_MASK);
> + /* enable output level shifters */
> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +  LVL_SHFTR_ENABLE_PS_TO_PL);
> +
> + /* create a rising edge on PCFG_INIT. PCFG_INIT follows
> +  * PCFG_PROG_B, so we need to poll it after setting PCFG_PROG_B
> +  * to make sure the rising edge actually happens
> +  */
> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> + ctrl |= CTRL_PCFG_PROG_B_MASK;
> +
> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
> +STATUS_PCFG_INIT_MASK, 20, 0);

And if we timeout?

> +
> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> + ctrl &= ~CTRL_PCFG_PROG_B_MASK;
> +
> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, !(status &
> +STATUS_PCFG_INIT_MASK), 20, 0);

And if we timeout?

> +
> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> + ctrl |= CTRL_PCFG_PROG_B_MASK;
> +
> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
> +STATUS_PCFG_INIT_MASK, 20, 0);

And if we timeout?

> + }
> +
> + clk_disable(priv->clk);
> +
> + return 0;
> +}
> +
> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
> +const char *buf, size_t count)
> +{
> + struct zynq_fpga_priv *priv;
> + int err;
> + char *kbuf;
> + size_t i, in_count;
> + dma_addr_t dma_addr;
> + u32 transfer_length = 0;
> + bool endian_swap = false;
> +
> + in_count = count;
> + priv = mgr->priv;
> +
> + kbuf = dma_alloc_coherent(priv->dev, count, _addr, GFP_KERNEL);
> + if (!kbuf)
> + return -ENOMEM;
> +
> + memcpy(kbuf, buf, count);
> +
> + /* look for the sync word */
> + for (i = 0; i < count - 4; i++) {
> + if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
> + dev_dbg(priv->dev, "Found normal sync word\n");
> + endian_swap = false;
> + break;
> + }
> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
> + dev_dbg(priv->dev, "Found swapped sync word\n");
> + endian_swap = true;
> + break;
> + }
> + }

How much control do we have over mandating the format of firmware at
this point?  It'd be swell if we could just mandate a specific
endianness, and leave this munging to usermode.

> + /* remove the 

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Moritz Fischer]

Hi Josh,

thanks for the review!

On Fri, Oct 9, 2015 at 6:33 PM, Josh Cartwright  wrote:
> Hey Moritz-
>
> On Fri, Oct 09, 2015 at 12:45:07AM +0200, Moritz Fischer wrote:
>> This commit adds FPGA Manager support for the Xilinx Zynq chip.
>> The code heavily borrows from the xdevcfg driver in Xilinx'
>> vendor tree.
>>
>> Signed-off-by: Moritz Fischer 
> [..]
>> +++ b/drivers/fpga/zynq-fpga.c
> [..]
>> +static irqreturn_t zynq_fpga_isr(int irq, void *data)
>> +{
>> + u32 intr_status;
>> + struct zynq_fpga_priv *priv = data;
>> +
>> + spin_lock(>lock);
>
> I'm confused about the locking here.  You have this lock, but it's only
> used in the isr.  It's claimed purpose is to protect 'error', but that
> clearly isn't happening.

Ouch, yes ...
>
>> + intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
>> +
>> + if (!intr_status) {
>> + spin_unlock(>lock);
>> + return IRQ_NONE;
>> + }
>> +
>> + zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
>> +
>> + if ((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)
>> + complete(>dma_done);
>
> Just so I understand, wouldn't you also want to complete() in the error
> case, too?

Ehrm ... yes. Definitely.
>
>> + if ((intr_status & IXR_ERROR_FLAGS_MASK) ==
>> + IXR_ERROR_FLAGS_MASK) {
>> + priv->error = true;
>> + dev_err(priv->dev, "%s dma error\n", __func__);
>> + }
>> + spin_unlock(>lock);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>> + const char *buf, size_t count)
>> +{
>> + struct zynq_fpga_priv *priv;
>> + u32 ctrl, status;
>> + int err;
>> +
>> + priv = mgr->priv;
>> +
>> + err = clk_enable(priv->clk);
>> + if (err)
>> + return err;
>> +
>> + /* only reset if we're not doing partial reconfig */
>> + if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
>> + /* assert AXI interface resets */
>> + regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
>> +  FPGA_RST_ALL_MASK);
>> +
>> + /* disable level shifters */
>> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
>> +  LVL_SHFTR_DISABLE_ALL_MASK);
>> + /* enable output level shifters */
>> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
>> +  LVL_SHFTR_ENABLE_PS_TO_PL);
>> +
>> + /* create a rising edge on PCFG_INIT. PCFG_INIT follows
>> +  * PCFG_PROG_B, so we need to poll it after setting PCFG_PROG_B
>> +  * to make sure the rising edge actually happens
>> +  */
>> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
>> + ctrl |= CTRL_PCFG_PROG_B_MASK;
>> +
>> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
>> +
>> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
>> +STATUS_PCFG_INIT_MASK, 20, 0);
>
> And if we timeout?

Ehrm ... then we should cleanup & return ...
>
>> +
>> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
>> + ctrl &= ~CTRL_PCFG_PROG_B_MASK;
>> +
>> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
>> +
>> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, !(status &
>> +STATUS_PCFG_INIT_MASK), 20, 0);
>
> And if we timeout?

See above ...
>
>> +
>> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
>> + ctrl |= CTRL_PCFG_PROG_B_MASK;
>> +
>> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
>> +
>> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
>> +STATUS_PCFG_INIT_MASK, 20, 0);
>
> And if we timeout?

Ok ok ... got it...
>
>> + }
>> +
>> + clk_disable(priv->clk);
>> +
>> + return 0;
>> +}
>> +
>> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>> +const char *buf, size_t count)
>> +{
>> + struct zynq_fpga_priv *priv;
>> + int err;
>> + char *kbuf;
>> + size_t i, in_count;
>> + dma_addr_t dma_addr;
>> + u32 transfer_length = 0;
>> + bool endian_swap = false;
>> +
>> + in_count = count;
>> + priv = mgr->priv;
>> +
>> + kbuf = dma_alloc_coherent(priv->dev, count, _addr, GFP_KERNEL);
>> + if (!kbuf)
>> + return -ENOMEM;
>> +
>> + memcpy(kbuf, buf, count);
>> +
>> + /* look for the sync word */
>> + for (i = 0; i < count - 4; i++) {
>> + if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
>> + dev_dbg(priv->dev, "Found normal sync word\n");
>> + endian_swap = false;
>> + break;
>> + }
>> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Greg KH]

On Fri, Oct 09, 2015 at 07:09:15PM +0100, atull wrote:
> On Thu, 8 Oct 2015, Moritz Fischer wrote:
> 
> > --- /dev/null
> > +++ b/drivers/fpga/zynq-fpga.c
> > @@ -0,0 +1,478 @@
> > +/*
> > + * Copyright (c) 2011-2015 Xilinx Inc.
> > + * Copyright (c) 2015, National Instruments Corp.
> > + *
> > + * FPGA Manager Driver for Xilinx Zynq, heavily based on xdevcfg driver
> > + * in their vendor tree.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; version 2 of the License.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> 
> Hi Moritz,
> 
> That was fast!  I just have a couple of very minor comments...
> 
> Please alphabetize the #includes.

Bah, who cares about that, it's not a requirement at all.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[atull]

On Thu, 8 Oct 2015, Moritz Fischer wrote:

> --- /dev/null
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -0,0 +1,478 @@
> +/*
> + * Copyright (c) 2011-2015 Xilinx Inc.
> + * Copyright (c) 2015, National Instruments Corp.
> + *
> + * FPGA Manager Driver for Xilinx Zynq, heavily based on xdevcfg driver
> + * in their vendor tree.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Hi Moritz,

That was fast!  I just have a couple of very minor comments...

Please alphabetize the #includes.

> +
> +/* Offsets into SLCR regmap */
> +#define SLCR_FPGA_RST_CTRL_OFFSET0x240 /* FPGA Software Reset Control */
> +#define SLCR_LVL_SHFTR_EN_OFFSET 0x900 /* Level Shifters Enable */
> +
> +/* Constant Definitions */
> +#define CTRL_OFFSET  0x00 /* Control Register */
> +#define LOCK_OFFSET  0x04 /* Lock Register */
> +#define INT_STS_OFFSET   0x0c /* Interrupt Status Register */
> +#define INT_MASK_OFFSET  0x10 /* Interrupt Mask Register */
> +#define STATUS_OFFSET0x14 /* Status Register */
> +#define DMA_SRC_ADDR_OFFSET  0x18 /* DMA Source Address Register */
> +#define DMA_DEST_ADDR_OFFSET 0x1c /* DMA Destination Address Reg */
> +#define DMA_SRC_LEN_OFFSET   0x20 /* DMA Source Transfer Length */
> +#define DMA_DEST_LEN_OFFSET  0x24 /* DMA Destination Transfer */
> +#define UNLOCK_OFFSET0x34 /* Unlock Register */
> +#define MCTRL_OFFSET 0x80 /* Misc. Control Register */

Please fix up the indenting.

> +
> +/* Control Register Bit definitions */
> +#define CTRL_PCFG_PROG_B_MASKBIT(30) /* Program signal to reset FPGA 
> */
> +#define CTRL_PCAP_PR_MASKBIT(27) /* Enable PCAP for PR */
> +#define CTRL_PCAP_MODE_MASK  BIT(26) /* Enable PCAP */
> +
> +/* Miscellaneous Control Register bit definitions */
> +#define MCTRL_PCAP_LPBK_MASK BIT(4) /* Internal PCAP loopback */
> +
> +/* Status register bit definitions */
> +#define STATUS_PCFG_INIT_MASKBIT(4) /* FPGA init status */
> +
> +/* Interrupt Status/Mask Register Bit definitions */
> +#define IXR_DMA_DONE_MASKBIT(13) /* DMA command done */
> +#define IXR_D_P_DONE_MASKBIT(12) /* DMA and PCAP cmd done */
> +#define IXR_PCFG_DONE_MASK   BIT(2)  /* FPGA programmed */
> +#define IXR_ERROR_FLAGS_MASK 0x00F0F860
> +#define IXR_ALL_MASK 0xF8F7F87F
> +
> +/* Miscellaneous constant values */
> +#define DMA_INVALID_ADDRESS  GENMASK(31, 0)  /* Invalid DMA address */
> +#define UNLOCK_MASK  0x757bdf0d /* Used to unlock the device */
> +
> +/* Masks for controlling stuff in SLCR */
> +#define LVL_SHFTR_DISABLE_ALL_MASK   0x0 /* Disable all Level shifters */
> +#define LVL_SHFTR_ENABLE_PS_TO_PL0xa /* Enable all Level shifters */
> +#define LVL_SHFTR_ENABLE_PL_TO_PS0xf /* Enable all Level shifters */
> +#define FPGA_RST_ALL_MASK0xf /* Enable global resets */
> +#define FPGA_RST_NONE_MASK   0x0 /* Disable global resets */
> +

> +static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> + const char *buf, size_t count)
> +{
> + struct zynq_fpga_priv *priv;
> + u32 ctrl, status;
> + int err;
> +
> + priv = mgr->priv;
> +
> + err = clk_enable(priv->clk);
> + if (err)
> + return err;

You might not even need to enable/disable the clock if not doing PR.
Does this driver support both full reconfig and partial reconfig?

> +
> + /* only reset if we're not doing partial reconfig */
> + if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> + /* assert AXI interface resets */
> + regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
> +  FPGA_RST_ALL_MASK);
> +
> + /* disable level shifters */
> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +  LVL_SHFTR_DISABLE_ALL_MASK);
> + /* enable output level shifters */
> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +  LVL_SHFTR_ENABLE_PS_TO_PL);
> +
> + /* create a rising edge on PCFG_INIT. PCFG_INIT follows
> +  * PCFG_PROG_B, so we need to poll it after setting PCFG_PROG_B
> +  * to make sure the rising edge actually happens
> +  */
> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> + 

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[atull]

On Fri, 9 Oct 2015, Greg KH wrote:

> On Fri, Oct 09, 2015 at 07:09:15PM +0100, atull wrote:
> > On Thu, 8 Oct 2015, Moritz Fischer wrote:
> > 
> > > --- /dev/null
> > > +++ b/drivers/fpga/zynq-fpga.c
> > > @@ -0,0 +1,478 @@
> > > +/*
> > > + * Copyright (c) 2011-2015 Xilinx Inc.
> > > + * Copyright (c) 2015, National Instruments Corp.
> > > + *
> > > + * FPGA Manager Driver for Xilinx Zynq, heavily based on xdevcfg driver
> > > + * in their vendor tree.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; version 2 of the License.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > 
> > Hi Moritz,
> > 
> > That was fast!  I just have a couple of very minor comments...
> > 
> > Please alphabetize the #includes.
> 
> Bah, who cares about that, it's not a requirement at all.
> 
> greg k-h
> 

Nice to know!  I get slammed every time for that!

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/