[U-Boot,v2,3/4] arm: socfpga: stratix10: Add Stratix10 FPGA into FPGA device table

From: "Ang, Chee Hong" <chee.hong.ang@intel.com>

Enable 'fpga' command in u-boot. User will be able to use the fpga
command to program the FPGA on Stratix10 SoC.

Signed-off-by: Ang, Chee Hong <chee.hong.ang@intel.com>
---
 arch/arm/mach-socfpga/misc.c     | 29 +++++++++++++++++++++++++++++
 arch/arm/mach-socfpga/misc_s10.c |  2 ++
 drivers/fpga/altera.c            |  6 ++++++
 include/altera.h                 |  4 ++++
 4 files changed, 41 insertions(+)

On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote:
> From: "Ang, Chee Hong" <chee.hong.ang@intel.com>
> 
> Enable 'fpga' command in u-boot. User will be able to use the fpga
> command to program the FPGA on Stratix10 SoC.
> 
> Signed-off-by: Ang, Chee Hong <chee.hong.ang@intel.com>
> ---
>  arch/arm/mach-socfpga/misc.c     | 29 +++++++++++++++++++++++++++++
>  arch/arm/mach-socfpga/misc_s10.c |  2 ++
>  drivers/fpga/altera.c            |  6 ++++++
>  include/altera.h                 |  4 ++++
>  4 files changed, 41 insertions(+)
> 
> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
> index a4f6d5c..7986b58 100644
> --- a/arch/arm/mach-socfpga/misc.c
> +++ b/arch/arm/mach-socfpga/misc.c
> @@ -88,6 +88,27 @@ int overwrite_console(void)
>  #endif
>  
>  #ifdef CONFIG_FPGA
> +#ifdef CONFIG_FPGA_STRATIX10
> +/*
> + * FPGA programming support for SoC FPGA Stratix 10
> + */
> +static Altera_desc altera_fpga[] = {
> +	{
> +		/* Family */
> +		Intel_FPGA_Stratix10,
> +		/* Interface type */
> +		secure_device_manager_mailbox,
> +		/* No limitation as additional data will be ignored */
> +		-1,
> +		/* No device function table */
> +		NULL,
> +		/* Base interface address specified in driver */
> +		NULL,
> +		/* No cookie implementation */
> +		0
> +	},
> +};
> +#else
>  /*
>   * FPGA programming support for SoC FPGA Cyclone V
>   */
> @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = {
>  		0
>  	},
>  };
> +#endif
>  
>  /* add device descriptor to FPGA device table */
>  void socfpga_fpga_add(void)
> @@ -116,6 +138,13 @@ void socfpga_fpga_add(void)
>  	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
>  		fpga_add(fpga_altera, &altera_fpga[i]);
>  }
> +
> +#else
> +
> +__weak void socfpga_fpga_add(void)
> +{
> +}

Why is a __weak function defined only in else-statement ?

It should be defined always, with a sane default implementation.

On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
> On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote:
> > 
> > From: "Ang, Chee Hong" <chee.hong.ang@intel.com>
> > 
> > Enable 'fpga' command in u-boot. User will be able to use the fpga
> > command to program the FPGA on Stratix10 SoC.
> > 
> > Signed-off-by: Ang, Chee Hong <chee.hong.ang@intel.com>
> > ---
> >  arch/arm/mach-socfpga/misc.c     | 29
> > +++++++++++++++++++++++++++++
> >  arch/arm/mach-socfpga/misc_s10.c |  2 ++
> >  drivers/fpga/altera.c            |  6 ++++++
> >  include/altera.h                 |  4 ++++
> >  4 files changed, 41 insertions(+)
> > 
> > diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-
> > socfpga/misc.c
> > index a4f6d5c..7986b58 100644
> > --- a/arch/arm/mach-socfpga/misc.c
> > +++ b/arch/arm/mach-socfpga/misc.c
> > @@ -88,6 +88,27 @@ int overwrite_console(void)
> >  #endif
> >  
> >  #ifdef CONFIG_FPGA
> > +#ifdef CONFIG_FPGA_STRATIX10
> > +/*
> > + * FPGA programming support for SoC FPGA Stratix 10
> > + */
> > +static Altera_desc altera_fpga[] = {
> > +	{
> > +		/* Family */
> > +		Intel_FPGA_Stratix10,
> > +		/* Interface type */
> > +		secure_device_manager_mailbox,
> > +		/* No limitation as additional data will be
> > ignored */
> > +		-1,
> > +		/* No device function table */
> > +		NULL,
> > +		/* Base interface address specified in driver */
> > +		NULL,
> > +		/* No cookie implementation */
> > +		0
> > +	},
> > +};
> > +#else
> >  /*
> >   * FPGA programming support for SoC FPGA Cyclone V
> >   */
> > @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = {
> >  		0
> >  	},
> >  };
> > +#endif
> >  
> >  /* add device descriptor to FPGA device table */
> >  void socfpga_fpga_add(void)
> > @@ -116,6 +138,13 @@ void socfpga_fpga_add(void)
> >  	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> >  		fpga_add(fpga_altera, &altera_fpga[i]);
> >  }
> > +
> > +#else
> > +
> > +__weak void socfpga_fpga_add(void)
> > +{
> > +}
> Why is a __weak function defined only in else-statement ?
> 
> It should be defined always, with a sane default implementation.

I will remove the empty function in #else-statement and define the
default function like this :

/* add device descriptor to FPGA device table */
void socfpga_fpga_add(void)
{
#ifdef CONFIG_FPGA
	int i;
	fpga_init();
	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
		fpga_add(fpga_altera, &altera_fpga[i]);
#endif
}

Is that OK?

On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
> On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
>> On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote:
>>>
>>> From: "Ang, Chee Hong" <chee.hong.ang@intel.com>
>>>
>>> Enable 'fpga' command in u-boot. User will be able to use the fpga
>>> command to program the FPGA on Stratix10 SoC.
>>>
>>> Signed-off-by: Ang, Chee Hong <chee.hong.ang@intel.com>
>>> ---
>>>  arch/arm/mach-socfpga/misc.c     | 29
>>> +++++++++++++++++++++++++++++
>>>  arch/arm/mach-socfpga/misc_s10.c |  2 ++
>>>  drivers/fpga/altera.c            |  6 ++++++
>>>  include/altera.h                 |  4 ++++
>>>  4 files changed, 41 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-
>>> socfpga/misc.c
>>> index a4f6d5c..7986b58 100644
>>> --- a/arch/arm/mach-socfpga/misc.c
>>> +++ b/arch/arm/mach-socfpga/misc.c
>>> @@ -88,6 +88,27 @@ int overwrite_console(void)
>>>  #endif
>>>  
>>>  #ifdef CONFIG_FPGA
>>> +#ifdef CONFIG_FPGA_STRATIX10
>>> +/*
>>> + * FPGA programming support for SoC FPGA Stratix 10
>>> + */
>>> +static Altera_desc altera_fpga[] = {
>>> +	{
>>> +		/* Family */
>>> +		Intel_FPGA_Stratix10,
>>> +		/* Interface type */
>>> +		secure_device_manager_mailbox,
>>> +		/* No limitation as additional data will be
>>> ignored */
>>> +		-1,
>>> +		/* No device function table */
>>> +		NULL,
>>> +		/* Base interface address specified in driver */
>>> +		NULL,
>>> +		/* No cookie implementation */
>>> +		0
>>> +	},
>>> +};
>>> +#else
>>>  /*
>>>   * FPGA programming support for SoC FPGA Cyclone V
>>>   */
>>> @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = {
>>>  		0
>>>  	},
>>>  };
>>> +#endif
>>>  
>>>  /* add device descriptor to FPGA device table */
>>>  void socfpga_fpga_add(void)
>>> @@ -116,6 +138,13 @@ void socfpga_fpga_add(void)
>>>  	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
>>>  		fpga_add(fpga_altera, &altera_fpga[i]);
>>>  }
>>> +
>>> +#else
>>> +
>>> +__weak void socfpga_fpga_add(void)
>>> +{
>>> +}
>> Why is a __weak function defined only in else-statement ?
>>
>> It should be defined always, with a sane default implementation.
> 
> I will remove the empty function in #else-statement and define the
> default function like this :
> 
> /* add device descriptor to FPGA device table */
> void socfpga_fpga_add(void)
> {
> #ifdef CONFIG_FPGA
> 	int i;
> 	fpga_init();
> 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> 		fpga_add(fpga_altera, &altera_fpga[i]);
> #endif
> }
> 
> Is that OK?

Can't you have __weak empty implementation of socfpga_fpga_add() and
implement a version per platform ? Would that work and make sense ?

btw. the best solution would be to fix this proper and implement a DM/DT
based probing of the FPGA, including a proper driver(s) in drivers/fpga/
instead of putting all the crud into arch/arm/mach-socfpga ...

On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
> On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
> > 
> > On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
> > > 
> > > On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote:
> > > > 
> > > > 
> > > > From: "Ang, Chee Hong" <chee.hong.ang@intel.com>
> > > > 
> > > > Enable 'fpga' command in u-boot. User will be able to use the
> > > > fpga
> > > > command to program the FPGA on Stratix10 SoC.
> > > > 
> > > > Signed-off-by: Ang, Chee Hong <chee.hong.ang@intel.com>
> > > > ---
> > > >  arch/arm/mach-socfpga/misc.c     | 29
> > > > +++++++++++++++++++++++++++++
> > > >  arch/arm/mach-socfpga/misc_s10.c |  2 ++
> > > >  drivers/fpga/altera.c            |  6 ++++++
> > > >  include/altera.h                 |  4 ++++
> > > >  4 files changed, 41 insertions(+)
> > > > 
> > > > diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-
> > > > socfpga/misc.c
> > > > index a4f6d5c..7986b58 100644
> > > > --- a/arch/arm/mach-socfpga/misc.c
> > > > +++ b/arch/arm/mach-socfpga/misc.c
> > > > @@ -88,6 +88,27 @@ int overwrite_console(void)
> > > >  #endif
> > > >  
> > > >  #ifdef CONFIG_FPGA
> > > > +#ifdef CONFIG_FPGA_STRATIX10
> > > > +/*
> > > > + * FPGA programming support for SoC FPGA Stratix 10
> > > > + */
> > > > +static Altera_desc altera_fpga[] = {
> > > > +	{
> > > > +		/* Family */
> > > > +		Intel_FPGA_Stratix10,
> > > > +		/* Interface type */
> > > > +		secure_device_manager_mailbox,
> > > > +		/* No limitation as additional data will be
> > > > ignored */
> > > > +		-1,
> > > > +		/* No device function table */
> > > > +		NULL,
> > > > +		/* Base interface address specified in driver
> > > > */
> > > > +		NULL,
> > > > +		/* No cookie implementation */
> > > > +		0
> > > > +	},
> > > > +};
> > > > +#else
> > > >  /*
> > > >   * FPGA programming support for SoC FPGA Cyclone V
> > > >   */
> > > > @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = {
> > > >  		0
> > > >  	},
> > > >  };
> > > > +#endif
> > > >  
> > > >  /* add device descriptor to FPGA device table */
> > > >  void socfpga_fpga_add(void)
> > > > @@ -116,6 +138,13 @@ void socfpga_fpga_add(void)
> > > >  	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> > > >  		fpga_add(fpga_altera, &altera_fpga[i]);
> > > >  }
> > > > +
> > > > +#else
> > > > +
> > > > +__weak void socfpga_fpga_add(void)
> > > > +{
> > > > +}
> > > Why is a __weak function defined only in else-statement ?
> > > 
> > > It should be defined always, with a sane default implementation.
> > I will remove the empty function in #else-statement and define the
> > default function like this :
> > 
> > /* add device descriptor to FPGA device table */
> > void socfpga_fpga_add(void)
> > {
> > #ifdef CONFIG_FPGA
> > 	int i;
> > 	fpga_init();
> > 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> > 		fpga_add(fpga_altera, &altera_fpga[i]);
> > #endif
> > }
> > 
> > Is that OK?
> Can't you have __weak empty implementation of socfpga_fpga_add() and
> implement a version per platform ? Would that work and make sense ?
socfpga_fpga_add() as shown above is a generic function for adding FPGA
devices to FPGA driver which applies to all our platforms. This is the
reason why it is defined in misc.c instead of misc_<platform_name>.c.

It turned out we already have this defined in misc.h:
#ifdef CONFIG_FPGA
void socfpga_fpga_add(void);
#else
static inline void socfpga_fpga_add(void) {}
#endif

So I don't think I need to make any changes to socfpga_fpga_add() in
misc.c. I just have to remove ifdef CONFIG_FPGA in misc_s10.c because
it was unnecessary. I will submit v3 for this patch and you can comment
further. The v3 patch will be simpler. Thanks.

> 
> btw. the best solution would be to fix this proper and implement a
> DM/DT
> based probing of the FPGA, including a proper driver(s) in
> drivers/fpga/
> instead of putting all the crud into arch/arm/mach-socfpga ...
>

On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
> On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
>> On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
>>>
>>> On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
>>>>
>>>> On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote:
>>>>>
>>>>>
>>>>> From: "Ang, Chee Hong" <chee.hong.ang@intel.com>
>>>>>
>>>>> Enable 'fpga' command in u-boot. User will be able to use the
>>>>> fpga
>>>>> command to program the FPGA on Stratix10 SoC.
>>>>>
>>>>> Signed-off-by: Ang, Chee Hong <chee.hong.ang@intel.com>
>>>>> ---
>>>>>  arch/arm/mach-socfpga/misc.c     | 29
>>>>> +++++++++++++++++++++++++++++
>>>>>  arch/arm/mach-socfpga/misc_s10.c |  2 ++
>>>>>  drivers/fpga/altera.c            |  6 ++++++
>>>>>  include/altera.h                 |  4 ++++
>>>>>  4 files changed, 41 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-
>>>>> socfpga/misc.c
>>>>> index a4f6d5c..7986b58 100644
>>>>> --- a/arch/arm/mach-socfpga/misc.c
>>>>> +++ b/arch/arm/mach-socfpga/misc.c
>>>>> @@ -88,6 +88,27 @@ int overwrite_console(void)
>>>>>  #endif
>>>>>  
>>>>>  #ifdef CONFIG_FPGA
>>>>> +#ifdef CONFIG_FPGA_STRATIX10
>>>>> +/*
>>>>> + * FPGA programming support for SoC FPGA Stratix 10
>>>>> + */
>>>>> +static Altera_desc altera_fpga[] = {
>>>>> +	{
>>>>> +		/* Family */
>>>>> +		Intel_FPGA_Stratix10,
>>>>> +		/* Interface type */
>>>>> +		secure_device_manager_mailbox,
>>>>> +		/* No limitation as additional data will be
>>>>> ignored */
>>>>> +		-1,
>>>>> +		/* No device function table */
>>>>> +		NULL,
>>>>> +		/* Base interface address specified in driver
>>>>> */
>>>>> +		NULL,
>>>>> +		/* No cookie implementation */
>>>>> +		0
>>>>> +	},
>>>>> +};
>>>>> +#else
>>>>>  /*
>>>>>   * FPGA programming support for SoC FPGA Cyclone V
>>>>>   */
>>>>> @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = {
>>>>>  		0
>>>>>  	},
>>>>>  };
>>>>> +#endif
>>>>>  
>>>>>  /* add device descriptor to FPGA device table */
>>>>>  void socfpga_fpga_add(void)
>>>>> @@ -116,6 +138,13 @@ void socfpga_fpga_add(void)
>>>>>  	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
>>>>>  		fpga_add(fpga_altera, &altera_fpga[i]);
>>>>>  }
>>>>> +
>>>>> +#else
>>>>> +
>>>>> +__weak void socfpga_fpga_add(void)
>>>>> +{
>>>>> +}
>>>> Why is a __weak function defined only in else-statement ?
>>>>
>>>> It should be defined always, with a sane default implementation.
>>> I will remove the empty function in #else-statement and define the
>>> default function like this :
>>>
>>> /* add device descriptor to FPGA device table */
>>> void socfpga_fpga_add(void)
>>> {
>>> #ifdef CONFIG_FPGA
>>> 	int i;
>>> 	fpga_init();
>>> 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
>>> 		fpga_add(fpga_altera, &altera_fpga[i]);
>>> #endif
>>> }
>>>
>>> Is that OK?
>> Can't you have __weak empty implementation of socfpga_fpga_add() and
>> implement a version per platform ? Would that work and make sense ?
> socfpga_fpga_add() as shown above is a generic function for adding FPGA
> devices to FPGA driver which applies to all our platforms. This is the
> reason why it is defined in misc.c instead of misc_<platform_name>.c.
> 
> It turned out we already have this defined in misc.h:
> #ifdef CONFIG_FPGA
> void socfpga_fpga_add(void);
> #else
> static inline void socfpga_fpga_add(void) {}
> #endif

Right, if you had one socfpga_fpga_add() per platform + generic empty
one, you could drop that whole thing ^.

> So I don't think I need to make any changes to socfpga_fpga_add() in
> misc.c. I just have to remove ifdef CONFIG_FPGA in misc_s10.c because
> it was unnecessary. I will submit v3 for this patch and you can comment
> further. The v3 patch will be simpler. Thanks.

Please don't submit stuff before the discussion concluded, it's pointless.

>>
>> btw. the best solution would be to fix this proper and implement a
>> DM/DT
>> based probing of the FPGA, including a proper driver(s) in
>> drivers/fpga/
>> instead of putting all the crud into arch/arm/mach-socfpga ...

What do you think about this ^

On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote:
> On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
> > 
> > On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
> > > 
> > > On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
> > > > 
> > > > 
> > > > On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > From: "Ang, Chee Hong" <chee.hong.ang@intel.com>
> > > > > > 
> > > > > > Enable 'fpga' command in u-boot. User will be able to use
> > > > > > the
> > > > > > fpga
> > > > > > command to program the FPGA on Stratix10 SoC.
> > > > > > 
> > > > > > Signed-off-by: Ang, Chee Hong <chee.hong.ang@intel.com>
> > > > > > ---
> > > > > >  arch/arm/mach-socfpga/misc.c     | 29
> > > > > > +++++++++++++++++++++++++++++
> > > > > >  arch/arm/mach-socfpga/misc_s10.c |  2 ++
> > > > > >  drivers/fpga/altera.c            |  6 ++++++
> > > > > >  include/altera.h                 |  4 ++++
> > > > > >  4 files changed, 41 insertions(+)
> > > > > > 
> > > > > > diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-
> > > > > > socfpga/misc.c
> > > > > > index a4f6d5c..7986b58 100644
> > > > > > --- a/arch/arm/mach-socfpga/misc.c
> > > > > > +++ b/arch/arm/mach-socfpga/misc.c
> > > > > > @@ -88,6 +88,27 @@ int overwrite_console(void)
> > > > > >  #endif
> > > > > >  
> > > > > >  #ifdef CONFIG_FPGA
> > > > > > +#ifdef CONFIG_FPGA_STRATIX10
> > > > > > +/*
> > > > > > + * FPGA programming support for SoC FPGA Stratix 10
> > > > > > + */
> > > > > > +static Altera_desc altera_fpga[] = {
> > > > > > +	{
> > > > > > +		/* Family */
> > > > > > +		Intel_FPGA_Stratix10,
> > > > > > +		/* Interface type */
> > > > > > +		secure_device_manager_mailbox,
> > > > > > +		/* No limitation as additional data will
> > > > > > be
> > > > > > ignored */
> > > > > > +		-1,
> > > > > > +		/* No device function table */
> > > > > > +		NULL,
> > > > > > +		/* Base interface address specified in
> > > > > > driver
> > > > > > */
> > > > > > +		NULL,
> > > > > > +		/* No cookie implementation */
> > > > > > +		0
> > > > > > +	},
> > > > > > +};
> > > > > > +#else
> > > > > >  /*
> > > > > >   * FPGA programming support for SoC FPGA Cyclone V
> > > > > >   */
> > > > > > @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = {
> > > > > >  		0
> > > > > >  	},
> > > > > >  };
> > > > > > +#endif
> > > > > >  
> > > > > >  /* add device descriptor to FPGA device table */
> > > > > >  void socfpga_fpga_add(void)
> > > > > > @@ -116,6 +138,13 @@ void socfpga_fpga_add(void)
> > > > > >  	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> > > > > >  		fpga_add(fpga_altera, &altera_fpga[i]);
> > > > > >  }
> > > > > > +
> > > > > > +#else
> > > > > > +
> > > > > > +__weak void socfpga_fpga_add(void)
> > > > > > +{
> > > > > > +}
> > > > > Why is a __weak function defined only in else-statement ?
> > > > > 
> > > > > It should be defined always, with a sane default
> > > > > implementation.
> > > > I will remove the empty function in #else-statement and define
> > > > the
> > > > default function like this :
> > > > 
> > > > /* add device descriptor to FPGA device table */
> > > > void socfpga_fpga_add(void)
> > > > {
> > > > #ifdef CONFIG_FPGA
> > > > 	int i;
> > > > 	fpga_init();
> > > > 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> > > > 		fpga_add(fpga_altera, &altera_fpga[i]);
> > > > #endif
> > > > }
> > > > 
> > > > Is that OK?
> > > Can't you have __weak empty implementation of socfpga_fpga_add()
> > > and
> > > implement a version per platform ? Would that work and make sense
> > > ?
> > socfpga_fpga_add() as shown above is a generic function for adding
> > FPGA
> > devices to FPGA driver which applies to all our platforms. This is
> > the
> > reason why it is defined in misc.c instead of
> > misc_<platform_name>.c.
> > 
> > It turned out we already have this defined in misc.h:
> > #ifdef CONFIG_FPGA
> > void socfpga_fpga_add(void);
> > #else
> > static inline void socfpga_fpga_add(void) {}
> > #endif
> Right, if you had one socfpga_fpga_add() per platform + generic empty
> one, you could drop that whole thing ^.
Yes. It's being addressed in v3 patch:
https://lists.denx.de/pipermail/u-boot/2018-October/343561.html

> 
> > 
> > So I don't think I need to make any changes to socfpga_fpga_add()
> > in
> > misc.c. I just have to remove ifdef CONFIG_FPGA in misc_s10.c
> > because
> > it was unnecessary. I will submit v3 for this patch and you can
> > comment
> > further. The v3 patch will be simpler. Thanks.
> Please don't submit stuff before the discussion concluded, it's
> pointless.
OK.
> 
> > 
> > > 
> > > 
> > > btw. the best solution would be to fix this proper and implement
> > > a
> > > DM/DT
> > > based probing of the FPGA, including a proper driver(s) in
> > > drivers/fpga/
> > > instead of putting all the crud into arch/arm/mach-socfpga ...
> What do you think about this ^
>
I do agree DM/DT is the proper way to implement driver.
But right now those FPGA drivers in drivers/fpga need to be at least
call fpga_add() to add themselves into FPGA device table so that their
callback functions can be invoked correctly when user issue 'fpga
load', 'fpga info' at the command prompt.
So in other words, all drivers in drivers/fpga rely on
drivers/fpga/fpga.c (FPGA core driver) to work.

We already have all our fpga drivers in drivers/fpga :
drivers/fpga/stratix10.c (NEW. In this patchset)
drivers/fpga/stratixII.c (upstreamed)
drivers/fpga/strixv.c (upstreamed)
drivers/fpga/cyclon2.c (upstreamed)
and others...

We only define the FPGA device structure in arch/arm/mach-
socfpga/misc.c and call fpga_add() to add our FPGA device driver into
the global FPGA device table then FPGA core driver will handle the FPGA
operations by invoking the FPGA driver's callback functions.

So for proper DM/DT implementation, drivers/fpga/fpga.c need to be
changed as well because this is the core of the FPGA driver.I think
changing the core of the FPGA driver to support DM/DT would make more
sense than I only change my FPGA driver to extract info from DTB file
into a device structure then specifically call fpga_add() again to add
the device structure to the FPGA core driver.

On 10/10/2018 07:30 AM, Ang, Chee Hong wrote:
> On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote:
>> On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
>>>
>>> On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
>>>>
>>>> On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
>>>>>
>>>>>
>>>>> On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
>>>>>>
>>>>>>
>>>>>> On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> From: "Ang, Chee Hong" <chee.hong.ang@intel.com>
>>>>>>>
>>>>>>> Enable 'fpga' command in u-boot. User will be able to use
>>>>>>> the
>>>>>>> fpga
>>>>>>> command to program the FPGA on Stratix10 SoC.
>>>>>>>
>>>>>>> Signed-off-by: Ang, Chee Hong <chee.hong.ang@intel.com>
>>>>>>> ---
>>>>>>>  arch/arm/mach-socfpga/misc.c     | 29
>>>>>>> +++++++++++++++++++++++++++++
>>>>>>>  arch/arm/mach-socfpga/misc_s10.c |  2 ++
>>>>>>>  drivers/fpga/altera.c            |  6 ++++++
>>>>>>>  include/altera.h                 |  4 ++++
>>>>>>>  4 files changed, 41 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-
>>>>>>> socfpga/misc.c
>>>>>>> index a4f6d5c..7986b58 100644
>>>>>>> --- a/arch/arm/mach-socfpga/misc.c
>>>>>>> +++ b/arch/arm/mach-socfpga/misc.c
>>>>>>> @@ -88,6 +88,27 @@ int overwrite_console(void)
>>>>>>>  #endif
>>>>>>>  
>>>>>>>  #ifdef CONFIG_FPGA
>>>>>>> +#ifdef CONFIG_FPGA_STRATIX10
>>>>>>> +/*
>>>>>>> + * FPGA programming support for SoC FPGA Stratix 10
>>>>>>> + */
>>>>>>> +static Altera_desc altera_fpga[] = {
>>>>>>> +	{
>>>>>>> +		/* Family */
>>>>>>> +		Intel_FPGA_Stratix10,
>>>>>>> +		/* Interface type */
>>>>>>> +		secure_device_manager_mailbox,
>>>>>>> +		/* No limitation as additional data will
>>>>>>> be
>>>>>>> ignored */
>>>>>>> +		-1,
>>>>>>> +		/* No device function table */
>>>>>>> +		NULL,
>>>>>>> +		/* Base interface address specified in
>>>>>>> driver
>>>>>>> */
>>>>>>> +		NULL,
>>>>>>> +		/* No cookie implementation */
>>>>>>> +		0
>>>>>>> +	},
>>>>>>> +};
>>>>>>> +#else
>>>>>>>  /*
>>>>>>>   * FPGA programming support for SoC FPGA Cyclone V
>>>>>>>   */
>>>>>>> @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = {
>>>>>>>  		0
>>>>>>>  	},
>>>>>>>  };
>>>>>>> +#endif
>>>>>>>  
>>>>>>>  /* add device descriptor to FPGA device table */
>>>>>>>  void socfpga_fpga_add(void)
>>>>>>> @@ -116,6 +138,13 @@ void socfpga_fpga_add(void)
>>>>>>>  	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
>>>>>>>  		fpga_add(fpga_altera, &altera_fpga[i]);
>>>>>>>  }
>>>>>>> +
>>>>>>> +#else
>>>>>>> +
>>>>>>> +__weak void socfpga_fpga_add(void)
>>>>>>> +{
>>>>>>> +}
>>>>>> Why is a __weak function defined only in else-statement ?
>>>>>>
>>>>>> It should be defined always, with a sane default
>>>>>> implementation.
>>>>> I will remove the empty function in #else-statement and define
>>>>> the
>>>>> default function like this :
>>>>>
>>>>> /* add device descriptor to FPGA device table */
>>>>> void socfpga_fpga_add(void)
>>>>> {
>>>>> #ifdef CONFIG_FPGA
>>>>> 	int i;
>>>>> 	fpga_init();
>>>>> 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
>>>>> 		fpga_add(fpga_altera, &altera_fpga[i]);
>>>>> #endif
>>>>> }
>>>>>
>>>>> Is that OK?
>>>> Can't you have __weak empty implementation of socfpga_fpga_add()
>>>> and
>>>> implement a version per platform ? Would that work and make sense
>>>> ?
>>> socfpga_fpga_add() as shown above is a generic function for adding
>>> FPGA
>>> devices to FPGA driver which applies to all our platforms. This is
>>> the
>>> reason why it is defined in misc.c instead of
>>> misc_<platform_name>.c.
>>>
>>> It turned out we already have this defined in misc.h:
>>> #ifdef CONFIG_FPGA
>>> void socfpga_fpga_add(void);
>>> #else
>>> static inline void socfpga_fpga_add(void) {}
>>> #endif
>> Right, if you had one socfpga_fpga_add() per platform + generic empty
>> one, you could drop that whole thing ^.
> Yes. It's being addressed in v3 patch:
> https://lists.denx.de/pipermail/u-boot/2018-October/343561.html

So where did the function go in there ? I don't see any __weak anything.

>>>> btw. the best solution would be to fix this proper and implement
>>>> a
>>>> DM/DT
>>>> based probing of the FPGA, including a proper driver(s) in
>>>> drivers/fpga/
>>>> instead of putting all the crud into arch/arm/mach-socfpga ...
>> What do you think about this ^
>>
> I do agree DM/DT is the proper way to implement driver.
> But right now those FPGA drivers in drivers/fpga need to be at least
> call fpga_add() to add themselves into FPGA device table so that their
> callback functions can be invoked correctly when user issue 'fpga
> load', 'fpga info' at the command prompt.
> So in other words, all drivers in drivers/fpga rely on
> drivers/fpga/fpga.c (FPGA core driver) to work.

Well, that should be fixed so that they probe from DT, just like any
other driver. I'm not fond of adding stuff to arch/arm/ ...

> We already have all our fpga drivers in drivers/fpga :
> drivers/fpga/stratix10.c (NEW. In this patchset)
> drivers/fpga/stratixII.c (upstreamed)
> drivers/fpga/strixv.c (upstreamed)
> drivers/fpga/cyclon2.c (upstreamed)
> and others...
> 
> We only define the FPGA device structure in arch/arm/mach-
> socfpga/misc.c and call fpga_add() to add our FPGA device driver into
> the global FPGA device table then FPGA core driver will handle the FPGA
> operations by invoking the FPGA driver's callback functions.

Right, which should be moved to drivers too and which should use DT.

> So for proper DM/DT implementation, drivers/fpga/fpga.c need to be
> changed as well because this is the core of the FPGA driver.I think
> changing the core of the FPGA driver to support DM/DT would make more
> sense than I only change my FPGA driver to extract info from DTB file
> into a device structure then specifically call fpga_add() again to add
> the device structure to the FPGA core driver.

Yes, can you add it to your list once we flesh out this patchset ?

On Wed, 2018-10-10 at 12:27 +0200, Marek Vasut wrote:
> On 10/10/2018 07:30 AM, Ang, Chee Hong wrote:
> > 
> > On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote:
> > > 
> > > On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
> > > > 
> > > > 
> > > > On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > From: "Ang, Chee Hong" <chee.hong.ang@intel.com>
> > > > > > > > 
> > > > > > > > Enable 'fpga' command in u-boot. User will be able to
> > > > > > > > use
> > > > > > > > the
> > > > > > > > fpga
> > > > > > > > command to program the FPGA on Stratix10 SoC.
> > > > > > > > 
> > > > > > > > Signed-off-by: Ang, Chee Hong <chee.hong.ang@intel.com>
> > > > > > > > ---
> > > > > > > >  arch/arm/mach-socfpga/misc.c     | 29
> > > > > > > > +++++++++++++++++++++++++++++
> > > > > > > >  arch/arm/mach-socfpga/misc_s10.c |  2 ++
> > > > > > > >  drivers/fpga/altera.c            |  6 ++++++
> > > > > > > >  include/altera.h                 |  4 ++++
> > > > > > > >  4 files changed, 41 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/arch/arm/mach-socfpga/misc.c
> > > > > > > > b/arch/arm/mach-
> > > > > > > > socfpga/misc.c
> > > > > > > > index a4f6d5c..7986b58 100644
> > > > > > > > --- a/arch/arm/mach-socfpga/misc.c
> > > > > > > > +++ b/arch/arm/mach-socfpga/misc.c
> > > > > > > > @@ -88,6 +88,27 @@ int overwrite_console(void)
> > > > > > > >  #endif
> > > > > > > >  
> > > > > > > >  #ifdef CONFIG_FPGA
> > > > > > > > +#ifdef CONFIG_FPGA_STRATIX10
> > > > > > > > +/*
> > > > > > > > + * FPGA programming support for SoC FPGA Stratix 10
> > > > > > > > + */
> > > > > > > > +static Altera_desc altera_fpga[] = {
> > > > > > > > +	{
> > > > > > > > +		/* Family */
> > > > > > > > +		Intel_FPGA_Stratix10,
> > > > > > > > +		/* Interface type */
> > > > > > > > +		secure_device_manager_mailbox,
> > > > > > > > +		/* No limitation as additional data
> > > > > > > > will
> > > > > > > > be
> > > > > > > > ignored */
> > > > > > > > +		-1,
> > > > > > > > +		/* No device function table */
> > > > > > > > +		NULL,
> > > > > > > > +		/* Base interface address specified in
> > > > > > > > driver
> > > > > > > > */
> > > > > > > > +		NULL,
> > > > > > > > +		/* No cookie implementation */
> > > > > > > > +		0
> > > > > > > > +	},
> > > > > > > > +};
> > > > > > > > +#else
> > > > > > > >  /*
> > > > > > > >   * FPGA programming support for SoC FPGA Cyclone V
> > > > > > > >   */
> > > > > > > > @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] =
> > > > > > > > {
> > > > > > > >  		0
> > > > > > > >  	},
> > > > > > > >  };
> > > > > > > > +#endif
> > > > > > > >  
> > > > > > > >  /* add device descriptor to FPGA device table */
> > > > > > > >  void socfpga_fpga_add(void)
> > > > > > > > @@ -116,6 +138,13 @@ void socfpga_fpga_add(void)
> > > > > > > >  	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> > > > > > > >  		fpga_add(fpga_altera,
> > > > > > > > &altera_fpga[i]);
> > > > > > > >  }
> > > > > > > > +
> > > > > > > > +#else
> > > > > > > > +
> > > > > > > > +__weak void socfpga_fpga_add(void)
> > > > > > > > +{
> > > > > > > > +}
> > > > > > > Why is a __weak function defined only in else-statement ?
> > > > > > > 
> > > > > > > It should be defined always, with a sane default
> > > > > > > implementation.
> > > > > > I will remove the empty function in #else-statement and
> > > > > > define
> > > > > > the
> > > > > > default function like this :
> > > > > > 
> > > > > > /* add device descriptor to FPGA device table */
> > > > > > void socfpga_fpga_add(void)
> > > > > > {
> > > > > > #ifdef CONFIG_FPGA
> > > > > > 	int i;
> > > > > > 	fpga_init();
> > > > > > 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> > > > > > 		fpga_add(fpga_altera, &altera_fpga[i]);
> > > > > > #endif
> > > > > > }
> > > > > > 
> > > > > > Is that OK?
> > > > > Can't you have __weak empty implementation of
> > > > > socfpga_fpga_add()
> > > > > and
> > > > > implement a version per platform ? Would that work and make
> > > > > sense
> > > > > ?
> > > > socfpga_fpga_add() as shown above is a generic function for
> > > > adding
> > > > FPGA
> > > > devices to FPGA driver which applies to all our platforms. This
> > > > is
> > > > the
> > > > reason why it is defined in misc.c instead of
> > > > misc_<platform_name>.c.
> > > > 
> > > > It turned out we already have this defined in misc.h:
> > > > #ifdef CONFIG_FPGA
> > > > void socfpga_fpga_add(void);
> > > > #else
> > > > static inline void socfpga_fpga_add(void) {}
> > > > #endif
> > > Right, if you had one socfpga_fpga_add() per platform + generic
> > > empty
> > > one, you could drop that whole thing ^.
> > Yes. It's being addressed in v3 patch:
> > https://lists.denx.de/pipermail/u-boot/2018-October/343561.html
> So where did the function go in there ? I don't see any __weak
> anything.
I don't have to add anything in my v3 patchsets to make this work.
It's already taken care by arch/arm/mach-socfpga/include/mach/misc.h as
shown below:

#ifdef CONFIG_FPGA
void socfpga_fpga_add(void);
#else
static inline void socfpga_fpga_add(void) {}
#endif

An empty default socfpga_fpga_add() will be defined if CONFIG_FPGA is
not defined.
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > btw. the best solution would be to fix this proper and
> > > > > implement
> > > > > a
> > > > > DM/DT
> > > > > based probing of the FPGA, including a proper driver(s) in
> > > > > drivers/fpga/
> > > > > instead of putting all the crud into arch/arm/mach-socfpga
> > > > > ...
> > > What do you think about this ^
> > > 
> > I do agree DM/DT is the proper way to implement driver.
> > But right now those FPGA drivers in drivers/fpga need to be at
> > least
> > call fpga_add() to add themselves into FPGA device table so that
> > their
> > callback functions can be invoked correctly when user issue 'fpga
> > load', 'fpga info' at the command prompt.
> > So in other words, all drivers in drivers/fpga rely on
> > drivers/fpga/fpga.c (FPGA core driver) to work.
> Well, that should be fixed so that they probe from DT, just like any
> other driver. I'm not fond of adding stuff to arch/arm/ ...
> 
> > 
> > We already have all our fpga drivers in drivers/fpga :
> > drivers/fpga/stratix10.c (NEW. In this patchset)
> > drivers/fpga/stratixII.c (upstreamed)
> > drivers/fpga/strixv.c (upstreamed)
> > drivers/fpga/cyclon2.c (upstreamed)
> > and others...
> > 
> > We only define the FPGA device structure in arch/arm/mach-
> > socfpga/misc.c and call fpga_add() to add our FPGA device driver
> > into
> > the global FPGA device table then FPGA core driver will handle the
> > FPGA
> > operations by invoking the FPGA driver's callback functions.
> Right, which should be moved to drivers too and which should use DT.
> 
> > 
> > So for proper DM/DT implementation, drivers/fpga/fpga.c need to be
> > changed as well because this is the core of the FPGA driver.I think
> > changing the core of the FPGA driver to support DM/DT would make
> > more
> > sense than I only change my FPGA driver to extract info from DTB
> > file
> > into a device structure then specifically call fpga_add() again to
> > add
> > the device structure to the FPGA core driver.
> Yes, can you add it to your list once we flesh out this patchset ?
> 
OK.

On 10/11/2018 08:21 AM, Ang, Chee Hong wrote:
> On Wed, 2018-10-10 at 12:27 +0200, Marek Vasut wrote:
>> On 10/10/2018 07:30 AM, Ang, Chee Hong wrote:
>>>
>>> On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote:
>>>>
>>>> On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
>>>>>
>>>>>
>>>>> On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
>>>>>>
>>>>>>
>>>>>> On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> From: "Ang, Chee Hong" <chee.hong.ang@intel.com>
>>>>>>>>>
>>>>>>>>> Enable 'fpga' command in u-boot. User will be able to
>>>>>>>>> use
>>>>>>>>> the
>>>>>>>>> fpga
>>>>>>>>> command to program the FPGA on Stratix10 SoC.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Ang, Chee Hong <chee.hong.ang@intel.com>
>>>>>>>>> ---
>>>>>>>>>  arch/arm/mach-socfpga/misc.c     | 29
>>>>>>>>> +++++++++++++++++++++++++++++
>>>>>>>>>  arch/arm/mach-socfpga/misc_s10.c |  2 ++
>>>>>>>>>  drivers/fpga/altera.c            |  6 ++++++
>>>>>>>>>  include/altera.h                 |  4 ++++
>>>>>>>>>  4 files changed, 41 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/mach-socfpga/misc.c
>>>>>>>>> b/arch/arm/mach-
>>>>>>>>> socfpga/misc.c
>>>>>>>>> index a4f6d5c..7986b58 100644
>>>>>>>>> --- a/arch/arm/mach-socfpga/misc.c
>>>>>>>>> +++ b/arch/arm/mach-socfpga/misc.c
>>>>>>>>> @@ -88,6 +88,27 @@ int overwrite_console(void)
>>>>>>>>>  #endif
>>>>>>>>>  
>>>>>>>>>  #ifdef CONFIG_FPGA
>>>>>>>>> +#ifdef CONFIG_FPGA_STRATIX10
>>>>>>>>> +/*
>>>>>>>>> + * FPGA programming support for SoC FPGA Stratix 10
>>>>>>>>> + */
>>>>>>>>> +static Altera_desc altera_fpga[] = {
>>>>>>>>> +	{
>>>>>>>>> +		/* Family */
>>>>>>>>> +		Intel_FPGA_Stratix10,
>>>>>>>>> +		/* Interface type */
>>>>>>>>> +		secure_device_manager_mailbox,
>>>>>>>>> +		/* No limitation as additional data
>>>>>>>>> will
>>>>>>>>> be
>>>>>>>>> ignored */
>>>>>>>>> +		-1,
>>>>>>>>> +		/* No device function table */
>>>>>>>>> +		NULL,
>>>>>>>>> +		/* Base interface address specified in
>>>>>>>>> driver
>>>>>>>>> */
>>>>>>>>> +		NULL,
>>>>>>>>> +		/* No cookie implementation */
>>>>>>>>> +		0
>>>>>>>>> +	},
>>>>>>>>> +};
>>>>>>>>> +#else
>>>>>>>>>  /*
>>>>>>>>>   * FPGA programming support for SoC FPGA Cyclone V
>>>>>>>>>   */
>>>>>>>>> @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] =
>>>>>>>>> {
>>>>>>>>>  		0
>>>>>>>>>  	},
>>>>>>>>>  };
>>>>>>>>> +#endif
>>>>>>>>>  
>>>>>>>>>  /* add device descriptor to FPGA device table */
>>>>>>>>>  void socfpga_fpga_add(void)
>>>>>>>>> @@ -116,6 +138,13 @@ void socfpga_fpga_add(void)
>>>>>>>>>  	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
>>>>>>>>>  		fpga_add(fpga_altera,
>>>>>>>>> &altera_fpga[i]);
>>>>>>>>>  }
>>>>>>>>> +
>>>>>>>>> +#else
>>>>>>>>> +
>>>>>>>>> +__weak void socfpga_fpga_add(void)
>>>>>>>>> +{
>>>>>>>>> +}
>>>>>>>> Why is a __weak function defined only in else-statement ?
>>>>>>>>
>>>>>>>> It should be defined always, with a sane default
>>>>>>>> implementation.
>>>>>>> I will remove the empty function in #else-statement and
>>>>>>> define
>>>>>>> the
>>>>>>> default function like this :
>>>>>>>
>>>>>>> /* add device descriptor to FPGA device table */
>>>>>>> void socfpga_fpga_add(void)
>>>>>>> {
>>>>>>> #ifdef CONFIG_FPGA
>>>>>>> 	int i;
>>>>>>> 	fpga_init();
>>>>>>> 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
>>>>>>> 		fpga_add(fpga_altera, &altera_fpga[i]);
>>>>>>> #endif
>>>>>>> }
>>>>>>>
>>>>>>> Is that OK?
>>>>>> Can't you have __weak empty implementation of
>>>>>> socfpga_fpga_add()
>>>>>> and
>>>>>> implement a version per platform ? Would that work and make
>>>>>> sense
>>>>>> ?
>>>>> socfpga_fpga_add() as shown above is a generic function for
>>>>> adding
>>>>> FPGA
>>>>> devices to FPGA driver which applies to all our platforms. This
>>>>> is
>>>>> the
>>>>> reason why it is defined in misc.c instead of
>>>>> misc_<platform_name>.c.
>>>>>
>>>>> It turned out we already have this defined in misc.h:
>>>>> #ifdef CONFIG_FPGA
>>>>> void socfpga_fpga_add(void);
>>>>> #else
>>>>> static inline void socfpga_fpga_add(void) {}
>>>>> #endif
>>>> Right, if you had one socfpga_fpga_add() per platform + generic
>>>> empty
>>>> one, you could drop that whole thing ^.
>>> Yes. It's being addressed in v3 patch:
>>> https://lists.denx.de/pipermail/u-boot/2018-October/343561.html
>> So where did the function go in there ? I don't see any __weak
>> anything.
> I don't have to add anything in my v3 patchsets to make this work.
> It's already taken care by arch/arm/mach-socfpga/include/mach/misc.h as
> shown below:
> 
> #ifdef CONFIG_FPGA
> void socfpga_fpga_add(void);
> #else
> static inline void socfpga_fpga_add(void) {}
> #endif
> 
> An empty default socfpga_fpga_add() will be defined if CONFIG_FPGA is
> not defined.

I was hoping to turn this into __weak function.

>>>>>> btw. the best solution would be to fix this proper and
>>>>>> implement
>>>>>> a
>>>>>> DM/DT
>>>>>> based probing of the FPGA, including a proper driver(s) in
>>>>>> drivers/fpga/
>>>>>> instead of putting all the crud into arch/arm/mach-socfpga
>>>>>> ...
>>>> What do you think about this ^
>>>>
>>> I do agree DM/DT is the proper way to implement driver.
>>> But right now those FPGA drivers in drivers/fpga need to be at
>>> least
>>> call fpga_add() to add themselves into FPGA device table so that
>>> their
>>> callback functions can be invoked correctly when user issue 'fpga
>>> load', 'fpga info' at the command prompt.
>>> So in other words, all drivers in drivers/fpga rely on
>>> drivers/fpga/fpga.c (FPGA core driver) to work.
>> Well, that should be fixed so that they probe from DT, just like any
>> other driver. I'm not fond of adding stuff to arch/arm/ ...
>>
>>>
>>> We already have all our fpga drivers in drivers/fpga :
>>> drivers/fpga/stratix10.c (NEW. In this patchset)
>>> drivers/fpga/stratixII.c (upstreamed)
>>> drivers/fpga/strixv.c (upstreamed)
>>> drivers/fpga/cyclon2.c (upstreamed)
>>> and others...
>>>
>>> We only define the FPGA device structure in arch/arm/mach-
>>> socfpga/misc.c and call fpga_add() to add our FPGA device driver
>>> into
>>> the global FPGA device table then FPGA core driver will handle the
>>> FPGA
>>> operations by invoking the FPGA driver's callback functions.
>> Right, which should be moved to drivers too and which should use DT.
>>
>>>
>>> So for proper DM/DT implementation, drivers/fpga/fpga.c need to be
>>> changed as well because this is the core of the FPGA driver.I think
>>> changing the core of the FPGA driver to support DM/DT would make
>>> more
>>> sense than I only change my FPGA driver to extract info from DTB
>>> file
>>> into a device structure then specifically call fpga_add() again to
>>> add
>>> the device structure to the FPGA core driver.
>> Yes, can you add it to your list once we flesh out this patchset ?
>>
> OK.

Thanks

On Thu, 2018-10-11 at 10:03 +0000, Marek Vasut wrote:
> On 10/11/2018 08:21 AM, Ang, Chee Hong wrote:
> > 
> > On Wed, 2018-10-10 at 12:27 +0200, Marek Vasut wrote:
> > > 
> > > On 10/10/2018 07:30 AM, Ang, Chee Hong wrote:
> > > > 
> > > > 
> > > > On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 10/08/2018 11:48 AM, chee.hong.ang@intel.com
> > > > > > > > > wrote:
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > From: "Ang, Chee Hong" <chee.hong.ang@intel.com>
> > > > > > > > > > 
> > > > > > > > > > Enable 'fpga' command in u-boot. User will be able
> > > > > > > > > > to
> > > > > > > > > > use
> > > > > > > > > > the
> > > > > > > > > > fpga
> > > > > > > > > > command to program the FPGA on Stratix10 SoC.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Ang, Chee Hong <chee.hong.ang@intel.
> > > > > > > > > > com>
> > > > > > > > > > ---
> > > > > > > > > >  arch/arm/mach-socfpga/misc.c     | 29
> > > > > > > > > > +++++++++++++++++++++++++++++
> > > > > > > > > >  arch/arm/mach-socfpga/misc_s10.c |  2 ++
> > > > > > > > > >  drivers/fpga/altera.c            |  6 ++++++
> > > > > > > > > >  include/altera.h                 |  4 ++++
> > > > > > > > > >  4 files changed, 41 insertions(+)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/arch/arm/mach-socfpga/misc.c
> > > > > > > > > > b/arch/arm/mach-
> > > > > > > > > > socfpga/misc.c
> > > > > > > > > > index a4f6d5c..7986b58 100644
> > > > > > > > > > --- a/arch/arm/mach-socfpga/misc.c
> > > > > > > > > > +++ b/arch/arm/mach-socfpga/misc.c
> > > > > > > > > > @@ -88,6 +88,27 @@ int overwrite_console(void)
> > > > > > > > > >  #endif
> > > > > > > > > >  
> > > > > > > > > >  #ifdef CONFIG_FPGA
> > > > > > > > > > +#ifdef CONFIG_FPGA_STRATIX10
> > > > > > > > > > +/*
> > > > > > > > > > + * FPGA programming support for SoC FPGA Stratix
> > > > > > > > > > 10
> > > > > > > > > > + */
> > > > > > > > > > +static Altera_desc altera_fpga[] = {
> > > > > > > > > > +	{
> > > > > > > > > > +		/* Family */
> > > > > > > > > > +		Intel_FPGA_Stratix10,
> > > > > > > > > > +		/* Interface type */
> > > > > > > > > > +		secure_device_manager_mailbox,
> > > > > > > > > > +		/* No limitation as additional
> > > > > > > > > > data
> > > > > > > > > > will
> > > > > > > > > > be
> > > > > > > > > > ignored */
> > > > > > > > > > +		-1,
> > > > > > > > > > +		/* No device function table */
> > > > > > > > > > +		NULL,
> > > > > > > > > > +		/* Base interface address
> > > > > > > > > > specified in
> > > > > > > > > > driver
> > > > > > > > > > */
> > > > > > > > > > +		NULL,
> > > > > > > > > > +		/* No cookie implementation */
> > > > > > > > > > +		0
> > > > > > > > > > +	},
> > > > > > > > > > +};
> > > > > > > > > > +#else
> > > > > > > > > >  /*
> > > > > > > > > >   * FPGA programming support for SoC FPGA Cyclone V
> > > > > > > > > >   */
> > > > > > > > > > @@ -107,6 +128,7 @@ static Altera_desc
> > > > > > > > > > altera_fpga[] =
> > > > > > > > > > {
> > > > > > > > > >  		0
> > > > > > > > > >  	},
> > > > > > > > > >  };
> > > > > > > > > > +#endif
> > > > > > > > > >  
> > > > > > > > > >  /* add device descriptor to FPGA device table */
> > > > > > > > > >  void socfpga_fpga_add(void)
> > > > > > > > > > @@ -116,6 +138,13 @@ void socfpga_fpga_add(void)
> > > > > > > > > >  	for (i = 0; i < ARRAY_SIZE(altera_fpga);
> > > > > > > > > > i++)
> > > > > > > > > >  		fpga_add(fpga_altera,
> > > > > > > > > > &altera_fpga[i]);
> > > > > > > > > >  }
> > > > > > > > > > +
> > > > > > > > > > +#else
> > > > > > > > > > +
> > > > > > > > > > +__weak void socfpga_fpga_add(void)
> > > > > > > > > > +{
> > > > > > > > > > +}
> > > > > > > > > Why is a __weak function defined only in else-
> > > > > > > > > statement ?
> > > > > > > > > 
> > > > > > > > > It should be defined always, with a sane default
> > > > > > > > > implementation.
> > > > > > > > I will remove the empty function in #else-statement and
> > > > > > > > define
> > > > > > > > the
> > > > > > > > default function like this :
> > > > > > > > 
> > > > > > > > /* add device descriptor to FPGA device table */
> > > > > > > > void socfpga_fpga_add(void)
> > > > > > > > {
> > > > > > > > #ifdef CONFIG_FPGA
> > > > > > > > 	int i;
> > > > > > > > 	fpga_init();
> > > > > > > > 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> > > > > > > > 		fpga_add(fpga_altera, &altera_fpga[i]);
> > > > > > > > #endif
> > > > > > > > }
> > > > > > > > 
> > > > > > > > Is that OK?
> > > > > > > Can't you have __weak empty implementation of
> > > > > > > socfpga_fpga_add()
> > > > > > > and
> > > > > > > implement a version per platform ? Would that work and
> > > > > > > make
> > > > > > > sense
> > > > > > > ?
> > > > > > socfpga_fpga_add() as shown above is a generic function for
> > > > > > adding
> > > > > > FPGA
> > > > > > devices to FPGA driver which applies to all our platforms.
> > > > > > This
> > > > > > is
> > > > > > the
> > > > > > reason why it is defined in misc.c instead of
> > > > > > misc_<platform_name>.c.
> > > > > > 
> > > > > > It turned out we already have this defined in misc.h:
> > > > > > #ifdef CONFIG_FPGA
> > > > > > void socfpga_fpga_add(void);
> > > > > > #else
> > > > > > static inline void socfpga_fpga_add(void) {}
> > > > > > #endif
> > > > > Right, if you had one socfpga_fpga_add() per platform +
> > > > > generic
> > > > > empty
> > > > > one, you could drop that whole thing ^.
> > > > Yes. It's being addressed in v3 patch:
> > > > https://lists.denx.de/pipermail/u-boot/2018-October/343561.html
> > > So where did the function go in there ? I don't see any __weak
> > > anything.
> > I don't have to add anything in my v3 patchsets to make this work.
> > It's already taken care by arch/arm/mach-
> > socfpga/include/mach/misc.h as
> > shown below:
> > 
> > #ifdef CONFIG_FPGA
> > void socfpga_fpga_add(void);
> > #else
> > static inline void socfpga_fpga_add(void) {}
> > #endif
> > 
> > An empty default socfpga_fpga_add() will be defined if CONFIG_FPGA
> > is
> > not defined.
> I was hoping to turn this into __weak function.

Below are the new changes for new patch:
Empty weak function in arch/arm/mach-socfpga/misc.c:

/* add device descriptor to FPGA device table */
__weak void socfpga_fpga_add(void)
{
}


In arch/arm/mach-socfpga/misc_aria10.c and arch/arm/mach-
socfpga/misc_gen5.c:

#ifdef CONFIG_FPGA
/*
 * FPGA programming support for SoC FPGA Cyclone V
 */
static Altera_desc altera_fpga[] = {
	{
		/* Family */
		Altera_SoCFPGA,
		/* Interface type */
		fast_passive_parallel,
		/* No limitation as additional data will be ignored */
		-1,
		/* No device function table */
		NULL,
		/* Base interface address specified in driver */
		NULL,
		/* No cookie implementation */
		0
	},
};

/* add device descriptor to FPGA device table */
void socfpga_fpga_add(void)
{
	int i;
	fpga_init();
	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
		fpga_add(fpga_altera, &altera_fpga[i]);
}
#endif


In arch/arm/mach-socfpga/misc_s10.c:

#ifdef CONFIG_FPGA
/*
 * FPGA programming support for SoC FPGA Stratix 10
 */
static Altera_desc altera_fpga[] = {
	{
		/* Family */
		Intel_FPGA_Stratix10,
		/* Interface type */
		secure_device_manager_mailbox,
		/* No limitation as additional data will be ignored */
		-1,
		/* No device function table */
		NULL,
		/* Base interface address specified in driver */
		NULL,
		/* No cookie implementation */
		0
	},
};

/* add device descriptor to FPGA device table */
void socfpga_fpga_add(void)
{
	int i;
	fpga_init();
	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
		fpga_add(fpga_altera, &altera_fpga[i]);
}
#endif

With this new implementation, each platform overrides the
'socfpga_fpga_add' to add its own fpga device. The problem here is
since our aria10 and gen5 are adding same fpga device, there will be
duplication of code for these 2 platforms.
What do you think ?
If you are OK with this implementation, I can submit a new patch for
review again. Thanks.
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > btw. the best solution would be to fix this proper and
> > > > > > > implement
> > > > > > > a
> > > > > > > DM/DT
> > > > > > > based probing of the FPGA, including a proper driver(s)
> > > > > > > in
> > > > > > > drivers/fpga/
> > > > > > > instead of putting all the crud into arch/arm/mach-
> > > > > > > socfpga
> > > > > > > ...
> > > > > What do you think about this ^
> > > > > 
> > > > I do agree DM/DT is the proper way to implement driver.
> > > > But right now those FPGA drivers in drivers/fpga need to be at
> > > > least
> > > > call fpga_add() to add themselves into FPGA device table so
> > > > that
> > > > their
> > > > callback functions can be invoked correctly when user issue
> > > > 'fpga
> > > > load', 'fpga info' at the command prompt.
> > > > So in other words, all drivers in drivers/fpga rely on
> > > > drivers/fpga/fpga.c (FPGA core driver) to work.
> > > Well, that should be fixed so that they probe from DT, just like
> > > any
> > > other driver. I'm not fond of adding stuff to arch/arm/ ...
> > > 
> > > > 
> > > > 
> > > > We already have all our fpga drivers in drivers/fpga :
> > > > drivers/fpga/stratix10.c (NEW. In this patchset)
> > > > drivers/fpga/stratixII.c (upstreamed)
> > > > drivers/fpga/strixv.c (upstreamed)
> > > > drivers/fpga/cyclon2.c (upstreamed)
> > > > and others...
> > > > 
> > > > We only define the FPGA device structure in arch/arm/mach-
> > > > socfpga/misc.c and call fpga_add() to add our FPGA device
> > > > driver
> > > > into
> > > > the global FPGA device table then FPGA core driver will handle
> > > > the
> > > > FPGA
> > > > operations by invoking the FPGA driver's callback functions.
> > > Right, which should be moved to drivers too and which should use
> > > DT.
> > > 
> > > > 
> > > > 
> > > > So for proper DM/DT implementation, drivers/fpga/fpga.c need to
> > > > be
> > > > changed as well because this is the core of the FPGA driver.I
> > > > think
> > > > changing the core of the FPGA driver to support DM/DT would
> > > > make
> > > > more
> > > > sense than I only change my FPGA driver to extract info from
> > > > DTB
> > > > file
> > > > into a device structure then specifically call fpga_add() again
> > > > to
> > > > add
> > > > the device structure to the FPGA core driver.
> > > Yes, can you add it to your list once we flesh out this patchset
> > > ?
> > > 
> > OK.
> Thanks
> 
> -- 
> Best regards,
> Marek Vasut

On 11/14/2018 08:09 AM, Ang, Chee Hong wrote:
> On Thu, 2018-10-11 at 10:03 +0000, Marek Vasut wrote:
>> On 10/11/2018 08:21 AM, Ang, Chee Hong wrote:
>>>
>>> On Wed, 2018-10-10 at 12:27 +0200, Marek Vasut wrote:
>>>>
>>>> On 10/10/2018 07:30 AM, Ang, Chee Hong wrote:
>>>>>
>>>>>
>>>>> On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote:
>>>>>>
>>>>>>
>>>>>> On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 10/08/2018 11:48 AM, chee.hong.ang@intel.com
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> From: "Ang, Chee Hong" <chee.hong.ang@intel.com>
>>>>>>>>>>>
>>>>>>>>>>> Enable 'fpga' command in u-boot. User will be able
>>>>>>>>>>> to
>>>>>>>>>>> use
>>>>>>>>>>> the
>>>>>>>>>>> fpga
>>>>>>>>>>> command to program the FPGA on Stratix10 SoC.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Ang, Chee Hong <chee.hong.ang@intel.
>>>>>>>>>>> com>
>>>>>>>>>>> ---
>>>>>>>>>>>  arch/arm/mach-socfpga/misc.c     | 29
>>>>>>>>>>> +++++++++++++++++++++++++++++
>>>>>>>>>>>  arch/arm/mach-socfpga/misc_s10.c |  2 ++
>>>>>>>>>>>  drivers/fpga/altera.c            |  6 ++++++
>>>>>>>>>>>  include/altera.h                 |  4 ++++
>>>>>>>>>>>  4 files changed, 41 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/arch/arm/mach-socfpga/misc.c
>>>>>>>>>>> b/arch/arm/mach-
>>>>>>>>>>> socfpga/misc.c
>>>>>>>>>>> index a4f6d5c..7986b58 100644
>>>>>>>>>>> --- a/arch/arm/mach-socfpga/misc.c
>>>>>>>>>>> +++ b/arch/arm/mach-socfpga/misc.c
>>>>>>>>>>> @@ -88,6 +88,27 @@ int overwrite_console(void)
>>>>>>>>>>>  #endif
>>>>>>>>>>>  
>>>>>>>>>>>  #ifdef CONFIG_FPGA
>>>>>>>>>>> +#ifdef CONFIG_FPGA_STRATIX10
>>>>>>>>>>> +/*
>>>>>>>>>>> + * FPGA programming support for SoC FPGA Stratix
>>>>>>>>>>> 10
>>>>>>>>>>> + */
>>>>>>>>>>> +static Altera_desc altera_fpga[] = {
>>>>>>>>>>> +	{
>>>>>>>>>>> +		/* Family */
>>>>>>>>>>> +		Intel_FPGA_Stratix10,
>>>>>>>>>>> +		/* Interface type */
>>>>>>>>>>> +		secure_device_manager_mailbox,
>>>>>>>>>>> +		/* No limitation as additional
>>>>>>>>>>> data
>>>>>>>>>>> will
>>>>>>>>>>> be
>>>>>>>>>>> ignored */
>>>>>>>>>>> +		-1,
>>>>>>>>>>> +		/* No device function table */
>>>>>>>>>>> +		NULL,
>>>>>>>>>>> +		/* Base interface address
>>>>>>>>>>> specified in
>>>>>>>>>>> driver
>>>>>>>>>>> */
>>>>>>>>>>> +		NULL,
>>>>>>>>>>> +		/* No cookie implementation */
>>>>>>>>>>> +		0
>>>>>>>>>>> +	},
>>>>>>>>>>> +};
>>>>>>>>>>> +#else
>>>>>>>>>>>  /*
>>>>>>>>>>>   * FPGA programming support for SoC FPGA Cyclone V
>>>>>>>>>>>   */
>>>>>>>>>>> @@ -107,6 +128,7 @@ static Altera_desc
>>>>>>>>>>> altera_fpga[] =
>>>>>>>>>>> {
>>>>>>>>>>>  		0
>>>>>>>>>>>  	},
>>>>>>>>>>>  };
>>>>>>>>>>> +#endif
>>>>>>>>>>>  
>>>>>>>>>>>  /* add device descriptor to FPGA device table */
>>>>>>>>>>>  void socfpga_fpga_add(void)
>>>>>>>>>>> @@ -116,6 +138,13 @@ void socfpga_fpga_add(void)
>>>>>>>>>>>  	for (i = 0; i < ARRAY_SIZE(altera_fpga);
>>>>>>>>>>> i++)
>>>>>>>>>>>  		fpga_add(fpga_altera,
>>>>>>>>>>> &altera_fpga[i]);
>>>>>>>>>>>  }
>>>>>>>>>>> +
>>>>>>>>>>> +#else
>>>>>>>>>>> +
>>>>>>>>>>> +__weak void socfpga_fpga_add(void)
>>>>>>>>>>> +{
>>>>>>>>>>> +}
>>>>>>>>>> Why is a __weak function defined only in else-
>>>>>>>>>> statement ?
>>>>>>>>>>
>>>>>>>>>> It should be defined always, with a sane default
>>>>>>>>>> implementation.
>>>>>>>>> I will remove the empty function in #else-statement and
>>>>>>>>> define
>>>>>>>>> the
>>>>>>>>> default function like this :
>>>>>>>>>
>>>>>>>>> /* add device descriptor to FPGA device table */
>>>>>>>>> void socfpga_fpga_add(void)
>>>>>>>>> {
>>>>>>>>> #ifdef CONFIG_FPGA
>>>>>>>>> 	int i;
>>>>>>>>> 	fpga_init();
>>>>>>>>> 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
>>>>>>>>> 		fpga_add(fpga_altera, &altera_fpga[i]);
>>>>>>>>> #endif
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> Is that OK?
>>>>>>>> Can't you have __weak empty implementation of
>>>>>>>> socfpga_fpga_add()
>>>>>>>> and
>>>>>>>> implement a version per platform ? Would that work and
>>>>>>>> make
>>>>>>>> sense
>>>>>>>> ?
>>>>>>> socfpga_fpga_add() as shown above is a generic function for
>>>>>>> adding
>>>>>>> FPGA
>>>>>>> devices to FPGA driver which applies to all our platforms.
>>>>>>> This
>>>>>>> is
>>>>>>> the
>>>>>>> reason why it is defined in misc.c instead of
>>>>>>> misc_<platform_name>.c.
>>>>>>>
>>>>>>> It turned out we already have this defined in misc.h:
>>>>>>> #ifdef CONFIG_FPGA
>>>>>>> void socfpga_fpga_add(void);
>>>>>>> #else
>>>>>>> static inline void socfpga_fpga_add(void) {}
>>>>>>> #endif
>>>>>> Right, if you had one socfpga_fpga_add() per platform +
>>>>>> generic
>>>>>> empty
>>>>>> one, you could drop that whole thing ^.
>>>>> Yes. It's being addressed in v3 patch:
>>>>> https://lists.denx.de/pipermail/u-boot/2018-October/343561.html
>>>> So where did the function go in there ? I don't see any __weak
>>>> anything.
>>> I don't have to add anything in my v3 patchsets to make this work.
>>> It's already taken care by arch/arm/mach-
>>> socfpga/include/mach/misc.h as
>>> shown below:
>>>
>>> #ifdef CONFIG_FPGA
>>> void socfpga_fpga_add(void);
>>> #else
>>> static inline void socfpga_fpga_add(void) {}
>>> #endif
>>>
>>> An empty default socfpga_fpga_add() will be defined if CONFIG_FPGA
>>> is
>>> not defined.
>> I was hoping to turn this into __weak function.
> 
> Below are the new changes for new patch:
> Empty weak function in arch/arm/mach-socfpga/misc.c:
> 
> /* add device descriptor to FPGA device table */
> __weak void socfpga_fpga_add(void)
> {
> }
> 
> 
> In arch/arm/mach-socfpga/misc_aria10.c and arch/arm/mach-
> socfpga/misc_gen5.c:
> 
> #ifdef CONFIG_FPGA
> /*
>  * FPGA programming support for SoC FPGA Cyclone V
>  */
> static Altera_desc altera_fpga[] = {
> 	{
> 		/* Family */
> 		Altera_SoCFPGA,
> 		/* Interface type */
> 		fast_passive_parallel,
> 		/* No limitation as additional data will be ignored */
> 		-1,
> 		/* No device function table */
> 		NULL,
> 		/* Base interface address specified in driver */
> 		NULL,
> 		/* No cookie implementation */
> 		0
> 	},
> };
> 
> /* add device descriptor to FPGA device table */
> void socfpga_fpga_add(void)
> {
> 	int i;
> 	fpga_init();
> 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> 		fpga_add(fpga_altera, &altera_fpga[i]);
> }
> #endif
> 
> 
> In arch/arm/mach-socfpga/misc_s10.c:
> 
> #ifdef CONFIG_FPGA
> /*
>  * FPGA programming support for SoC FPGA Stratix 10
>  */
> static Altera_desc altera_fpga[] = {
> 	{
> 		/* Family */
> 		Intel_FPGA_Stratix10,
> 		/* Interface type */
> 		secure_device_manager_mailbox,
> 		/* No limitation as additional data will be ignored */
> 		-1,
> 		/* No device function table */
> 		NULL,
> 		/* Base interface address specified in driver */
> 		NULL,
> 		/* No cookie implementation */
> 		0
> 	},
> };
> 
> /* add device descriptor to FPGA device table */
> void socfpga_fpga_add(void)
> {
> 	int i;
> 	fpga_init();
> 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> 		fpga_add(fpga_altera, &altera_fpga[i]);
> }
> #endif
> 
> With this new implementation, each platform overrides the
> 'socfpga_fpga_add' to add its own fpga device. The problem here is
> since our aria10 and gen5 are adding same fpga device, there will be
> duplication of code for these 2 platforms.
> What do you think ?

I think you can create a common code for Gen5 somehow, right ?

> If you are OK with this implementation, I can submit a new patch for
> review again. Thanks.

Submit the patches, yes.

On Wed, 2018-11-14 at 12:52 +0100, Marek Vasut wrote:
> On 11/14/2018 08:09 AM, Ang, Chee Hong wrote:
> > 
> > On Thu, 2018-10-11 at 10:03 +0000, Marek Vasut wrote:
> > > 
> > > On 10/11/2018 08:21 AM, Ang, Chee Hong wrote:
> > > > 
> > > > 
> > > > On Wed, 2018-10-10 at 12:27 +0200, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 10/10/2018 07:30 AM, Ang, Chee Hong wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut
> > > > > > > > > > wrote:
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > On 10/08/2018 11:48 AM, chee.hong.ang@intel.com
> > > > > > > > > > > wrote:
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > From: "Ang, Chee Hong" <chee.hong.ang@intel.com
> > > > > > > > > > > > >
> > > > > > > > > > > > 
> > > > > > > > > > > > Enable 'fpga' command in u-boot. User will be
> > > > > > > > > > > > able
> > > > > > > > > > > > to
> > > > > > > > > > > > use
> > > > > > > > > > > > the
> > > > > > > > > > > > fpga
> > > > > > > > > > > > command to program the FPGA on Stratix10 SoC.
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Ang, Chee Hong <chee.hong.ang@in
> > > > > > > > > > > > tel.
> > > > > > > > > > > > com>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  arch/arm/mach-socfpga/misc.c     | 29
> > > > > > > > > > > > +++++++++++++++++++++++++++++
> > > > > > > > > > > >  arch/arm/mach-socfpga/misc_s10.c |  2 ++
> > > > > > > > > > > >  drivers/fpga/altera.c            |  6 ++++++
> > > > > > > > > > > >  include/altera.h                 |  4 ++++
> > > > > > > > > > > >  4 files changed, 41 insertions(+)
> > > > > > > > > > > > 
> > > > > > > > > > > > diff --git a/arch/arm/mach-socfpga/misc.c
> > > > > > > > > > > > b/arch/arm/mach-
> > > > > > > > > > > > socfpga/misc.c
> > > > > > > > > > > > index a4f6d5c..7986b58 100644
> > > > > > > > > > > > --- a/arch/arm/mach-socfpga/misc.c
> > > > > > > > > > > > +++ b/arch/arm/mach-socfpga/misc.c
> > > > > > > > > > > > @@ -88,6 +88,27 @@ int overwrite_console(void)
> > > > > > > > > > > >  #endif
> > > > > > > > > > > >  
> > > > > > > > > > > >  #ifdef CONFIG_FPGA
> > > > > > > > > > > > +#ifdef CONFIG_FPGA_STRATIX10
> > > > > > > > > > > > +/*
> > > > > > > > > > > > + * FPGA programming support for SoC FPGA
> > > > > > > > > > > > Stratix
> > > > > > > > > > > > 10
> > > > > > > > > > > > + */
> > > > > > > > > > > > +static Altera_desc altera_fpga[] = {
> > > > > > > > > > > > +	{
> > > > > > > > > > > > +		/* Family */
> > > > > > > > > > > > +		Intel_FPGA_Stratix10,
> > > > > > > > > > > > +		/* Interface type */
> > > > > > > > > > > > +		secure_device_manager_mailbox,
> > > > > > > > > > > > +		/* No limitation as additional
> > > > > > > > > > > > data
> > > > > > > > > > > > will
> > > > > > > > > > > > be
> > > > > > > > > > > > ignored */
> > > > > > > > > > > > +		-1,
> > > > > > > > > > > > +		/* No device function table */
> > > > > > > > > > > > +		NULL,
> > > > > > > > > > > > +		/* Base interface address
> > > > > > > > > > > > specified in
> > > > > > > > > > > > driver
> > > > > > > > > > > > */
> > > > > > > > > > > > +		NULL,
> > > > > > > > > > > > +		/* No cookie implementation */
> > > > > > > > > > > > +		0
> > > > > > > > > > > > +	},
> > > > > > > > > > > > +};
> > > > > > > > > > > > +#else
> > > > > > > > > > > >  /*
> > > > > > > > > > > >   * FPGA programming support for SoC FPGA
> > > > > > > > > > > > Cyclone V
> > > > > > > > > > > >   */
> > > > > > > > > > > > @@ -107,6 +128,7 @@ static Altera_desc
> > > > > > > > > > > > altera_fpga[] =
> > > > > > > > > > > > {
> > > > > > > > > > > >  		0
> > > > > > > > > > > >  	},
> > > > > > > > > > > >  };
> > > > > > > > > > > > +#endif
> > > > > > > > > > > >  
> > > > > > > > > > > >  /* add device descriptor to FPGA device table
> > > > > > > > > > > > */
> > > > > > > > > > > >  void socfpga_fpga_add(void)
> > > > > > > > > > > > @@ -116,6 +138,13 @@ void
> > > > > > > > > > > > socfpga_fpga_add(void)
> > > > > > > > > > > >  	for (i = 0; i <
> > > > > > > > > > > > ARRAY_SIZE(altera_fpga);
> > > > > > > > > > > > i++)
> > > > > > > > > > > >  		fpga_add(fpga_altera,
> > > > > > > > > > > > &altera_fpga[i]);
> > > > > > > > > > > >  }
> > > > > > > > > > > > +
> > > > > > > > > > > > +#else
> > > > > > > > > > > > +
> > > > > > > > > > > > +__weak void socfpga_fpga_add(void)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +}
> > > > > > > > > > > Why is a __weak function defined only in else-
> > > > > > > > > > > statement ?
> > > > > > > > > > > 
> > > > > > > > > > > It should be defined always, with a sane default
> > > > > > > > > > > implementation.
> > > > > > > > > > I will remove the empty function in #else-statement 
> > > > > > > > > > and
> > > > > > > > > > define
> > > > > > > > > > the
> > > > > > > > > > default function like this :
> > > > > > > > > > 
> > > > > > > > > > /* add device descriptor to FPGA device table */
> > > > > > > > > > void socfpga_fpga_add(void)
> > > > > > > > > > {
> > > > > > > > > > #ifdef CONFIG_FPGA
> > > > > > > > > > 	int i;
> > > > > > > > > > 	fpga_init();
> > > > > > > > > > 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> > > > > > > > > > 		fpga_add(fpga_altera, &altera_fpga[i]);
> > > > > > > > > > #endif
> > > > > > > > > > }
> > > > > > > > > > 
> > > > > > > > > > Is that OK?
> > > > > > > > > Can't you have __weak empty implementation of
> > > > > > > > > socfpga_fpga_add()
> > > > > > > > > and
> > > > > > > > > implement a version per platform ? Would that work
> > > > > > > > > and
> > > > > > > > > make
> > > > > > > > > sense
> > > > > > > > > ?
> > > > > > > > socfpga_fpga_add() as shown above is a generic function
> > > > > > > > for
> > > > > > > > adding
> > > > > > > > FPGA
> > > > > > > > devices to FPGA driver which applies to all our
> > > > > > > > platforms.
> > > > > > > > This
> > > > > > > > is
> > > > > > > > the
> > > > > > > > reason why it is defined in misc.c instead of
> > > > > > > > misc_<platform_name>.c.
> > > > > > > > 
> > > > > > > > It turned out we already have this defined in misc.h:
> > > > > > > > #ifdef CONFIG_FPGA
> > > > > > > > void socfpga_fpga_add(void);
> > > > > > > > #else
> > > > > > > > static inline void socfpga_fpga_add(void) {}
> > > > > > > > #endif
> > > > > > > Right, if you had one socfpga_fpga_add() per platform +
> > > > > > > generic
> > > > > > > empty
> > > > > > > one, you could drop that whole thing ^.
> > > > > > Yes. It's being addressed in v3 patch:
> > > > > > https://lists.denx.de/pipermail/u-boot/2018-October/343561.
> > > > > > html
> > > > > So where did the function go in there ? I don't see any
> > > > > __weak
> > > > > anything.
> > > > I don't have to add anything in my v3 patchsets to make this
> > > > work.
> > > > It's already taken care by arch/arm/mach-
> > > > socfpga/include/mach/misc.h as
> > > > shown below:
> > > > 
> > > > #ifdef CONFIG_FPGA
> > > > void socfpga_fpga_add(void);
> > > > #else
> > > > static inline void socfpga_fpga_add(void) {}
> > > > #endif
> > > > 
> > > > An empty default socfpga_fpga_add() will be defined if
> > > > CONFIG_FPGA
> > > > is
> > > > not defined.
> > > I was hoping to turn this into __weak function.
> > Below are the new changes for new patch:
> > Empty weak function in arch/arm/mach-socfpga/misc.c:
> > 
> > /* add device descriptor to FPGA device table */
> > __weak void socfpga_fpga_add(void)
> > {
> > }
> > 
> > 
> > In arch/arm/mach-socfpga/misc_aria10.c and arch/arm/mach-
> > socfpga/misc_gen5.c:
> > 
> > #ifdef CONFIG_FPGA
> > /*
> >  * FPGA programming support for SoC FPGA Cyclone V
> >  */
> > static Altera_desc altera_fpga[] = {
> > 	{
> > 		/* Family */
> > 		Altera_SoCFPGA,
> > 		/* Interface type */
> > 		fast_passive_parallel,
> > 		/* No limitation as additional data will be ignored */
> > 		-1,
> > 		/* No device function table */
> > 		NULL,
> > 		/* Base interface address specified in driver */
> > 		NULL,
> > 		/* No cookie implementation */
> > 		0
> > 	},
> > };
> > 
> > /* add device descriptor to FPGA device table */
> > void socfpga_fpga_add(void)
> > {
> > 	int i;
> > 	fpga_init();
> > 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> > 		fpga_add(fpga_altera, &altera_fpga[i]);
> > }
> > #endif
> > 
> > 
> > In arch/arm/mach-socfpga/misc_s10.c:
> > 
> > #ifdef CONFIG_FPGA
> > /*
> >  * FPGA programming support for SoC FPGA Stratix 10
> >  */
> > static Altera_desc altera_fpga[] = {
> > 	{
> > 		/* Family */
> > 		Intel_FPGA_Stratix10,
> > 		/* Interface type */
> > 		secure_device_manager_mailbox,
> > 		/* No limitation as additional data will be ignored */
> > 		-1,
> > 		/* No device function table */
> > 		NULL,
> > 		/* Base interface address specified in driver */
> > 		NULL,
> > 		/* No cookie implementation */
> > 		0
> > 	},
> > };
> > 
> > /* add device descriptor to FPGA device table */
> > void socfpga_fpga_add(void)
> > {
> > 	int i;
> > 	fpga_init();
> > 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> > 		fpga_add(fpga_altera, &altera_fpga[i]);
> > }
> > #endif
> > 
> > With this new implementation, each platform overrides the
> > 'socfpga_fpga_add' to add its own fpga device. The problem here is
> > since our aria10 and gen5 are adding same fpga device, there will
> > be
> > duplication of code for these 2 platforms.
> > What do you think ?
> I think you can create a common code for Gen5 somehow, right ?
I think I will add a new file arch/arm/mach-socfpga/fpga_devices.c and
put the common code in it so that different platforms can share the
common implementation which override the weak 'socfpga_fpga_add'
function. The new file will have the following code:

#ifdef CONFIG_FPGA_STRATIX10
/*
 * FPGA programming support for SoC FPGA Stratix 10
 */
static Altera_desc altera_fpga[] = {
	{
		/* Family */
		Intel_FPGA_Stratix10,
		/* Interface type */
		secure_device_manager_mailbox,
		/* No limitation as additional data will be ignored */
		-1,
		/* No device function table */
		NULL,
		/* Base interface address specified in driver */
		NULL,
		/* No cookie implementation */
		0
	},
};
#else
/*
 * FPGA programming support for SoC FPGA Cyclone V
 */
static Altera_desc altera_fpga[] = {
	{
		/* Family */
		Altera_SoCFPGA,
		/* Interface type */
		fast_passive_parallel,
		/* No limitation as additional data will be ignored */
		-1,
		/* No device function table */
		NULL,
		/* Base interface address specified in driver */
		NULL,
		/* No cookie implementation */
		0
	},
};
#endif

/* add device descriptor to FPGA device table */
void socfpga_fpga_add(void)
{
	int i;
	fpga_init();
	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
		fpga_add(fpga_altera, &altera_fpga[i]);
}

> 
> > 
> > If you are OK with this implementation, I can submit a new patch
> > for
> > review again. Thanks.
> Submit the patches, yes.
>

On 11/15/2018 08:13 AM, Ang, Chee Hong wrote:
> On Wed, 2018-11-14 at 12:52 +0100, Marek Vasut wrote:
>> On 11/14/2018 08:09 AM, Ang, Chee Hong wrote:
>>>
>>> On Thu, 2018-10-11 at 10:03 +0000, Marek Vasut wrote:
>>>>
>>>> On 10/11/2018 08:21 AM, Ang, Chee Hong wrote:
>>>>>
>>>>>
>>>>> On Wed, 2018-10-10 at 12:27 +0200, Marek Vasut wrote:
>>>>>>
>>>>>>
>>>>>> On 10/10/2018 07:30 AM, Ang, Chee Hong wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 10/08/2018 11:48 AM, chee.hong.ang@intel.com
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> From: "Ang, Chee Hong" <chee.hong.ang@intel.com
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Enable 'fpga' command in u-boot. User will be
>>>>>>>>>>>>> able
>>>>>>>>>>>>> to
>>>>>>>>>>>>> use
>>>>>>>>>>>>> the
>>>>>>>>>>>>> fpga
>>>>>>>>>>>>> command to program the FPGA on Stratix10 SoC.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Ang, Chee Hong <chee.hong.ang@in
>>>>>>>>>>>>> tel.
>>>>>>>>>>>>> com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>  arch/arm/mach-socfpga/misc.c     | 29
>>>>>>>>>>>>> +++++++++++++++++++++++++++++
>>>>>>>>>>>>>  arch/arm/mach-socfpga/misc_s10.c |  2 ++
>>>>>>>>>>>>>  drivers/fpga/altera.c            |  6 ++++++
>>>>>>>>>>>>>  include/altera.h                 |  4 ++++
>>>>>>>>>>>>>  4 files changed, 41 insertions(+)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/arch/arm/mach-socfpga/misc.c
>>>>>>>>>>>>> b/arch/arm/mach-
>>>>>>>>>>>>> socfpga/misc.c
>>>>>>>>>>>>> index a4f6d5c..7986b58 100644
>>>>>>>>>>>>> --- a/arch/arm/mach-socfpga/misc.c
>>>>>>>>>>>>> +++ b/arch/arm/mach-socfpga/misc.c
>>>>>>>>>>>>> @@ -88,6 +88,27 @@ int overwrite_console(void)
>>>>>>>>>>>>>  #endif
>>>>>>>>>>>>>  
>>>>>>>>>>>>>  #ifdef CONFIG_FPGA
>>>>>>>>>>>>> +#ifdef CONFIG_FPGA_STRATIX10
>>>>>>>>>>>>> +/*
>>>>>>>>>>>>> + * FPGA programming support for SoC FPGA
>>>>>>>>>>>>> Stratix
>>>>>>>>>>>>> 10
>>>>>>>>>>>>> + */
>>>>>>>>>>>>> +static Altera_desc altera_fpga[] = {
>>>>>>>>>>>>> +	{
>>>>>>>>>>>>> +		/* Family */
>>>>>>>>>>>>> +		Intel_FPGA_Stratix10,
>>>>>>>>>>>>> +		/* Interface type */
>>>>>>>>>>>>> +		secure_device_manager_mailbox,
>>>>>>>>>>>>> +		/* No limitation as additional
>>>>>>>>>>>>> data
>>>>>>>>>>>>> will
>>>>>>>>>>>>> be
>>>>>>>>>>>>> ignored */
>>>>>>>>>>>>> +		-1,
>>>>>>>>>>>>> +		/* No device function table */
>>>>>>>>>>>>> +		NULL,
>>>>>>>>>>>>> +		/* Base interface address
>>>>>>>>>>>>> specified in
>>>>>>>>>>>>> driver
>>>>>>>>>>>>> */
>>>>>>>>>>>>> +		NULL,
>>>>>>>>>>>>> +		/* No cookie implementation */
>>>>>>>>>>>>> +		0
>>>>>>>>>>>>> +	},
>>>>>>>>>>>>> +};
>>>>>>>>>>>>> +#else
>>>>>>>>>>>>>  /*
>>>>>>>>>>>>>   * FPGA programming support for SoC FPGA
>>>>>>>>>>>>> Cyclone V
>>>>>>>>>>>>>   */
>>>>>>>>>>>>> @@ -107,6 +128,7 @@ static Altera_desc
>>>>>>>>>>>>> altera_fpga[] =
>>>>>>>>>>>>> {
>>>>>>>>>>>>>  		0
>>>>>>>>>>>>>  	},
>>>>>>>>>>>>>  };
>>>>>>>>>>>>> +#endif
>>>>>>>>>>>>>  
>>>>>>>>>>>>>  /* add device descriptor to FPGA device table
>>>>>>>>>>>>> */
>>>>>>>>>>>>>  void socfpga_fpga_add(void)
>>>>>>>>>>>>> @@ -116,6 +138,13 @@ void
>>>>>>>>>>>>> socfpga_fpga_add(void)
>>>>>>>>>>>>>  	for (i = 0; i <
>>>>>>>>>>>>> ARRAY_SIZE(altera_fpga);
>>>>>>>>>>>>> i++)
>>>>>>>>>>>>>  		fpga_add(fpga_altera,
>>>>>>>>>>>>> &altera_fpga[i]);
>>>>>>>>>>>>>  }
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +#else
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +__weak void socfpga_fpga_add(void)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> +}
>>>>>>>>>>>> Why is a __weak function defined only in else-
>>>>>>>>>>>> statement ?
>>>>>>>>>>>>
>>>>>>>>>>>> It should be defined always, with a sane default
>>>>>>>>>>>> implementation.
>>>>>>>>>>> I will remove the empty function in #else-statement 
>>>>>>>>>>> and
>>>>>>>>>>> define
>>>>>>>>>>> the
>>>>>>>>>>> default function like this :
>>>>>>>>>>>
>>>>>>>>>>> /* add device descriptor to FPGA device table */
>>>>>>>>>>> void socfpga_fpga_add(void)
>>>>>>>>>>> {
>>>>>>>>>>> #ifdef CONFIG_FPGA
>>>>>>>>>>> 	int i;
>>>>>>>>>>> 	fpga_init();
>>>>>>>>>>> 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
>>>>>>>>>>> 		fpga_add(fpga_altera, &altera_fpga[i]);
>>>>>>>>>>> #endif
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> Is that OK?
>>>>>>>>>> Can't you have __weak empty implementation of
>>>>>>>>>> socfpga_fpga_add()
>>>>>>>>>> and
>>>>>>>>>> implement a version per platform ? Would that work
>>>>>>>>>> and
>>>>>>>>>> make
>>>>>>>>>> sense
>>>>>>>>>> ?
>>>>>>>>> socfpga_fpga_add() as shown above is a generic function
>>>>>>>>> for
>>>>>>>>> adding
>>>>>>>>> FPGA
>>>>>>>>> devices to FPGA driver which applies to all our
>>>>>>>>> platforms.
>>>>>>>>> This
>>>>>>>>> is
>>>>>>>>> the
>>>>>>>>> reason why it is defined in misc.c instead of
>>>>>>>>> misc_<platform_name>.c.
>>>>>>>>>
>>>>>>>>> It turned out we already have this defined in misc.h:
>>>>>>>>> #ifdef CONFIG_FPGA
>>>>>>>>> void socfpga_fpga_add(void);
>>>>>>>>> #else
>>>>>>>>> static inline void socfpga_fpga_add(void) {}
>>>>>>>>> #endif
>>>>>>>> Right, if you had one socfpga_fpga_add() per platform +
>>>>>>>> generic
>>>>>>>> empty
>>>>>>>> one, you could drop that whole thing ^.
>>>>>>> Yes. It's being addressed in v3 patch:
>>>>>>> https://lists.denx.de/pipermail/u-boot/2018-October/343561.
>>>>>>> html
>>>>>> So where did the function go in there ? I don't see any
>>>>>> __weak
>>>>>> anything.
>>>>> I don't have to add anything in my v3 patchsets to make this
>>>>> work.
>>>>> It's already taken care by arch/arm/mach-
>>>>> socfpga/include/mach/misc.h as
>>>>> shown below:
>>>>>
>>>>> #ifdef CONFIG_FPGA
>>>>> void socfpga_fpga_add(void);
>>>>> #else
>>>>> static inline void socfpga_fpga_add(void) {}
>>>>> #endif
>>>>>
>>>>> An empty default socfpga_fpga_add() will be defined if
>>>>> CONFIG_FPGA
>>>>> is
>>>>> not defined.
>>>> I was hoping to turn this into __weak function.
>>> Below are the new changes for new patch:
>>> Empty weak function in arch/arm/mach-socfpga/misc.c:
>>>
>>> /* add device descriptor to FPGA device table */
>>> __weak void socfpga_fpga_add(void)
>>> {
>>> }
>>>
>>>
>>> In arch/arm/mach-socfpga/misc_aria10.c and arch/arm/mach-
>>> socfpga/misc_gen5.c:
>>>
>>> #ifdef CONFIG_FPGA
>>> /*
>>>  * FPGA programming support for SoC FPGA Cyclone V
>>>  */
>>> static Altera_desc altera_fpga[] = {
>>> 	{
>>> 		/* Family */
>>> 		Altera_SoCFPGA,
>>> 		/* Interface type */
>>> 		fast_passive_parallel,
>>> 		/* No limitation as additional data will be ignored */
>>> 		-1,
>>> 		/* No device function table */
>>> 		NULL,
>>> 		/* Base interface address specified in driver */
>>> 		NULL,
>>> 		/* No cookie implementation */
>>> 		0
>>> 	},
>>> };
>>>
>>> /* add device descriptor to FPGA device table */
>>> void socfpga_fpga_add(void)
>>> {
>>> 	int i;
>>> 	fpga_init();
>>> 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
>>> 		fpga_add(fpga_altera, &altera_fpga[i]);
>>> }
>>> #endif
>>>
>>>
>>> In arch/arm/mach-socfpga/misc_s10.c:
>>>
>>> #ifdef CONFIG_FPGA
>>> /*
>>>  * FPGA programming support for SoC FPGA Stratix 10
>>>  */
>>> static Altera_desc altera_fpga[] = {
>>> 	{
>>> 		/* Family */
>>> 		Intel_FPGA_Stratix10,
>>> 		/* Interface type */
>>> 		secure_device_manager_mailbox,
>>> 		/* No limitation as additional data will be ignored */
>>> 		-1,
>>> 		/* No device function table */
>>> 		NULL,
>>> 		/* Base interface address specified in driver */
>>> 		NULL,
>>> 		/* No cookie implementation */
>>> 		0
>>> 	},
>>> };
>>>
>>> /* add device descriptor to FPGA device table */
>>> void socfpga_fpga_add(void)
>>> {
>>> 	int i;
>>> 	fpga_init();
>>> 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
>>> 		fpga_add(fpga_altera, &altera_fpga[i]);
>>> }
>>> #endif
>>>
>>> With this new implementation, each platform overrides the
>>> 'socfpga_fpga_add' to add its own fpga device. The problem here is
>>> since our aria10 and gen5 are adding same fpga device, there will
>>> be
>>> duplication of code for these 2 platforms.
>>> What do you think ?
>> I think you can create a common code for Gen5 somehow, right ?
> I think I will add a new file arch/arm/mach-socfpga/fpga_devices.c and
> put the common code in it so that different platforms can share the
> common implementation which override the weak 'socfpga_fpga_add'
> function. The new file will have the following code:
> 
> #ifdef CONFIG_FPGA_STRATIX10
> /*
>  * FPGA programming support for SoC FPGA Stratix 10
>  */
> static Altera_desc altera_fpga[] = {
> 	{
> 		/* Family */
> 		Intel_FPGA_Stratix10,
> 		/* Interface type */
> 		secure_device_manager_mailbox,
> 		/* No limitation as additional data will be ignored */
> 		-1,
> 		/* No device function table */
> 		NULL,
> 		/* Base interface address specified in driver */
> 		NULL,
> 		/* No cookie implementation */
> 		0
> 	},
> };
> #else
> /*
>  * FPGA programming support for SoC FPGA Cyclone V
>  */
> static Altera_desc altera_fpga[] = {
> 	{
> 		/* Family */
> 		Altera_SoCFPGA,
> 		/* Interface type */
> 		fast_passive_parallel,
> 		/* No limitation as additional data will be ignored */
> 		-1,
> 		/* No device function table */
> 		NULL,
> 		/* Base interface address specified in driver */
> 		NULL,
> 		/* No cookie implementation */
> 		0
> 	},
> };
> #endif
> 
> /* add device descriptor to FPGA device table */
> void socfpga_fpga_add(void)
> {
> 	int i;
> 	fpga_init();
> 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> 		fpga_add(fpga_altera, &altera_fpga[i]);
> }
> 

Better submit the whole patch, it's hard to review pieces.