OK.. resolved. I failed to understand the point you were making, along with the face that as coded it did as required. The clock gating did not affect the ability to write the registers.
Given that the state of the DMA clock varies depending on just where the initial u-boot build started from, it was easiest to ensure the dma was definitely clocking.
It doesn't seem relevant what u-boot did or didn't do - since your proposed fix is to enable it unconditionally. That's always a better position to be in, rather than relying on some undocumented side effects of whatever suited the u-boot developers.
Regardless, that doesn't address the comment I've made: the function as written accesses the DMA peripheral before the lines you're adding.
Given that the state of the DMA clock varies depending on just where the initial u-boot build started from, it was easiest to ensure the dma was definitely clocking. I suggest it be left as is as this port is now pretty settled.
True, and now resolved
There's a missing GET CCU.hdr
in this file.
[This comment 01-Dec-2022 copy/pasted from the earlier identical !7 (closed) and !11 (closed) which was closed with unresolved threads]
The implication, from the comment, is that some part of the video system is performing a DMA operation before the controller is activated. Shouldn't this be done in the Activate entry? Or even in VideoDevice_Activate? PineVideo 0.10 doesn't appear to interact with this HAL device at all.
[From 03-Dec-2022]
this enable (/re-enable) was found necessary [...] boot mechanism for inital boot
Since it's ungating the clock to the DMA block, at the very least shouldn't it be done earlier in the function, after the controller's mapped in, but before you attempt to write to DMA_IRQ_EN_REG?
Indeed. This was the channel used in initial code. It may be possible to use a different channel. It wasn't something I investigated when working on this.
Feel free to attempt changes, but I suggest in this case it will probably yield little other than 'purist satisfaction'.
I'd rather leave this as is.
At present the ROOL git does not specifically host GPL projects. This way of storing things should provide sufficient adherence to the GPL, as it enables the recreation of the binary blobs used.
The build process produces 2 programmers, one for programming in situ, and the other for progamming a sd card in a scsi adaptor. As they could be supplied either 1 or both, each really needs the GPL compliance.
FWIW this is how ROOL have done this in other projects, and is as @bavison has advised me.
no.. but perhaps it helps radability? I'll sort that hopefully in a final update if needed once @rsprowson (I presume) is happy,
As an aside, I noticed that both the PineA64 and BCM2835 HALs exclude one of the DMA channels from the HAL device so that the video driver can use it directly. Is there an advantage to doing this compared to going via the DMA manager?
(This isn’t a major issue BTW, so can be put to one side if it allows this MR to be merged quicker.)
I mentioned in the other MR about the size increase caused by copying the u-boot and trusted firmware repositories multiple times, but it also occurred to me that it would be a good idea to have them as proper GitLab repositories so that it’s easier to track modifications.
LDR a1, [a2, #BUS_CLK_GATING_REG0] ; dma gate register
ORR a1, a1, #&40 ; ensure dma gate enabled
STR a1, [a2, #BUS_CLK_GATING_REG0]
LDR a1, [a2, #BUS_SOFT_RST_REG0] ; dma reset register
ORR a1, a1, #&40 ; ensure dma reset cleared
STR a1, [a2, #BUS_SOFT_RST_REG0]
Was reverting the whitespace corrections from commit 06adcbf9 intentional?
@ccawley I may be mistaken there. It is possible it was wrong at initial commit time, but is certainly correct now to agree with both the components file and the programmers in the MR. Because my master copies had things correctly I 'assumed' it was introduced earlier. Mea culpa.
Thanks for your efforts BTW
Now all we need is to get this MR accepted (without or with further adjustment) ...
It also corrects a fault introduced in the recent 'cleanup' update from Cameron, the effect of which was disastrous for SDCMOS. FWIW one could make the change he requested, removing the DCD 0 at &fc000004, but matching changes would be needed on ALL the programmers and in the components file too.
Sorry about that, it wasn't intentional. That said, it's not entirely obvious to me which change had that effect - the only change in s.Top from commit 06adcbf9 just removes a commented-out include.
Rob I think this covers all you have commented on. Thanks for the CCU/txt. It also corrects a fault introduced in the recent 'cleanup' update from Cameron, the effect of which was disastrous for SDCMOS. FWIW one could make the change he requested, removing the DCD 0 at &fc000004, but matching changes would be needed on ALL the programmers and in the components file too. It is also supplied in a separate branch. Whilst I could possibly have done all this within the previous merge request, I felt this would be a cleaneer way to do it. Incidentally, I have built roms for both A64 variants using this HAL branch and successfully programmed then with the programmers created by the autobuilder script Thanks JB
Detail: Provides a sanitised update to all programmers. Reflects most previous merge attempt comments. Add CCU header file. Correct SDCMOS storage address at ROM start to match that expected both in the components file AND the programmers. It really shouldn't have been changed.
Admin:
Detail: In s/Top aligned with where components file points SDCMOS. In s/DMA ensure DMA engine always enabled for video DMA use (some laptop variants booted leaving this off).
Programmers are deliberately machine specific as each machine has different boot (length and code) segments.
Will delete this strand, reclone and resync, add ccu stuff and resubmit, having worked in a branch. Hope we'll be able to progress... Thanjs JB
[This comment 01-Dec-2022 copy/pasted from the earlier identical !7 (closed) which was closed with unresolved threads]
The implication, from the comment, is that some part of the video system is performing a DMA operation before the controller is activated. Shouldn't this be done in the Activate entry? Or even in VideoDevice_Activate? PineVideo 0.10 doesn't appear to interact with this HAL device at all.
The CCU register bits/offsets would benefit from being in a header with meaningful names rather than magic numbers. Top of Audio.s
has started with a few too. That would make the comments about which register is which unnecessary.
[From 03-Dec-2022]
this enable (/re-enable) was found necessary [...] boot mechanism for inital boot
Since it's ungating the clock to the DMA block, at the very least shouldn't it be done earlier in the function, after the controller's mapped in, but before you attempt to write to DMA_IRQ_EN_REG?
The CCU is a 'block' that has many registers
There are 94 registers I can see in the datasheet, pretty light for a modern SOC, and trivially screen scraped from the datasheet. In fact - here they are CCU.txt. You'd want to add the bit definitions (for the couple of registers that you do use) then I suggest just pasting it into PINEA64.hdr
, after the base address definition, and deleting the duplicates from Audio.s
.