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

[Marek Vasut]

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 ?

-- 
Best regards,
Marek Vasut