| Message ID | 1411481256-29141-2-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 23, 2014 at 04:07:34PM +0200, Boris BREZILLON wrote: > Add a new function to move bits (not bytes) from a memory region to > another one. > This function is similar to memmove except it acts at bit level. > This function is needed to implement GPMI raw access functions, given the > fact that ECC engine does not pad ECC bits to the next byte boundary. sorry for not comment your v2 patch set. > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > --- > drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 88 ++++++++++++++++++++++++++++++++++ > drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 4 ++ > 2 files changed, 92 insertions(+) > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > index 87e658c..e2f706a 100644 > --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > @@ -1353,3 +1353,91 @@ int gpmi_read_page(struct gpmi_nand_data *this, > set_dma_type(this, DMA_FOR_READ_ECC_PAGE); > return start_dma_with_bch_irq(this, desc); > } > + > +void gpmi_move_bits(u8 *dst, size_t dst_bit_off, > + const u8 *src, size_t src_bit_off, > + size_t nbits) we can simplify the code. We could use the bytes to replace the @nbits. The chunk data is always byte aligned. Btw: please add more comments in this function. thanks Huang Shijie
On Tue, 23 Sep 2014 22:54:15 +0800 Huang Shijie <shijie8@gmail.com> wrote: > On Tue, Sep 23, 2014 at 04:07:34PM +0200, Boris BREZILLON wrote: > > Add a new function to move bits (not bytes) from a memory region to > > another one. > > This function is similar to memmove except it acts at bit level. > > This function is needed to implement GPMI raw access functions, given the > > fact that ECC engine does not pad ECC bits to the next byte boundary. > sorry for not comment your v2 patch set. > > > > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > > --- > > drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 88 ++++++++++++++++++++++++++++++++++ > > drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 4 ++ > > 2 files changed, 92 insertions(+) > > > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > > index 87e658c..e2f706a 100644 > > --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > > @@ -1353,3 +1353,91 @@ int gpmi_read_page(struct gpmi_nand_data *this, > > set_dma_type(this, DMA_FOR_READ_ECC_PAGE); > > return start_dma_with_bch_irq(this, desc); > > } > > + > > +void gpmi_move_bits(u8 *dst, size_t dst_bit_off, > > + const u8 *src, size_t src_bit_off, > > + size_t nbits) > we can simplify the code. Any suggestions ? > > We could use the bytes to replace the @nbits. > > The chunk data is always byte aligned. This function is also used to store ECC bits in the OOB buffer and these chunk of data are not byte aligned :-). > > > Btw: please add more comments in this function. Yep, I should definitely comment it, and I'll do. Best Regards, Boris
On Tue, Sep 23, 2014 at 04:58:22PM +0200, Boris BREZILLON wrote: > On Tue, 23 Sep 2014 22:54:15 +0800 > Huang Shijie <shijie8@gmail.com> wrote: > > > On Tue, Sep 23, 2014 at 04:07:34PM +0200, Boris BREZILLON wrote: > > > Add a new function to move bits (not bytes) from a memory region to > > > another one. > > > This function is similar to memmove except it acts at bit level. > > > This function is needed to implement GPMI raw access functions, given the > > > fact that ECC engine does not pad ECC bits to the next byte boundary. > > sorry for not comment your v2 patch set. > > > > > > > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > > > --- > > > drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 88 ++++++++++++++++++++++++++++++++++ > > > drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 4 ++ > > > 2 files changed, 92 insertions(+) > > > > > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > > > index 87e658c..e2f706a 100644 > > > --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > > > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > > > @@ -1353,3 +1353,91 @@ int gpmi_read_page(struct gpmi_nand_data *this, > > > set_dma_type(this, DMA_FOR_READ_ECC_PAGE); > > > return start_dma_with_bch_irq(this, desc); > > > } > > > + > > > +void gpmi_move_bits(u8 *dst, size_t dst_bit_off, > > > + const u8 *src, size_t src_bit_off, > > > + size_t nbits) > > we can simplify the code. > > Any suggestions ? > > > > > We could use the bytes to replace the @nbits. > > > > The chunk data is always byte aligned. > > This function is also used to store ECC bits in the OOB buffer and > these chunk of data are not byte aligned :-). yes. you are right. I missed it. could you also comment these two hooks in the patch set. I hope Brian also can check it, and we can make it more clear about how to implement them. thanks Huang Shijie
On Tue, Sep 23, 2014 at 11:04:28PM +0800, Huang Shijie wrote: > On Tue, Sep 23, 2014 at 04:58:22PM +0200, Boris BREZILLON wrote: > > On Tue, 23 Sep 2014 22:54:15 +0800 > > Huang Shijie <shijie8@gmail.com> wrote: > > > > > On Tue, Sep 23, 2014 at 04:07:34PM +0200, Boris BREZILLON wrote: > > > > Add a new function to move bits (not bytes) from a memory region to > > > > another one. > > > > This function is similar to memmove except it acts at bit level. > > > > This function is needed to implement GPMI raw access functions, given the > > > > fact that ECC engine does not pad ECC bits to the next byte boundary. > > > sorry for not comment your v2 patch set. > > > > > > > > > > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > > > > --- > > > > drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 88 ++++++++++++++++++++++++++++++++++ > > > > drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 4 ++ > > > > 2 files changed, 92 insertions(+) > > > > > > > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > > > > index 87e658c..e2f706a 100644 > > > > --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > > > > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > > > > @@ -1353,3 +1353,91 @@ int gpmi_read_page(struct gpmi_nand_data *this, > > > > set_dma_type(this, DMA_FOR_READ_ECC_PAGE); > > > > return start_dma_with_bch_irq(this, desc); > > > > } > > > > + > > > > +void gpmi_move_bits(u8 *dst, size_t dst_bit_off, > > > > + const u8 *src, size_t src_bit_off, > > > > + size_t nbits) > > > we can simplify the code. > > > > Any suggestions ? > > > > > > > > We could use the bytes to replace the @nbits. > > > > > > The chunk data is always byte aligned. > > > > This function is also used to store ECC bits in the OOB buffer and > > these chunk of data are not byte aligned :-). > yes. you are right. I missed it. > > could you also comment these two hooks in the patch set. I mean the @ecc->read_page_raw and @ecc->write_page_raw. thanks Huang Shijie
Hi Huang, I've added some code comments inline (and I'll squash them in my next version). On Tue, 23 Sep 2014 16:07:34 +0200 Boris BREZILLON <boris.brezillon@free-electrons.com> wrote: > Add a new function to move bits (not bytes) from a memory region to > another one. > This function is similar to memmove except it acts at bit level. > This function is needed to implement GPMI raw access functions, given the > fact that ECC engine does not pad ECC bits to the next byte boundary. > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > --- > drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 88 ++++++++++++++++++++++++++++++++++ > drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 4 ++ > 2 files changed, 92 insertions(+) > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > index 87e658c..e2f706a 100644 > --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > @@ -1353,3 +1353,91 @@ int gpmi_read_page(struct gpmi_nand_data *this, > set_dma_type(this, DMA_FOR_READ_ECC_PAGE); > return start_dma_with_bch_irq(this, desc); > } > + > +void gpmi_move_bits(u8 *dst, size_t dst_bit_off, > + const u8 *src, size_t src_bit_off, > + size_t nbits) > +{ > + size_t i; > + size_t nbytes; > + u32 src_byte = 0; > + /* * Move src and dst pointers to the closest byte pointer and * store bit offsets within a byte. */ > + src += src_bit_off / 8; > + src_bit_off %= 8; > + > + dst += dst_bit_off / 8; > + dst_bit_off %= 8; > + /* * Initialize the src_byte value with bits available in the * first byte of data so that we end up with a byte aligned * src pointer. */ > + if (src_bit_off) { > + src_byte = src[0] >> src_bit_off; > + nbits -= 8 - src_bit_off; > + src++; > + } > + /* * Calculate the number of bytes that can be copied from src to * dst. */ > + nbytes = nbits / 8; > + /* * Try to align dst to a byte boundary by peeking some bits * from the source. */ > + if (dst_bit_off) { > + if (src_bit_off <= dst_bit_off) { > + dst[0] &= GENMASK(dst_bit_off - 1, 0); > + dst[0] |= src_byte << dst_bit_off; > + src_bit_off += (8 - dst_bit_off); > + src_byte >>= (8 - dst_bit_off); > + dst_bit_off = 0; > + dst++; > + } else if (nbytes) { > + src_byte |= src[0] << (8 - src_bit_off); > + dst[0] &= GENMASK(dst_bit_off - 1, 0); > + dst[0] |= src_byte << dst_bit_off; > + src_bit_off += dst_bit_off; > + src_byte >>= (8 - dst_bit_off); > + dst_bit_off = 0; > + dst++; > + nbytes--; > + src++; > + if (src_bit_off > 7) { > + src_bit_off -= 8; > + dst[0] = src_byte; > + dst++; > + src_byte >>= 8; > + } > + } > + } > + > + if (!src_bit_off && !dst_bit_off) { /* * Both src and dst pointers are byte aligned, thus we * can just use the optimized memcpy function */ > + if (nbytes) > + memcpy(dst, src, nbytes); > + } else { /* * src buffer is not byte aligned, hence we have to copy * each src byte to the src_byte variable (after * applying the appropriate shift depending on the * src bit offset). * We still try to work on bytes until there's not * enough available data in the src buffer. */ > + for (i = 0; i < nbytes; i++) { > + src_byte |= src[i] << (8 - src_bit_off); > + dst[i] = src_byte; > + src_byte >>= 8; > + } > + } > + /* move dst and src buffers */ > + dst += nbytes; > + src += nbytes; /* * nbits is the number of remaining bits. It should not exceed * 8 as we've already worked on bytes as much as possible. */ > + nbits %= 8; > + /* * if there's no more bits to copy to the destination and src * buffer was already byte aligned, then we're done. */ > + if (!nbits && !src_bit_off) > + return; > + /* Copy the remaining bits to the src_byte variable */ > + if (nbits) > + src_byte |= (*src & GENMASK(nbits - 1, 0)) << > + ((8 - src_bit_off) % 8); > + nbits += (8 - src_bit_off) % 8; > + /* * There were not enough bits to copy from src to dst to get a * byte aligned dst buffer. In this case prepare the src_byte * variable to match the dst organization (just shift src_byte * by dst bit offset and retrieve least significant bits from * dst). */ > + if (dst_bit_off) > + src_byte = (src_byte << dst_bit_off) | > + (*dst & GENMASK(dst_bit_off - 1, 0)); > + nbits += dst_bit_off; > + /* * Keep most significant bits from dst if we end up with an * unaligned number bits. */ > + if (nbits % 8) > + src_byte |= (dst[nbits / 8] & GENMASK(7, nbits % 8)) << > + (nbits / 8); > + /* Copy the remaining bytes to the destination */ > + nbytes = DIV_ROUND_UP(nbits, 8); > + for (i = 0; i < nbytes; i++) { > + dst[i] = src_byte; > + src_byte >>= 8; > + } > +} > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h > index 32c6ba4..17d0736 100644 > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h > @@ -290,6 +290,10 @@ extern int gpmi_send_page(struct gpmi_nand_data *, > extern int gpmi_read_page(struct gpmi_nand_data *, > dma_addr_t payload, dma_addr_t auxiliary); > > +void gpmi_move_bits(u8 *dst, size_t dst_bit_off, > + const u8 *src, size_t src_bit_off, > + size_t nbits); > + > /* BCH : Status Block Completion Codes */ > #define STATUS_GOOD 0x00 > #define STATUS_ERASED 0xff
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c index 87e658c..e2f706a 100644 --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c @@ -1353,3 +1353,91 @@ int gpmi_read_page(struct gpmi_nand_data *this, set_dma_type(this, DMA_FOR_READ_ECC_PAGE); return start_dma_with_bch_irq(this, desc); } + +void gpmi_move_bits(u8 *dst, size_t dst_bit_off, + const u8 *src, size_t src_bit_off, + size_t nbits) +{ + size_t i; + size_t nbytes; + u32 src_byte = 0; + + src += src_bit_off / 8; + src_bit_off %= 8; + + dst += dst_bit_off / 8; + dst_bit_off %= 8; + + if (src_bit_off) { + src_byte = src[0] >> src_bit_off; + nbits -= 8 - src_bit_off; + src++; + } + + nbytes = nbits / 8; + + if (dst_bit_off) { + if (src_bit_off <= dst_bit_off) { + dst[0] &= GENMASK(dst_bit_off - 1, 0); + dst[0] |= src_byte << dst_bit_off; + src_bit_off += (8 - dst_bit_off); + src_byte >>= (8 - dst_bit_off); + dst_bit_off = 0; + dst++; + } else if (nbytes) { + src_byte |= src[0] << (8 - src_bit_off); + dst[0] &= GENMASK(dst_bit_off - 1, 0); + dst[0] |= src_byte << dst_bit_off; + src_bit_off += dst_bit_off; + src_byte >>= (8 - dst_bit_off); + dst_bit_off = 0; + dst++; + nbytes--; + src++; + if (src_bit_off > 7) { + src_bit_off -= 8; + dst[0] = src_byte; + dst++; + src_byte >>= 8; + } + } + } + + if (!src_bit_off && !dst_bit_off) { + if (nbytes) + memcpy(dst, src, nbytes); + } else { + for (i = 0; i < nbytes; i++) { + src_byte |= src[i] << (8 - src_bit_off); + dst[i] = src_byte; + src_byte >>= 8; + } + } + + dst += nbytes; + src += nbytes; + nbits %= 8; + + if (!nbits && !src_bit_off) + return; + + if (nbits) + src_byte |= (*src & GENMASK(nbits - 1, 0)) << + ((8 - src_bit_off) % 8); + nbits += (8 - src_bit_off) % 8; + + if (dst_bit_off) + src_byte = (src_byte << dst_bit_off) | + (*dst & GENMASK(dst_bit_off - 1, 0)); + nbits += dst_bit_off; + + if (nbits % 8) + src_byte |= (dst[nbits / 8] & GENMASK(7, nbits % 8)) << + (nbits / 8); + + nbytes = DIV_ROUND_UP(nbits, 8); + for (i = 0; i < nbytes; i++) { + dst[i] = src_byte; + src_byte >>= 8; + } +} diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h index 32c6ba4..17d0736 100644 --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h @@ -290,6 +290,10 @@ extern int gpmi_send_page(struct gpmi_nand_data *, extern int gpmi_read_page(struct gpmi_nand_data *, dma_addr_t payload, dma_addr_t auxiliary); +void gpmi_move_bits(u8 *dst, size_t dst_bit_off, + const u8 *src, size_t src_bit_off, + size_t nbits); + /* BCH : Status Block Completion Codes */ #define STATUS_GOOD 0x00 #define STATUS_ERASED 0xff
Add a new function to move bits (not bytes) from a memory region to another one. This function is similar to memmove except it acts at bit level. This function is needed to implement GPMI raw access functions, given the fact that ECC engine does not pad ECC bits to the next byte boundary. Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> --- drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 88 ++++++++++++++++++++++++++++++++++ drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 4 ++ 2 files changed, 92 insertions(+)