nicocvn / cppreg

A C++11 header-only library for MMIO registers
https://nicocvn.github.io/cppreg/
Other
60 stars 6 forks source link

Toggle-only access policy support? #14

Open stephenwhittle opened 5 years ago

stephenwhittle commented 5 years ago

I'm working on refactoring a small USB stack for STM32 to avoid its dependencies (CMSIS, HAL etc) and have my own svd->cppreg header generation flow which is working great. However, the STM32 USB registers, specifically the endpoint registers, have a number of fields which are toggle-only (ie read is permitted, write 1 to flip) mixed in with normal read-write fields.

Would it be feasible to devise an access policy which changes the way cppreg attempts to maintain the value of a field?

Writing the existing value back, as I presume the existing implementation would do (to try to preserve the value of the toggle fields) when I'm trying to write to a normal field in the same register, will cause all the toggles to flip if they are currently set.

Happy to entertain other suggestions, too of course. CMSIS handles this just fine by forcing all toggle field values to 0 during read-modify-write, but to do the same I'd have to be able to read the entire packed register at once, then do a merge_write to all fields, and packed registers don't support read(). Looking over the documentation it seems that perhaps a modification of the shadow register functionality could achieve this, too?

sendyne-nicocvn commented 5 years ago

Hi @stephenwhittle First, I am glad to hear that cppreg is working fine for you.

I first want to confirm what this issue is about. Say we have a register of the form:

.... Field A Field B ....
.... 1 0 ....

Assuming Field A and Field B are "write 1 to toggle" the issue is that if you write 1 to Field B the current cppreg implementation will "rewrite" the value 1 in Field A thus leading to:

.... Field A Field B ....
.... 0 1 ....

while you would want to have:

.... Field A Field B ....
.... 1 1 ....

Can you confirm that my understanding is correct? I can think of several ways to tackle this issue but I first want to make sure I understand correctly.

As a side note, it is possible to read the register memory (for both packed and normal registers). Register memory handling in the current version of cppreg (v0.4) was significantly revised compared to previous versions: registers and packed registers now go through the same facility to manager the memory (see the Memory header). This means that if you have a type Reg (packed or not) you can use:

Note that similar functions are also available in previous cppreg versions but the details of their implementations differ.

stephenwhittle commented 5 years ago

Yes, you're correct in your understanding of the issue at hand. Thanks for the reminder re cppreg versions too - I'm using it as a submodule and had somehow forgotten to update. It might be worth mentioning rw_mem_device etc in the quick start guide, if you have a few moments to include a note on it, as I wasn't aware of that at all. With that in mind, it looks like a call to ro_mem_device to get the existing state, followed by a merge_write call to all the fields in the register, would give me the equivalent behaviour to the original implementation, but I'm happy to hear your alternative suggestions.

sendyne-nicocvn commented 5 years ago

Ok I created #15 and documentation will be updated very soon.

Regarding this issue a few comments first:

Your approach (reading the whole register and managing the writes through merge_write) is typically what we do in various edge cases that requires additional logic for group of fields. If you want I can add to cppreg a generic implementation of a "managed register" that could handle toggle fields or other edge cases. This is in my opinion the best approach.

The other solution is to add more logic to the register implementation with custom access policies but this will be less generic than the "managed register" approach and will take more time as I would have to think on how to do it in practice.

stephenwhittle commented 5 years ago

The managed register idea sounds like the best way to accomplish this in an extensible way, so that gets my vote. I was just really dreading having to manage this single register CMSIS-style to avoid messing up the toggles :)

sendyne-nicocvn commented 5 years ago

Sounds good; will add the manager register implementation as soon as possible.