Message ID | 20230714232659.76434-1-chris@laplante.io |
---|---|
Headers | show |
Series | Add nRF51 DETECT signal with test | expand |
On Sat, 15 Jul 2023 at 00:27, Chris Laplante <chris@laplante.io> wrote: > > This patch series implements the nRF51 DETECT signal > in the GPIO peripheral. A qtest is added exercising the signal. > > To implement the test, named out-GPIO IRQ interception had to be added > to the qtest framework. I also took the opportunity to improve IRQ > interception a bit by adding 'FAIL' responses when interception fails. > Otherwise, it is frustrating to troubleshoot why calls to > qtest_irq_intercept_out and friends appears to do nothing. Thanks for this patchset and especially for the work improving the qtest infrastructure. I've given my comments on the different patches, and in some cases reviewed-by tags. (Where I've given one of those, you should add it to your commit message for the relevant patch under your Signed-off-by: line, so that when you send the version 2 of the patchset we know that those parts are already reviewed and don't need re-examining. If I said "make some change; otherwise Reviewed-by" that means "make that minor change, and then you can add the tag, etc".) Do you have the parts of this feature that use the DETECT signal in the POWER device, or have you not written those yet ? If you have them, you could send those too in v2. -- PMM
Hi Peter, > Thanks for this patchset and especially for the work > improving the qtest infrastructure. I've given my > comments on the different patches, and in some cases > reviewed-by tags. (Where I've given one of those, you should > add it to your commit message for the relevant patch under > your Signed-off-by: line, so that when you send the version > 2 of the patchset we know that those parts are already > reviewed and don't need re-examining. If I said "make > some change; otherwise Reviewed-by" that means "make > that minor change, and then you can add the tag, etc".) Thanks very much for the feedback and help! > Do you have the parts of this feature that use the DETECT > signal in the POWER device, or have you not written those > yet ? If you have them, you could send those too in v2. That part is halfway done, so I will work on finishing it before submitting v2. Two questions regarding that (to potentially save us a v3): 1. The nRF51 POWER device overlaps with the memory maps of the CLOCK and MPU devices. So I have created a CPM (CLOCK, POWER, MPU) device in hw/misc. Does that sound reasonable naming-wise? 2. I also have some implementations for pieces of CLOCK, namely the HFCLKSTART/HFCLKSTOP events and HFCLKSTARTED event. Should I include that in this patch series, or would you prefer it in a separate series? It is unrelated to DETECT and POWER. Thanks, Chris
On Tue, 25 Jul 2023 at 04:25, Chris Laplante <chris@laplante.io> wrote: > > Hi Peter, > > > Thanks for this patchset and especially for the work > > improving the qtest infrastructure. I've given my > > comments on the different patches, and in some cases > > reviewed-by tags. (Where I've given one of those, you should > > add it to your commit message for the relevant patch under > > your Signed-off-by: line, so that when you send the version > > 2 of the patchset we know that those parts are already > > reviewed and don't need re-examining. If I said "make > > some change; otherwise Reviewed-by" that means "make > > that minor change, and then you can add the tag, etc".) > > Thanks very much for the feedback and help! > > > Do you have the parts of this feature that use the DETECT > > signal in the POWER device, or have you not written those > > yet ? If you have them, you could send those too in v2. > > That part is halfway done, so I will work on finishing it before submitting v2. Two questions regarding that (to potentially save us a v3): > > 1. The nRF51 POWER device overlaps with the memory maps of the CLOCK and MPU devices. So I have created a CPM (CLOCK, POWER, MPU) device in hw/misc. Does that sound reasonable naming-wise? Yes, I think from QEMU's point of view the massive register overlap makes them a single device. The name sounds OK (give it the same kind of nrf51 prefix the rng device has). > 2. I also have some implementations for pieces of CLOCK, namely the HFCLKSTART/HFCLKSTOP events and HFCLKSTARTED event. Should I include that in this patch series, or would you prefer it in a separate series? It is unrelated to DETECT and POWER. If you think they're ready to go in, and it doesn't make the series more than about 12-15 patches long, you can put them on the end of the series. If the patchset is starting to get a bit big then it might be easier to get the POWER/DETECT parts reviewed first. thanks -- PMM
> > 2. I also have some implementations for pieces of CLOCK, namely the HFCLKSTART/HFCLKSTOP events and HFCLKSTARTED event. Should I include that in this patch series, or would you prefer it in a separate series? It is unrelated to DETECT and POWER. > > > If you think they're ready to go in, and it doesn't > make the series more than about 12-15 patches long, > you can put them on the end of the series. If the > patchset is starting to get a bit big then it might > be easier to get the POWER/DETECT parts reviewed > first. I'm just going to send the POWER/DETECT bits first. There is quite a lot to emulate in CLOCK, POWER, and MPU, and I'd like to do a good job on it. Thanks. Chris
Hi Peter, > > 2. I also have some implementations for pieces of CLOCK, namely the HFCLKSTART/HFCLKSTOP events and HFCLKSTARTED event. Should I include that in this patch series, or would you prefer it in a separate series? It is unrelated to DETECT and POWER. > > > If you think they're ready to go in, and it doesn't > make the series more than about 12-15 patches long, > you can put them on the end of the series. If the > patchset is starting to get a bit big then it might > be easier to get the POWER/DETECT parts reviewed > first. Unrelated question regarding the CLOCK module. Should I model the startup times for the various crystal oscillators? Or should I just assume they start instantly for simplicity? External xtal startup time is 750-800 us. Internal RC startup time is 4-5 us. I've already modeled the delay for the external xtal, but just wondering if its worth the extra code. Thanks, Chris
On Thu, 27 Jul 2023 at 23:51, Chris Laplante <chris@laplante.io> wrote: > > Hi Peter, > > > > 2. I also have some implementations for pieces of CLOCK, namely the HFCLKSTART/HFCLKSTOP events and HFCLKSTARTED event. Should I include that in this patch series, or would you prefer it in a separate series? It is unrelated to DETECT and POWER. > > > > > > If you think they're ready to go in, and it doesn't > > make the series more than about 12-15 patches long, > > you can put them on the end of the series. If the > > patchset is starting to get a bit big then it might > > be easier to get the POWER/DETECT parts reviewed > > first. > > Unrelated question regarding the CLOCK module. Should I model the startup times for the various crystal oscillators? Or should I just assume they start instantly for simplicity? External xtal startup time is 750-800 us. Internal RC startup time is 4-5 us. I've already modeled the delay for the external xtal, but just wondering if its worth the extra code. We typically just have that sort of thing start instantly, unless there's some specific guest workload that falls over if you don't model the startup delay. Usually modelling the delay is unnecessary complexity. thanks -- PMM