Re: [PATCH 1/1] arm: rust: Enable Rust support for ARMv7

[Christian Schrefl]

On 08.06.24 2:16 PM, Miguel Ojeda wrote:
> On Sat, Jun 8, 2024 at 12:30 AM Christian Schrefl
> <chrisi.schrefl@xxxxxxxxx> wrote:
>>
>> This is based on the code by Sven Van Asbroeck from the original
>> rust branch and inspired by the AArch version by Jamie Cunliffe.
> 
> Thanks for mentioning that!
> 
> (AArch64 perhaps?)

Ah yes I meant AArch64. Fixed it locally.

>> I have tested the rust samples and a custom simple MMIO module
>> on on hardware (De1SoC FPGA + Arm A9 CPU).
> 
> Very nice to have tested it in real hardware, thanks!
> 
> (Duplicate "on on").

I fixed that locally, but forgot to run format-patch again, sorry! 

>> +``arm``        Maintained        ARMv7 Little Endian only.
> 
> How hard would it be to test and maintain other versions, eventually?
> I met Jamie recently, he may have some ideas here.

I think it would just be setting the correct rust target,
but I have no hardware to test it and I think its best to
only do v7 for the initial support.

>> -BINDGEN_TARGET_arm64   := aarch64-linux-gnu
>> +BINDGEN_TARGET_arm64:= aarch64-linux-gnu
> 
> Spurious change?

I've changed that so the := aligns with the others, but I can
remove that for v2 if you want

>> +# Depending on how the architecute defines ARCH_SLAB_MINALIGN, bindgen might generate a binding.
>> +# Disable this here as there is a const that will always be generated in bindings_helper.c
> 
> Typo in "architecute". Also, I guess you mean `.h` instead. Please use
> Markdown formatting for consistency with the rest of the file.

Fixed.

>> +        panic!("arm uses the builtin rustc target");
> 
> Since ARM64 puts the name of the target in the message, it would be
> nice to do the same (or perhaps it can be removed from the ARM64 one
> otherwise).

I can add the target here but then the message would have to depend
on the sub architecture if we support v6 in the future. 

> Cheers,
> Miguel