SVD contains clear-on-write fields that aren't actually clear-on-write #1466

New issue

Closed

opened 2023-08-01 12:17:10 -05:00 by jannic · 2 comments

jannic commented

2023-08-01 12:17:10 -05:00

(Migrated from github.com)

As encouraged by https://github.com/raspberrypi/pico-sdk/pull/1063#issuecomment-1279954374, please let me forward an issue @9names found: The SVD file uses modifiedWriteValues: clear to annotate some fields which are not actually cleared by the register write, but where writing a 1 triggers some action and then the hardware automatically resets the register value back to 0: https://github.com/rp-rs/rp2040-pac/issues/69

The example given are the fields PH_ADV and PH_RET in the PWM registers. The datasheet lists them as clear type SC ("This is a single bit that is written to by the processor and then cleared on the next clock cycle."). I agree with the mentioned issue that this shouldn't be translated to modifiedWriteValues: clear in the SVD.

As I understand it, SC should translate to the default behavior, modifiedWriteValues: modify: The write doesn't do anything special, it just sets the bit. That the hardware clears the bit automatically on the next clock cycle isn't a side effect of the write itself, which doesn't have a special representation in modifiedWriteValues.

Also please note that SC is probably not correct for PH_ADV and PH_RET, as SC means "cleared on the next clock cycle". But those two bits don't work like that, according to further explanations in the data sheet: "To advance or retard the phase by one count, software writes 1 to PH_ADV or PH_RET. Once an enable pulse has been inserted or deleted, the PH_ADV or PH_RET register bit will return to 0, and software can poll the CSR until this happens." That wouldn't make sense of those bits would always be cleared on the next clock cycle. So those register fields should have the default type RW instead.

As encouraged by https://github.com/raspberrypi/pico-sdk/pull/1063#issuecomment-1279954374, please let me forward an issue @9names found: The SVD file uses `modifiedWriteValues: clear` to annotate some fields which are not actually cleared by the register write, but where writing a 1 triggers some action and then the hardware automatically resets the register value back to 0: https://github.com/rp-rs/rp2040-pac/issues/69 The example given are the fields PH_ADV and PH_RET in the PWM registers. The datasheet lists them as clear type `SC` ("This is a single bit that is written to by the processor and then cleared on the next clock cycle."). I agree with the mentioned issue that this shouldn't be translated to `modifiedWriteValues: clear` in the SVD. As I understand it, `SC` should translate to the default behavior, `modifiedWriteValues: modify`: The write doesn't do anything special, it just sets the bit. That the hardware clears the bit automatically on the next clock cycle isn't a side effect of the write itself, which doesn't have a special representation in `modifiedWriteValues`. Also please note that `SC` is probably not correct for PH_ADV and PH_RET, as `SC` means "cleared on the next clock cycle". But those two bits don't work like that, according to further explanations in the data sheet: "To advance or retard the phase by one count, software writes 1 to PH_ADV or PH_RET. Once an enable pulse has been inserted or deleted, the PH_ADV or PH_RET register bit will return to 0, and software can poll the CSR until this happens." That wouldn't make sense of those bits would always be cleared on the next clock cycle. So those register fields should have the default type `RW` instead.

lurch commented

2023-08-01 12:25:33 -05:00

(Migrated from github.com)

I'm not a hardware-engineer, so I'm not aware of the full intricacies, but (in this particular example) IMHO a regular RW field wouldn't be appropriate either, as you (probably) wouldn't expect a RW field to be "changed behind your back by hardware" when you aren't looking? 🤷‍♂️

I'm not a hardware-engineer, so I'm not aware of the full intricacies, but (in this particular example) IMHO a regular `RW` field wouldn't be appropriate either, as you (probably) wouldn't expect a RW field to be "changed behind your back by hardware" when you aren't looking? :man_shrugging:

jannic commented

2023-08-01 12:30:56 -05:00

(Migrated from github.com)

There are a lot of registers which can be written by the user but which are also modified by the hardware, and they are usually marked as RW. So a RW field being "changed behind your back by hardware" isn't unusual at all.

There are a lot of registers which can be written by the user but which are also modified by the hardware, and they are usually marked as `RW`. So a `RW` field being "changed behind your back by hardware" isn't unusual at all.