Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing Renesas register write protection and some compiler warnings in static code analysis #1580

Merged
merged 6 commits into from Apr 9, 2024

Conversation

MatiMcFly
Copy link

Describe the PR
The Renesas RX controllers possess system-wide write protection for certain registers. Setup functions within the tinyUSB stack have to disable this write protection. This happens in order to disable the USB hardware's module stop function (e.g. MSTP(USB0) in dcd_init).
I changed the the write protection register's value handling in a way that the previous state is recovered. This leads to a more intuitive behavior of tinyUSB's initialization code

Additional context
Normally, peripheral modules are set up sequentially e.g. in a startup task. Bigger projects require lots of different modules whereas every single peripheral needs to have the module stop function disabled.
Threrefor, it is wise to disable the write protection at the very beginning of the initialization routine. At the end it would be reenabled again.

With this new behavior, developers don't have to think about register write protection when calling tinyUSB init-functions.

Here are two examples that show how the simplified behavior works:

Old behavior:

void StartTask(void *pv) {
  (void) pv;
  /* disable write protection of module stop functions */
  SYSTEM.PRCR.WORD = SYSTEM_PRCR_PRKEY | SYSTEM_PRCR_PRC1;
  
  /* call peripheral init functions */
  SetupADC();
  SetupXY();

  /* call tinyUSB init */
  UsbDeviceInit();   /* calls e.g. dcd_init() => This function in turn enables register write protection... */
  
  /* disable register protection again */
  SYSTEM.PRCR.WORD = SYSTEM_PRCR_PRKEY | SYSTEM_PRCR_PRC1;

  /* call further init functions
  lwIPInit();

  /*
  :
  :
  */

  /* reenable register write protection */
  SYSTEM.PRCR.WORD = SYSTEM_PRCR_PRKEY;
}

New behavior:

void StartTask(void *pv) {
  (void) pv;
  /* disable write protection of module stop functions */
  SYSTEM.PRCR.WORD = SYSTEM_PRCR_PRKEY | SYSTEM_PRCR_PRC1;
  
  /* call peripheral init functions */
  SetupADC();
  SetupXY();

  /* call tinyUSB init */
  UsbDeviceInit();   /* calls e.g. dcd_init() => Write protection will be disabled (again), but the previous state is recovered. */

  /* call further init functions, without having to disable the write protection a second time */
  lwIPInit();

  /*
  :
  :
  */

  /* reenable register write protection */
  SYSTEM.PRCR.WORD = SYSTEM_PRCR_PRKEY;
}

@MatiMcFly
Copy link
Author

I noticed an additional change that needs to be made. It has to do with the software configurable peripheral interrupt of the USB hardware.

The interrupt vector (PERIB_INTB185) needs to know which peripheral triggers it, therefore the following line needs to be added:

ICU.SLIBR185.BYTE = 62; /* set software configurable interrupt vector 185 to USBI0 */

Please add this line to the files "dcd_usba.c" (insert at 632) and "hcd_usba.c" (insert at 554).

Not adding the line means that users of the stack have to add it manually in the stack or outside of tusb_init. Let me know if you prefer a new pull request or if you'd like to get further clarification.

@perigoso
Copy link
Collaborator

perigoso commented Aug 3, 2022

consider using uint32_t and declaring the variable at the time of attribution

@MatiMcFly
Copy link
Author

MatiMcFly commented Aug 5, 2022

@perigoso I totally agree with you. I prefer the stdint types over the underlying types, too. Not exactly sure why I used the underlying types. Probably because they were used in the FreeRTOS files.
Declaring a variable anywhere else than at the start of a scope can lead to problems when using older C standards such as C89 (I think). I just wanted to avoid that.

Thanks for your proposals though!
Since the build seems to have failed it looks like I'm going to have to make an additional PR anyways...

@MatiMcFly
Copy link
Author

Commit c1c1238:

Static code checks generate unnecessary warnings when using the CCRX toolchain.
This happens if the macro TU_VERIFY_STATIC is used on the same line numbers in different files. The macro only uses the current line number as "unique ID".

The new solution includes the COUNTER macro in the static code check which gets rid of that issue.

@MatiMcFly MatiMcFly changed the title Handling of the Renesas RX write protection during initialization Fixing Renesas register write protection and some compiler warnings in static code analysis Sep 28, 2022
@@ -56,7 +56,7 @@
#elif defined (__STDC_VERSION__) && __STDC_VERSION__ >= 201112L
#define TU_VERIFY_STATIC _Static_assert
#elif defined(__CCRX__)
#define TU_VERIFY_STATIC(const_expr, _mess) typedef char TU_XSTRCAT(Line, __LINE__)[(const_expr) ? 1 : 0];
#define TU_VERIFY_STATIC(const_expr, _mess) typedef char TU_XSTRCAT3(Line, __LINE__, _TU_COUNTER_)[(const_expr) ? 1 : 0];
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the macro that expands to something like:

typedef char Line<XY>[(const_expr) ? 1 : 0];

This commit changes the macro to:

typedef char Line<XY><Z>[(const_expr) ? 1 : 0];
With Z being the value of _TU_COUNTER_.

Yes, the number does not contain the exact line number (because _TU_COUNTER_ was appended). But this is not an issue since the compiler error generated in case of a failed verification is the following:
E0520094: The size of an array must be greater than zero

Meaning the line index was unused anyways.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, actually we don't need the line for indicator, gcc would be able to emit the exact location. We can just replace LINE with TU_COUNTER to make it consistent with the followed ``#else`.

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for late response, thank you for the PR. Everything makes sense. Though the usba driver is rename to rusb2 and has been generalized to work with ra family. Therefore I have rebased and apply changes manually where it applicable.

Though I noticed there is some hipcup espcially msc example does not work anymore with rx. Haven't spent much time to testing more. But it has nothing to do with this PR. Maybe I will revisit and/or file an seperated issues for that later. Will merge when ci passes.

@@ -56,7 +56,7 @@
#elif defined (__STDC_VERSION__) && __STDC_VERSION__ >= 201112L
#define TU_VERIFY_STATIC _Static_assert
#elif defined(__CCRX__)
#define TU_VERIFY_STATIC(const_expr, _mess) typedef char TU_XSTRCAT(Line, __LINE__)[(const_expr) ? 1 : 0];
#define TU_VERIFY_STATIC(const_expr, _mess) typedef char TU_XSTRCAT3(Line, __LINE__, _TU_COUNTER_)[(const_expr) ? 1 : 0];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, actually we don't need the line for indicator, gcc would be able to emit the exact location. We can just replace LINE with TU_COUNTER to make it consistent with the followed ``#else`.

@hathach hathach merged commit a5b109b into hathach:master Apr 9, 2024
49 checks passed
@MatiMcFly
Copy link
Author

Thank you very much for merging the PR.
Yes, I also noticed that there is an issue with the msc example code. If I remember correctly, the issues start as soon as USB version 2.1 is specified and certain msc descriptors are read.
I think there are even some open discussions regarding this topic.

I'll be back to work next week. If you want, I could probably provide further information.

@hathach
Copy link
Owner

hathach commented Apr 9, 2024

Thank you very much for merging the PR. Yes, I also noticed that there is an issue with the msc example code. If I remember correctly, the issues start as soon as USB version 2.1 is specified and certain msc descriptors are read. I think there are even some open discussions regarding this topic.

I'll be back to work next week. If you want, I could probably provide further information.

that would be awesome, since I am not really familliar with rx much.

PS: I just got an rx62n board here https://www.ebay.com/itm/354627188176 with on-board jlink. Probably will make it easier to work with rx. I have only flash with e2 lite (remove/insert jumper) with rx65n target board so far.

@hathach
Copy link
Owner

hathach commented Apr 10, 2024

maybe worth retested after (will try myself as well) #2587

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants