On 06/05/2014 10:41 AM, Mark Rutland wrote: > On Thu, Jun 05, 2014 at 04:07:17PM +0100, York Sun wrote: >> On 06/05/2014 03:09 AM, Mark Rutland wrote: >>> On Wed, Jun 04, 2014 at 05:27:30PM +0100, York Sun wrote: >>>> On 06/02/2014 11:01 AM, Mark Rutland wrote: >>>>> On Mon, Jun 02, 2014 at 05:06:13PM +0100, York Sun wrote: >>>>>> On 06/02/2014 04:34 AM, Mark Rutland wrote: >>>>>>> On Thu, May 29, 2014 at 09:49:05PM +0100, York Sun wrote: >>>>>>>> Make MMU functions reusable. Platform code can setup its own MMU >>>>>>>> tables. >>>>>>> >>>>>>> What exactly does platform code need to setup its own tables for? >>>>>> >>>>>> The general ARMv8 MMU table is not detail enough to control memory >>>>>> attribute >>>>>> like cache for all addresses. We have devices mapping to addresses with >>>>>> different requirement for cache control. >>>>> >>>>> And there are no APIs for creating device mappings rather than exporting >>>>> the raw pagetable accessors and hard-coding them differently in every >>>>> board file? >>>>> >>>> >>>> That's a good question. At this point, only two platforms are using ARMv8 >>>> code. >>>> I am expecting FSL ARMv8 implementation will stay similar, i.e. covered by >>>> the >>>> file I added. If that's not the case, or more ARMv8 SoCs need special MMU >>>> table, >>>> we then should introduce such API. Having a full function MMU API may be an >>>> overkill for U-boot. We don't need dynamic MMU anyway. >>> >>> Maybe. It just seems to me that it would be possible to pre-allocate an >>> empty table that we could place device (nGnRnE?) mappings in. Then all >>> you'd need to call from board code is a function to map a range, rather >>> than having to duplicate logic for creating the tables you want. >> >> It sounds good, but not the case. For the three level tables I am using >> (level0, >> level1, level2), I don't have level2 table for every address, that will be >> too >> many. Instead, I have a lot of blocks for level1. When I need some fine >> control >> within a level1 block range, I have to create a new level2 table. It is >> doable, >> but I will hold on that if I can use static table. > > While my suggestion might not be the best, I'm not sure I follow, unless > you always want to idmap devices? > > If you don't idmap devices, then you can place all of the disparate > physical mappings within a single table unless you have very large > peripherals to map?
If you mean identical map as idmap, yes I am creating identical map for devices. I got your point. For this particular SoC, if I can get it work with these simple static tables, I will stay with them. But if I need to maintain the tables for various SoCs, I will convert to dynamic API. > >> >>> >>>> >>>>>> >>>>>>> >>>>>>>> Also fix a typo of "TCR_EL3_IPS_BITS" in cache_v8.c. >>>>>>>> >>>>>>>> Signed-off-by: York Sun <york...@freescale.com> >>>>>>>> CC: David Feng <feng...@phytium.com.cn> >>>>>>>> --- >>>>>>>> Change log: >>>>>>>> v4: new patch, splitted from v3 2/4 >>>>>>>> Revise set_pgtable_section() to be reused by platform MMU code >>>>>>>> Add inline function set_ttbr_tcr_mair() to be used by this and >>>>>>>> platform mmu code >>>>>>>> >>>>>>>> arch/arm/cpu/armv8/cache_v8.c | 49 >>>>>>>> ++++++++++++++++---------------------- >>>>>>>> arch/arm/include/asm/armv8/mmu.h | 23 ++++++++++++++++++ >>>>>>>> 2 files changed, 43 insertions(+), 29 deletions(-) >>>>>>>> >>>>>>>> diff --git a/arch/arm/cpu/armv8/cache_v8.c >>>>>>>> b/arch/arm/cpu/armv8/cache_v8.c >>>>>>>> index a96ecda..67dbd46 100644 >>>>>>>> --- a/arch/arm/cpu/armv8/cache_v8.c >>>>>>>> +++ b/arch/arm/cpu/armv8/cache_v8.c >>>>>>>> @@ -12,15 +12,14 @@ >>>>>>>> DECLARE_GLOBAL_DATA_PTR; >>>>>>>> >>>>>>>> #ifndef CONFIG_SYS_DCACHE_OFF >>>>>>>> - >>>>>>>> -static void set_pgtable_section(u64 section, u64 memory_type) >>>>>>>> +void set_pgtable_section(u64 *page_table, u64 index, u64 section, >>>>>>>> + u64 memory_type) >>>>>>>> { >>>>>>>> - u64 *page_table = (u64 *)gd->arch.tlb_addr; >>>>>>>> u64 value; >>>>>>>> >>>>>>>> - value = (section << SECTION_SHIFT) | PMD_TYPE_SECT | >>>>>>>> PMD_SECT_AF; >>>>>>>> + value = section | PMD_TYPE_SECT | PMD_SECT_AF; >>>>>>>> value |= PMD_ATTRINDX(memory_type); >>>>>>>> - page_table[section] = value; >>>>>>>> + page_table[index] = value; >>>>>>>> } >>>>>>>> >>>>>>>> /* to activate the MMU we need to set up virtual memory */ >>>>>>>> @@ -28,10 +27,13 @@ static void mmu_setup(void) >>>>>>>> { >>>>>>>> int i, j, el; >>>>>>>> bd_t *bd = gd->bd; >>>>>>>> + u64 *page_table = (u64 *)gd->arch.tlb_addr; >>>>>>>> >>>>>>>> /* Setup an identity-mapping for all spaces */ >>>>>>>> - for (i = 0; i < (PGTABLE_SIZE >> 3); i++) >>>>>>>> - set_pgtable_section(i, MT_DEVICE_NGNRNE); >>>>>>>> + for (i = 0; i < (PGTABLE_SIZE >> 3); i++) { >>>>>>>> + set_pgtable_section(page_table, i, i << SECTION_SHIFT, >>>>>>>> + MT_DEVICE_NGNRNE); >>>>>>>> + } >>>>>>>> >>>>>>>> /* Setup an identity-mapping for all RAM space */ >>>>>>>> for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) { >>>>>>>> @@ -39,36 +41,25 @@ static void mmu_setup(void) >>>>>>>> ulong end = bd->bi_dram[i].start + >>>>>>>> bd->bi_dram[i].size; >>>>>>>> for (j = start >> SECTION_SHIFT; >>>>>>>> j < end >> SECTION_SHIFT; j++) { >>>>>>>> - set_pgtable_section(j, MT_NORMAL); >>>>>>>> + set_pgtable_section(page_table, j, j << >>>>>>>> SECTION_SHIFT, >>>>>>>> + MT_NORMAL); >>>>>>>> } >>>>>>>> } >>>>>>>> >>>>>>>> /* load TTBR0 */ >>>>>>>> el = current_el(); >>>>>>>> if (el == 1) { >>>>>>>> - asm volatile("msr ttbr0_el1, %0" >>>>>>>> - : : "r" (gd->arch.tlb_addr) : "memory"); >>>>>>>> - asm volatile("msr tcr_el1, %0" >>>>>>>> - : : "r" (TCR_FLAGS | TCR_EL1_IPS_BITS) >>>>>>>> - : "memory"); >>>>>>>> - asm volatile("msr mair_el1, %0" >>>>>>>> - : : "r" (MEMORY_ATTRIBUTES) : "memory"); >>>>>>>> + set_ttbr_tcr_mair(el, gd->arch.tlb_addr, >>>>>>>> + TCR_FLAGS | TCR_EL1_IPS_BITS, >>>>>>>> + MEMORY_ATTRIBUTES); >>>>>>>> } else if (el == 2) { >>>>>>>> - asm volatile("msr ttbr0_el2, %0" >>>>>>>> - : : "r" (gd->arch.tlb_addr) : "memory"); >>>>>>>> - asm volatile("msr tcr_el2, %0" >>>>>>>> - : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS) >>>>>>>> - : "memory"); >>>>>>>> - asm volatile("msr mair_el2, %0" >>>>>>>> - : : "r" (MEMORY_ATTRIBUTES) : "memory"); >>>>>>>> + set_ttbr_tcr_mair(el, gd->arch.tlb_addr, >>>>>>>> + TCR_FLAGS | TCR_EL2_IPS_BITS, >>>>>>>> + MEMORY_ATTRIBUTES); >>>>>>>> } else { >>>>>>>> - asm volatile("msr ttbr0_el3, %0" >>>>>>>> - : : "r" (gd->arch.tlb_addr) : "memory"); >>>>>>>> - asm volatile("msr tcr_el3, %0" >>>>>>>> - : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS) >>>>>>>> - : "memory"); >>>>>>>> - asm volatile("msr mair_el3, %0" >>>>>>>> - : : "r" (MEMORY_ATTRIBUTES) : "memory"); >>>>>>>> + set_ttbr_tcr_mair(el, gd->arch.tlb_addr, >>>>>>>> + TCR_FLAGS | TCR_EL3_IPS_BITS, >>>>>>>> + MEMORY_ATTRIBUTES); >>>>>>>> } >>>>>>>> >>>>>>>> /* enable the mmu */ >>>>>>>> diff --git a/arch/arm/include/asm/armv8/mmu.h >>>>>>>> b/arch/arm/include/asm/armv8/mmu.h >>>>>>>> index 1193e76..7de4ff9 100644 >>>>>>>> --- a/arch/arm/include/asm/armv8/mmu.h >>>>>>>> +++ b/arch/arm/include/asm/armv8/mmu.h >>>>>>>> @@ -108,4 +108,27 @@ >>>>>>>> TCR_IRGN_WBWA | \ >>>>>>>> TCR_T0SZ(VA_BITS)) >>>>>>>> >>>>>>>> +#ifndef __ASSEMBLY__ >>>>>>>> +void set_pgtable_section(u64 *page_table, u64 index, >>>>>>>> + u64 section, u64 memory_type); >>>>>>>> +static inline void set_ttbr_tcr_mair(int el, u64 table, u64 tcr, u64 >>>>>>>> attr) >>>>>>>> +{ >>>>>>>> + asm volatile("dsb sy;isb"); >>>>>>> >>>>>>> Huh? This wasn't anywhere before. Is the isb necessary? >>>>>> >>>>>> Probably not, but to make sure all previous instructions finish. >>>>> >>>>> Which instructions do you care about completing? >>>>> >>>>> You'll certainly want any page table writes to complete, but is anything >>>>> else in-flight at this point in time? >>>> >>>> Before calling this inline function, MMU tables were created. We want the >>>> writing completes before setting these registers. >>> >>> Sure, but the dsb sy guarantees that. >>> >>> The dsb sy will ensure that all reads and writes before it have become >>> visible to all observers in the full system shareability domain >>> (including the MMU) before any subsequent instructions (in program >>> order) can execute. >>> >>> I'd only expect to see a "dsb $DOMAIN; isb" sequence is when we're >>> changing things that affect the instruction stream (I-cache maintenance, >>> changing the mapping underlying the currently executing stream). >> >> Thanks. I can drop "isb". Please comment on v5 patches. > > Will do momentarily. > >> >>> >>>>> >>>>>>> >>>>>>> When is this function expected to be called? Is the MMU expected to be >>>>>>> on? >>>>>> >>>>>> The general ARMv8 code calls this when MMU is off. For the SoC I am >>>>>> enabling, it >>>>>> is called when MMU is off for the first time, but MMU on for the 2nd >>>>>> time. >>>>> >>>>> Ok. >>>>> >>>>>> >>>>>>> >>>>>>>> + if (el == 1) { >>>>>>>> + asm volatile("msr ttbr0_el1, %0" : : "r" (table) : >>>>>>>> "memory"); >>>>>>>> + asm volatile("msr tcr_el1, %0" : : "r" (tcr) : >>>>>>>> "memory"); >>>>>>>> + asm volatile("msr mair_el1, %0" : : "r" (attr) : >>>>>>>> "memory"); >>>>>>> >>>>>>> If the MMU is on, this looks really scary -- what are you expecting to >>>>>>> change in a single invocation? >>>>>> >>>>>> It is not scary for general ARMv8 code. MMU is off then this is called. >>>>>> For FSL >>>>>> SoC, I have these called for the 2nd time, when MMU is on. The 2nd time >>>>>> call >>>>>> points to a new MMU table. >>>>> >>>>> Oh but it is ;) >>>>> >>>>>> >>>>>>> >>>>>>>> + } else if (el == 2) { >>>>>>>> + asm volatile("msr ttbr0_el2, %0" : : "r" (table) : >>>>>>>> "memory"); >>>>>>>> + asm volatile("msr tcr_el2, %0" : : "r" (tcr) : >>>>>>>> "memory"); >>>>>>>> + asm volatile("msr mair_el2, %0" : : "r" (attr) : >>>>>>>> "memory"); >>>>>>>> + } else if (el == 3) { >>>>>>>> + asm volatile("msr ttbr0_el3, %0" : : "r" (table) : >>>>>>>> "memory"); >>>>>>>> + asm volatile("msr tcr_el3, %0" : : "r" (tcr) : >>>>>>>> "memory"); >>>>>>>> + asm volatile("msr mair_el3, %0" : : "r" (attr) : >>>>>>>> "memory"); >>>>>>>> + } else { >>>>>>>> + hang(); >>>>>>>> + } >>>>>>> >>>>>>> And no synchronisation to ensure that these writes are complete or even >>>>>>> ordered w.r.t. each other? >>>>>>> >>>>>> >>>>>> That's why I added asm volatile("dsb sy;isb") before them. The order of >>>>>> these >>>>>> write doesn't matter. See the code before my change >>>>>> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv8/cache_v8.c;h=a96ecda7e3146996d20a2e46b975dced2769255c;hb=HEAD >>>>> >>>>> Ok, so that orders them against everything _before_ them, but not with >>>>> respect to each other, or anything _after_ them. >>>>> >>>>> When the MMU is off, you are correct that the ordering of these >>>>> operations w.r.t. each other doesn't matter. With the MMU on that's not >>>>> true as the CPU can rerder the operations and can walk the page tables >>>>> asynchronously. >>>>> >>>>> Changing the MAIR at runtime is somewhat scary because you may be >>>>> changing attributes for entries which are active in the cache and/or >>>>> TLBs. I'm not sure I follow why you need to change it once the MMU is on >>>>> -- there are plenty of subfields in the MAIR that you could configure >>>>> before turning the MMU on for use later. >>>>> >>>>> Likewise changing the TCR at runtime is somewhat scary because you can >>>>> change attributes of active cache and/or TLB entries, change fields that >>>>> affect the way page tables are walked (TG*. T*SZ), all asynchronously >>>>> w.r.t. the logic that walsk the page tables. >>>>> >>>>> Changing the TTBR is fine, except that we didn't invalidate old entries >>>>> with a TLBI, so we may even have partial translations cahced in the >>>>> TLBs, and we can get odd translations generated from the combination of >>>>> the old and new tables. >>>> >>>> >>>> Maybe a more proper way to do so is to change TTBR only. That's actually >>>> what I >>>> really need. Or if I really need to change all of them, I should turn off >>>> MMU first. >>> >>> In fact, if you had an API for creating device mappings, you wouldn't >>> even need to change the TTBR -- you'd just need to replace some existing >>> invalid entries with the new mapping, make them visible to the MMU with >>> a dsb, and then carry on. The TLBs aren't allowed to cache invalid >>> mappings, so on the next access to those mappings, the appropriate page >>> table entry will be read into the TLBs. >>> >>> If you alter multiple levels of a live table then you'll need barriers >>> (DMBs) between populating those levels. If you back device mappings by a >>> single level of table then you can avoid that. Even better, with an API >>> you can do the simple thing today then make it more advanced in future >>> if necessary without having to change your board code. >>> >> >> No objection here on the idea. But again this is not the case. My first MMU >> table is in SRAM, which is small and will be used for other purpose. The 2nd >> MMU >> table is in DDR. I could copy the table and do the maintenance as you said. >> For >> now, I want to stick with the static table and only create the API when I >> have to. > > Sure, if your tables are in SRAM then trying to do a load of dynamic > allocation isn't going to work. > > My fear is that while that sounds OK with a single user to do a quick > havk and poke the tables directly, we'll end up with everyone doing > that, and no-one will try to unify things. It is very diffifcult to > unify such variation after the fact. That's a good reason. Let me start to code the API. It will take a while to cover the complexity of the multilevel tables and sizes. It will be a separated patch for later review. I don't want that to delay this patch set. I am hoping to get this set in for 2014.07 release. > >>>>>> For the 2nd write used by FSL SoC, I need to flush the cache to make >>>>>> sure the >>>>>> table is in main DDR and perform TLB invalidation. That's when the >>>>>> loading of >>>>>> new MMU table happens. >>>>> >>>>> I missed the TLB invaliation -- where is that meant to happen? >>>>> >>>> After flushing the new MMU table into DDR. To your point above, I will >>>> send a >>>> new version with updating TTBR only. >>> >>> Unfortunately, that's too early. The MMU is asynchronous w.r.t. the CPU >>> and so it can re-read the existing tables into the TLBs _before_ it has >>> been programmed with the new table address. The TLBI has to happen >>> _after_ the old tables are no longer pointed to by the TTBR. >>> >>> What you need to do to replace the active set of tables (assuming that >>> the new mapping has the instruction stream mapped in an identical way) >>> is: >>> >>> - Write the tables. >>> >>> - DSB to make them visible to the MMU. >>> >>> - Write to the appropriate TTBR_*. >>> >>> - ISB to complete the write to the TTBR_*. >>> >>> - TLBI to invalidate the old mappings the the TLBs. >>> >>> - DSB to complete the TLBI. >>> >>> - If you've changed the instruction stream or system state that will >>> affect the instruction stream, ISB to flush the CPU pipeline. >>> >>> >> Here is the flow I have (as of v5 patch) >> >> Write the tables >> >> (I removed dsb here in v5, need to add back) >> >> Write TTBR >> >> (I missed isb here, need to add) >> >> Flush dcache (otherwise the table will not be in DDR. Yes, I verified) > > This looks odd -- why do we need the tables to be in DDR? Why would we > flush them here, when the address is partially visible to the MMU? This sounds odd but it actually makes sense. Let's say we have new tables created by u-boot. The new tables are in the address of DDR with D-cache enabled. If we don't flush the cache, the moment TLBs are invalidated, MMU will fetch the tables from TTBR which points to DDR. Without a valid TLB, MMU cannot get the table from D-cache, it has to fetch from the DDR directly. I have verified this by checking waveforms of the SoC and exercised code in both ways. > >> TLBI >> >> (DSB and ISB TLBI function) > > Why another TLBI? Not another one. I just point out the DSB and ISB are part of the existing function in u-boot. > >> >> I will need to send v6 patch to add the missing parts. > > Sure, I'll hold off replies here until then. > v6 is coming soon today. York _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot