[v2,10/19] spi: mpc8xxx: Simplify logic a bit
diff mbox series

Message ID 20190428202854.8590-11-jagan@amarulasolutions.com
State New
Headers show
Series
  • [v2,01/19] spi: mpc8xxx: Use short type names
Related show

Commit Message

Jagan Teki April 28, 2019, 8:28 p.m. UTC 
From: Mario Six <mario.six@gdsys.cc>

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 <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Comments

Joakim Tjernlund April 29, 2019, 9:17 a.m. UTC | #1 
On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> 
> From: Mario Six <mario.six@gdsys.cc>
> 
> 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 <mario.six@gdsys.cc>
> ---
>  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(&spi->rx);
> -                               setbits_be32(&spi->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(&spi->rx);
> +                       setbits_be32(&spi->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-boot&amp;data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7Cb423ca475f53471860b308d6cc195be8%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C636920806635383891&amp;sdata=PxiqErmkjcpBVL4yBUi2UYiJ5oqtBTI4fCnb4XBTpmE%3D&amp;reserved=0
Jagan Teki April 29, 2019, 10:41 a.m. UTC | #2 
+ Mario

On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
<Joakim.Tjernlund@infinera.com> wrote:
>
> On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> >
> > From: Mario Six <mario.six@gdsys.cc>
> >
> > 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?
Mario Six May 2, 2019, 5:31 a.m. UTC | #3 
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 <jagan@amarulasolutions.com> wrote:
>
> + Mario
>
> On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> <Joakim.Tjernlund@infinera.com> wrote:
> >
> > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > >
> > > From: Mario Six <mario.six@gdsys.cc>
> > >
> > > 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
Joakim Tjernlund May 2, 2019, 9:07 a.m. UTC | #4 
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 <jagan@amarulasolutions.com> wrote:
> > + Mario
> > 
> > On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> > <Joakim.Tjernlund@infinera.com> wrote:
> > > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > > > From: Mario Six <mario.six@gdsys.cc>
> > > > 
> > > > 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
Jagan Teki May 14, 2019, 1:53 p.m. UTC | #5 
On Thu, May 2, 2019 at 2:37 PM Joakim Tjernlund
<Joakim.Tjernlund@infinera.com> 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 <jagan@amarulasolutions.com> wrote:
> > > + Mario
> > >
> > > On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> > > <Joakim.Tjernlund@infinera.com> wrote:
> > > > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > > > > From: Mario Six <mario.six@gdsys.cc>
> > > > >
> > > > > 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?
Mario Six May 15, 2019, 5:02 a.m. UTC | #6 
On Tue, May 14, 2019 at 3:53 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Thu, May 2, 2019 at 2:37 PM Joakim Tjernlund
> <Joakim.Tjernlund@infinera.com> 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 <jagan@amarulasolutions.com> wrote:
> > > > + Mario
> > > >
> > > > On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> > > > <Joakim.Tjernlund@infinera.com> wrote:
> > > > > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > > > > > From: Mario Six <mario.six@gdsys.cc>
> > > > > >
> > > > > > 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
Joakim Tjernlund May 15, 2019, 7:16 a.m. UTC | #7 
On Wed, 2019-05-15 at 07:02 +0200, Mario Six wrote:
> On Tue, May 14, 2019 at 3:53 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > On Thu, May 2, 2019 at 2:37 PM Joakim Tjernlund
> > <Joakim.Tjernlund@infinera.com> 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 <jagan@amarulasolutions.com> wrote:
> > > > > + Mario
> > > > > 
> > > > > On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> > > > > <Joakim.Tjernlund@infinera.com> wrote:
> > > > > > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > > > > > > From: Mario Six <mario.six@gdsys.cc>
> > > > > > > 
> > > > > > > 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

Patch
diff mbox series 

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(&spi->rx);
-				setbits_be32(&spi->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(&spi->rx);
+			setbits_be32(&spi->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__);