Dear Eric Nelson, > On 03/06/2012 12:45 PM, Marek Vasut wrote: > > Dear Eric Nelson, > > > >> On 03/05/2012 01:06 PM, Marek Vasut wrote: > >>> Dear Eric Nelson, > >>> > >>>> ensure that transmit and receive buffers are cache-line aligned > >>>> > >>>> invalidate cache after each packet received > >>>> flush cache before transmitting > >>>> > >>>> Original patch by Marek: > >>>> http://lists.denx.de/pipermail/u-boot/2012-February/117695.html > >>> > >>> Would be cool to Cc me :-p > >> > >> Sorry about that. > > > > It's ok, don't worry about it ;-) > > > > [...] > > > >>> I think this "writel()" call is bogus and should be removed in > >>> subsequent patch and replaced with simple assignment. It was here > >>> probably due to cache issues on PPC? > >> > >> The RBD has me puzzled. Do we treat them like registers and use > >> readx/writex or like in-RAM data structures... > > > > I'd go for the in-RAM data structures, but such conversion should happen > > in a separate patch only AFTER the cache support is in. > > > > [...] > > Sounds good. > > >>>> + if (!fec->rbd_base) { > >>>> + ret = -ENOMEM; > >>>> + goto err2; > >>>> + } > >>>> + memset(fec->rbd_base, 0, size); > >>>> + } > >>> > >>> We might want to flush the descriptors to memory after they have been > >>> inited? > >> > >> Again, good catch. > >> > >> On this topic (initialization of RBD), I had a bit of a concern > >> regarding the initialization of things. > >> > >> In fec_open, the receive buffer descriptors are initialized and the > >> last one set is to 'wrap'. If this code were to execute when the > >> controller is live, bad things would surely happen. > >> > >> I traced through all of the paths I can see, and I believe that > >> we're safe. It appears that fec_halt() will be called prior to > >> any call to fec_init() and fec_open(). > > > > Yes, this will only happen if something went wrong. > > > >> In fec_open() a number of calls to fec_rbd_clean() are made and > >> a single flush_dcache() is made afterwards. > >> > >> While this works and causes less thrashing than per-RBD flushes, > >> I think the code would be simpler if fec_rbd_clean just did the > >> flush itself. This would also remove the need for a separate > >> flush in fec_recv. > > > > Not really, rbd might be (and likely is) smaller than cache line, > > therefore you won't be able to flush single rbd, unless it's cacheline > > aligned, which wastes space. > > > > [...] > > Yeah. Please disregard my comments. I wrote that before I fully > appreciated what was being done in fec_recv(). > > >>>> + invalidate_dcache_range(addr, addr + size); > >>>> + > >>> > >>> The idea here is the following (demo uses 32byte long cachelines): > >>> > >>> [DESC1][DESC2][DESC3][DESC4][DESC5][DESC6] > >>> `------- cacheline --------' > >>> > >>> We want to start retrieving contents of DESC3, therefore "addr" points > >>> to DESC1, "size" is size of cacheline (I hope there's no hardware with > >>> 8 byte cachelines, but this should be ok here). > >>> > >>> NOTE[5]: Here we can invalidate the whole cacheline, because the > >>> descriptors so far were modified only be hardware, not by us. We are > >>> not writing anything there so we won't loose any information. > >>> > >>> NOTE[6]: This invalidation ensures that we always have a fresh copy of > >>> the cacheline containing all the descriptors, therefore we always have > >>> a fresh status of the descriptors we are about to pick. Since this is > >>> a sequential execution, the cache eviction should not kick in here (or > >>> might it?). > >> > >> Another way to look at this is this: > >> After fec_open(), the hardware owns the rbd, and all we should do > >> is read it. In order to make sure we don't have a stale copy, we > >> need to call invalidate() before looking at the values. > >> > >> Tracing the code to find out whether this is true, the only write I see > >> is within fec_recv() when the last descriptor is full, when the driver > >> takes ownership of **all** of the descriptors, calling fec_rbd_clean() > >> on each. > >> > >> The only thing that looks funky is this: > >> size = (CONFIG_FEC_ALIGN / sizeof(struct fec_bd)) - 1; > >> if ((fec->rbd_index& size) == size) { > >> > >> Wouldn't a test of rbd_index against FEC_RBD_NUM be more appropriate? > >> i.e. > >> > >> if (fec->rbd_index == FEC_RBD_NUM-1) { > > > > I believe the FEC doesn't always start from rbd_index == 0, and if you > > were to receive more than 64 rbds between open() and close(), this > > implementation works, your would fail. > > Yep. Disregard that too. > > <snip> > > >>> The solution is the following: > >>> > >>> 1) Compute how many descriptors are per-cache line > >>> 2) Make sure FEC_RBD_NUM * sizeof(struct fec_bd) is at least 2 * > >>> CONFIG_FEC_DATA_ALIGNMENT in size, see NOTE[11]. > >>> 3) Once the last RX buffer in the cacheline is processed, mark them all > >>> clean and flush them all, see NOTE[10]. > >>> > >>> NOTE[10]: This is legal, because the hardware won't use RX descriptors > >>> that it marked dirty (which means not picked up by software yet). We > >>> clean the desciptors in an order the hardware would pick them up again > >>> so there's no problem with race condition either. The only possible > >>> issue here is if there was hardware with cacheline size smaller than > >>> descriptor size (we should add a check for this at the begining of the > >>> file). > >>> > >>> NOTE[11]: This is because we want the FEC to overwrite descriptors > >>> below the other cacheline while we're marking the one containing > >>> retrieved descriptors clean. > >> > >> Ahah! Now I see what the size calculation is doing. > >> > >> A well-named constant, maybe "RXDESC_PER_CACHELINE" would be useful > >> here. > > > > Yes > > > >> #define RXDESC_PER_CACHELINE (CONFIG_FEC_ALIGN/sizeof(struct fec_bd)) > >> > >>>> - fec_rbd_clean(fec->rbd_index == (FEC_RBD_NUM - 1) ? 1 : 0, rbd); > >>>> + size = (CONFIG_FEC_DATA_ALIGNMENT / sizeof(struct fec_bd)) - 1; > >>>> > >> size = RXDESC_PER_CACHELINE-1; > >> > >>>> + if ((fec->rbd_index& size) == size) { > >> > >> The line above only works if RXDESC_PER_CACHELINE is a multiple of 2, > >> which is likely to work because sizeof(struct fec_bd) == 8. > > > > Adding such a comment (and maybe CPP check) won't hurt. > > I'm struggling getting the CPP to do the work at the moment... > > >>>> + i = fec->rbd_index - size; > >>>> + addr = (uint32_t)&fec->rbd_base[i]; > >>>> + for (; i<= fec->rbd_index + size; i++) { > >> > >> This flushes too many descriptors! This should be: > >> for (; i<= fec->rbd_index; i++) { > > > > Agreed > > V3 patch forthcoming. > > >>> Uh, this was tough. > >> > >> How bad do we want cache? > > > > We're almost there, why do you ask? :-) > > I was just bein' snarky... > > I'm making a couple of other small changes in V3: > > - change fec_rbd_clean to only have a single > call to write() and make it clearer that there's > only one additional flag iff 'last' > - use comparison to supply 'last' parameter in > the call to fec_rbd_clean > > I think each of these makes the intent clearer. > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > index d5d0d5e..f8691d4 100644 > --- a/drivers/net/fec_mxc.c > +++ b/drivers/net/fec_mxc.c > @@ -355,16 +355,10 @@ static void fec_tbd_init(struct fec_priv *fec) > */ > static void fec_rbd_clean(int last, struct fec_bd *pRbd) > { > - /* > - * Reset buffer descriptor as empty > - */ > + unsigned short flags = FEC_RBD_EMPTY; > if (last) > - writew(FEC_RBD_WRAP | FEC_RBD_EMPTY, &pRbd->status); > - else > - writew(FEC_RBD_EMPTY, &pRbd->status); > - /* > - * no data in it > - */ > + flags |= FEC_RBD_WRAP; > + writew(flags, &pRbd->status); > writew(0, &pRbd->data_length); > } > > @@ -880,10 +874,8 @@ static int fec_recv(struct eth_device *dev) > i = fec->rbd_index - size; > addr = (uint32_t)&fec->rbd_base[i]; > for (; i <= fec->rbd_index ; i++) { > - if (i == (FEC_RBD_NUM - 1)) > - fec_rbd_clean(1, > &fec->rbd_base[i]); - else > - fec_rbd_clean(0, > &fec->rbd_base[i]); + fec_rbd_clean(i == > (FEC_RBD_NUM - 1), + > &fec->rbd_base[i]); > } > flush_dcache_range(addr, > addr + CONFIG_FEC_ALIGN);
Looking forward to V3! _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot