| Message ID | 1443053976-9112-2-git-send-email-vikas.manocha@st.com |
|---|---|
| State | Deferred |
| Delegated to: | Marek Vasut |
| Headers | show |
Dear Vikas, In message <1443053976-9112-2-git-send-email-vikas.manocha@st.com> you wrote: > No need to configure indirect trigger address for every read/write. ... > /* Indirect mode configurations */ > writel((plat->sram_size/2), plat->regbase + CQSPI_REG_SRAMPARTITION); > + writel((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK, > + plat->regbase + CQSPI_REG_INDIRECTTRIGGER); I did not mention this explicitly, so I do it here: Please fix this type cast issue globally, in all your patches. Thanks. Best regards, Wolfgang Denk
Thanks Wolfgang, > -----Original Message----- > From: Wolfgang Denk [mailto:wd@denx.de] > Sent: Thursday, September 24, 2015 12:22 AM > To: Vikas MANOCHA > Cc: u-boot@lists.denx.de; sr@denx.de; grmoore@opensource.altera.com; > jteki@openedev.com; marex@denx.de > Subject: Re: [U-Boot] [PATCH v7 1/6] spi: cadence_qspi: move trigger base > configuration in init > > Dear Vikas, > > In message <1443053976-9112-2-git-send-email-vikas.manocha@st.com> > you wrote: > > No need to configure indirect trigger address for every read/write. > ... > > /* Indirect mode configurations */ > > writel((plat->sram_size/2), plat->regbase + > > CQSPI_REG_SRAMPARTITION); > > + writel((u32)plat->ahbbase & > CQSPI_INDIRECTTRIGGER_ADDR_MASK, > > + plat->regbase + CQSPI_REG_INDIRECTTRIGGER); > > I did not mention this explicitly, so I do it here: > > Please fix this type cast issue globally, in all your patches. I agree it should be done but this patchset is not introducing the typecasting, it only moves the statement to another logical location. e.g. the above code is not new, it was just moved from other location to init function. This fix to remove typecasting from all variables (triggerbase, flashbase,regbase) is a significant change in many routines in terms of parameters passing/handling & deserve separate patch/set. I am ready to send a separate patch/set for the same later. Please let me know if you agree. Rgds, Vikas > Thanks. > > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > "Marriage is like a cage; one sees the birds outside desperate to get > in, and those inside desperate to get out." - Montaigne
Dear Vikas, In message <9026814FBF99304F9FA3AC3FB72F3E2F04BFF85EFE@SAFEX1MAIL4.st.com> you wrote: > > > CQSPI_INDIRECTTRIGGER_ADDR_MASK, > > > + plat->regbase + CQSPI_REG_INDIRECTTRIGGER); > > > > I did not mention this explicitly, so I do it here: > > > > Please fix this type cast issue globally, in all your patches. > > I agree it should be done but this patchset is not introducing the > typecasting, it only moves the statement to another logical location. > e.g. the above code is not new, it was just moved from other location > to init function. I am fully aware of that. But I feel if we touch a piece of code, and notice it has issues, we should fix these while we are at it. Otherwise there is always the risk that we add more and more such bad code, and/or forget about cleaning up later. > This fix to remove typecasting from all variables (triggerbase, > flashbase,regbase) is a significant change in many routines in terms > of parameters passing/handling & deserve separate patch/set. Hm... this makes me wander about the overall code quality. I have to admit that I don't have any in-depth understanding of this specific driver, but it looks.... well, let's state it looks pretty much dfferent from the corresponding Linux kernel driver code. Which does not have such issues. So if you say it would take _significant_ efforts to clean up this implementation I'm asking myuself how much more (or less?) effort would it take to adapt the Linux driver for U-Boot instead? Having a common driver code base has been very beneficial in a number of other areas, too... > I am ready to send a separate patch/set for the same later. Please > let me know if you agree. If we follow strict rules, such a cleanup patch should go in first. Thanks. Wolfgang Denk
Hi Wolfgang, On 25 September 2015 at 00:26, Wolfgang Denk <wd@denx.de> wrote: > Dear Vikas, > > In message <9026814FBF99304F9FA3AC3FB72F3E2F04BFF85EFE@SAFEX1MAIL4.st.com> you wrote: >> >> > CQSPI_INDIRECTTRIGGER_ADDR_MASK, >> > > + plat->regbase + CQSPI_REG_INDIRECTTRIGGER); >> > >> > I did not mention this explicitly, so I do it here: >> > >> > Please fix this type cast issue globally, in all your patches. >> >> I agree it should be done but this patchset is not introducing the >> typecasting, it only moves the statement to another logical location. >> e.g. the above code is not new, it was just moved from other location >> to init function. > > I am fully aware of that. But I feel if we touch a piece of code, and > notice it has issues, we should fix these while we are at it. > Otherwise there is always the risk that we add more and more such bad > code, and/or forget about cleaning up later. > >> This fix to remove typecasting from all variables (triggerbase, >> flashbase,regbase) is a significant change in many routines in terms >> of parameters passing/handling & deserve separate patch/set. > > Hm... this makes me wander about the overall code quality. I have to > admit that I don't have any in-depth understanding of this specific > driver, but it looks.... well, let's state it looks pretty much > dfferent from the corresponding Linux kernel driver code. Which does > not have such issues. So if you say it would take _significant_ > efforts to clean up this implementation I'm asking myuself how much > more (or less?) effort would it take to adapt the Linux driver for > U-Boot instead? Having a common driver code base has been very > beneficial in a number of other areas, too... > >> I am ready to send a separate patch/set for the same later. Please >> let me know if you agree. > > If we follow strict rules, such a cleanup patch should go in first. Looks like driver code itself has some issues and Vikas made changes wrt to existing code, cleaning up existing driver and then make Vikas changes might be reasonable and time consuming task. What if we move Vikas changes now for this release as he stated in previous mail "about sending patches later". My idea is someone is trying to clean it up existing code then give him a chance to move his code as well because he sent couple times. thanks!
Dear Jagan, In message <CAD6G_RSZAK7u=_ArT8SgqMvrdQ6ULi9szmxhF++kAEw-gv4Erw@mail.gmail.com> you wrote: > > What if we move Vikas changes now for this release as he stated in > previous mail "about sending patches later". Well, we've been there too many times before. In to many cases, the "later" just did not happen. > My idea is someone is trying to clean it up existing code then give > him a chance to move his code as well because he sent couple times. Well, I think it would be better to clean up first, instead of just reshuffeling the crappy code. Also, I would like to hear a comment about comparing the efforts (and benefits) of cleaning up this stuff versus doing a clean port of the Linux version of that driver. Best regards, Wolfgang Denk
Thanks Wolfgang, > -----Original Message----- > From: Wolfgang Denk [mailto:wd@denx.de] > Sent: Thursday, September 24, 2015 3:32 PM > To: Jagan Teki > Cc: Vikas MANOCHA; u-boot@lists.denx.de; marex@denx.de; sr@denx.de; > grmoore@opensource.altera.com > Subject: Re: [U-Boot] [PATCH v7 1/6] spi: cadence_qspi: move trigger base > configuration in init > > Dear Jagan, > > In message <CAD6G_RSZAK7u=_ArT8SgqMvrdQ6ULi9szmxhF++kAEw- > gv4Erw@mail.gmail.com> you wrote: > > > > What if we move Vikas changes now for this release as he stated in > > previous mail "about sending patches later". > > Well, we've been there too many times before. In to many cases, the "later" > just did not happen. To ensure it this time, I can add a patch for the same on top of these patches in next version :-) > > > My idea is someone is trying to clean it up existing code then give > > him a chance to move his code as well because he sent couple times. > > Well, I think it would be better to clean up first, instead of just reshuffeling > the crappy code. > > Also, I would like to hear a comment about comparing the efforts (and > benefits) of cleaning up this stuff versus doing a clean port of the Linux > version of that driver. I think it makes sense. Although both drivers (uboot, linux) have lot of similarities, there are differences also. It would be good to sync up. Rgds, Vikas > > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > Eureka! -- Archimedes
diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c index d053407..d377ad1 100644 --- a/drivers/spi/cadence_qspi_apb.c +++ b/drivers/spi/cadence_qspi_apb.c @@ -534,6 +534,8 @@ void cadence_qspi_apb_controller_init(struct cadence_spi_platdata *plat) /* Indirect mode configurations */ writel((plat->sram_size/2), plat->regbase + CQSPI_REG_SRAMPARTITION); + writel((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK, + plat->regbase + CQSPI_REG_INDIRECTTRIGGER); /* Disable all interrupts */ writel(0, plat->regbase + CQSPI_REG_IRQMASK); @@ -693,10 +695,6 @@ int cadence_qspi_apb_indirect_read_setup(struct cadence_spi_platdata *plat, /* for normal read (only ramtron as of now) */ addr_bytes = cmdlen - 1; - /* Setup the indirect trigger address */ - writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK), - plat->regbase + CQSPI_REG_INDIRECTTRIGGER); - /* Configure the opcode */ rd_reg = cmdbuf[0] << CQSPI_REG_RD_INSTR_OPCODE_LSB; @@ -790,9 +788,6 @@ int cadence_qspi_apb_indirect_write_setup(struct cadence_spi_platdata *plat, cmdlen, (unsigned int)cmdbuf); return -EINVAL; } - /* Setup the indirect trigger address */ - writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK), - plat->regbase + CQSPI_REG_INDIRECTTRIGGER); /* Configure the opcode */ reg = cmdbuf[0] << CQSPI_REG_WR_INSTR_OPCODE_LSB;
No need to configure indirect trigger address for every read/write. Signed-off-by: Vikas Manocha <vikas.manocha@st.com> --- Changes in v7: None Changes in v6: None Changes in v5: fixed type cast compilation warnings. Changes in v4: removed extra type casts. Changes in v3: added commit message & removed extra bracket. Changes in v2: Rebased to master drivers/spi/cadence_qspi_apb.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)