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....@intel.com wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> From: "Ang, Chee Hong" <chee.hong....@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....@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 ?
--
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
