[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon at bugs dot ecos dot sourceware dot org]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #23 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-03 14:57:22 BST ---
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #15)
> > > We need [stub_format_registers()] to skip
> > > registers (ditto stub_update_registers()). We will probably need to add a hook
> > > in generic-stub.c for this which cortexm_stub.h can define. Perhaps something
> > > like: 
> > > 
> > > #ifdef TARGET_G_PACKET_SKIPS_REGS
> > >     if ( !reg_in_g_packet( reg ) )
> > >         continue;
> > > #endif
> > 
> > This is a variation (Bug 1001559). Works for me and looks like solution.
> 
> Setting REGSIZE for them to 0 is good thinking.
> 
> 
> > > Have you set HAL_STUB_REGISTERS_SIZE for example? And if so, to
> > > what?
> > 
> > Thank you for the lead. (Bug 1001558)
> 
> Glad I guessed right :-).
> 
> 
> > > > Ideally the variant/platform will only need to implement
> > > > CYGINT_HAL_CORTEXM_FPU.
> > > 
> > > We can probably anticipate that other Cortex-Ms will come out in the future
> > > with different VFP versions, or a different number or size of registers. 
> > 
> > My point was supposed to be: "In ideal case you don't have to add anything to
> > variant other than implementing the interface in order to make it fit for FPU".
> > In reality you need to add appropriate CFLAGS to the platform.
> 
> Not necessarily. We potentially want to treat it like ARM's thumb mode, and
> have a higher layer manage adding/removing the appropriate flags to avoid
> unnecessary duplication of CDL into multiple platforms.
> 
> This is also because the user has ultimate control over whether the FPU support
> is included or not. Just because the hardware has an FPU doesn't mean they
> definitely want to use it. After all, it will use up space for the registers
> (even if they are only used lazily). And indeed your CDL below does allow it to
> be enabled/disabled, which is good, but rather than every relevant platform's
> CFLAGS having to test the value to decide how to construct the CFLAGS, arguably
> it's better for this option to use a 'requires' to set it in the CFLAGS, e.g.
> with the CDL you provided below (not my alternative):
> 
>    requires { (CYGHWR_HAL_CORTEXM_FPU == "FPV4_SP_D16") implies
> is_substr(CYGBLD_GLOBAL_CFLAGS, " -mfpu=fpv4-sp-d16") }
>    requires { !is_enabled(CYGHWR_HAL_CORTEXM_FPU) implies
> !is_substr(CYGBLD_GLOBAL_CFLAGS, "-mfpu=fpv4-sp-d16") }
> 
> 
> > > This should be anticipated. So it's unlikely it will be just be a question of > FPU or
> > > not FPU. Maybe we should follow the example of GCC and have something like
> > > CYGINT_HAL_CORTEXM_VFP4_SP_D16? Or maybe just CYGINT_HAL_CORTEXM_VFP4, and then
> > > variants must use a CDL 'requires' to set a specific FPU implementation, such
> > > as SP-D16.
> > 
> > I have changed CYGHWR_HAL_CORTEXM_FPU to booldata so now it can select desired
> > FPU.
> > 
> >     cdl_component CYGHWR_HAL_CORTEXM_FPU {
> >         display          "Floating Point Unit"
> >         flavor           booldata
> >         active_if        { CYGINT_HAL_CORTEXM_FPU }
> >         legal_values     { "FPV4_SP_D16" }
> >         default_value    { "FPV4_SP_D16" }
> >         description      "Floating Point Unit"
> 
> This doesn't feel quite right. Somewhere, something will have to do both:
>  implements CYGINT_HAL_CORTEXM_FPU
> _and_
>  requires { is_enabled(CYGHWR_HAL_CORTEXM_FPU) implies (CYGHWR_HAL_CORTEXM_FPU
> == "FPV4_SP_D16" }
> 
> This seems redundant.
> 
> For that matter, is there anything to be gained from CYGINT_HAL_CORTEXM_FPU
> anyway? I would have thought the HAL code would basically always have to be
> customised according to the specific register layout anyway, but you are better
> placed to decide that than I am.
> 
> I think it would be better to have something like:
> 
>    cdl_component CYGPKG_HAL_CORTEXM_FPU {
>        display    "Hardware floating point"
>        flavor     none
>        no_define
> 
>        cdl_interface CYGINT_HAL_CORTEXM_FPU {
>            display   "FPU present"
>        }
> 
>        cdl_interface CYGINT_HAL_CORTEXM_VFP4_SP_D16 {
>            display    "FPU uses vfp4-sp-d16 layout"
>            implements CYGINT_HAL_CORTEXM_FPU
>        }
> 
>        cdl_option CYGHWR_HAL_CORTEXM_FPU {
>            display   "Use hardware FP"
>            active_if CYGINT_HAL_CORTEXM_FPU
>        }
> 
>        requires   { 1 >= CYGINT_HAL_CORTEXM_FPU }
>        requires   { (CYGHWR_HAL_CORTEXM_FPU && CYGINT_HAL_CORTEXM_VFP4_SP_D16)
> == is_substr(CYGBLD_GLOBAL_CFLAGS, " -mfpu=fpv4-sp-d16") }
> 
> 
> (With appropriate "description" fields added of course).
> 
> In the last line, hopefully the == will make the CDL inference engine do the
> right thing, but if not, change it to:
>        requires   { (CYGHWR_HAL_CORTEXM_FPU && CYGINT_HAL_CORTEXM_VFP4_SP_D16)
> implies is_substr(CYGBLD_GLOBAL_CFLAGS, " -mfpu=fpv4-sp-d16") }
>        requires   { !(CYGHWR_HAL_CORTEXM_FPU && CYGINT_HAL_CORTEXM_VFP4_SP_D16)
> implies !is_substr(CYGBLD_GLOBAL_CFLAGS, " -mfpu=fpv4-sp-d16") }
> 
> 
> This way, HALs only need to implement CYGINT_HAL_CORTEXM_VFP4_SP_D16, and
> everything else is handled here at the arch HAL level, and presented to the
> user all in one place.
> 
> What do you think?
> 

For the record, some of your suggestions are accepted and integrated in Bug
1001607.

Thanks.

Ilija

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.