[PATCH v2 0/2] fpga: Convert some options to Kconfig

[PATCH v2 0/2] fpga: Convert some options to Kconfig

[Alexander Dahl]

Hei hei,

rebased this on recent master.  One patch dropped.  See changelog below.

Greets
Alex

(implicit) v1 -> v2:
  - dropped patch 3, same kconfig symbol addressed with commit
    60d45642fe0673514aced37e6cc95d4f0fe02a19 ("fpga: Remove
    CONFIG_FPGA_COUNT")

Cc: Wolfgang Wegner <w.wegner@astro-kom.de>
Cc: Michal Simek <michal.simek@amd.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: "Marek Behún" <marek.behun@nic.cz>

Alexander Dahl (2):
  fpga: Convert SYS_FPGA_CHECK_CTRLC to Kconfig
  fpga: Convert SYS_FPGA_PROG_FEEDBACK to Kconfig

 README                           |  7 -------
 configs/astro_mcf5373l_defconfig |  1 +
 drivers/fpga/Kconfig             | 10 ++++++++++
 include/configs/astro_mcf5373l.h |  1 -
 scripts/config_whitelist.txt     |  1 -
 5 files changed, 11 insertions(+), 9 deletions(-)


base-commit: 36b661dc919da318c163a45f4a220d2e3d9db608
-- 
2.30.2

[PATCH v2 1/2] fpga: Convert SYS_FPGA_CHECK_CTRLC to Kconfig

[Alexander Dahl]

From: Alexander Dahl <ada@thorsis.com>

After commit 8cca60a2cbf2 ("Kconfig: Remove some symbols from the
whitelist") downstream builds failed for boards setting this in
include/configs/…

Two FPGA drivers consider this definition.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 README               | 3 ---
 drivers/fpga/Kconfig | 4 ++++
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/README b/README
index ff0df3797d..8c31e5c0e3 100644
--- a/README
+++ b/README
@@ -1346,9 +1346,6 @@ The following options need to be configured:
 		If defined, a function that provides delays in the FPGA
 		configuration driver.
 
-		CONFIG_SYS_FPGA_CHECK_CTRLC
-		Allow Control-C to interrupt FPGA configuration
-
 		CONFIG_SYS_FPGA_CHECK_ERROR
 
 		Check for configuration errors during FPGA bitfile
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 76719517f5..53d91676e0 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -91,4 +91,8 @@ config FPGA_ZYNQPL
 	  Enable FPGA driver for loading bitstream in BIT and BIN format
 	  on Xilinx Zynq devices.
 
+config SYS_FPGA_CHECK_CTRLC
+	bool "Allow Control-C to interrupt FPGA configuration"
+	depends on FPGA
+
 endmenu
-- 
2.30.2

[PATCH v2 2/2] fpga: Convert SYS_FPGA_PROG_FEEDBACK to Kconfig

[Alexander Dahl]

From: Alexander Dahl <ada@thorsis.com>

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 README                           | 4 ----
 configs/astro_mcf5373l_defconfig | 1 +
 drivers/fpga/Kconfig             | 6 ++++++
 include/configs/astro_mcf5373l.h | 1 -
 scripts/config_whitelist.txt     | 1 -
 5 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/README b/README
index 8c31e5c0e3..2c4bde0b32 100644
--- a/README
+++ b/README
@@ -1330,10 +1330,6 @@ The following options need to be configured:
 		Enables support for FPGA family.
 		(SPARTAN2, SPARTAN3, VIRTEX2, CYCLONE2, ACEX1K, ACEX)
 
-		CONFIG_SYS_FPGA_PROG_FEEDBACK
-
-		Enable printing of hash marks during FPGA configuration.
-
 		CONFIG_SYS_FPGA_CHECK_BUSY
 
 		Enable checks on FPGA configuration interface busy
diff --git a/configs/astro_mcf5373l_defconfig b/configs/astro_mcf5373l_defconfig
index 3a44c7e8ec..9f5cb8702c 100644
--- a/configs/astro_mcf5373l_defconfig
+++ b/configs/astro_mcf5373l_defconfig
@@ -33,6 +33,7 @@ CONFIG_FPGA_ALTERA=y
 CONFIG_FPGA_CYCLON2=y
 CONFIG_FPGA_XILINX=y
 CONFIG_FPGA_SPARTAN3=y
+CONFIG_SYS_FPGA_PROG_FEEDBACK=y
 CONFIG_SYS_I2C_LEGACY=y
 CONFIG_SYS_I2C_FSL=y
 CONFIG_SYS_FSL_I2C_OFFSET=0x58000
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 53d91676e0..e152ea14dc 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -95,4 +95,10 @@ config SYS_FPGA_CHECK_CTRLC
 	bool "Allow Control-C to interrupt FPGA configuration"
 	depends on FPGA
 
+config SYS_FPGA_PROG_FEEDBACK
+	bool "Progress output during FPGA configuration"
+	depends on FPGA
+	help
+	  Enable printing of hash marks during FPGA configuration.
+
 endmenu
diff --git a/include/configs/astro_mcf5373l.h b/include/configs/astro_mcf5373l.h
index a8265e961a..da4d49741d 100644
--- a/include/configs/astro_mcf5373l.h
+++ b/include/configs/astro_mcf5373l.h
@@ -131,7 +131,6 @@
  * it needs non-blocking CFI routines.
  */
 
-#define CONFIG_SYS_FPGA_PROG_FEEDBACK
 #define CONFIG_SYS_FPGA_WAIT		1000
 
 /* End of user parameters to be customized */
diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
index efc2f3bcf7..fc07c5d257 100644
--- a/scripts/config_whitelist.txt
+++ b/scripts/config_whitelist.txt
@@ -743,7 +743,6 @@ CONFIG_SYS_FPGA_FTIM0
 CONFIG_SYS_FPGA_FTIM1
 CONFIG_SYS_FPGA_FTIM2
 CONFIG_SYS_FPGA_FTIM3
-CONFIG_SYS_FPGA_PROG_FEEDBACK
 CONFIG_SYS_FPGA_SIZE
 CONFIG_SYS_FPGA_WAIT
 CONFIG_SYS_FSL_BMAN_ADDR
-- 
2.30.2

Re: [PATCH v2 2/2] fpga: Convert SYS_FPGA_PROG_FEEDBACK to Kconfig

[Michal Simek]

On 7/13/22 14:33, Alexander Dahl wrote:
> From: Alexander Dahl <ada@thorsis.com>


WARNING: please write a paragraph that describes the config symbol fully

We don't allow patches with empty commit message.


> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
>   README                           | 4 ----
>   configs/astro_mcf5373l_defconfig | 1 +
>   drivers/fpga/Kconfig             | 6 ++++++
>   include/configs/astro_mcf5373l.h | 1 -
>   scripts/config_whitelist.txt     | 1 -

Tom can confirm this but IIRC you don't need to remove this from this file.
Tom is doing sync up time to time. It is enough to do conversion only.

M

Re: [PATCH v2 1/2] fpga: Convert SYS_FPGA_CHECK_CTRLC to Kconfig

[Michal Simek]

On 7/13/22 14:32, Alexander Dahl wrote:
> From: Alexander Dahl <ada@thorsis.com>
> 
> After commit 8cca60a2cbf2 ("Kconfig: Remove some symbols from the
> whitelist") downstream builds failed for boards setting this in
> include/configs/…
> 
> Two FPGA drivers consider this definition.

2?
board/astro/mcf5373l/fpga.c
drivers/fpga/ACEX1K.c
drivers/fpga/virtex2.c

> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
>   README               | 3 ---
>   drivers/fpga/Kconfig | 4 ++++
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/README b/README
> index ff0df3797d..8c31e5c0e3 100644
> --- a/README
> +++ b/README
> @@ -1346,9 +1346,6 @@ The following options need to be configured:
>   		If defined, a function that provides delays in the FPGA
>   		configuration driver.
>   
> -		CONFIG_SYS_FPGA_CHECK_CTRLC
> -		Allow Control-C to interrupt FPGA configuration
> -
>   		CONFIG_SYS_FPGA_CHECK_ERROR
>   
>   		Check for configuration errors during FPGA bitfile
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 76719517f5..53d91676e0 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -91,4 +91,8 @@ config FPGA_ZYNQPL
>   	  Enable FPGA driver for loading bitstream in BIT and BIN format
>   	  on Xilinx Zynq devices.
>   
> +config SYS_FPGA_CHECK_CTRLC
> +	bool "Allow Control-C to interrupt FPGA configuration"
> +	depends on FPGA

Please write help message.

> +
>   endmenu


And can you please remove this code from drivers/fpga/virtex2.c

  48 /*
  49  * Don't allow config cycle to be interrupted
  50  */
  51 #ifndef CONFIG_SYS_FPGA_CHECK_CTRLC
  52 #undef CONFIG_SYS_FPGA_CHECK_CTRLC
  53 #endif

it doesn't make any sense.

And with 2/2 please also remove
drivers/fpga/spartan2.c:18:#undef CONFIG_SYS_FPGA_PROG_FEEDBACK
drivers/fpga/virtex2.c:44:#ifndef CONFIG_SYS_FPGA_PROG_FEEDBACK
drivers/fpga/virtex2.c:45:#define CONFIG_SYS_FPGA_PROG_FEEDBACK

Thanks,
Michal

Re: [PATCH v2 2/2] fpga: Convert SYS_FPGA_PROG_FEEDBACK to Kconfig

[Alexander Dahl]

[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]

Hei hei,

On Wed, Jul 13, 2022 at 02:50:14PM +0200, Michal Simek wrote:
> 
> 
> On 7/13/22 14:33, Alexander Dahl wrote:
> > From: Alexander Dahl <ada@thorsis.com>
> 
> 
> WARNING: please write a paragraph that describes the config symbol fully
> 
> We don't allow patches with empty commit message.

In general I would agree.  However several of Tom's patches addressing
kconfig migration have a commit message like this:

  This converts the following to Kconfig: FOO_BAR

This is redundant to the subject saying exactly the same.  I can add
this if you want, but I saw no sense in it.

> 
> 
> > 
> > Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > ---
> >   README                           | 4 ----
> >   configs/astro_mcf5373l_defconfig | 1 +
> >   drivers/fpga/Kconfig             | 6 ++++++
> >   include/configs/astro_mcf5373l.h | 1 -
> >   scripts/config_whitelist.txt     | 1 -
> 
> Tom can confirm this but IIRC you don't need to remove this from this file.
> Tom is doing sync up time to time. It is enough to do conversion only.

This was done by the script ./tools/moveconfig.py where I just hit
enter with defaults presented.  The other Kconfig patches change
defconfig, but not whitelist.  Seems a little inconsistent to me.  But
I can remove that part, sure.

Greets
Alex

> 
> M

-- 
/"\ ASCII RIBBON | »With the first link, the chain is forged. The first
\ / CAMPAIGN     | speech censured, the first thought forbidden, the
 X  AGAINST      | first freedom denied, chains us all irrevocably.«
/ \ HTML MAIL    | (Jean-Luc Picard, quoting Judge Aaron Satie)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] fpga: Convert SYS_FPGA_CHECK_CTRLC to Kconfig

[Alexander Dahl]

[-- Attachment #1: Type: text/plain, Size: 3188 bytes --]

Hello Michal,

On Wed, Jul 13, 2022 at 02:56:08PM +0200, Michal Simek wrote:
> 
> 
> On 7/13/22 14:32, Alexander Dahl wrote:
> > From: Alexander Dahl <ada@thorsis.com>
> > 
> > After commit 8cca60a2cbf2 ("Kconfig: Remove some symbols from the
> > whitelist") downstream builds failed for boards setting this in
> > include/configs/…
> > 
> > Two FPGA drivers consider this definition.
> 
> 2?
> board/astro/mcf5373l/fpga.c
> drivers/fpga/ACEX1K.c
> drivers/fpga/virtex2.c

These are one board and two drivers.  What is your question?

Greets
Alex

> 
> > 
> > Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > ---
> >   README               | 3 ---
> >   drivers/fpga/Kconfig | 4 ++++
> >   2 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/README b/README
> > index ff0df3797d..8c31e5c0e3 100644
> > --- a/README
> > +++ b/README
> > @@ -1346,9 +1346,6 @@ The following options need to be configured:
> >   		If defined, a function that provides delays in the FPGA
> >   		configuration driver.
> > -		CONFIG_SYS_FPGA_CHECK_CTRLC
> > -		Allow Control-C to interrupt FPGA configuration
> > -
> >   		CONFIG_SYS_FPGA_CHECK_ERROR
> >   		Check for configuration errors during FPGA bitfile
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index 76719517f5..53d91676e0 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -91,4 +91,8 @@ config FPGA_ZYNQPL
> >   	  Enable FPGA driver for loading bitstream in BIT and BIN format
> >   	  on Xilinx Zynq devices.
> > +config SYS_FPGA_CHECK_CTRLC
> > +	bool "Allow Control-C to interrupt FPGA configuration"
> > +	depends on FPGA
> 
> Please write help message.

Okay, I'll have to invent a new message here, if the prompt is not
self explaining enough.  Since this is not conversion, but adding a
new message we did not have before, should this go into a separate
patch?

> 
> > +
> >   endmenu
> 
> 
> And can you please remove this code from drivers/fpga/virtex2.c
> 
>  48 /*
>  49  * Don't allow config cycle to be interrupted
>  50  */
>  51 #ifndef CONFIG_SYS_FPGA_CHECK_CTRLC
>  52 #undef CONFIG_SYS_FPGA_CHECK_CTRLC
>  53 #endif
> 
> it doesn't make any sense.

I have no hardware to test this and this is out of scope of the
conversion patch itself.

> 
> And with 2/2 please also remove
> drivers/fpga/spartan2.c:18:#undef CONFIG_SYS_FPGA_PROG_FEEDBACK
> drivers/fpga/virtex2.c:44:#ifndef CONFIG_SYS_FPGA_PROG_FEEDBACK
> drivers/fpga/virtex2.c:45:#define CONFIG_SYS_FPGA_PROG_FEEDBACK
> 
> Thanks,
> Michal

I may be able to add an additional patch or two, but those are all
FPGAs I have no experience with and I can not test those.  This would
be more or less guessing based on code reading.  I can try next week,
not able to do this currently.

Thanks for your review.

Greets
Alex

-- 
/"\ ASCII RIBBON | »With the first link, the chain is forged. The first
\ / CAMPAIGN     | speech censured, the first thought forbidden, the
 X  AGAINST      | first freedom denied, chains us all irrevocably.«
/ \ HTML MAIL    | (Jean-Luc Picard, quoting Judge Aaron Satie)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] fpga: Convert SYS_FPGA_CHECK_CTRLC to Kconfig

[Michal Simek]

On 7/13/22 15:20, Alexander Dahl wrote:
> Hello Michal,
> 
> On Wed, Jul 13, 2022 at 02:56:08PM +0200, Michal Simek wrote:
>>
>>
>> On 7/13/22 14:32, Alexander Dahl wrote:
>>> From: Alexander Dahl <ada@thorsis.com>
>>>
>>> After commit 8cca60a2cbf2 ("Kconfig: Remove some symbols from the
>>> whitelist") downstream builds failed for boards setting this in
>>> include/configs/…
>>>
>>> Two FPGA drivers consider this definition.
>>
>> 2?
>> board/astro/mcf5373l/fpga.c
>> drivers/fpga/ACEX1K.c
>> drivers/fpga/virtex2.c
> 
> These are one board and two drivers.  What is your question?

The first one looks like spartan3 fpga driver in board folder (incorrect 
location). It means I consider this as 3 fpga drivers not 2.

But at the end of day just remove that sentence and that's it. Everybody sees 
what you have changed.

Thanks,
Michal

Re: [PATCH v2 1/2] fpga: Convert SYS_FPGA_CHECK_CTRLC to Kconfig

[Alexander Dahl]

[-- Attachment #1: Type: text/plain, Size: 4024 bytes --]

Hello Michal,

On Wed, Jul 13, 2022 at 03:20:44PM +0200, Alexander Dahl wrote:
> Hello Michal,
> 
> On Wed, Jul 13, 2022 at 02:56:08PM +0200, Michal Simek wrote:
> > 
> > 
> > On 7/13/22 14:32, Alexander Dahl wrote:
> > > From: Alexander Dahl <ada@thorsis.com>
> > > 
> > > After commit 8cca60a2cbf2 ("Kconfig: Remove some symbols from the
> > > whitelist") downstream builds failed for boards setting this in
> > > include/configs/…
> > > 
> > > Two FPGA drivers consider this definition.
> > 
> > 2?
> > board/astro/mcf5373l/fpga.c
> > drivers/fpga/ACEX1K.c
> > drivers/fpga/virtex2.c
> 
> These are one board and two drivers.  What is your question?
> 
> Greets
> Alex

I'm sorry, this was misleading.  I added some more comments below and
forgot to remove this line indicating end of message.  Considering
your answer I guess you stopped reading here? 

> 
> > 
> > > 
> > > Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > > ---
> > >   README               | 3 ---
> > >   drivers/fpga/Kconfig | 4 ++++
> > >   2 files changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/README b/README
> > > index ff0df3797d..8c31e5c0e3 100644
> > > --- a/README
> > > +++ b/README
> > > @@ -1346,9 +1346,6 @@ The following options need to be configured:
> > >   		If defined, a function that provides delays in the FPGA
> > >   		configuration driver.
> > > -		CONFIG_SYS_FPGA_CHECK_CTRLC
> > > -		Allow Control-C to interrupt FPGA configuration
> > > -
> > >   		CONFIG_SYS_FPGA_CHECK_ERROR
> > >   		Check for configuration errors during FPGA bitfile
> > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > > index 76719517f5..53d91676e0 100644
> > > --- a/drivers/fpga/Kconfig
> > > +++ b/drivers/fpga/Kconfig
> > > @@ -91,4 +91,8 @@ config FPGA_ZYNQPL
> > >   	  Enable FPGA driver for loading bitstream in BIT and BIN format
> > >   	  on Xilinx Zynq devices.
> > > +config SYS_FPGA_CHECK_CTRLC
> > > +	bool "Allow Control-C to interrupt FPGA configuration"
> > > +	depends on FPGA
> > 
> > Please write help message.
> 
> Okay, I'll have to invent a new message here, if the prompt is not
> self explaining enough.  Since this is not conversion, but adding a
> new message we did not have before, should this go into a separate
> patch?
> 
> > 
> > > +
> > >   endmenu
> > 
> > 
> > And can you please remove this code from drivers/fpga/virtex2.c
> > 
> >  48 /*
> >  49  * Don't allow config cycle to be interrupted
> >  50  */
> >  51 #ifndef CONFIG_SYS_FPGA_CHECK_CTRLC
> >  52 #undef CONFIG_SYS_FPGA_CHECK_CTRLC
> >  53 #endif
> > 
> > it doesn't make any sense.
> 
> I have no hardware to test this and this is out of scope of the
> conversion patch itself.
> 
> > 
> > And with 2/2 please also remove
> > drivers/fpga/spartan2.c:18:#undef CONFIG_SYS_FPGA_PROG_FEEDBACK
> > drivers/fpga/virtex2.c:44:#ifndef CONFIG_SYS_FPGA_PROG_FEEDBACK
> > drivers/fpga/virtex2.c:45:#define CONFIG_SYS_FPGA_PROG_FEEDBACK
> > 
> > Thanks,
> > Michal
> 
> I may be able to add an additional patch or two, but those are all
> FPGAs I have no experience with and I can not test those.  This would
> be more or less guessing based on code reading.  I can try next week,
> not able to do this currently.
> 
> Thanks for your review.

I'll look into all this next week again.

Greets
Alex

> 
> Greets
> Alex
> 
> -- 
> /"\ ASCII RIBBON | »With the first link, the chain is forged. The first
> \ / CAMPAIGN     | speech censured, the first thought forbidden, the
>  X  AGAINST      | first freedom denied, chains us all irrevocably.«
> / \ HTML MAIL    | (Jean-Luc Picard, quoting Judge Aaron Satie)



-- 
/"\ ASCII RIBBON | »With the first link, the chain is forged. The first
\ / CAMPAIGN     | speech censured, the first thought forbidden, the
 X  AGAINST      | first freedom denied, chains us all irrevocably.«
/ \ HTML MAIL    | (Jean-Luc Picard, quoting Judge Aaron Satie)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] fpga: Convert SYS_FPGA_CHECK_CTRLC to Kconfig

[Michal Simek]

On 7/13/22 16:07, Alexander Dahl wrote:
> Hello Michal,
> 
> On Wed, Jul 13, 2022 at 03:20:44PM +0200, Alexander Dahl wrote:
>> Hello Michal,
>>
>> On Wed, Jul 13, 2022 at 02:56:08PM +0200, Michal Simek wrote:
>>>
>>>
>>> On 7/13/22 14:32, Alexander Dahl wrote:
>>>> From: Alexander Dahl <ada@thorsis.com>
>>>>
>>>> After commit 8cca60a2cbf2 ("Kconfig: Remove some symbols from the
>>>> whitelist") downstream builds failed for boards setting this in
>>>> include/configs/…
>>>>
>>>> Two FPGA drivers consider this definition.
>>>
>>> 2?
>>> board/astro/mcf5373l/fpga.c
>>> drivers/fpga/ACEX1K.c
>>> drivers/fpga/virtex2.c
>>
>> These are one board and two drivers.  What is your question?
>>
>> Greets
>> Alex
> 
> I'm sorry, this was misleading.  I added some more comments below and
> forgot to remove this line indicating end of message.  Considering
> your answer I guess you stopped reading here?

yes I did.


>>
>>>
>>>>
>>>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
>>>> ---
>>>>    README               | 3 ---
>>>>    drivers/fpga/Kconfig | 4 ++++
>>>>    2 files changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/README b/README
>>>> index ff0df3797d..8c31e5c0e3 100644
>>>> --- a/README
>>>> +++ b/README
>>>> @@ -1346,9 +1346,6 @@ The following options need to be configured:
>>>>    		If defined, a function that provides delays in the FPGA
>>>>    		configuration driver.
>>>> -		CONFIG_SYS_FPGA_CHECK_CTRLC
>>>> -		Allow Control-C to interrupt FPGA configuration
>>>> -
>>>>    		CONFIG_SYS_FPGA_CHECK_ERROR
>>>>    		Check for configuration errors during FPGA bitfile
>>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>>>> index 76719517f5..53d91676e0 100644
>>>> --- a/drivers/fpga/Kconfig
>>>> +++ b/drivers/fpga/Kconfig
>>>> @@ -91,4 +91,8 @@ config FPGA_ZYNQPL
>>>>    	  Enable FPGA driver for loading bitstream in BIT and BIN format
>>>>    	  on Xilinx Zynq devices.
>>>> +config SYS_FPGA_CHECK_CTRLC
>>>> +	bool "Allow Control-C to interrupt FPGA configuration"
>>>> +	depends on FPGA
>>>
>>> Please write help message.
>>
>> Okay, I'll have to invent a new message here, if the prompt is not
>> self explaining enough.  Since this is not conversion, but adding a
>> new message we did not have before, should this go into a separate
>> patch?

I can't see any reason to have it in separate patch.
And maybe CTRL+C is better then Control-C

>>
>>>
>>>> +
>>>>    endmenu
>>>
>>>
>>> And can you please remove this code from drivers/fpga/virtex2.c
>>>
>>>   48 /*
>>>   49  * Don't allow config cycle to be interrupted
>>>   50  */
>>>   51 #ifndef CONFIG_SYS_FPGA_CHECK_CTRLC
>>>   52 #undef CONFIG_SYS_FPGA_CHECK_CTRLC
>>>   53 #endif
>>>
>>> it doesn't make any sense.
>>
>> I have no hardware to test this and this is out of scope of the
>> conversion patch itself.

likely none has virtex2 around to test it. When you are converting that symbol 
it is good to fix this. Separate patch is fine to get rid of this.


>>>
>>> And with 2/2 please also remove
>>> drivers/fpga/spartan2.c:18:#undef CONFIG_SYS_FPGA_PROG_FEEDBACK
>>> drivers/fpga/virtex2.c:44:#ifndef CONFIG_SYS_FPGA_PROG_FEEDBACK
>>> drivers/fpga/virtex2.c:45:#define CONFIG_SYS_FPGA_PROG_FEEDBACK
>>>
>>> Thanks,
>>> Michal
>>
>> I may be able to add an additional patch or two, but those are all
>> FPGAs I have no experience with and I can not test those.  This would
>> be more or less guessing based on code reading.  I can try next week,
>> not able to do this currently.
>>
>> Thanks for your review.
> 
> I'll look into all this next week again.

None really has spartan2/virtex2 hw around. I personally started with Spartan3 
but didn't power it up for a lot of years. It is just about that there is no 
reason to undefine something if we have Kconfig symbol for it. Just enable it or 
disable it. That's it. No need to test it on any HW.

Thanks,
Michal

Re: [PATCH v2 2/2] fpga: Convert SYS_FPGA_PROG_FEEDBACK to Kconfig

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1991 bytes --]

On Wed, Jul 13, 2022 at 03:11:09PM +0200, Alexander Dahl wrote:
> Hei hei,
> 
> On Wed, Jul 13, 2022 at 02:50:14PM +0200, Michal Simek wrote:
> > 
> > 
> > On 7/13/22 14:33, Alexander Dahl wrote:
> > > From: Alexander Dahl <ada@thorsis.com>
> > 
> > 
> > WARNING: please write a paragraph that describes the config symbol fully
> > 
> > We don't allow patches with empty commit message.
> 
> In general I would agree.  However several of Tom's patches addressing
> kconfig migration have a commit message like this:
> 
>   This converts the following to Kconfig: FOO_BAR
> 
> This is redundant to the subject saying exactly the same.  I can add
> this if you want, but I saw no sense in it.

Yes, the moveconfig.py tool adds that and it's a little redundant
sometimes, but is (a) a starting point for the times when the conversion
required a little extra work or (b) lists all the symbols when you
migrate more than one at a time.

> > > Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > > ---
> > >   README                           | 4 ----
> > >   configs/astro_mcf5373l_defconfig | 1 +
> > >   drivers/fpga/Kconfig             | 6 ++++++
> > >   include/configs/astro_mcf5373l.h | 1 -
> > >   scripts/config_whitelist.txt     | 1 -
> > 
> > Tom can confirm this but IIRC you don't need to remove this from this file.
> > Tom is doing sync up time to time. It is enough to do conversion only.
> 
> This was done by the script ./tools/moveconfig.py where I just hit
> enter with defaults presented.  The other Kconfig patches change
> defconfig, but not whitelist.  Seems a little inconsistent to me.  But
> I can remove that part, sure.

So, I usually run "-yC" with moveconfig.py as that gives the commit
message.  It's not a problem, really, to touch
scripts/config_whitelist.txt it just also can conflict easily.  Omitting
it makes it easier to "git am", fixing up / dropping that hunk if it
conflicts later is also fine.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/2] fpga: Convert SYS_FPGA_CHECK_CTRLC to Kconfig

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2344 bytes --]

On Wed, Jul 13, 2022 at 03:20:46PM +0200, Alexander Dahl wrote:
> Hello Michal,
> 
> On Wed, Jul 13, 2022 at 02:56:08PM +0200, Michal Simek wrote:
[snip]
> > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > > index 76719517f5..53d91676e0 100644
> > > --- a/drivers/fpga/Kconfig
> > > +++ b/drivers/fpga/Kconfig
> > > @@ -91,4 +91,8 @@ config FPGA_ZYNQPL
> > >   	  Enable FPGA driver for loading bitstream in BIT and BIN format
> > >   	  on Xilinx Zynq devices.
> > > +config SYS_FPGA_CHECK_CTRLC
> > > +	bool "Allow Control-C to interrupt FPGA configuration"
> > > +	depends on FPGA
> > 
> > Please write help message.
> 
> Okay, I'll have to invent a new message here, if the prompt is not
> self explaining enough.  Since this is not conversion, but adding a
> new message we did not have before, should this go into a separate
> patch?

If you understand things well enough to add a line or two under "help",
that would be appreciated.  It may be a little redundant soundiing, and
if it's not long enough checkpatch might still complain (but can be
ignored).

> > And can you please remove this code from drivers/fpga/virtex2.c
> > 
> >  48 /*
> >  49  * Don't allow config cycle to be interrupted
> >  50  */
> >  51 #ifndef CONFIG_SYS_FPGA_CHECK_CTRLC
> >  52 #undef CONFIG_SYS_FPGA_CHECK_CTRLC
> >  53 #endif
> > 
> > it doesn't make any sense.
> 
> I have no hardware to test this and this is out of scope of the
> conversion patch itself.

This kind of code logic needs to be enforced in Kconfig instead with
depends lines.  We can make sure it's size-neutral.

> > And with 2/2 please also remove
> > drivers/fpga/spartan2.c:18:#undef CONFIG_SYS_FPGA_PROG_FEEDBACK
> > drivers/fpga/virtex2.c:44:#ifndef CONFIG_SYS_FPGA_PROG_FEEDBACK
> > drivers/fpga/virtex2.c:45:#define CONFIG_SYS_FPGA_PROG_FEEDBACK
> > 
> > Thanks,
> > Michal
> 
> I may be able to add an additional patch or two, but those are all
> FPGAs I have no experience with and I can not test those.  This would
> be more or less guessing based on code reading.  I can try next week,
> not able to do this currently.

Thanks.  It's OK to just check the logic by inspection, one of the tests
I end up running is making sure the code size doesn't change so that'll
catch bad migrations.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]