From owner-freebsd-mips@FreeBSD.ORG Wed Apr 17 19:12:27 2013 Return-Path: Delivered-To: freebsd-mips@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 1BCF4E99; Wed, 17 Apr 2013 19:12:27 +0000 (UTC) (envelope-from neelnatu@gmail.com) Received: from mail-ie0-x232.google.com (mail-ie0-x232.google.com [IPv6:2607:f8b0:4001:c03::232]) by mx1.freebsd.org (Postfix) with ESMTP id DB231FC8; Wed, 17 Apr 2013 19:12:26 +0000 (UTC) Received: by mail-ie0-f178.google.com with SMTP id aq17so886446iec.37 for ; Wed, 17 Apr 2013 12:12:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type; bh=NOFQa3S8RCtSDBbGBjXgCYhSaQ8b9FbG8PPC4OIPiSA=; b=cBTBKLC0KPhawtJFQDWfbPOie9hiJ5/8m0z8zFb8Ei5itJFt2GCta/cp+i1RhQIEHG zg3hACr3a7sYALiLJgdKs0e+QuXLLYnoCSr80x+fYG3XdJ2VHZDsURJtc/aL1I6RJa2w VXJeFpXxLD1X7pIvCsgmr55pgbCsC7SRfPIY4sdUyD6yJWHJhh4bbbjLUatbHm1t9w2q hpl+8BGYh0RVfs1h7lBqz3G9M5BZVNJdULCRUNcax2Qo8Pr6qkCeIDHAkNOxx4MoSkvw uYUQHlDRiyAsk57AcIOUdfPlZU9E3zaFBkz1SquO0J5ffQH6nE8cW4jh/19/+fOYjnTw L6kw== MIME-Version: 1.0 X-Received: by 10.50.213.97 with SMTP id nr1mr11098211igc.36.1366225946615; Wed, 17 Apr 2013 12:12:26 -0700 (PDT) Received: by 10.43.9.138 with HTTP; Wed, 17 Apr 2013 12:12:26 -0700 (PDT) In-Reply-To: <20130417160213.GB17916@lor.one-eyed-alien.net> References: <20130417010835.GA17779@lor.one-eyed-alien.net> <20130417160213.GB17916@lor.one-eyed-alien.net> Date: Wed, 17 Apr 2013 12:12:26 -0700 Message-ID: Subject: Re: [PATCH] partial 64-bit bus space generic implementation From: Neel Natu To: Brooks Davis Content-Type: text/plain; charset=ISO-8859-1 X-Content-Filtered-By: Mailman/MimeDel 2.1.14 Cc: freebsd-mips@freebsd.org X-BeenThere: freebsd-mips@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Porting FreeBSD to MIPS List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Apr 2013 19:12:27 -0000 Hi, On Wed, Apr 17, 2013 at 9:02 AM, Brooks Davis wrote: > On Wed, Apr 17, 2013 at 09:42:32AM -0600, Warner Losh wrote: > > > > On Apr 16, 2013, at 7:08 PM, Brooks Davis wrote: > > > > > I've implemented generic_bs_*_8() for a subset of MIPS platforms > > > (!sibyte and !o32 ABI) based on the mips3_ld and mips3_sd code in > > > cpufunc.h. I've verified that simple reads and writes work for our > > > MIPS64 ISA CPU. The patch passes make universe, but I've not tested it > > > on any mips32 systems. Any reviews or objections to this patch? > > > > > > -- Brooks > > > > > > http://people.freebsd.org/~brooks/patches/mips-bs8.diff > > > > > > MFP4 223084: > > > > > > Partially implement generic_bs_*_8() for MIPS platforms. > > > > > > This is known to work with TARGET_ARCH=mips64 with FreeBSD/BERI. > > > Assuming that other definitions in cpufunc.h are correct it will work > on > > > non-o64 ABI systems except sibyte. On sibyte and o32 systems > > > generic_bs_*_8() will remain panic() implementations. > > > > Why not include sibyte? Doesn't it have LD/SD instructions in 64-bit > mode? > > Because the sibyte implementation uses sb_big_endian_read## macros and > there is no 64-bit macro available. > Yup, that's right. We access the PCI address space through the "match bit lanes" window. Accessing PCI memory through this window does the endian-swap before loading the value into the processor registers. So, it hides the little-endian nature of the PCI bus from the big-endian processor. This works fine for accesses that are 32, 16 or 8 bits in size. The problem is that a 64-bit access is treated as two independent 32-bit accesses from the "endian-swap" point of view so the right thing does not happen transparently to software. We could fix it by swapping the two 32-bit words in the 64-bit dword after the readd() or before the writed(). I'll be happy to provide a patch if someone has the equipment to verify that it works. best Neel > > > > Index: sys/mips/mips/bus_space_generic.c > > > =================================================================== > > > --- sys/mips/mips/bus_space_generic.c (revision 249555) > > > +++ sys/mips/mips/bus_space_generic.c (working copy) > > > @@ -202,9 +202,11 @@ > > > #define rd8(a) cvmx_read64_uint8(a) > > > #define rd16(a) cvmx_read64_uint16(a) > > > #define rd32(a) cvmx_read64_uint32(a) > > > +#define rd64(a) cvmx_read64_uint64(a) > > > #define wr8(a, v) cvmx_write64_uint8(a, v) > > > #define wr16(a, v) cvmx_write64_uint16(a, v) > > > #define wr32(a, v) cvmx_write64_uint32(a, v) > > > +#define wr64(a, v) cvmx_write64_uint64(a, v) > > > #elif defined(CPU_SB1) && _BYTE_ORDER == _BIG_ENDIAN > > > #include > > > #define rd8(a) sb_big_endian_read8(a) > > > @@ -217,10 +219,16 @@ > > > #define rd8(a) readb(a) > > > #define rd16(a) readw(a) > > > #define rd32(a) readl(a) > > > +#ifdef readd > > > +#define rd64(a) readd((a)) > > > +#endif > > > #define wr8(a, v) writeb(a, v) > > > #define wr16(a, v) writew(a, v) > > > #define wr32(a, v) writel(a, v) > > > +#ifdef writed > > > +#define wr64(a, v) writed((uint64_t *)(a), v) > > > #endif > > > +#endif > > > > > > /* generic bus_space tag */ > > > bus_space_tag_t mips_bus_space_generic = &generic_space; > > > @@ -297,7 +305,11 @@ > > > generic_bs_r_8(void *t, bus_space_handle_t handle, bus_size_t offset) > > > { > > > > > > +#ifdef rd64 > > > + return(rd64(handle + offset)); > > > +#else > > > panic("%s: not implemented", __func__); > > > +#endif > > > } > > > > > > void > > > @@ -333,8 +345,14 @@ > > > generic_bs_rm_8(void *t, bus_space_handle_t bsh, bus_size_t offset, > > > uint64_t *addr, size_t count) > > > { > > > +#ifdef rd64 > > > + bus_addr_t baddr = bsh + offset; > > > > > > + while (count--) > > > + *addr++ = rd64(baddr); > > > +#else > > > panic("%s: not implemented", __func__); > > > +#endif > > > } > > > > > > /* > > > @@ -382,8 +400,16 @@ > > > generic_bs_rr_8(void *t, bus_space_handle_t bsh, bus_size_t offset, > > > uint64_t *addr, size_t count) > > > { > > > +#ifdef rd64 > > > + bus_addr_t baddr = bsh + offset; > > > > > > + while (count--) { > > > + *addr++ = rd64(baddr); > > > + baddr += 8; > > > + } > > > +#else > > > panic("%s: not implemented", __func__); > > > +#endif > > > } > > > > > > /* > > > @@ -419,7 +445,11 @@ > > > uint64_t value) > > > { > > > > > > +#ifdef wr64 > > > + wr64(bsh + offset, value); > > > +#else > > > panic("%s: not implemented", __func__); > > > +#endif > > > } > > > > > > /* > > > @@ -460,8 +490,14 @@ > > > generic_bs_wm_8(void *t, bus_space_handle_t bsh, bus_size_t offset, > > > const uint64_t *addr, size_t count) > > > { > > > +#ifdef wr64 > > > + bus_addr_t baddr = bsh + offset; > > > > > > + while (count--) > > > + wr64(baddr, *addr++); > > > +#else > > > panic("%s: not implemented", __func__); > > > +#endif > > > } > > > > > > /* > > > @@ -508,8 +544,16 @@ > > > generic_bs_wr_8(void *t, bus_space_handle_t bsh, bus_size_t offset, > > > const uint64_t *addr, size_t count) > > > { > > > +#ifdef wr64 > > > + bus_addr_t baddr = bsh + offset; > > > > > > + while (count--) { > > > + wr64(baddr, *addr++); > > > + baddr += 8; > > > + } > > > +#else > > > panic("%s: not implemented", __func__); > > > +#endif > > > } > > > > > > /* > > > @@ -550,8 +594,14 @@ > > > generic_bs_sm_8(void *t, bus_space_handle_t bsh, bus_size_t offset, > > > uint64_t value, size_t count) > > > { > > > +#ifdef wr64 > > > + bus_addr_t addr = bsh + offset; > > > > > > + while (count--) > > > + wr64(addr, value); > > > +#else > > > panic("%s: not implemented", __func__); > > > +#endif > > > } > > > > > > /* > > > @@ -592,8 +642,14 @@ > > > generic_bs_sr_8(void *t, bus_space_handle_t bsh, bus_size_t offset, > > > uint64_t value, size_t count) > > > { > > > +#ifdef wr64 > > > + bus_addr_t addr = bsh + offset; > > > > > > + for (; count != 0; count--, addr += 8) > > > + wr64(addr, value); > > > +#else > > > panic("%s: not implemented", __func__); > > > +#endif > > > } > > > > > > /* > > > @@ -664,8 +720,23 @@ > > > generic_bs_c_8(void *t, bus_space_handle_t bsh1, bus_size_t off1, > > > bus_space_handle_t bsh2, bus_size_t off2, size_t count) > > > { > > > +#if defined(rd64) && defined(wr64) > > > + bus_addr_t addr1 = bsh1 + off1; > > > + bus_addr_t addr2 = bsh2 + off2; > > > > > > + if (addr1 >= addr2) { > > > + /* src after dest: copy forward */ > > > + for (; count != 0; count--, addr1 += 8, addr2 += 8) > > > + wr64(addr2, rd64(addr1)); > > > + } else { > > > + /* dest after src: copy backwards */ > > > + for (addr1 += 8 * (count - 1), addr2 += 8 * (count - 1); > > > + count != 0; count--, addr1 -= 8, addr2 -= 8) > > > + wr64(addr2, rd64(addr1)); > > > + } > > > +#else > > > panic("%s: not implemented", __func__); > > > +#endif > > > } > > > > > > void > > > Index: sys/mips/include/cpufunc.h > > > =================================================================== > > > --- sys/mips/include/cpufunc.h (revision 249555) > > > +++ sys/mips/include/cpufunc.h (working copy) > > > @@ -354,9 +354,15 @@ > > > #define readb(va) (*(volatile uint8_t *) (va)) > > > #define readw(va) (*(volatile uint16_t *) (va)) > > > #define readl(va) (*(volatile uint32_t *) (va)) > > > +#if defined(__GNUC__) && !defined(__mips_o32) > > > +#define readd(a) (*(volatile uint64_t *)(a)) > > > +#endif > > > > Why __GNU_C__ here? > > Because that's the ifdef that was used a few lines up around this: > > #if defined(__GNUC__) && !defined(__mips_o32) > #define mips3_ld(a) (*(const volatile uint64_t *)(a)) > #define mips3_sd(a, v) (*(volatile uint64_t *)(a) = (v)) > #else > uint64_t mips3_ld(volatile uint64_t *va); > void mips3_sd(volatile uint64_t *, uint64_t); > #endif /* __GNUC__ */ > > It's not at all clear to me that's the right ifdef, but someone once > thought it was so I went with it. I'm happy to go with a more appropriate > ifdef. For my purposes something that pinned it to mips64 would be fine > since supporting mips32 on our platform is a non-goal. > > Thanks, > Brooks >