[U-Boot] [PATCH v8 4/8] ARM: socfpga: Add FPGA drivers for Arria 10 FPGA bitstream loading

[Marek Vasut]

On 2/14/19 4:23 PM, Chee, Tien Fong wrote:
> On Thu, 2019-02-14 at 16:15 +0100, Marek Vasut wrote:
>> On 2/14/19 4:14 PM, Chee, Tien Fong wrote:
>>>
>>> On Thu, 2019-02-14 at 13:29 +0100, Marek Vasut wrote:
>>>>
>>>> On 2/14/19 1:14 PM, Chee, Tien Fong wrote:
>>>>>
>>>>>
>>>>> On Thu, 2019-02-14 at 11:41 +0100, Marek Vasut wrote:
>>>>>>
>>>>>>
>>>>>> On 2/14/19 7:44 AM, Chee, Tien Fong wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Wed, 2019-02-13 at 17:20 +0100, Marek Vasut wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/13/19 3:18 PM, tien.fong.chee at intel.com wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> From: Tien Fong Chee <tien.fong.chee at intel.com>
>>>>>>>>>
>>>>>>>>> Add FPGA driver to support program FPGA with FPGA
>>>>>>>>> bitstream
>>>>>>>>> loading
>>>>>>>>> from
>>>>>>>>> filesystem. The driver are designed based on generic
>>>>>>>>> firmware
>>>>>>>>> loader
>>>>>>>>> framework. The driver can handle FPGA program operation
>>>>>>>>> from
>>>>>>>>> loading FPGA
>>>>>>>>> bitstream in flash to memory and then to program FPGA.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> changes for v8
>>>>>>>>> - Added codes to discern bitstream type based on fpga
>>>>>>>>> node
>>>>>>>>> name.
>>>>>>>>>
>>>>>>>>> changes for v7
>>>>>>>>> - Restructure the FPGA driver to support both
>>>>>>>>> peripheral
>>>>>>>>> bitstream
>>>>>>>>> and core
>>>>>>>>>   bitstream bundled into FIT image.
>>>>>>>>> - Support loadable property for core bitstream. User
>>>>>>>>> can
>>>>>>>>> set
>>>>>>>>> loadable
>>>>>>>>>   in DDR for better performance. This loading would be
>>>>>>>>> done
>>>>>>>>> in
>>>>>>>>> one
>>>>>>>>> large
>>>>>>>>>   chunk instead of chunk by chunk loading with small
>>>>>>>>> memory
>>>>>>>>> buffer.
>>>>>>>>> ---
>>>>>>>>>  arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts       |  
>>>>>>>>> 17 +
>>>>>>>>>  .../include/mach/fpga_manager_arria10.h            |  
>>>>>>>>> 39
>>>>>>>>> +-
>>>>>>>>>  drivers/fpga/socfpga_arria10.c                     |
>>>>>>>>> 467
>>>>>>>>> ++++++++++++++++++++-
>>>>>>>>>  3 files changed, 500 insertions(+), 23 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git
>>>>>>>>> a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
>>>>>>>>> b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
>>>>>>>>> index 998d811..14f1967 100644
>>>>>>>>> --- a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
>>>>>>>>> +++ b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
>>>>> [...]
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> - * FPGA Manager to program the FPGA. This is the
>>>>>>>>> interface
>>>>>>>>> used by
>>>>>>>>> FPGA driver.
>>>>>>>>> - * Return 0 for sucess, non-zero for error.
>>>>>>>>> - */
>>>>>>>>> +char *get_fpga_filename(const void *fdt, int *len)
>>>>>>>>> +{
>>>>>>>>> +	char *fpga_filename = NULL;
>>>>>>>>> +	int node_offset;
>>>>>>>>> +
>>>>>>>>> +	fdtdec_find_aliases_for_id(gd->fdt_blob,
>>>>>>>>> "fpga_mgr",
>>>>>>>>> +				COMPAT_ALTERA_SOCFPGA_
>>>>>>>>> FPGA
>>>>>>>>> 0,
>>>>>>>>> +				&node_offset, 1);
>>>>>>>>> +
>>>>>>>>> +	ofnode fpgamgr_node =
>>>>>>>>> offset_to_ofnode(node_offset);
>>>>>>>>> +
>>>>>>>>> +	if (ofnode_valid(fpgamgr_node))
>>>>>>>>> +		fpga_filename = (char
>>>>>>>>> *)ofnode_read_string(fpgamgr_node,
>>>>>>>>> +						"altr,
>>>>>>>>> bits
>>>>>>>>> trea
>>>>>>>>> m");
>>>>>>>>> +
>>>>>>>>> +
>>>>>>>> Why is the cast needed ?
>>>>>>> The return string would be eventually set to the char
>>>>>>> *filename
>>>>>>> in
>>>>>>> common struct fpga_fsinfo. So, the cast here is to avoid
>>>>>>> the
>>>>>>> warning
>>>>>>> from compiler.
>>>>>> I presume if the compiler generates a warning, it's for a
>>>>>> reason.
>>>>>> What
>>>>>> warning is that ?
>>>>> drivers/fpga/socfpga_arria10.c: In function
>>>>> 'get_fpga_filename':
>>>>> drivers/fpga/socfpga_arria10.c:466:17: warning: assignment
>>>>> discards
>>>>> 'const' qualifier from pointer target type [-Wdiscarded-
>>>>> qualifiers]
>>>>>    fpga_filename = ofnode_read_string(fpgamgr_node,
>>>> Add missing const then ?
>>> Then this requires change on common struct fpga_fsinfo, this would
>>> impact to other user using this. Why the cast is not allow as we
>>> only
>>> reading the filename?
>> If the string isn't const, someone can write it. If someone writes
>> this
>> string, won't it corrupt the DT ?
> Yes, but this would not happen in this driver, right?
> I don't know also why this common struct declare without the const, may
> be it supports the write?

I think so, maybe you should check that.

-- 
Best regards,
Marek Vasut