Re: [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
On Wed, 2019-05-15 at 07:02 +0200, Mario Six wrote:
> On Tue, May 14, 2019 at 3:53 PM Jagan Teki wrote:
> > On Thu, May 2, 2019 at 2:37 PM Joakim Tjernlund
> > wrote:
> > > On Thu, 2019-05-02 at 07:31 +0200, Mario Six wrote:
> > > > CAUTION: This email originated from outside of the organization. Do not
> > > > click links or open attachments unless you recognize the sender and
> > > > know the content is safe.
> > > >
> > > >
> > > > Hi Jagan and Jocke,
> > > >
> > > > I'm back from vacation, so here's my answer:
> > > >
> > > > On Mon, Apr 29, 2019 at 12:41 PM Jagan Teki
> > > > wrote:
> > > > > + Mario
> > > > >
> > > > > On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> > > > > wrote:
> > > > > > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > > > > > > From: Mario Six
> > > > > > >
> > > > > > > We do nothing in the loop if the "not empty" event was not
> > > > > > > detected. To
> > > > > > > simplify the logic, check if this is the case, and skip the
> > > > > > > execution of
> > > > > > > the loop early to reduce the nesting level and flag checking.
> > > > > >
> > > > > > Looked at the driver to refresh memory and noticed:
> > > > > > if (charSize == 32) {
> > > > > > /* Advance output buffer by 32 bits */
> > > > > > din += 4;
> > > > > > }
> > > > > > which suggests that only 32 bit char will increase the din ptr so
> > > > > > does other bitlens
> > > > > > work for reading?
> > > > Yes, It will work. When charSize < 32 in a loop execution, we
> > > > necessarily also
> > > > have numBlks == 0, since numBlks = DIV_ROUND_UP(bitlen, 32) and
> > > > charSize =
> > > > (bitlen >= 32 ? 32 : bitlen); in other words: we're at the very end of
> > > > the data
> > > > to be sent/received and also the final loop execution, so we don't have
> > > > to
> > > > advance the pointer anymore.
> > >
> > > Ahh, I see now.
> > >
> > > But this over use of always 32 bits cause complexity/subtile bugs. The
> > > driver should use
> > > the requested charsize(wordlen) throughout. As is now you cannot use the
> > > NF/NE flag as intended or
> > > toggle LSB_FIRST
> >
> > Look like Joakim has a point here. better we can check the required
> > charsize instead of looking for magic 32 number. What do you say
> > Mario?
>
> I agree, the driver code could be made more readable and cleaned up a bit, but
> I don't know if it's worth postponing this series for a code readability issue
> that's not a functional bug; I'd say that's an optimization that could be made
> later on.
I would not call it just an optimization but I am fine with applying this patch.
Jocke
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
On Tue, May 14, 2019 at 3:53 PM Jagan Teki wrote:
>
> On Thu, May 2, 2019 at 2:37 PM Joakim Tjernlund
> wrote:
> >
> > On Thu, 2019-05-02 at 07:31 +0200, Mario Six wrote:
> > > CAUTION: This email originated from outside of the organization. Do not
> > > click links or open attachments unless you recognize the sender and know
> > > the content is safe.
> > >
> > >
> > > Hi Jagan and Jocke,
> > >
> > > I'm back from vacation, so here's my answer:
> > >
> > > On Mon, Apr 29, 2019 at 12:41 PM Jagan Teki
> > > wrote:
> > > > + Mario
> > > >
> > > > On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> > > > wrote:
> > > > > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > > > > > From: Mario Six
> > > > > >
> > > > > > We do nothing in the loop if the "not empty" event was not
> > > > > > detected. To
> > > > > > simplify the logic, check if this is the case, and skip the
> > > > > > execution of
> > > > > > the loop early to reduce the nesting level and flag checking.
> > > > >
> > > > > Looked at the driver to refresh memory and noticed:
> > > > > if (charSize == 32) {
> > > > > /* Advance output buffer by 32 bits */
> > > > > din += 4;
> > > > > }
> > > > > which suggests that only 32 bit char will increase the din ptr so
> > > > > does other bitlens
> > > > > work for reading?
> > > Yes, It will work. When charSize < 32 in a loop execution, we necessarily
> > > also
> > > have numBlks == 0, since numBlks = DIV_ROUND_UP(bitlen, 32) and charSize =
> > > (bitlen >= 32 ? 32 : bitlen); in other words: we're at the very end of
> > > the data
> > > to be sent/received and also the final loop execution, so we don't have to
> > > advance the pointer anymore.
> >
> > Ahh, I see now.
> >
> > But this over use of always 32 bits cause complexity/subtile bugs. The
> > driver should use
> > the requested charsize(wordlen) throughout. As is now you cannot use the
> > NF/NE flag as intended or
> > toggle LSB_FIRST
>
> Look like Joakim has a point here. better we can check the required
> charsize instead of looking for magic 32 number. What do you say
> Mario?
I agree, the driver code could be made more readable and cleaned up a bit, but
I don't know if it's worth postponing this series for a code readability issue
that's not a functional bug; I'd say that's an optimization that could be made
later on.
What do you guys think?
Best regards,
Mario
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
On Thu, May 2, 2019 at 2:37 PM Joakim Tjernlund
wrote:
>
> On Thu, 2019-05-02 at 07:31 +0200, Mario Six wrote:
> > CAUTION: This email originated from outside of the organization. Do not
> > click links or open attachments unless you recognize the sender and know
> > the content is safe.
> >
> >
> > Hi Jagan and Jocke,
> >
> > I'm back from vacation, so here's my answer:
> >
> > On Mon, Apr 29, 2019 at 12:41 PM Jagan Teki
> > wrote:
> > > + Mario
> > >
> > > On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> > > wrote:
> > > > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > > > > From: Mario Six
> > > > >
> > > > > We do nothing in the loop if the "not empty" event was not detected.
> > > > > To
> > > > > simplify the logic, check if this is the case, and skip the execution
> > > > > of
> > > > > the loop early to reduce the nesting level and flag checking.
> > > >
> > > > Looked at the driver to refresh memory and noticed:
> > > > if (charSize == 32) {
> > > > /* Advance output buffer by 32 bits */
> > > > din += 4;
> > > > }
> > > > which suggests that only 32 bit char will increase the din ptr so does
> > > > other bitlens
> > > > work for reading?
> > Yes, It will work. When charSize < 32 in a loop execution, we necessarily
> > also
> > have numBlks == 0, since numBlks = DIV_ROUND_UP(bitlen, 32) and charSize =
> > (bitlen >= 32 ? 32 : bitlen); in other words: we're at the very end of the
> > data
> > to be sent/received and also the final loop execution, so we don't have to
> > advance the pointer anymore.
>
> Ahh, I see now.
>
> But this over use of always 32 bits cause complexity/subtile bugs. The driver
> should use
> the requested charsize(wordlen) throughout. As is now you cannot use the
> NF/NE flag as intended or
> toggle LSB_FIRST
Look like Joakim has a point here. better we can check the required
charsize instead of looking for magic 32 number. What do you say
Mario?
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
On Thu, 2019-05-02 at 07:31 +0200, Mario Six wrote:
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you recognize the sender and know the
> content is safe.
>
>
> Hi Jagan and Jocke,
>
> I'm back from vacation, so here's my answer:
>
> On Mon, Apr 29, 2019 at 12:41 PM Jagan Teki
> wrote:
> > + Mario
> >
> > On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> > wrote:
> > > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > > > From: Mario Six
> > > >
> > > > We do nothing in the loop if the "not empty" event was not detected. To
> > > > simplify the logic, check if this is the case, and skip the execution of
> > > > the loop early to reduce the nesting level and flag checking.
> > >
> > > Looked at the driver to refresh memory and noticed:
> > > if (charSize == 32) {
> > > /* Advance output buffer by 32 bits */
> > > din += 4;
> > > }
> > > which suggests that only 32 bit char will increase the din ptr so does
> > > other bitlens
> > > work for reading?
> Yes, It will work. When charSize < 32 in a loop execution, we necessarily also
> have numBlks == 0, since numBlks = DIV_ROUND_UP(bitlen, 32) and charSize =
> (bitlen >= 32 ? 32 : bitlen); in other words: we're at the very end of the
> data
> to be sent/received and also the final loop execution, so we don't have to
> advance the pointer anymore.
Ahh, I see now.
But this over use of always 32 bits cause complexity/subtile bugs. The driver
should use
the requested charsize(wordlen) throughout. As is now you cannot use the NF/NE
flag as intended or
toggle LSB_FIRST
I think fsl_espi driver is worse though.
Jocke
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
Hi Jagan and Jocke,
I'm back from vacation, so here's my answer:
On Mon, Apr 29, 2019 at 12:41 PM Jagan Teki wrote:
>
> + Mario
>
> On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> wrote:
> >
> > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > >
> > > From: Mario Six
> > >
> > > We do nothing in the loop if the "not empty" event was not detected. To
> > > simplify the logic, check if this is the case, and skip the execution of
> > > the loop early to reduce the nesting level and flag checking.
> >
> > Looked at the driver to refresh memory and noticed:
> > if (charSize == 32) {
> > /* Advance output buffer by 32 bits */
> > din += 4;
> > }
> > which suggests that only 32 bit char will increase the din ptr so does
> > other bitlens
> > work for reading?
>
Yes, It will work. When charSize < 32 in a loop execution, we necessarily also
have numBlks == 0, since numBlks = DIV_ROUND_UP(bitlen, 32) and charSize =
(bitlen >= 32 ? 32 : bitlen); in other words: we're at the very end of the data
to be sent/received and also the final loop execution, so we don't have to
advance the pointer anymore.
> Mario, can you respond this?
Best regards,
Mario
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
+ Mario
On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
wrote:
>
> On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> >
> > From: Mario Six
> >
> > We do nothing in the loop if the "not empty" event was not detected. To
> > simplify the logic, check if this is the case, and skip the execution of
> > the loop early to reduce the nesting level and flag checking.
>
> Looked at the driver to refresh memory and noticed:
> if (charSize == 32) {
> /* Advance output buffer by 32 bits */
> din += 4;
> }
> which suggests that only 32 bit char will increase the din ptr so does other
> bitlens
> work for reading?
Mario, can you respond this?
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
>
> From: Mario Six
>
> We do nothing in the loop if the "not empty" event was not detected. To
> simplify the logic, check if this is the case, and skip the execution of
> the loop early to reduce the nesting level and flag checking.
Looked at the driver to refresh memory and noticed:
if (charSize == 32) {
/* Advance output buffer by 32 bits */
din += 4;
}
which suggests that only 32 bit char will increase the din ptr so does other
bitlens
work for reading?
Jocke
>
> Signed-off-by: Mario Six
> ---
> drivers/spi/mpc8xxx_spi.c | 23 +--
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
> index 962ef710f8..a2e698ea17 100644
> --- a/drivers/spi/mpc8xxx_spi.c
> +++ b/drivers/spi/mpc8xxx_spi.c
> @@ -149,25 +149,28 @@ int spi_xfer(struct spi_slave *slave, uint bitlen,
> const void *dout, void *din,
> bool have_ne = event & SPI_EV_NE;
> bool have_nf = event & SPI_EV_NF;
>
> - if (have_ne) {
> - tmpdin = in_be32(>rx);
> - setbits_be32(>event, SPI_EV_NE);
> -
> - *(u32 *)din = (tmpdin << (32 - char_size));
> - if (char_size == 32) {
> - /* Advance output buffer by 32 bits */
> - din += 4;
> - }
> + if (!have_ne)
> + continue;
> +
> + tmpdin = in_be32(>rx);
> + setbits_be32(>event, SPI_EV_NE);
> +
> + *(u32 *)din = (tmpdin << (32 - char_size));
> + if (char_size == 32) {
> + /* Advance output buffer by 32 bits */
> + din += 4;
> }
> +
> /*
> * Only bail when we've had both NE and NF events.
> * This will cause timeouts on RO devices, so maybe
> * in the future put an arbitrary delay after writing
> * the device. Arbitrary delays suck, though...
> */
> - if (have_ne && have_nf)
> + if (have_nf)
> break;
> }
> +
> if (tm >= SPI_TIMEOUT)
> debug("*** %s: Time out during SPI transfer\n",
> __func__);
> --
> 2.18.0.321.gffc6fa0e3
>
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.denx.de%2Flistinfo%2Fu-bootdata=02%7C01%7Cjoakim.tjernlund%40infinera.com%7Cb423ca475f53471860b308d6cc195be8%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C636920806635383891sdata=PxiqErmkjcpBVL4yBUi2UYiJ5oqtBTI4fCnb4XBTpmE%3Dreserved=0
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
From: Mario Six
We do nothing in the loop if the "not empty" event was not detected. To
simplify the logic, check if this is the case, and skip the execution of
the loop early to reduce the nesting level and flag checking.
Signed-off-by: Mario Six
---
drivers/spi/mpc8xxx_spi.c | 23 +--
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 962ef710f8..a2e698ea17 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -149,25 +149,28 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const
void *dout, void *din,
bool have_ne = event & SPI_EV_NE;
bool have_nf = event & SPI_EV_NF;
- if (have_ne) {
- tmpdin = in_be32(>rx);
- setbits_be32(>event, SPI_EV_NE);
-
- *(u32 *)din = (tmpdin << (32 - char_size));
- if (char_size == 32) {
- /* Advance output buffer by 32 bits */
- din += 4;
- }
+ if (!have_ne)
+ continue;
+
+ tmpdin = in_be32(>rx);
+ setbits_be32(>event, SPI_EV_NE);
+
+ *(u32 *)din = (tmpdin << (32 - char_size));
+ if (char_size == 32) {
+ /* Advance output buffer by 32 bits */
+ din += 4;
}
+
/*
* Only bail when we've had both NE and NF events.
* This will cause timeouts on RO devices, so maybe
* in the future put an arbitrary delay after writing
* the device. Arbitrary delays suck, though...
*/
- if (have_ne && have_nf)
+ if (have_nf)
break;
}
+
if (tm >= SPI_TIMEOUT)
debug("*** %s: Time out during SPI transfer\n",
__func__);
--
2.18.0.321.gffc6fa0e3
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
