* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check @ 2018-12-05 12:27 Stefan Mavrodiev 2018-12-05 15:46 ` Maxime Ripard 2018-12-14 10:18 ` Jagan Teki 0 siblings, 2 replies; 25+ messages in thread From: Stefan Mavrodiev @ 2018-12-05 12:27 UTC (permalink / raw) To: u-boot Current driver doesn't check if the destination pointer is NULL. This cause the data from the FIFO to be stored inside the internal SDRAM ( address 0 ). The patch add simple check if the destination pointer is NULL. Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> --- drivers/spi/sun4i_spi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c index b86b5a00ad..38cc743c61 100644 --- a/drivers/spi/sun4i_spi.c +++ b/drivers/spi/sun4i_spi.c @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) while (len--) { byte = readb(&priv->regs->rxdata); - *priv->rx_buf++ = byte; + if (priv->rx_buf) + *priv->rx_buf++ = byte; } } -- 2.17.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-05 12:27 [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check Stefan Mavrodiev @ 2018-12-05 15:46 ` Maxime Ripard 2018-12-06 6:41 ` Stefan Mavrodiev 2018-12-16 11:16 ` André Przywara 2018-12-14 10:18 ` Jagan Teki 1 sibling, 2 replies; 25+ messages in thread From: Maxime Ripard @ 2018-12-05 15:46 UTC (permalink / raw) To: u-boot On Wed, Dec 05, 2018 at 02:27:57PM +0200, Stefan Mavrodiev wrote: > Current driver doesn't check if the destination pointer is NULL. > This cause the data from the FIFO to be stored inside the internal > SDRAM ( address 0 ). > > The patch add simple check if the destination pointer is NULL. > > Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> > --- > drivers/spi/sun4i_spi.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c > index b86b5a00ad..38cc743c61 100644 > --- a/drivers/spi/sun4i_spi.c > +++ b/drivers/spi/sun4i_spi.c > @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > > while (len--) { > byte = readb(&priv->regs->rxdata); > - *priv->rx_buf++ = byte; > + if (priv->rx_buf) > + *priv->rx_buf++ = byte; It seems pretty inefficient to test the pointer at each access, it would be better to check it once before starting the transfer. I'm not sure if that can even happen? Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 228 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181205/278b4f81/attachment.sig> ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-05 15:46 ` Maxime Ripard @ 2018-12-06 6:41 ` Stefan Mavrodiev 2018-12-13 7:12 ` Stefan Mavrodiev 2018-12-16 11:16 ` André Przywara 1 sibling, 1 reply; 25+ messages in thread From: Stefan Mavrodiev @ 2018-12-06 6:41 UTC (permalink / raw) To: u-boot On 12/5/18 5:46 PM, Maxime Ripard wrote: > On Wed, Dec 05, 2018 at 02:27:57PM +0200, Stefan Mavrodiev wrote: >> Current driver doesn't check if the destination pointer is NULL. >> This cause the data from the FIFO to be stored inside the internal >> SDRAM ( address 0 ). >> >> The patch add simple check if the destination pointer is NULL. >> >> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >> --- >> drivers/spi/sun4i_spi.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c >> index b86b5a00ad..38cc743c61 100644 >> --- a/drivers/spi/sun4i_spi.c >> +++ b/drivers/spi/sun4i_spi.c >> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) >> >> while (len--) { >> byte = readb(&priv->regs->rxdata); >> - *priv->rx_buf++ = byte; >> + if (priv->rx_buf) >> + *priv->rx_buf++ = byte; > It seems pretty inefficient to test the pointer at each access, it > would be better to check it once before starting the transfer. > > I'm not sure if that can even happen? I've tried to check that before draining the receive fifo, but then the controller doesn't work. I'm thinking that the fifo must be drained in any case. > > Maxime > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-06 6:41 ` Stefan Mavrodiev @ 2018-12-13 7:12 ` Stefan Mavrodiev 2018-12-14 9:25 ` Maxime Ripard 0 siblings, 1 reply; 25+ messages in thread From: Stefan Mavrodiev @ 2018-12-13 7:12 UTC (permalink / raw) To: u-boot On 12/6/18 8:41 AM, Stefan Mavrodiev wrote: > > On 12/5/18 5:46 PM, Maxime Ripard wrote: >> On Wed, Dec 05, 2018 at 02:27:57PM +0200, Stefan Mavrodiev wrote: >>> Current driver doesn't check if the destination pointer is NULL. >>> This cause the data from the FIFO to be stored inside the internal >>> SDRAM ( address 0 ). >>> >>> The patch add simple check if the destination pointer is NULL. >>> >>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >>> --- >>> drivers/spi/sun4i_spi.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c >>> index b86b5a00ad..38cc743c61 100644 >>> --- a/drivers/spi/sun4i_spi.c >>> +++ b/drivers/spi/sun4i_spi.c >>> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct >>> sun4i_spi_priv *priv, int len) >>> while (len--) { >>> byte = readb(&priv->regs->rxdata); >>> - *priv->rx_buf++ = byte; >>> + if (priv->rx_buf) >>> + *priv->rx_buf++ = byte; >> It seems pretty inefficient to test the pointer at each access, it >> would be better to check it once before starting the transfer. >> >> I'm not sure if that can even happen? > > I've tried to check that before draining the receive fifo, but then > the controller doesn't > work. I'm thinking that the fifo must be drained in any case. > >> >> Maxime >> Any further comments? ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-13 7:12 ` Stefan Mavrodiev @ 2018-12-14 9:25 ` Maxime Ripard 2018-12-14 14:14 ` Stefan Mavrodiev 0 siblings, 1 reply; 25+ messages in thread From: Maxime Ripard @ 2018-12-14 9:25 UTC (permalink / raw) To: u-boot On Thu, Dec 13, 2018 at 09:12:57AM +0200, Stefan Mavrodiev wrote: > > On 12/6/18 8:41 AM, Stefan Mavrodiev wrote: > > > > On 12/5/18 5:46 PM, Maxime Ripard wrote: > > > On Wed, Dec 05, 2018 at 02:27:57PM +0200, Stefan Mavrodiev wrote: > > > > Current driver doesn't check if the destination pointer is NULL. > > > > This cause the data from the FIFO to be stored inside the internal > > > > SDRAM ( address 0 ). > > > > > > > > The patch add simple check if the destination pointer is NULL. > > > > > > > > Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> > > > > --- > > > > drivers/spi/sun4i_spi.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c > > > > index b86b5a00ad..38cc743c61 100644 > > > > --- a/drivers/spi/sun4i_spi.c > > > > +++ b/drivers/spi/sun4i_spi.c > > > > @@ -129,7 +129,8 @@ static inline void > > > > sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > > > > while (len--) { > > > > byte = readb(&priv->regs->rxdata); > > > > - *priv->rx_buf++ = byte; > > > > + if (priv->rx_buf) > > > > + *priv->rx_buf++ = byte; > > > It seems pretty inefficient to test the pointer at each access, it > > > would be better to check it once before starting the transfer. > > > > > > I'm not sure if that can even happen? > > > > I've tried to check that before draining the receive fifo, but > > then the controller doesn't work. I'm thinking that the fifo must > > be drained in any case. > > Any further comments? I was expecting you to comment on whether the FIFO needed to be drained or not :) Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 228 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181214/474b96eb/attachment.sig> ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-14 9:25 ` Maxime Ripard @ 2018-12-14 14:14 ` Stefan Mavrodiev 2018-12-14 15:55 ` Maxime Ripard 0 siblings, 1 reply; 25+ messages in thread From: Stefan Mavrodiev @ 2018-12-14 14:14 UTC (permalink / raw) To: u-boot On 12/14/18 11:25 AM, Maxime Ripard wrote: > On Thu, Dec 13, 2018 at 09:12:57AM +0200, Stefan Mavrodiev wrote: >> On 12/6/18 8:41 AM, Stefan Mavrodiev wrote: >>> On 12/5/18 5:46 PM, Maxime Ripard wrote: >>>> On Wed, Dec 05, 2018 at 02:27:57PM +0200, Stefan Mavrodiev wrote: >>>>> Current driver doesn't check if the destination pointer is NULL. >>>>> This cause the data from the FIFO to be stored inside the internal >>>>> SDRAM ( address 0 ). >>>>> >>>>> The patch add simple check if the destination pointer is NULL. >>>>> >>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >>>>> --- >>>>> drivers/spi/sun4i_spi.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c >>>>> index b86b5a00ad..38cc743c61 100644 >>>>> --- a/drivers/spi/sun4i_spi.c >>>>> +++ b/drivers/spi/sun4i_spi.c >>>>> @@ -129,7 +129,8 @@ static inline void >>>>> sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) >>>>> while (len--) { >>>>> byte = readb(&priv->regs->rxdata); >>>>> - *priv->rx_buf++ = byte; >>>>> + if (priv->rx_buf) >>>>> + *priv->rx_buf++ = byte; >>>> It seems pretty inefficient to test the pointer at each access, it >>>> would be better to check it once before starting the transfer. >>>> >>>> I'm not sure if that can even happen? >>> I've tried to check that before draining the receive fifo, but >>> then the controller doesn't work. I'm thinking that the fifo must >>> be drained in any case. >> Any further comments? > I was expecting you to comment on whether the FIFO needed to be > drained or not :) Sorry. I didn't understand that. Anyway. After some code checking, I found that the FIFO needs to be drained because TP_EN (Transmit Pause Enable) bit is set during bus claim. ".... In master mode, it is used to control transmit state machine to stop smart burst sending when RX FIFO is full. ..." Perhaps this bit should be enabled only when we want to read back data? > > Maxime > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-14 14:14 ` Stefan Mavrodiev @ 2018-12-14 15:55 ` Maxime Ripard 2018-12-17 6:37 ` Stefan Mavrodiev 0 siblings, 1 reply; 25+ messages in thread From: Maxime Ripard @ 2018-12-14 15:55 UTC (permalink / raw) To: u-boot On Fri, Dec 14, 2018 at 04:14:31PM +0200, Stefan Mavrodiev wrote: > > On 12/14/18 11:25 AM, Maxime Ripard wrote: > > On Thu, Dec 13, 2018 at 09:12:57AM +0200, Stefan Mavrodiev wrote: > > > On 12/6/18 8:41 AM, Stefan Mavrodiev wrote: > > > > On 12/5/18 5:46 PM, Maxime Ripard wrote: > > > > > On Wed, Dec 05, 2018 at 02:27:57PM +0200, Stefan Mavrodiev wrote: > > > > > > Current driver doesn't check if the destination pointer is NULL. > > > > > > This cause the data from the FIFO to be stored inside the internal > > > > > > SDRAM ( address 0 ). > > > > > > > > > > > > The patch add simple check if the destination pointer is NULL. > > > > > > > > > > > > Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> > > > > > > --- > > > > > > drivers/spi/sun4i_spi.c | 3 ++- > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c > > > > > > index b86b5a00ad..38cc743c61 100644 > > > > > > --- a/drivers/spi/sun4i_spi.c > > > > > > +++ b/drivers/spi/sun4i_spi.c > > > > > > @@ -129,7 +129,8 @@ static inline void > > > > > > sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > > > > > > while (len--) { > > > > > > byte = readb(&priv->regs->rxdata); > > > > > > - *priv->rx_buf++ = byte; > > > > > > + if (priv->rx_buf) > > > > > > + *priv->rx_buf++ = byte; > > > > > It seems pretty inefficient to test the pointer at each access, it > > > > > would be better to check it once before starting the transfer. > > > > > > > > > > I'm not sure if that can even happen? > > > > I've tried to check that before draining the receive fifo, but > > > > then the controller doesn't work. I'm thinking that the fifo must > > > > be drained in any case. > > > Any further comments? > > I was expecting you to comment on whether the FIFO needed to be > > drained or not :) > > Sorry. I didn't understand that. > > Anyway. After some code checking, I found that the FIFO needs to be drained > because TP_EN (Transmit Pause Enable) bit is set during bus claim. > > ".... > > In master mode, it is used to control transmit state machine to > stop smart burst sending when RX FIFO is full. > > ..." > > Perhaps this bit should be enabled only when we want to read back data? It's been a while since I last looked at the spi driver. What is linux doing? Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 228 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181214/a68f5355/attachment.sig> ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-14 15:55 ` Maxime Ripard @ 2018-12-17 6:37 ` Stefan Mavrodiev 2018-12-20 6:45 ` Stefan Mavrodiev 0 siblings, 1 reply; 25+ messages in thread From: Stefan Mavrodiev @ 2018-12-17 6:37 UTC (permalink / raw) To: u-boot On 12/14/18 5:55 PM, Maxime Ripard wrote: > On Fri, Dec 14, 2018 at 04:14:31PM +0200, Stefan Mavrodiev wrote: >> On 12/14/18 11:25 AM, Maxime Ripard wrote: >>> On Thu, Dec 13, 2018 at 09:12:57AM +0200, Stefan Mavrodiev wrote: >>>> On 12/6/18 8:41 AM, Stefan Mavrodiev wrote: >>>>> On 12/5/18 5:46 PM, Maxime Ripard wrote: >>>>>> On Wed, Dec 05, 2018 at 02:27:57PM +0200, Stefan Mavrodiev wrote: >>>>>>> Current driver doesn't check if the destination pointer is NULL. >>>>>>> This cause the data from the FIFO to be stored inside the internal >>>>>>> SDRAM ( address 0 ). >>>>>>> >>>>>>> The patch add simple check if the destination pointer is NULL. >>>>>>> >>>>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >>>>>>> --- >>>>>>> drivers/spi/sun4i_spi.c | 3 ++- >>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c >>>>>>> index b86b5a00ad..38cc743c61 100644 >>>>>>> --- a/drivers/spi/sun4i_spi.c >>>>>>> +++ b/drivers/spi/sun4i_spi.c >>>>>>> @@ -129,7 +129,8 @@ static inline void >>>>>>> sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) >>>>>>> while (len--) { >>>>>>> byte = readb(&priv->regs->rxdata); >>>>>>> - *priv->rx_buf++ = byte; >>>>>>> + if (priv->rx_buf) >>>>>>> + *priv->rx_buf++ = byte; >>>>>> It seems pretty inefficient to test the pointer at each access, it >>>>>> would be better to check it once before starting the transfer. >>>>>> >>>>>> I'm not sure if that can even happen? >>>>> I've tried to check that before draining the receive fifo, but >>>>> then the controller doesn't work. I'm thinking that the fifo must >>>>> be drained in any case. >>>> Any further comments? >>> I was expecting you to comment on whether the FIFO needed to be >>> drained or not :) >> Sorry. I didn't understand that. >> >> Anyway. After some code checking, I found that the FIFO needs to be drained >> because TP_EN (Transmit Pause Enable) bit is set during bus claim. >> >> ".... >> >> In master mode, it is used to control transmit state machine to >> stop smart burst sending when RX FIFO is full. >> >> ..." >> >> Perhaps this bit should be enabled only when we want to read back data? > It's been a while since I last looked at the spi driver. What is linux > doing? In the kernel version there is the same check, like the one in the patch: drivers/spi/spi-sun4i.c: static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len) ...... while (len--) { byte = readb(sspi->base_addr + SUN4I_RXDATA_REG); if (sspi->rx_buf) *sspi->rx_buf++ = byte; } ..... Guess I've missed this check when I was adopting the driver for u-boot. > > Maxime > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-17 6:37 ` Stefan Mavrodiev @ 2018-12-20 6:45 ` Stefan Mavrodiev 0 siblings, 0 replies; 25+ messages in thread From: Stefan Mavrodiev @ 2018-12-20 6:45 UTC (permalink / raw) To: u-boot On 12/17/18 8:37 AM, Stefan Mavrodiev wrote: > > On 12/14/18 5:55 PM, Maxime Ripard wrote: >> On Fri, Dec 14, 2018 at 04:14:31PM +0200, Stefan Mavrodiev wrote: >>> On 12/14/18 11:25 AM, Maxime Ripard wrote: >>>> On Thu, Dec 13, 2018 at 09:12:57AM +0200, Stefan Mavrodiev wrote: >>>>> On 12/6/18 8:41 AM, Stefan Mavrodiev wrote: >>>>>> On 12/5/18 5:46 PM, Maxime Ripard wrote: >>>>>>> On Wed, Dec 05, 2018 at 02:27:57PM +0200, Stefan Mavrodiev wrote: >>>>>>>> Current driver doesn't check if the destination pointer is NULL. >>>>>>>> This cause the data from the FIFO to be stored inside the internal >>>>>>>> SDRAM ( address 0 ). >>>>>>>> >>>>>>>> The patch add simple check if the destination pointer is NULL. >>>>>>>> >>>>>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >>>>>>>> --- >>>>>>>> drivers/spi/sun4i_spi.c | 3 ++- >>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c >>>>>>>> index b86b5a00ad..38cc743c61 100644 >>>>>>>> --- a/drivers/spi/sun4i_spi.c >>>>>>>> +++ b/drivers/spi/sun4i_spi.c >>>>>>>> @@ -129,7 +129,8 @@ static inline void >>>>>>>> sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) >>>>>>>> while (len--) { >>>>>>>> byte = readb(&priv->regs->rxdata); >>>>>>>> - *priv->rx_buf++ = byte; >>>>>>>> + if (priv->rx_buf) >>>>>>>> + *priv->rx_buf++ = byte; >>>>>>> It seems pretty inefficient to test the pointer at each access, it >>>>>>> would be better to check it once before starting the transfer. >>>>>>> >>>>>>> I'm not sure if that can even happen? >>>>>> I've tried to check that before draining the receive fifo, but >>>>>> then the controller doesn't work. I'm thinking that the fifo must >>>>>> be drained in any case. >>>>> Any further comments? >>>> I was expecting you to comment on whether the FIFO needed to be >>>> drained or not :) >>> Sorry. I didn't understand that. >>> >>> Anyway. After some code checking, I found that the FIFO needs to be >>> drained >>> because TP_EN (Transmit Pause Enable) bit is set during bus claim. >>> >>> ".... >>> >>> In master mode, it is used to control transmit state machine to >>> stop smart burst sending when RX FIFO is full. >>> >>> ..." >>> >>> Perhaps this bit should be enabled only when we want to read back data? >> It's been a while since I last looked at the spi driver. What is linux >> doing? > > In the kernel version there is the same check, like the one in the patch: > > drivers/spi/spi-sun4i.c: > static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len) > ...... > > while (len--) { > byte = readb(sspi->base_addr + SUN4I_RXDATA_REG); > if (sspi->rx_buf) > *sspi->rx_buf++ = byte; > } > ..... > > Guess I've missed this check when I was adopting the driver for u-boot. > > >> >> Maxime >> Any comments? I think with all said, the patch can be merged as it is. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-05 15:46 ` Maxime Ripard 2018-12-06 6:41 ` Stefan Mavrodiev @ 2018-12-16 11:16 ` André Przywara 1 sibling, 0 replies; 25+ messages in thread From: André Przywara @ 2018-12-16 11:16 UTC (permalink / raw) To: u-boot On 05/12/2018 15:46, Maxime Ripard wrote: Hi, > On Wed, Dec 05, 2018 at 02:27:57PM +0200, Stefan Mavrodiev wrote: >> Current driver doesn't check if the destination pointer is NULL. >> This cause the data from the FIFO to be stored inside the internal >> SDRAM ( address 0 ). >> >> The patch add simple check if the destination pointer is NULL. >> >> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >> --- >> drivers/spi/sun4i_spi.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c >> index b86b5a00ad..38cc743c61 100644 >> --- a/drivers/spi/sun4i_spi.c >> +++ b/drivers/spi/sun4i_spi.c >> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) >> >> while (len--) { >> byte = readb(&priv->regs->rxdata); >> - *priv->rx_buf++ = byte; >> + if (priv->rx_buf) >> + *priv->rx_buf++ = byte; > > It seems pretty inefficient to test the pointer at each access, it > would be better to check it once before starting the transfer. I appreciate the intention to avoid bloat and the attention to detail, but: This check boils down to exactly one 16-bit instruction: 10c: b11a cbz r2, 116 <sun4i_spi_xfer+0x116> - which only accesses a register - inside a loop with does an MMIO(!) read from a device - handling transfers from a serial device transferring 100s of KB/s - in a system which's sole purpose is to read data on a single core - in U-Boot ;-) So the "performance impact" of this check is probably totally negligible. There are quite some spi_xfer() calls with explicit NULL arguments, for instance when we just want to transfer something. So I wonder if we just want to take this patch, since it fixes a memory corruption issue, plus we have a similar check in the TX path. Cheers, Andre. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-05 12:27 [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check Stefan Mavrodiev 2018-12-05 15:46 ` Maxime Ripard @ 2018-12-14 10:18 ` Jagan Teki 2018-12-14 10:49 ` Maxime Ripard 2018-12-20 10:14 ` Jagan Teki 1 sibling, 2 replies; 25+ messages in thread From: Jagan Teki @ 2018-12-14 10:18 UTC (permalink / raw) To: u-boot On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > > Current driver doesn't check if the destination pointer is NULL. > This cause the data from the FIFO to be stored inside the internal > SDRAM ( address 0 ). > > The patch add simple check if the destination pointer is NULL. > > Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> > --- > drivers/spi/sun4i_spi.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c > index b86b5a00ad..38cc743c61 100644 > --- a/drivers/spi/sun4i_spi.c > +++ b/drivers/spi/sun4i_spi.c > @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > > while (len--) { > byte = readb(&priv->regs->rxdata); > - *priv->rx_buf++ = byte; > + if (priv->rx_buf) > + *priv->rx_buf++ = byte; Acked-by: Jagan Teki <jagan@openedev.com> But, have you tested how much data in the fifo before drained? It's better we can get the available data before reading via fifo_sta ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-14 10:18 ` Jagan Teki @ 2018-12-14 10:49 ` Maxime Ripard 2018-12-14 12:09 ` Jagan Teki 2018-12-20 10:14 ` Jagan Teki 1 sibling, 1 reply; 25+ messages in thread From: Maxime Ripard @ 2018-12-14 10:49 UTC (permalink / raw) To: u-boot On Fri, Dec 14, 2018 at 03:48:18PM +0530, Jagan Teki wrote: > On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > > > > Current driver doesn't check if the destination pointer is NULL. > > This cause the data from the FIFO to be stored inside the internal > > SDRAM ( address 0 ). > > > > The patch add simple check if the destination pointer is NULL. > > > > Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> > > --- > > drivers/spi/sun4i_spi.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c > > index b86b5a00ad..38cc743c61 100644 > > --- a/drivers/spi/sun4i_spi.c > > +++ b/drivers/spi/sun4i_spi.c > > @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > > > > while (len--) { > > byte = readb(&priv->regs->rxdata); > > - *priv->rx_buf++ = byte; > > + if (priv->rx_buf) > > + *priv->rx_buf++ = byte; > > Acked-by: Jagan Teki <jagan@openedev.com> > > But, have you tested how much data in the fifo before drained? It's > better we can get the available data before reading via fifo_sta This patch shouldn't be merged in its current state. Please look at the on-going discussion. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 228 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181214/fa60dfa5/attachment.sig> ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-14 10:49 ` Maxime Ripard @ 2018-12-14 12:09 ` Jagan Teki 0 siblings, 0 replies; 25+ messages in thread From: Jagan Teki @ 2018-12-14 12:09 UTC (permalink / raw) To: u-boot On Fri, Dec 14, 2018 at 4:19 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Fri, Dec 14, 2018 at 03:48:18PM +0530, Jagan Teki wrote: > > On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > > > > > > Current driver doesn't check if the destination pointer is NULL. > > > This cause the data from the FIFO to be stored inside the internal > > > SDRAM ( address 0 ). > > > > > > The patch add simple check if the destination pointer is NULL. > > > > > > Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> > > > --- > > > drivers/spi/sun4i_spi.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c > > > index b86b5a00ad..38cc743c61 100644 > > > --- a/drivers/spi/sun4i_spi.c > > > +++ b/drivers/spi/sun4i_spi.c > > > @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > > > > > > while (len--) { > > > byte = readb(&priv->regs->rxdata); > > > - *priv->rx_buf++ = byte; > > > + if (priv->rx_buf) > > > + *priv->rx_buf++ = byte; > > > > Acked-by: Jagan Teki <jagan@openedev.com> > > > > But, have you tested how much data in the fifo before drained? It's > > better we can get the available data before reading via fifo_sta > > This patch shouldn't be merged in its current state. Please look at > the on-going discussion. I have given my comments, no plan to merge as of now since the discussion is going-on. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-14 10:18 ` Jagan Teki 2018-12-14 10:49 ` Maxime Ripard @ 2018-12-20 10:14 ` Jagan Teki 2018-12-20 10:29 ` Stefan Mavrodiev 1 sibling, 1 reply; 25+ messages in thread From: Jagan Teki @ 2018-12-20 10:14 UTC (permalink / raw) To: u-boot On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > > > > Current driver doesn't check if the destination pointer is NULL. > > This cause the data from the FIFO to be stored inside the internal > > SDRAM ( address 0 ). > > > > The patch add simple check if the destination pointer is NULL. > > > > Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> > > --- > > drivers/spi/sun4i_spi.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c > > index b86b5a00ad..38cc743c61 100644 > > --- a/drivers/spi/sun4i_spi.c > > +++ b/drivers/spi/sun4i_spi.c > > @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > > > > while (len--) { > > byte = readb(&priv->regs->rxdata); > > - *priv->rx_buf++ = byte; > > + if (priv->rx_buf) > > + *priv->rx_buf++ = byte; > > Acked-by: Jagan Teki <jagan@openedev.com> > > But, have you tested how much data in the fifo before drained? It's > better we can get the available data before reading via fifo_sta Didn't find any response on this? Indeed I'm waiting for it. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-20 10:14 ` Jagan Teki @ 2018-12-20 10:29 ` Stefan Mavrodiev 2018-12-20 10:45 ` Jagan Teki 0 siblings, 1 reply; 25+ messages in thread From: Stefan Mavrodiev @ 2018-12-20 10:29 UTC (permalink / raw) To: u-boot On 12/20/18 12:14 PM, Jagan Teki wrote: > On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki <jagan@amarulasolutions.com> wrote: >> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>> Current driver doesn't check if the destination pointer is NULL. >>> This cause the data from the FIFO to be stored inside the internal >>> SDRAM ( address 0 ). >>> >>> The patch add simple check if the destination pointer is NULL. >>> >>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >>> --- >>> drivers/spi/sun4i_spi.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c >>> index b86b5a00ad..38cc743c61 100644 >>> --- a/drivers/spi/sun4i_spi.c >>> +++ b/drivers/spi/sun4i_spi.c >>> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) >>> >>> while (len--) { >>> byte = readb(&priv->regs->rxdata); >>> - *priv->rx_buf++ = byte; >>> + if (priv->rx_buf) >>> + *priv->rx_buf++ = byte; >> Acked-by: Jagan Teki <jagan@openedev.com> >> >> But, have you tested how much data in the fifo before drained? It's >> better we can get the available data before reading via fifo_sta I don't understand what's the point of doing this? > Didn't find any response on this? Indeed I'm waiting for it. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-20 10:29 ` Stefan Mavrodiev @ 2018-12-20 10:45 ` Jagan Teki 2018-12-20 10:48 ` Stefan Mavrodiev 0 siblings, 1 reply; 25+ messages in thread From: Jagan Teki @ 2018-12-20 10:45 UTC (permalink / raw) To: u-boot On Thu, Dec 20, 2018 at 3:59 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > > > On 12/20/18 12:14 PM, Jagan Teki wrote: > > On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > >> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > >>> Current driver doesn't check if the destination pointer is NULL. > >>> This cause the data from the FIFO to be stored inside the internal > >>> SDRAM ( address 0 ). > >>> > >>> The patch add simple check if the destination pointer is NULL. > >>> > >>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> > >>> --- > >>> drivers/spi/sun4i_spi.c | 3 ++- > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c > >>> index b86b5a00ad..38cc743c61 100644 > >>> --- a/drivers/spi/sun4i_spi.c > >>> +++ b/drivers/spi/sun4i_spi.c > >>> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > >>> > >>> while (len--) { > >>> byte = readb(&priv->regs->rxdata); > >>> - *priv->rx_buf++ = byte; > >>> + if (priv->rx_buf) > >>> + *priv->rx_buf++ = byte; > >> Acked-by: Jagan Teki <jagan@openedev.com> > >> > >> But, have you tested how much data in the fifo before drained? It's > >> better we can get the available data before reading via fifo_sta > I don't understand what's the point of doing this? Didn't get? don't you understand what I'm saying or it not require from point of you? ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-20 10:45 ` Jagan Teki @ 2018-12-20 10:48 ` Stefan Mavrodiev 2018-12-20 10:56 ` Jagan Teki 0 siblings, 1 reply; 25+ messages in thread From: Stefan Mavrodiev @ 2018-12-20 10:48 UTC (permalink / raw) To: u-boot On 12/20/18 12:45 PM, Jagan Teki wrote: > On Thu, Dec 20, 2018 at 3:59 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >> >> On 12/20/18 12:14 PM, Jagan Teki wrote: >>> On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki <jagan@amarulasolutions.com> wrote: >>>> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>>>> Current driver doesn't check if the destination pointer is NULL. >>>>> This cause the data from the FIFO to be stored inside the internal >>>>> SDRAM ( address 0 ). >>>>> >>>>> The patch add simple check if the destination pointer is NULL. >>>>> >>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >>>>> --- >>>>> drivers/spi/sun4i_spi.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c >>>>> index b86b5a00ad..38cc743c61 100644 >>>>> --- a/drivers/spi/sun4i_spi.c >>>>> +++ b/drivers/spi/sun4i_spi.c >>>>> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) >>>>> >>>>> while (len--) { >>>>> byte = readb(&priv->regs->rxdata); >>>>> - *priv->rx_buf++ = byte; >>>>> + if (priv->rx_buf) >>>>> + *priv->rx_buf++ = byte; >>>> Acked-by: Jagan Teki <jagan@openedev.com> >>>> >>>> But, have you tested how much data in the fifo before drained? It's >>>> better we can get the available data before reading via fifo_sta >> I don't understand what's the point of doing this? > Didn't get? don't you understand what I'm saying or it not require > from point of you? Maybe I don't understand correctly what you're saying. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-20 10:48 ` Stefan Mavrodiev @ 2018-12-20 10:56 ` Jagan Teki 2018-12-20 11:19 ` Stefan Mavrodiev 0 siblings, 1 reply; 25+ messages in thread From: Jagan Teki @ 2018-12-20 10:56 UTC (permalink / raw) To: u-boot On Thu, Dec 20, 2018 at 4:18 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > > > On 12/20/18 12:45 PM, Jagan Teki wrote: > > On Thu, Dec 20, 2018 at 3:59 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > >> > >> On 12/20/18 12:14 PM, Jagan Teki wrote: > >>> On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > >>>> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > >>>>> Current driver doesn't check if the destination pointer is NULL. > >>>>> This cause the data from the FIFO to be stored inside the internal > >>>>> SDRAM ( address 0 ). > >>>>> > >>>>> The patch add simple check if the destination pointer is NULL. > >>>>> > >>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> > >>>>> --- > >>>>> drivers/spi/sun4i_spi.c | 3 ++- > >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c > >>>>> index b86b5a00ad..38cc743c61 100644 > >>>>> --- a/drivers/spi/sun4i_spi.c > >>>>> +++ b/drivers/spi/sun4i_spi.c > >>>>> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > >>>>> > >>>>> while (len--) { > >>>>> byte = readb(&priv->regs->rxdata); > >>>>> - *priv->rx_buf++ = byte; > >>>>> + if (priv->rx_buf) > >>>>> + *priv->rx_buf++ = byte; > >>>> Acked-by: Jagan Teki <jagan@openedev.com> > >>>> > >>>> But, have you tested how much data in the fifo before drained? It's > >>>> better we can get the available data before reading via fifo_sta > >> I don't understand what's the point of doing this? > > Didn't get? don't you understand what I'm saying or it not require > > from point of you? > Maybe I don't understand correctly what you're saying. You comment now and previous mail doesn't match. better be specific. For draining fifo. We can find how much data available before reading from fifo and assign to local rx. static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) { u32 reg, cnt u8 byte; /* See how much data is available */ reg = readl(&priv->regs->fifo_sta); reg &= SUN4I_FIFO_STA_RF_CNT_MASK; cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; if (len > cnt) len = cnt; while (len--) { byte = readb(&priv->regs->rxdata); *priv->rx_buf++ = byte; } } This can be perfect drain fifo, and this is what I'm trying to test you with existing code and after your patch and verify whether all the data perfectly drain or not before and after. Hope you understand, this time. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-20 10:56 ` Jagan Teki @ 2018-12-20 11:19 ` Stefan Mavrodiev 2018-12-20 11:54 ` Jagan Teki 0 siblings, 1 reply; 25+ messages in thread From: Stefan Mavrodiev @ 2018-12-20 11:19 UTC (permalink / raw) To: u-boot On 12/20/18 12:56 PM, Jagan Teki wrote: > On Thu, Dec 20, 2018 at 4:18 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >> >> On 12/20/18 12:45 PM, Jagan Teki wrote: >>> On Thu, Dec 20, 2018 at 3:59 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>>> On 12/20/18 12:14 PM, Jagan Teki wrote: >>>>> On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki <jagan@amarulasolutions.com> wrote: >>>>>> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>>>>>> Current driver doesn't check if the destination pointer is NULL. >>>>>>> This cause the data from the FIFO to be stored inside the internal >>>>>>> SDRAM ( address 0 ). >>>>>>> >>>>>>> The patch add simple check if the destination pointer is NULL. >>>>>>> >>>>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >>>>>>> --- >>>>>>> drivers/spi/sun4i_spi.c | 3 ++- >>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c >>>>>>> index b86b5a00ad..38cc743c61 100644 >>>>>>> --- a/drivers/spi/sun4i_spi.c >>>>>>> +++ b/drivers/spi/sun4i_spi.c >>>>>>> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) >>>>>>> >>>>>>> while (len--) { >>>>>>> byte = readb(&priv->regs->rxdata); >>>>>>> - *priv->rx_buf++ = byte; >>>>>>> + if (priv->rx_buf) >>>>>>> + *priv->rx_buf++ = byte; >>>>>> Acked-by: Jagan Teki <jagan@openedev.com> >>>>>> >>>>>> But, have you tested how much data in the fifo before drained? It's >>>>>> better we can get the available data before reading via fifo_sta >>>> I don't understand what's the point of doing this? >>> Didn't get? don't you understand what I'm saying or it not require >>> from point of you? >> Maybe I don't understand correctly what you're saying. > You comment now and previous mail doesn't match. better be specific. > > For draining fifo. > We can find how much data available before reading from fifo and > assign to local rx. > > static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > { > u32 reg, cnt > u8 byte; > > /* See how much data is available */ > reg = readl(&priv->regs->fifo_sta); > reg &= SUN4I_FIFO_STA_RF_CNT_MASK; > cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; > > if (len > cnt) > len = cnt; > > while (len--) { > byte = readb(&priv->regs->rxdata); > *priv->rx_buf++ = byte; > } > } > > This can be perfect drain fifo, and this is what I'm trying to test > you with existing code and after your patch and verify whether all the > data perfectly drain or not before and after. > > Hope you understand, this time. Hope I understood this time... I've made some modification to sun4i_spi_xfer: static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen, const void *dout, void *din, unsigned long flags) { struct udevice *bus = dev->parent; struct sun4i_spi_priv *priv = dev_get_priv(bus); struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev); u32 len = bitlen / 8; u32 reg, cnt; u8 nbytes; int ret; priv->tx_buf = dout; priv->rx_buf = din; if (bitlen % 8) { debug("%s: non byte-aligned SPI transfer.\n", __func__); return -ENAVAIL; } if (flags & SPI_XFER_BEGIN) sun4i_spi_set_cs(bus, slave_plat->cs, true); reg = readl(&priv->regs->ctl); /* Reset FIFOs */ writel(reg | SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST, &priv->regs->ctl); while (len) { /* Setup the transfer now... */ nbytes = min(len, (u32)(SUN4I_FIFO_DEPTH - 1)); if (!priv->rx_buf) printf("%s: Sending %d bytes, ", __func__, nbytes); /* Setup the counters */ writel(SUN4I_BURST_CNT(nbytes), &priv->regs->bc); writel(SUN4I_XMIT_CNT(nbytes), &priv->regs->tc); /* Fill the TX FIFO */ sun4i_spi_fill_fifo(priv, nbytes); /* Start the transfer */ reg = readl(&priv->regs->ctl); writel(reg | SUN4I_CTL_XCH, &priv->regs->ctl); /* Wait transfer to complete */ ret = wait_for_bit_le32(&priv->regs->ctl, SUN4I_CTL_XCH_MASK, false, SUN4I_SPI_TIMEOUT_US, false); if (ret) { printf("ERROR: sun4i_spi: Timeout transferring data\n"); sun4i_spi_set_cs(bus, slave_plat->cs, false); return ret; } /* Drain the RX FIFO */ if (!priv->rx_buf) { reg = readl(&priv->regs->fifo_sta); reg &= SUN4I_FIFO_STA_RF_CNT_MASK; cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; printf("rx fifo: before: %d, ", cnt); } sun4i_spi_drain_fifo(priv, nbytes); if (!priv->rx_buf) { reg = readl(&priv->regs->fifo_sta); reg &= SUN4I_FIFO_STA_RF_CNT_MASK; cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; printf("after: %d\n", cnt); } len -= nbytes; } if (flags & SPI_XFER_END) sun4i_spi_set_cs(bus, slave_plat->cs, false); return 0; } This will print only when destination buffer is NULL. This gave me the following output: sun4i_spi_xfer: Sending 1 bytes, rx fifo: before: 1, after: 0 sun4i_spi_xfer: Sending 1 bytes, rx fifo: before: 1, after: 0 sun4i_spi_xfer: Sending 1 bytes, rx fifo: before: 1, after: 0 sun4i_spi_xfer: Sending 4 bytes, rx fifo: before: 4, after: 0 sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0 sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0 sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0 sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0 sun4i_spi_xfer: Sending 4 bytes, rx fifo: before: 4, after: 0 sun4i_spi_xfer: Sending 1 bytes, rx fifo: before: 1, after: 0 sun4i_spi_xfer: Sending 1 bytes, rx fifo: before: 1, after: 0 sun4i_spi_xfer: Sending 4 bytes, rx fifo: before: 4, after: 0 sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0 sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0 sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0 sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0 .... etc As you can see the fifo is drained. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-20 11:19 ` Stefan Mavrodiev @ 2018-12-20 11:54 ` Jagan Teki 2018-12-20 12:07 ` Stefan Mavrodiev 0 siblings, 1 reply; 25+ messages in thread From: Jagan Teki @ 2018-12-20 11:54 UTC (permalink / raw) To: u-boot On Thu, Dec 20, 2018 at 4:49 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > > > On 12/20/18 12:56 PM, Jagan Teki wrote: > > On Thu, Dec 20, 2018 at 4:18 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > >> > >> On 12/20/18 12:45 PM, Jagan Teki wrote: > >>> On Thu, Dec 20, 2018 at 3:59 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > >>>> On 12/20/18 12:14 PM, Jagan Teki wrote: > >>>>> On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > >>>>>> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > >>>>>>> Current driver doesn't check if the destination pointer is NULL. > >>>>>>> This cause the data from the FIFO to be stored inside the internal > >>>>>>> SDRAM ( address 0 ). > >>>>>>> > >>>>>>> The patch add simple check if the destination pointer is NULL. > >>>>>>> > >>>>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> > >>>>>>> --- > >>>>>>> drivers/spi/sun4i_spi.c | 3 ++- > >>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>>>>> > >>>>>>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c > >>>>>>> index b86b5a00ad..38cc743c61 100644 > >>>>>>> --- a/drivers/spi/sun4i_spi.c > >>>>>>> +++ b/drivers/spi/sun4i_spi.c > >>>>>>> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > >>>>>>> > >>>>>>> while (len--) { > >>>>>>> byte = readb(&priv->regs->rxdata); > >>>>>>> - *priv->rx_buf++ = byte; > >>>>>>> + if (priv->rx_buf) > >>>>>>> + *priv->rx_buf++ = byte; > >>>>>> Acked-by: Jagan Teki <jagan@openedev.com> > >>>>>> > >>>>>> But, have you tested how much data in the fifo before drained? It's > >>>>>> better we can get the available data before reading via fifo_sta > >>>> I don't understand what's the point of doing this? > >>> Didn't get? don't you understand what I'm saying or it not require > >>> from point of you? > >> Maybe I don't understand correctly what you're saying. > > You comment now and previous mail doesn't match. better be specific. > > > > For draining fifo. > > We can find how much data available before reading from fifo and > > assign to local rx. > > > > static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > > { > > u32 reg, cnt > > u8 byte; > > > > /* See how much data is available */ > > reg = readl(&priv->regs->fifo_sta); > > reg &= SUN4I_FIFO_STA_RF_CNT_MASK; > > cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; > > > > if (len > cnt) > > len = cnt; > > > > while (len--) { > > byte = readb(&priv->regs->rxdata); > > *priv->rx_buf++ = byte; > > } > > } > > > > This can be perfect drain fifo, and this is what I'm trying to test > > you with existing code and after your patch and verify whether all the > > data perfectly drain or not before and after. > > > > Hope you understand, this time. > > > Hope I understood this time... > > > I've made some modification to sun4i_spi_xfer: > > static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen, > const void *dout, void *din, unsigned long flags) > { > struct udevice *bus = dev->parent; > struct sun4i_spi_priv *priv = dev_get_priv(bus); > struct dm_spi_slave_platdata *slave_plat = > dev_get_parent_platdata(dev); > > u32 len = bitlen / 8; > u32 reg, cnt; > u8 nbytes; > int ret; > > priv->tx_buf = dout; > priv->rx_buf = din; > > if (bitlen % 8) { > debug("%s: non byte-aligned SPI transfer.\n", __func__); > return -ENAVAIL; > } > > if (flags & SPI_XFER_BEGIN) > sun4i_spi_set_cs(bus, slave_plat->cs, true); > > reg = readl(&priv->regs->ctl); > > /* Reset FIFOs */ > writel(reg | SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST, &priv->regs->ctl); > > while (len) { > /* Setup the transfer now... */ > nbytes = min(len, (u32)(SUN4I_FIFO_DEPTH - 1)); > > if (!priv->rx_buf) > printf("%s: Sending %d bytes, ", __func__, nbytes); > > /* Setup the counters */ > writel(SUN4I_BURST_CNT(nbytes), &priv->regs->bc); > writel(SUN4I_XMIT_CNT(nbytes), &priv->regs->tc); > > /* Fill the TX FIFO */ > sun4i_spi_fill_fifo(priv, nbytes); > > /* Start the transfer */ > reg = readl(&priv->regs->ctl); > writel(reg | SUN4I_CTL_XCH, &priv->regs->ctl); > > /* Wait transfer to complete */ > ret = wait_for_bit_le32(&priv->regs->ctl, SUN4I_CTL_XCH_MASK, > false, SUN4I_SPI_TIMEOUT_US, false); > if (ret) { > printf("ERROR: sun4i_spi: Timeout transferring data\n"); > sun4i_spi_set_cs(bus, slave_plat->cs, false); > return ret; > } > > /* Drain the RX FIFO */ > if (!priv->rx_buf) { > reg = readl(&priv->regs->fifo_sta); > reg &= SUN4I_FIFO_STA_RF_CNT_MASK; > cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; > printf("rx fifo: before: %d, ", cnt); > } > > sun4i_spi_drain_fifo(priv, nbytes); So it's draining fifo without need to checking the available count insun4i_spi_drain_fifo(), I mean the below code not require. /* See how much data is available */ reg = readl(&priv->regs->fifo_sta); reg &= SUN4I_FIFO_STA_RF_CNT_MASK; cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; if (len > cnt) len = cnt; ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-20 11:54 ` Jagan Teki @ 2018-12-20 12:07 ` Stefan Mavrodiev 2018-12-20 12:38 ` Jagan Teki 0 siblings, 1 reply; 25+ messages in thread From: Stefan Mavrodiev @ 2018-12-20 12:07 UTC (permalink / raw) To: u-boot On 12/20/18 1:54 PM, Jagan Teki wrote: > On Thu, Dec 20, 2018 at 4:49 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >> >> On 12/20/18 12:56 PM, Jagan Teki wrote: >>> On Thu, Dec 20, 2018 at 4:18 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>>> On 12/20/18 12:45 PM, Jagan Teki wrote: >>>>> On Thu, Dec 20, 2018 at 3:59 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>>>>> On 12/20/18 12:14 PM, Jagan Teki wrote: >>>>>>> On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki <jagan@amarulasolutions.com> wrote: >>>>>>>> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>>>>>>>> Current driver doesn't check if the destination pointer is NULL. >>>>>>>>> This cause the data from the FIFO to be stored inside the internal >>>>>>>>> SDRAM ( address 0 ). >>>>>>>>> >>>>>>>>> The patch add simple check if the destination pointer is NULL. >>>>>>>>> >>>>>>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >>>>>>>>> --- >>>>>>>>> drivers/spi/sun4i_spi.c | 3 ++- >>>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c >>>>>>>>> index b86b5a00ad..38cc743c61 100644 >>>>>>>>> --- a/drivers/spi/sun4i_spi.c >>>>>>>>> +++ b/drivers/spi/sun4i_spi.c >>>>>>>>> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) >>>>>>>>> >>>>>>>>> while (len--) { >>>>>>>>> byte = readb(&priv->regs->rxdata); >>>>>>>>> - *priv->rx_buf++ = byte; >>>>>>>>> + if (priv->rx_buf) >>>>>>>>> + *priv->rx_buf++ = byte; >>>>>>>> Acked-by: Jagan Teki <jagan@openedev.com> >>>>>>>> >>>>>>>> But, have you tested how much data in the fifo before drained? It's >>>>>>>> better we can get the available data before reading via fifo_sta >>>>>> I don't understand what's the point of doing this? >>>>> Didn't get? don't you understand what I'm saying or it not require >>>>> from point of you? >>>> Maybe I don't understand correctly what you're saying. >>> You comment now and previous mail doesn't match. better be specific. >>> >>> For draining fifo. >>> We can find how much data available before reading from fifo and >>> assign to local rx. >>> >>> static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) >>> { >>> u32 reg, cnt >>> u8 byte; >>> >>> /* See how much data is available */ >>> reg = readl(&priv->regs->fifo_sta); >>> reg &= SUN4I_FIFO_STA_RF_CNT_MASK; >>> cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; >>> >>> if (len > cnt) >>> len = cnt; >>> >>> while (len--) { >>> byte = readb(&priv->regs->rxdata); >>> *priv->rx_buf++ = byte; >>> } >>> } >>> >>> This can be perfect drain fifo, and this is what I'm trying to test >>> you with existing code and after your patch and verify whether all the >>> data perfectly drain or not before and after. >>> >>> Hope you understand, this time. >> >> Hope I understood this time... >> >> >> I've made some modification to sun4i_spi_xfer: >> >> static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen, >> const void *dout, void *din, unsigned long flags) >> { >> struct udevice *bus = dev->parent; >> struct sun4i_spi_priv *priv = dev_get_priv(bus); >> struct dm_spi_slave_platdata *slave_plat = >> dev_get_parent_platdata(dev); >> >> u32 len = bitlen / 8; >> u32 reg, cnt; >> u8 nbytes; >> int ret; >> >> priv->tx_buf = dout; >> priv->rx_buf = din; >> >> if (bitlen % 8) { >> debug("%s: non byte-aligned SPI transfer.\n", __func__); >> return -ENAVAIL; >> } >> >> if (flags & SPI_XFER_BEGIN) >> sun4i_spi_set_cs(bus, slave_plat->cs, true); >> >> reg = readl(&priv->regs->ctl); >> >> /* Reset FIFOs */ >> writel(reg | SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST, &priv->regs->ctl); >> >> while (len) { >> /* Setup the transfer now... */ >> nbytes = min(len, (u32)(SUN4I_FIFO_DEPTH - 1)); >> >> if (!priv->rx_buf) >> printf("%s: Sending %d bytes, ", __func__, nbytes); >> >> /* Setup the counters */ >> writel(SUN4I_BURST_CNT(nbytes), &priv->regs->bc); >> writel(SUN4I_XMIT_CNT(nbytes), &priv->regs->tc); >> >> /* Fill the TX FIFO */ >> sun4i_spi_fill_fifo(priv, nbytes); >> >> /* Start the transfer */ >> reg = readl(&priv->regs->ctl); >> writel(reg | SUN4I_CTL_XCH, &priv->regs->ctl); >> >> /* Wait transfer to complete */ >> ret = wait_for_bit_le32(&priv->regs->ctl, SUN4I_CTL_XCH_MASK, >> false, SUN4I_SPI_TIMEOUT_US, false); >> if (ret) { >> printf("ERROR: sun4i_spi: Timeout transferring data\n"); >> sun4i_spi_set_cs(bus, slave_plat->cs, false); >> return ret; >> } >> >> /* Drain the RX FIFO */ >> if (!priv->rx_buf) { >> reg = readl(&priv->regs->fifo_sta); >> reg &= SUN4I_FIFO_STA_RF_CNT_MASK; >> cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; >> printf("rx fifo: before: %d, ", cnt); >> } >> >> sun4i_spi_drain_fifo(priv, nbytes); > So it's draining fifo without need to checking the available count > insun4i_spi_drain_fifo(), I mean the below code not require. > > /* See how much data is available */ > reg = readl(&priv->regs->fifo_sta); > reg &= SUN4I_FIFO_STA_RF_CNT_MASK; > cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; > > if (len > cnt) > len = cnt; Yes, I didn't made any modification to sun4i_spi_drain_fifo() function: static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) { u8 byte; while (len--) { byte = readb(&priv->regs->rxdata); if (priv->rx_buf) *priv->rx_buf++ = byte; } } ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-20 12:07 ` Stefan Mavrodiev @ 2018-12-20 12:38 ` Jagan Teki 2018-12-20 12:41 ` Stefan Mavrodiev 0 siblings, 1 reply; 25+ messages in thread From: Jagan Teki @ 2018-12-20 12:38 UTC (permalink / raw) To: u-boot On Thu, Dec 20, 2018 at 5:38 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > > > On 12/20/18 1:54 PM, Jagan Teki wrote: > > On Thu, Dec 20, 2018 at 4:49 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > >> > >> On 12/20/18 12:56 PM, Jagan Teki wrote: > >>> On Thu, Dec 20, 2018 at 4:18 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > >>>> On 12/20/18 12:45 PM, Jagan Teki wrote: > >>>>> On Thu, Dec 20, 2018 at 3:59 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > >>>>>> On 12/20/18 12:14 PM, Jagan Teki wrote: > >>>>>>> On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > >>>>>>>> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > >>>>>>>>> Current driver doesn't check if the destination pointer is NULL. > >>>>>>>>> This cause the data from the FIFO to be stored inside the internal > >>>>>>>>> SDRAM ( address 0 ). > >>>>>>>>> > >>>>>>>>> The patch add simple check if the destination pointer is NULL. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> > >>>>>>>>> --- > >>>>>>>>> drivers/spi/sun4i_spi.c | 3 ++- > >>>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c > >>>>>>>>> index b86b5a00ad..38cc743c61 100644 > >>>>>>>>> --- a/drivers/spi/sun4i_spi.c > >>>>>>>>> +++ b/drivers/spi/sun4i_spi.c > >>>>>>>>> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > >>>>>>>>> > >>>>>>>>> while (len--) { > >>>>>>>>> byte = readb(&priv->regs->rxdata); > >>>>>>>>> - *priv->rx_buf++ = byte; > >>>>>>>>> + if (priv->rx_buf) > >>>>>>>>> + *priv->rx_buf++ = byte; > >>>>>>>> Acked-by: Jagan Teki <jagan@openedev.com> > >>>>>>>> > >>>>>>>> But, have you tested how much data in the fifo before drained? It's > >>>>>>>> better we can get the available data before reading via fifo_sta > >>>>>> I don't understand what's the point of doing this? > >>>>> Didn't get? don't you understand what I'm saying or it not require > >>>>> from point of you? > >>>> Maybe I don't understand correctly what you're saying. > >>> You comment now and previous mail doesn't match. better be specific. > >>> > >>> For draining fifo. > >>> We can find how much data available before reading from fifo and > >>> assign to local rx. > >>> > >>> static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > >>> { > >>> u32 reg, cnt > >>> u8 byte; > >>> > >>> /* See how much data is available */ > >>> reg = readl(&priv->regs->fifo_sta); > >>> reg &= SUN4I_FIFO_STA_RF_CNT_MASK; > >>> cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; > >>> > >>> if (len > cnt) > >>> len = cnt; > >>> > >>> while (len--) { > >>> byte = readb(&priv->regs->rxdata); > >>> *priv->rx_buf++ = byte; > >>> } > >>> } > >>> > >>> This can be perfect drain fifo, and this is what I'm trying to test > >>> you with existing code and after your patch and verify whether all the > >>> data perfectly drain or not before and after. > >>> > >>> Hope you understand, this time. > >> > >> Hope I understood this time... > >> > >> > >> I've made some modification to sun4i_spi_xfer: > >> > >> static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen, > >> const void *dout, void *din, unsigned long flags) > >> { > >> struct udevice *bus = dev->parent; > >> struct sun4i_spi_priv *priv = dev_get_priv(bus); > >> struct dm_spi_slave_platdata *slave_plat = > >> dev_get_parent_platdata(dev); > >> > >> u32 len = bitlen / 8; > >> u32 reg, cnt; > >> u8 nbytes; > >> int ret; > >> > >> priv->tx_buf = dout; > >> priv->rx_buf = din; > >> > >> if (bitlen % 8) { > >> debug("%s: non byte-aligned SPI transfer.\n", __func__); > >> return -ENAVAIL; > >> } > >> > >> if (flags & SPI_XFER_BEGIN) > >> sun4i_spi_set_cs(bus, slave_plat->cs, true); > >> > >> reg = readl(&priv->regs->ctl); > >> > >> /* Reset FIFOs */ > >> writel(reg | SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST, &priv->regs->ctl); > >> > >> while (len) { > >> /* Setup the transfer now... */ > >> nbytes = min(len, (u32)(SUN4I_FIFO_DEPTH - 1)); > >> > >> if (!priv->rx_buf) > >> printf("%s: Sending %d bytes, ", __func__, nbytes); > >> > >> /* Setup the counters */ > >> writel(SUN4I_BURST_CNT(nbytes), &priv->regs->bc); > >> writel(SUN4I_XMIT_CNT(nbytes), &priv->regs->tc); > >> > >> /* Fill the TX FIFO */ > >> sun4i_spi_fill_fifo(priv, nbytes); > >> > >> /* Start the transfer */ > >> reg = readl(&priv->regs->ctl); > >> writel(reg | SUN4I_CTL_XCH, &priv->regs->ctl); > >> > >> /* Wait transfer to complete */ > >> ret = wait_for_bit_le32(&priv->regs->ctl, SUN4I_CTL_XCH_MASK, > >> false, SUN4I_SPI_TIMEOUT_US, false); > >> if (ret) { > >> printf("ERROR: sun4i_spi: Timeout transferring data\n"); > >> sun4i_spi_set_cs(bus, slave_plat->cs, false); > >> return ret; > >> } > >> > >> /* Drain the RX FIFO */ > >> if (!priv->rx_buf) { > >> reg = readl(&priv->regs->fifo_sta); > >> reg &= SUN4I_FIFO_STA_RF_CNT_MASK; > >> cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; > >> printf("rx fifo: before: %d, ", cnt); > >> } > >> > >> sun4i_spi_drain_fifo(priv, nbytes); > > So it's draining fifo without need to checking the available count > > insun4i_spi_drain_fifo(), I mean the below code not require. > > > > /* See how much data is available */ > > reg = readl(&priv->regs->fifo_sta); > > reg &= SUN4I_FIFO_STA_RF_CNT_MASK; > > cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; > > > > if (len > cnt) > > len = cnt; > > Yes, I didn't made any modification to sun4i_spi_drain_fifo() function: OK, thanks for the details and test. Please send the v2 with proper commit head it should be "spi: sun4i: ..." Also I didn't find this patch on patchwork, don't know why? ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-20 12:38 ` Jagan Teki @ 2018-12-20 12:41 ` Stefan Mavrodiev 2018-12-20 12:44 ` Jagan Teki 0 siblings, 1 reply; 25+ messages in thread From: Stefan Mavrodiev @ 2018-12-20 12:41 UTC (permalink / raw) To: u-boot On 12/20/18 2:38 PM, Jagan Teki wrote: > On Thu, Dec 20, 2018 at 5:38 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >> >> On 12/20/18 1:54 PM, Jagan Teki wrote: >>> On Thu, Dec 20, 2018 at 4:49 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>>> On 12/20/18 12:56 PM, Jagan Teki wrote: >>>>> On Thu, Dec 20, 2018 at 4:18 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>>>>> On 12/20/18 12:45 PM, Jagan Teki wrote: >>>>>>> On Thu, Dec 20, 2018 at 3:59 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>>>>>>> On 12/20/18 12:14 PM, Jagan Teki wrote: >>>>>>>>> On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki <jagan@amarulasolutions.com> wrote: >>>>>>>>>> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>>>>>>>>>> Current driver doesn't check if the destination pointer is NULL. >>>>>>>>>>> This cause the data from the FIFO to be stored inside the internal >>>>>>>>>>> SDRAM ( address 0 ). >>>>>>>>>>> >>>>>>>>>>> The patch add simple check if the destination pointer is NULL. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >>>>>>>>>>> --- >>>>>>>>>>> drivers/spi/sun4i_spi.c | 3 ++- >>>>>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c >>>>>>>>>>> index b86b5a00ad..38cc743c61 100644 >>>>>>>>>>> --- a/drivers/spi/sun4i_spi.c >>>>>>>>>>> +++ b/drivers/spi/sun4i_spi.c >>>>>>>>>>> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) >>>>>>>>>>> >>>>>>>>>>> while (len--) { >>>>>>>>>>> byte = readb(&priv->regs->rxdata); >>>>>>>>>>> - *priv->rx_buf++ = byte; >>>>>>>>>>> + if (priv->rx_buf) >>>>>>>>>>> + *priv->rx_buf++ = byte; >>>>>>>>>> Acked-by: Jagan Teki <jagan@openedev.com> >>>>>>>>>> >>>>>>>>>> But, have you tested how much data in the fifo before drained? It's >>>>>>>>>> better we can get the available data before reading via fifo_sta >>>>>>>> I don't understand what's the point of doing this? >>>>>>> Didn't get? don't you understand what I'm saying or it not require >>>>>>> from point of you? >>>>>> Maybe I don't understand correctly what you're saying. >>>>> You comment now and previous mail doesn't match. better be specific. >>>>> >>>>> For draining fifo. >>>>> We can find how much data available before reading from fifo and >>>>> assign to local rx. >>>>> >>>>> static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) >>>>> { >>>>> u32 reg, cnt >>>>> u8 byte; >>>>> >>>>> /* See how much data is available */ >>>>> reg = readl(&priv->regs->fifo_sta); >>>>> reg &= SUN4I_FIFO_STA_RF_CNT_MASK; >>>>> cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; >>>>> >>>>> if (len > cnt) >>>>> len = cnt; >>>>> >>>>> while (len--) { >>>>> byte = readb(&priv->regs->rxdata); >>>>> *priv->rx_buf++ = byte; >>>>> } >>>>> } >>>>> >>>>> This can be perfect drain fifo, and this is what I'm trying to test >>>>> you with existing code and after your patch and verify whether all the >>>>> data perfectly drain or not before and after. >>>>> >>>>> Hope you understand, this time. >>>> Hope I understood this time... >>>> >>>> >>>> I've made some modification to sun4i_spi_xfer: >>>> >>>> static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen, >>>> const void *dout, void *din, unsigned long flags) >>>> { >>>> struct udevice *bus = dev->parent; >>>> struct sun4i_spi_priv *priv = dev_get_priv(bus); >>>> struct dm_spi_slave_platdata *slave_plat = >>>> dev_get_parent_platdata(dev); >>>> >>>> u32 len = bitlen / 8; >>>> u32 reg, cnt; >>>> u8 nbytes; >>>> int ret; >>>> >>>> priv->tx_buf = dout; >>>> priv->rx_buf = din; >>>> >>>> if (bitlen % 8) { >>>> debug("%s: non byte-aligned SPI transfer.\n", __func__); >>>> return -ENAVAIL; >>>> } >>>> >>>> if (flags & SPI_XFER_BEGIN) >>>> sun4i_spi_set_cs(bus, slave_plat->cs, true); >>>> >>>> reg = readl(&priv->regs->ctl); >>>> >>>> /* Reset FIFOs */ >>>> writel(reg | SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST, &priv->regs->ctl); >>>> >>>> while (len) { >>>> /* Setup the transfer now... */ >>>> nbytes = min(len, (u32)(SUN4I_FIFO_DEPTH - 1)); >>>> >>>> if (!priv->rx_buf) >>>> printf("%s: Sending %d bytes, ", __func__, nbytes); >>>> >>>> /* Setup the counters */ >>>> writel(SUN4I_BURST_CNT(nbytes), &priv->regs->bc); >>>> writel(SUN4I_XMIT_CNT(nbytes), &priv->regs->tc); >>>> >>>> /* Fill the TX FIFO */ >>>> sun4i_spi_fill_fifo(priv, nbytes); >>>> >>>> /* Start the transfer */ >>>> reg = readl(&priv->regs->ctl); >>>> writel(reg | SUN4I_CTL_XCH, &priv->regs->ctl); >>>> >>>> /* Wait transfer to complete */ >>>> ret = wait_for_bit_le32(&priv->regs->ctl, SUN4I_CTL_XCH_MASK, >>>> false, SUN4I_SPI_TIMEOUT_US, false); >>>> if (ret) { >>>> printf("ERROR: sun4i_spi: Timeout transferring data\n"); >>>> sun4i_spi_set_cs(bus, slave_plat->cs, false); >>>> return ret; >>>> } >>>> >>>> /* Drain the RX FIFO */ >>>> if (!priv->rx_buf) { >>>> reg = readl(&priv->regs->fifo_sta); >>>> reg &= SUN4I_FIFO_STA_RF_CNT_MASK; >>>> cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; >>>> printf("rx fifo: before: %d, ", cnt); >>>> } >>>> >>>> sun4i_spi_drain_fifo(priv, nbytes); >>> So it's draining fifo without need to checking the available count >>> insun4i_spi_drain_fifo(), I mean the below code not require. >>> >>> /* See how much data is available */ >>> reg = readl(&priv->regs->fifo_sta); >>> reg &= SUN4I_FIFO_STA_RF_CNT_MASK; >>> cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; >>> >>> if (len > cnt) >>> len = cnt; >> Yes, I didn't made any modification to sun4i_spi_drain_fifo() function: > OK, thanks for the details and test. > > Please send the v2 with proper commit head it should be "spi: sun4i: ..." > Also I didn't find this patch on patchwork, don't know why? No idea. I can see it here: https://patchwork.ozlabs.org/patch/1008181/ ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-20 12:41 ` Stefan Mavrodiev @ 2018-12-20 12:44 ` Jagan Teki 2018-12-20 12:46 ` Stefan Mavrodiev 0 siblings, 1 reply; 25+ messages in thread From: Jagan Teki @ 2018-12-20 12:44 UTC (permalink / raw) To: u-boot On Thu, Dec 20, 2018 at 6:12 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > > > On 12/20/18 2:38 PM, Jagan Teki wrote: > > On Thu, Dec 20, 2018 at 5:38 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > >> > >> On 12/20/18 1:54 PM, Jagan Teki wrote: > >>> On Thu, Dec 20, 2018 at 4:49 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > >>>> On 12/20/18 12:56 PM, Jagan Teki wrote: > >>>>> On Thu, Dec 20, 2018 at 4:18 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > >>>>>> On 12/20/18 12:45 PM, Jagan Teki wrote: > >>>>>>> On Thu, Dec 20, 2018 at 3:59 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > >>>>>>>> On 12/20/18 12:14 PM, Jagan Teki wrote: > >>>>>>>>> On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > >>>>>>>>>> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@olimex.com> wrote: > >>>>>>>>>>> Current driver doesn't check if the destination pointer is NULL. > >>>>>>>>>>> This cause the data from the FIFO to be stored inside the internal > >>>>>>>>>>> SDRAM ( address 0 ). > >>>>>>>>>>> > >>>>>>>>>>> The patch add simple check if the destination pointer is NULL. > >>>>>>>>>>> > >>>>>>>>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> > >>>>>>>>>>> --- > >>>>>>>>>>> drivers/spi/sun4i_spi.c | 3 ++- > >>>>>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>>>>>>>>> > >>>>>>>>>>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c > >>>>>>>>>>> index b86b5a00ad..38cc743c61 100644 > >>>>>>>>>>> --- a/drivers/spi/sun4i_spi.c > >>>>>>>>>>> +++ b/drivers/spi/sun4i_spi.c > >>>>>>>>>>> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > >>>>>>>>>>> > >>>>>>>>>>> while (len--) { > >>>>>>>>>>> byte = readb(&priv->regs->rxdata); > >>>>>>>>>>> - *priv->rx_buf++ = byte; > >>>>>>>>>>> + if (priv->rx_buf) > >>>>>>>>>>> + *priv->rx_buf++ = byte; > >>>>>>>>>> Acked-by: Jagan Teki <jagan@openedev.com> > >>>>>>>>>> > >>>>>>>>>> But, have you tested how much data in the fifo before drained? It's > >>>>>>>>>> better we can get the available data before reading via fifo_sta > >>>>>>>> I don't understand what's the point of doing this? > >>>>>>> Didn't get? don't you understand what I'm saying or it not require > >>>>>>> from point of you? > >>>>>> Maybe I don't understand correctly what you're saying. > >>>>> You comment now and previous mail doesn't match. better be specific. > >>>>> > >>>>> For draining fifo. > >>>>> We can find how much data available before reading from fifo and > >>>>> assign to local rx. > >>>>> > >>>>> static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) > >>>>> { > >>>>> u32 reg, cnt > >>>>> u8 byte; > >>>>> > >>>>> /* See how much data is available */ > >>>>> reg = readl(&priv->regs->fifo_sta); > >>>>> reg &= SUN4I_FIFO_STA_RF_CNT_MASK; > >>>>> cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; > >>>>> > >>>>> if (len > cnt) > >>>>> len = cnt; > >>>>> > >>>>> while (len--) { > >>>>> byte = readb(&priv->regs->rxdata); > >>>>> *priv->rx_buf++ = byte; > >>>>> } > >>>>> } > >>>>> > >>>>> This can be perfect drain fifo, and this is what I'm trying to test > >>>>> you with existing code and after your patch and verify whether all the > >>>>> data perfectly drain or not before and after. > >>>>> > >>>>> Hope you understand, this time. > >>>> Hope I understood this time... > >>>> > >>>> > >>>> I've made some modification to sun4i_spi_xfer: > >>>> > >>>> static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen, > >>>> const void *dout, void *din, unsigned long flags) > >>>> { > >>>> struct udevice *bus = dev->parent; > >>>> struct sun4i_spi_priv *priv = dev_get_priv(bus); > >>>> struct dm_spi_slave_platdata *slave_plat = > >>>> dev_get_parent_platdata(dev); > >>>> > >>>> u32 len = bitlen / 8; > >>>> u32 reg, cnt; > >>>> u8 nbytes; > >>>> int ret; > >>>> > >>>> priv->tx_buf = dout; > >>>> priv->rx_buf = din; > >>>> > >>>> if (bitlen % 8) { > >>>> debug("%s: non byte-aligned SPI transfer.\n", __func__); > >>>> return -ENAVAIL; > >>>> } > >>>> > >>>> if (flags & SPI_XFER_BEGIN) > >>>> sun4i_spi_set_cs(bus, slave_plat->cs, true); > >>>> > >>>> reg = readl(&priv->regs->ctl); > >>>> > >>>> /* Reset FIFOs */ > >>>> writel(reg | SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST, &priv->regs->ctl); > >>>> > >>>> while (len) { > >>>> /* Setup the transfer now... */ > >>>> nbytes = min(len, (u32)(SUN4I_FIFO_DEPTH - 1)); > >>>> > >>>> if (!priv->rx_buf) > >>>> printf("%s: Sending %d bytes, ", __func__, nbytes); > >>>> > >>>> /* Setup the counters */ > >>>> writel(SUN4I_BURST_CNT(nbytes), &priv->regs->bc); > >>>> writel(SUN4I_XMIT_CNT(nbytes), &priv->regs->tc); > >>>> > >>>> /* Fill the TX FIFO */ > >>>> sun4i_spi_fill_fifo(priv, nbytes); > >>>> > >>>> /* Start the transfer */ > >>>> reg = readl(&priv->regs->ctl); > >>>> writel(reg | SUN4I_CTL_XCH, &priv->regs->ctl); > >>>> > >>>> /* Wait transfer to complete */ > >>>> ret = wait_for_bit_le32(&priv->regs->ctl, SUN4I_CTL_XCH_MASK, > >>>> false, SUN4I_SPI_TIMEOUT_US, false); > >>>> if (ret) { > >>>> printf("ERROR: sun4i_spi: Timeout transferring data\n"); > >>>> sun4i_spi_set_cs(bus, slave_plat->cs, false); > >>>> return ret; > >>>> } > >>>> > >>>> /* Drain the RX FIFO */ > >>>> if (!priv->rx_buf) { > >>>> reg = readl(&priv->regs->fifo_sta); > >>>> reg &= SUN4I_FIFO_STA_RF_CNT_MASK; > >>>> cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; > >>>> printf("rx fifo: before: %d, ", cnt); > >>>> } > >>>> > >>>> sun4i_spi_drain_fifo(priv, nbytes); > >>> So it's draining fifo without need to checking the available count > >>> insun4i_spi_drain_fifo(), I mean the below code not require. > >>> > >>> /* See how much data is available */ > >>> reg = readl(&priv->regs->fifo_sta); > >>> reg &= SUN4I_FIFO_STA_RF_CNT_MASK; > >>> cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; > >>> > >>> if (len > cnt) > >>> len = cnt; > >> Yes, I didn't made any modification to sun4i_spi_drain_fifo() function: > > OK, thanks for the details and test. > > > > Please send the v2 with proper commit head it should be "spi: sun4i: ..." > > Also I didn't find this patch on patchwork, don't know why? > > No idea. I can see it here: > https://patchwork.ozlabs.org/patch/1008181/ OK, got it. I will format the commit and apply if are OK? ^ permalink raw reply [flat|nested] 25+ messages in thread
* [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check 2018-12-20 12:44 ` Jagan Teki @ 2018-12-20 12:46 ` Stefan Mavrodiev 0 siblings, 0 replies; 25+ messages in thread From: Stefan Mavrodiev @ 2018-12-20 12:46 UTC (permalink / raw) To: u-boot On 12/20/18 2:44 PM, Jagan Teki wrote: > On Thu, Dec 20, 2018 at 6:12 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >> >> On 12/20/18 2:38 PM, Jagan Teki wrote: >>> On Thu, Dec 20, 2018 at 5:38 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>>> On 12/20/18 1:54 PM, Jagan Teki wrote: >>>>> On Thu, Dec 20, 2018 at 4:49 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>>>>> On 12/20/18 12:56 PM, Jagan Teki wrote: >>>>>>> On Thu, Dec 20, 2018 at 4:18 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>>>>>>> On 12/20/18 12:45 PM, Jagan Teki wrote: >>>>>>>>> On Thu, Dec 20, 2018 at 3:59 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>>>>>>>>> On 12/20/18 12:14 PM, Jagan Teki wrote: >>>>>>>>>>> On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki <jagan@amarulasolutions.com> wrote: >>>>>>>>>>>> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@olimex.com> wrote: >>>>>>>>>>>>> Current driver doesn't check if the destination pointer is NULL. >>>>>>>>>>>>> This cause the data from the FIFO to be stored inside the internal >>>>>>>>>>>>> SDRAM ( address 0 ). >>>>>>>>>>>>> >>>>>>>>>>>>> The patch add simple check if the destination pointer is NULL. >>>>>>>>>>>>> >>>>>>>>>>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >>>>>>>>>>>>> --- >>>>>>>>>>>>> drivers/spi/sun4i_spi.c | 3 ++- >>>>>>>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>>>>>>>> >>>>>>>>>>>>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c >>>>>>>>>>>>> index b86b5a00ad..38cc743c61 100644 >>>>>>>>>>>>> --- a/drivers/spi/sun4i_spi.c >>>>>>>>>>>>> +++ b/drivers/spi/sun4i_spi.c >>>>>>>>>>>>> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) >>>>>>>>>>>>> >>>>>>>>>>>>> while (len--) { >>>>>>>>>>>>> byte = readb(&priv->regs->rxdata); >>>>>>>>>>>>> - *priv->rx_buf++ = byte; >>>>>>>>>>>>> + if (priv->rx_buf) >>>>>>>>>>>>> + *priv->rx_buf++ = byte; >>>>>>>>>>>> Acked-by: Jagan Teki <jagan@openedev.com> >>>>>>>>>>>> >>>>>>>>>>>> But, have you tested how much data in the fifo before drained? It's >>>>>>>>>>>> better we can get the available data before reading via fifo_sta >>>>>>>>>> I don't understand what's the point of doing this? >>>>>>>>> Didn't get? don't you understand what I'm saying or it not require >>>>>>>>> from point of you? >>>>>>>> Maybe I don't understand correctly what you're saying. >>>>>>> You comment now and previous mail doesn't match. better be specific. >>>>>>> >>>>>>> For draining fifo. >>>>>>> We can find how much data available before reading from fifo and >>>>>>> assign to local rx. >>>>>>> >>>>>>> static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len) >>>>>>> { >>>>>>> u32 reg, cnt >>>>>>> u8 byte; >>>>>>> >>>>>>> /* See how much data is available */ >>>>>>> reg = readl(&priv->regs->fifo_sta); >>>>>>> reg &= SUN4I_FIFO_STA_RF_CNT_MASK; >>>>>>> cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; >>>>>>> >>>>>>> if (len > cnt) >>>>>>> len = cnt; >>>>>>> >>>>>>> while (len--) { >>>>>>> byte = readb(&priv->regs->rxdata); >>>>>>> *priv->rx_buf++ = byte; >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> This can be perfect drain fifo, and this is what I'm trying to test >>>>>>> you with existing code and after your patch and verify whether all the >>>>>>> data perfectly drain or not before and after. >>>>>>> >>>>>>> Hope you understand, this time. >>>>>> Hope I understood this time... >>>>>> >>>>>> >>>>>> I've made some modification to sun4i_spi_xfer: >>>>>> >>>>>> static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen, >>>>>> const void *dout, void *din, unsigned long flags) >>>>>> { >>>>>> struct udevice *bus = dev->parent; >>>>>> struct sun4i_spi_priv *priv = dev_get_priv(bus); >>>>>> struct dm_spi_slave_platdata *slave_plat = >>>>>> dev_get_parent_platdata(dev); >>>>>> >>>>>> u32 len = bitlen / 8; >>>>>> u32 reg, cnt; >>>>>> u8 nbytes; >>>>>> int ret; >>>>>> >>>>>> priv->tx_buf = dout; >>>>>> priv->rx_buf = din; >>>>>> >>>>>> if (bitlen % 8) { >>>>>> debug("%s: non byte-aligned SPI transfer.\n", __func__); >>>>>> return -ENAVAIL; >>>>>> } >>>>>> >>>>>> if (flags & SPI_XFER_BEGIN) >>>>>> sun4i_spi_set_cs(bus, slave_plat->cs, true); >>>>>> >>>>>> reg = readl(&priv->regs->ctl); >>>>>> >>>>>> /* Reset FIFOs */ >>>>>> writel(reg | SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST, &priv->regs->ctl); >>>>>> >>>>>> while (len) { >>>>>> /* Setup the transfer now... */ >>>>>> nbytes = min(len, (u32)(SUN4I_FIFO_DEPTH - 1)); >>>>>> >>>>>> if (!priv->rx_buf) >>>>>> printf("%s: Sending %d bytes, ", __func__, nbytes); >>>>>> >>>>>> /* Setup the counters */ >>>>>> writel(SUN4I_BURST_CNT(nbytes), &priv->regs->bc); >>>>>> writel(SUN4I_XMIT_CNT(nbytes), &priv->regs->tc); >>>>>> >>>>>> /* Fill the TX FIFO */ >>>>>> sun4i_spi_fill_fifo(priv, nbytes); >>>>>> >>>>>> /* Start the transfer */ >>>>>> reg = readl(&priv->regs->ctl); >>>>>> writel(reg | SUN4I_CTL_XCH, &priv->regs->ctl); >>>>>> >>>>>> /* Wait transfer to complete */ >>>>>> ret = wait_for_bit_le32(&priv->regs->ctl, SUN4I_CTL_XCH_MASK, >>>>>> false, SUN4I_SPI_TIMEOUT_US, false); >>>>>> if (ret) { >>>>>> printf("ERROR: sun4i_spi: Timeout transferring data\n"); >>>>>> sun4i_spi_set_cs(bus, slave_plat->cs, false); >>>>>> return ret; >>>>>> } >>>>>> >>>>>> /* Drain the RX FIFO */ >>>>>> if (!priv->rx_buf) { >>>>>> reg = readl(&priv->regs->fifo_sta); >>>>>> reg &= SUN4I_FIFO_STA_RF_CNT_MASK; >>>>>> cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; >>>>>> printf("rx fifo: before: %d, ", cnt); >>>>>> } >>>>>> >>>>>> sun4i_spi_drain_fifo(priv, nbytes); >>>>> So it's draining fifo without need to checking the available count >>>>> insun4i_spi_drain_fifo(), I mean the below code not require. >>>>> >>>>> /* See how much data is available */ >>>>> reg = readl(&priv->regs->fifo_sta); >>>>> reg &= SUN4I_FIFO_STA_RF_CNT_MASK; >>>>> cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; >>>>> >>>>> if (len > cnt) >>>>> len = cnt; >>>> Yes, I didn't made any modification to sun4i_spi_drain_fifo() function: >>> OK, thanks for the details and test. >>> >>> Please send the v2 with proper commit head it should be "spi: sun4i: ..." >>> Also I didn't find this patch on patchwork, don't know why? >> No idea. I can see it here: >> https://patchwork.ozlabs.org/patch/1008181/ > OK, got it. I will format the commit and apply if are OK? Sure. Thank you. ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2018-12-20 12:46 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-05 12:27 [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check Stefan Mavrodiev 2018-12-05 15:46 ` Maxime Ripard 2018-12-06 6:41 ` Stefan Mavrodiev 2018-12-13 7:12 ` Stefan Mavrodiev 2018-12-14 9:25 ` Maxime Ripard 2018-12-14 14:14 ` Stefan Mavrodiev 2018-12-14 15:55 ` Maxime Ripard 2018-12-17 6:37 ` Stefan Mavrodiev 2018-12-20 6:45 ` Stefan Mavrodiev 2018-12-16 11:16 ` André Przywara 2018-12-14 10:18 ` Jagan Teki 2018-12-14 10:49 ` Maxime Ripard 2018-12-14 12:09 ` Jagan Teki 2018-12-20 10:14 ` Jagan Teki 2018-12-20 10:29 ` Stefan Mavrodiev 2018-12-20 10:45 ` Jagan Teki 2018-12-20 10:48 ` Stefan Mavrodiev 2018-12-20 10:56 ` Jagan Teki 2018-12-20 11:19 ` Stefan Mavrodiev 2018-12-20 11:54 ` Jagan Teki 2018-12-20 12:07 ` Stefan Mavrodiev 2018-12-20 12:38 ` Jagan Teki 2018-12-20 12:41 ` Stefan Mavrodiev 2018-12-20 12:44 ` Jagan Teki 2018-12-20 12:46 ` Stefan Mavrodiev
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.