| Message ID | 1437013654-29387-3-git-send-email-vikas.manocha@st.com |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote: > There is no need to check for sram fill level. If sram is empty, cpu will > go in the wait state till the time data is available from flash. Consider the following scenario: - CPU core reads some memory area, but there are no data available just yet => CPU core goes into wait state - The data never arrive Will the CPU be stuck forever ? If we checked the fill level first instead, we would never enter such stuck-state. > Also Relying on SRAM fill level only for deciding when the data should be > fetched from the local SRAM is not most efficient approach, particularly > if we are working on high data rates. > > It should be noticed that after one SRAM word is collected, the information > is forwarded into system domain and then synchronized into register domain > (apb). If we are using slow APB and AHB clks, SRAM fill level might not be > up-to-dated because of latency cycles needed for synchronization. For > example in case we are getting SRAM fill level equal to 10 locations but > in reality there were 2 another words completed and actual level is 12 but > information may not be synchronized yet because of the synchronization > latency on APB domain. > > Signed-off-by: Vikas Manocha <vikas.manocha@st.com> > --- [...] Best regards, Marek Vasut
Hi Marek, On 08/12/2015 07:09 PM, Marek Vasut wrote: > On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote: >> There is no need to check for sram fill level. If sram is empty, cpu will >> go in the wait state till the time data is available from flash. > > > Consider the following scenario: > - CPU core reads some memory area, but there are no data available just yet > => CPU core goes into wait state > - The data never arrive > > Will the CPU be stuck forever ? If we checked the fill level first instead, > we would never enter such stuck-state. This indirect mode of reading/writing would be entered when the read/write addresses are in the programmed valid range of addresses. Even in case of "data never arrive" scenario, a simple timeout seems better then currently implemented read sram level with timeout. Rgds, Vikas > >> Also Relying on SRAM fill level only for deciding when the data should be >> fetched from the local SRAM is not most efficient approach, particularly >> if we are working on high data rates. >> >> It should be noticed that after one SRAM word is collected, the information >> is forwarded into system domain and then synchronized into register domain >> (apb). If we are using slow APB and AHB clks, SRAM fill level might not be >> up-to-dated because of latency cycles needed for synchronization. For >> example in case we are getting SRAM fill level equal to 10 locations but >> in reality there were 2 another words completed and actual level is 12 but >> information may not be synchronized yet because of the synchronization >> latency on APB domain. >> >> Signed-off-by: Vikas Manocha <vikas.manocha@st.com> >> --- > [...] > Best regards, > Marek Vasut > . >
On Thursday, August 13, 2015 at 06:27:08 PM, vikasm wrote: > Hi Marek, Hi vikasm, (you might want to fix your name in your mailer) > On 08/12/2015 07:09 PM, Marek Vasut wrote: > > On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote: > >> There is no need to check for sram fill level. If sram is empty, cpu > >> will go in the wait state till the time data is available from flash. > > > > Consider the following scenario: > > - CPU core reads some memory area, but there are no data available just > > yet > > > > => CPU core goes into wait state > > > > - The data never arrive > > > > Will the CPU be stuck forever ? If we checked the fill level first > > instead, we would never enter such stuck-state. > > This indirect mode of reading/writing would be entered when the read/write > addresses are in the programmed valid range of addresses. > > Even in case of "data never arrive" scenario, a simple timeout seems better > then currently implemented read sram level with timeout. How do you implement a "simple timeout" if the CPU core is stuck and does not execute instructions ? If you mean interrupt, then forget it, U-Boot does not do interrupts ;-) Best regards, Marek Vasut
Hi Marek, On 08/13/2015 10:33 AM, Marek Vasut wrote: > On Thursday, August 13, 2015 at 06:27:08 PM, vikasm wrote: >> Hi Marek, > > Hi vikasm, > > (you might want to fix your name in your mailer) ok :-) > >> On 08/12/2015 07:09 PM, Marek Vasut wrote: >>> On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote: >>>> There is no need to check for sram fill level. If sram is empty, cpu >>>> will go in the wait state till the time data is available from flash. >>> >>> Consider the following scenario: >>> - CPU core reads some memory area, but there are no data available just >>> yet >>> >>> => CPU core goes into wait state >>> >>> - The data never arrive >>> >>> Will the CPU be stuck forever ? If we checked the fill level first >>> instead, we would never enter such stuck-state. >> >> This indirect mode of reading/writing would be entered when the read/write >> addresses are in the programmed valid range of addresses. >> >> Even in case of "data never arrive" scenario, a simple timeout seems better >> then currently implemented read sram level with timeout. > > How do you implement a "simple timeout" if the CPU core is stuck and does > not execute instructions ? If you mean interrupt, then forget it, U-Boot > does not do interrupts ;-) Oh yes, you are right. Rgds, Vikas > > Best regards, > Marek Vasut > . >
On Thursday, August 13, 2015 at 09:49:49 PM, vikas wrote: Hi! > >> On 08/12/2015 07:09 PM, Marek Vasut wrote: > >>> On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote: > >>>> There is no need to check for sram fill level. If sram is empty, cpu > >>>> will go in the wait state till the time data is available from flash. > >>> > >>> Consider the following scenario: > >>> - CPU core reads some memory area, but there are no data available just > >>> yet > >>> > >>> => CPU core goes into wait state > >>> > >>> - The data never arrive > >>> > >>> Will the CPU be stuck forever ? If we checked the fill level first > >>> instead, we would never enter such stuck-state. > >> > >> This indirect mode of reading/writing would be entered when the > >> read/write addresses are in the programmed valid range of addresses. > >> > >> Even in case of "data never arrive" scenario, a simple timeout seems > >> better then currently implemented read sram level with timeout. > > > > How do you implement a "simple timeout" if the CPU core is stuck and does > > not execute instructions ? If you mean interrupt, then forget it, U-Boot > > does not do interrupts ;-) > > Oh yes, you are right. So shall we keep the SRAM piece ? Best regards, Marek Vasut
Hi Marek, On 08/13/2015 01:35 PM, Marek Vasut wrote: > On Thursday, August 13, 2015 at 09:49:49 PM, vikas wrote: > > Hi! > >>>> On 08/12/2015 07:09 PM, Marek Vasut wrote: >>>>> On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote: >>>>>> There is no need to check for sram fill level. If sram is empty, cpu >>>>>> will go in the wait state till the time data is available from flash. >>>>> >>>>> Consider the following scenario: >>>>> - CPU core reads some memory area, but there are no data available just >>>>> yet >>>>> >>>>> => CPU core goes into wait state >>>>> >>>>> - The data never arrive >>>>> >>>>> Will the CPU be stuck forever ? If we checked the fill level first >>>>> instead, we would never enter such stuck-state. >>>> >>>> This indirect mode of reading/writing would be entered when the >>>> read/write addresses are in the programmed valid range of addresses. >>>> >>>> Even in case of "data never arrive" scenario, a simple timeout seems >>>> better then currently implemented read sram level with timeout. >>> >>> How do you implement a "simple timeout" if the CPU core is stuck and does >>> not execute instructions ? If you mean interrupt, then forget it, U-Boot >>> does not do interrupts ;-) >> >> Oh yes, you are right. > > So shall we keep the SRAM piece ? Although in this case the better solution would be to have watermark interrupt/status check based on sram fill level, let us keep the existing piece of SRAM. Can we make it configurable (SRAM Level test or not) like from DT or #define ? Rgds, Vikas > > Best regards, > Marek Vasut > . >
On Thursday, August 13, 2015 at 11:04:59 PM, vikas wrote: > Hi Marek, Hi! > On 08/13/2015 01:35 PM, Marek Vasut wrote: > > On Thursday, August 13, 2015 at 09:49:49 PM, vikas wrote: > > > > Hi! > > > >>>> On 08/12/2015 07:09 PM, Marek Vasut wrote: > >>>>> On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote: > >>>>>> There is no need to check for sram fill level. If sram is empty, cpu > >>>>>> will go in the wait state till the time data is available from > >>>>>> flash. > >>>>> > >>>>> Consider the following scenario: > >>>>> - CPU core reads some memory area, but there are no data available > >>>>> just yet > >>>>> > >>>>> => CPU core goes into wait state > >>>>> > >>>>> - The data never arrive > >>>>> > >>>>> Will the CPU be stuck forever ? If we checked the fill level first > >>>>> instead, we would never enter such stuck-state. > >>>> > >>>> This indirect mode of reading/writing would be entered when the > >>>> read/write addresses are in the programmed valid range of addresses. > >>>> > >>>> Even in case of "data never arrive" scenario, a simple timeout seems > >>>> better then currently implemented read sram level with timeout. > >>> > >>> How do you implement a "simple timeout" if the CPU core is stuck and > >>> does not execute instructions ? If you mean interrupt, then forget it, > >>> U-Boot does not do interrupts ;-) > >> > >> Oh yes, you are right. > > > > So shall we keep the SRAM piece ? > > Although in this case the better solution would be to have watermark > interrupt/status check based on sram fill level, let us keep the existing > piece of SRAM. Good. > Can we make it configurable (SRAM Level test or not) like from DT or > #define ? How would you call such an option ? Something like CONFIG_SYS_YOLO (to indicate that life is too short to use correct, but slower code) ? :-) I don't want to have two different codepaths in the codebase, one of which is buggy. So no, I disagree we should add this option. I also don't think it would be such a performance improvement, so I only see downsides in such a code. Best regards, Marek Vasut
Hi Marek, On 08/13/2015 03:47 PM, Marek Vasut wrote: > On Thursday, August 13, 2015 at 11:04:59 PM, vikas wrote: >> Hi Marek, > > Hi! > >> On 08/13/2015 01:35 PM, Marek Vasut wrote: >>> On Thursday, August 13, 2015 at 09:49:49 PM, vikas wrote: >>> >>> Hi! >>> >>>>>> On 08/12/2015 07:09 PM, Marek Vasut wrote: >>>>>>> On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote: >>>>>>>> There is no need to check for sram fill level. If sram is empty, cpu >>>>>>>> will go in the wait state till the time data is available from >>>>>>>> flash. >>>>>>> >>>>>>> Consider the following scenario: >>>>>>> - CPU core reads some memory area, but there are no data available >>>>>>> just yet >>>>>>> >>>>>>> => CPU core goes into wait state >>>>>>> >>>>>>> - The data never arrive >>>>>>> >>>>>>> Will the CPU be stuck forever ? If we checked the fill level first >>>>>>> instead, we would never enter such stuck-state. >>>>>> >>>>>> This indirect mode of reading/writing would be entered when the >>>>>> read/write addresses are in the programmed valid range of addresses. >>>>>> >>>>>> Even in case of "data never arrive" scenario, a simple timeout seems >>>>>> better then currently implemented read sram level with timeout. >>>>> >>>>> How do you implement a "simple timeout" if the CPU core is stuck and >>>>> does not execute instructions ? If you mean interrupt, then forget it, >>>>> U-Boot does not do interrupts ;-) >>>> >>>> Oh yes, you are right. >>> >>> So shall we keep the SRAM piece ? >> >> Although in this case the better solution would be to have watermark >> interrupt/status check based on sram fill level, let us keep the existing >> piece of SRAM. > > Good. > >> Can we make it configurable (SRAM Level test or not) like from DT or >> #define ? > > How would you call such an option ? Something like CONFIG_SYS_YOLO (to indicate > that life is too short to use correct, but slower code) ? :-) > > I don't want to have two different codepaths in the codebase, one of which is > buggy. So no, I disagree we should add this option. I also don't think it would > be such a performance improvement, so I only see downsides in such a code. I expected the same answer :-) & agree also. ok, the issue is SRAM Fill Level register is not being updated on my SOC, seems like design issue. Any idea how can i add this fix (to avoid sram level polling) to my soc in u-boot mainline. Rgds, Vikas > > Best regards, > Marek Vasut > . >
On Friday, August 14, 2015 at 01:18:59 AM, vikas wrote: > Hi Marek, Hi! > On 08/13/2015 03:47 PM, Marek Vasut wrote: > > On Thursday, August 13, 2015 at 11:04:59 PM, vikas wrote: > >> Hi Marek, > > > > Hi! > > > >> On 08/13/2015 01:35 PM, Marek Vasut wrote: > >>> On Thursday, August 13, 2015 at 09:49:49 PM, vikas wrote: > >>> > >>> Hi! > >>> > >>>>>> On 08/12/2015 07:09 PM, Marek Vasut wrote: > >>>>>>> On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote: > >>>>>>>> There is no need to check for sram fill level. If sram is empty, > >>>>>>>> cpu will go in the wait state till the time data is available > >>>>>>>> from flash. > >>>>>>> > >>>>>>> Consider the following scenario: > >>>>>>> - CPU core reads some memory area, but there are no data available > >>>>>>> just yet > >>>>>>> > >>>>>>> => CPU core goes into wait state > >>>>>>> > >>>>>>> - The data never arrive > >>>>>>> > >>>>>>> Will the CPU be stuck forever ? If we checked the fill level first > >>>>>>> instead, we would never enter such stuck-state. > >>>>>> > >>>>>> This indirect mode of reading/writing would be entered when the > >>>>>> read/write addresses are in the programmed valid range of addresses. > >>>>>> > >>>>>> Even in case of "data never arrive" scenario, a simple timeout seems > >>>>>> better then currently implemented read sram level with timeout. > >>>>> > >>>>> How do you implement a "simple timeout" if the CPU core is stuck and > >>>>> does not execute instructions ? If you mean interrupt, then forget > >>>>> it, U-Boot does not do interrupts ;-) > >>>> > >>>> Oh yes, you are right. > >>> > >>> So shall we keep the SRAM piece ? > >> > >> Although in this case the better solution would be to have watermark > >> interrupt/status check based on sram fill level, let us keep the > >> existing piece of SRAM. > > > > Good. > > > >> Can we make it configurable (SRAM Level test or not) like from DT or > >> #define ? > > > > How would you call such an option ? Something like CONFIG_SYS_YOLO (to > > indicate that life is too short to use correct, but slower code) ? :-) > > > > I don't want to have two different codepaths in the codebase, one of > > which is buggy. So no, I disagree we should add this option. I also > > don't think it would be such a performance improvement, so I only see > > downsides in such a code. > > I expected the same answer :-) & agree also. Heh, thanks :-) > ok, the issue is SRAM Fill Level register is not being updated on my SOC, > seems like design issue. Any idea how can i add this fix (to avoid sram > level polling) to my soc in u-boot mainline. Mask all interrupts in the controller, set watermark to N bytes (depending on the length of your transfer) and poll the interrupt status register until the watermark is reached ? Is this possible ?
Hi, On 08/13/2015 04:46 PM, Marek Vasut wrote: > On Friday, August 14, 2015 at 01:18:59 AM, vikas wrote: >> Hi Marek, > > Hi! > >> On 08/13/2015 03:47 PM, Marek Vasut wrote: >>> On Thursday, August 13, 2015 at 11:04:59 PM, vikas wrote: >>>> Hi Marek, >>> >>> Hi! >>> >>>> On 08/13/2015 01:35 PM, Marek Vasut wrote: >>>>> On Thursday, August 13, 2015 at 09:49:49 PM, vikas wrote: >>>>> >>>>> Hi! >>>>> >>>>>>>> On 08/12/2015 07:09 PM, Marek Vasut wrote: >>>>>>>>> On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote: >>>>>>>>>> There is no need to check for sram fill level. If sram is empty, >>>>>>>>>> cpu will go in the wait state till the time data is available >>>>>>>>>> from flash. >>>>>>>>> >>>>>>>>> Consider the following scenario: >>>>>>>>> - CPU core reads some memory area, but there are no data available >>>>>>>>> just yet >>>>>>>>> >>>>>>>>> => CPU core goes into wait state >>>>>>>>> >>>>>>>>> - The data never arrive >>>>>>>>> >>>>>>>>> Will the CPU be stuck forever ? If we checked the fill level first >>>>>>>>> instead, we would never enter such stuck-state. >>>>>>>> >>>>>>>> This indirect mode of reading/writing would be entered when the >>>>>>>> read/write addresses are in the programmed valid range of addresses. >>>>>>>> >>>>>>>> Even in case of "data never arrive" scenario, a simple timeout seems >>>>>>>> better then currently implemented read sram level with timeout. >>>>>>> >>>>>>> How do you implement a "simple timeout" if the CPU core is stuck and >>>>>>> does not execute instructions ? If you mean interrupt, then forget >>>>>>> it, U-Boot does not do interrupts ;-) >>>>>> >>>>>> Oh yes, you are right. >>>>> >>>>> So shall we keep the SRAM piece ? >>>> >>>> Although in this case the better solution would be to have watermark >>>> interrupt/status check based on sram fill level, let us keep the >>>> existing piece of SRAM. >>> >>> Good. >>> >>>> Can we make it configurable (SRAM Level test or not) like from DT or >>>> #define ? >>> >>> How would you call such an option ? Something like CONFIG_SYS_YOLO (to >>> indicate that life is too short to use correct, but slower code) ? :-) >>> >>> I don't want to have two different codepaths in the codebase, one of >>> which is buggy. So no, I disagree we should add this option. I also >>> don't think it would be such a performance improvement, so I only see >>> downsides in such a code. >> >> I expected the same answer :-) & agree also. > > Heh, thanks :-) > >> ok, the issue is SRAM Fill Level register is not being updated on my SOC, >> seems like design issue. Any idea how can i add this fix (to avoid sram >> level polling) to my soc in u-boot mainline. > > Mask all interrupts in the controller, set watermark to N bytes (depending > on the length of your transfer) and poll the interrupt status register until > the watermark is reached ? Is this possible ? Watermark is for sram level and might not work as well. In general also for such soc/platform issues for any driver, any idea how to apply fixes. Rgds, Vikas > . >
On Friday, August 14, 2015 at 02:26:02 AM, vikas wrote: Hi! > >>>>>>>>>> There is no need to check for sram fill level. If sram is empty, > >>>>>>>>>> cpu will go in the wait state till the time data is available > >>>>>>>>>> from flash. > >>>>>>>>> > >>>>>>>>> Consider the following scenario: > >>>>>>>>> - CPU core reads some memory area, but there are no data > >>>>>>>>> available just yet > >>>>>>>>> > >>>>>>>>> => CPU core goes into wait state > >>>>>>>>> > >>>>>>>>> - The data never arrive > >>>>>>>>> > >>>>>>>>> Will the CPU be stuck forever ? If we checked the fill level > >>>>>>>>> first instead, we would never enter such stuck-state. > >>>>>>>> > >>>>>>>> This indirect mode of reading/writing would be entered when the > >>>>>>>> read/write addresses are in the programmed valid range of > >>>>>>>> addresses. > >>>>>>>> > >>>>>>>> Even in case of "data never arrive" scenario, a simple timeout > >>>>>>>> seems better then currently implemented read sram level with > >>>>>>>> timeout. > >>>>>>> > >>>>>>> How do you implement a "simple timeout" if the CPU core is stuck > >>>>>>> and does not execute instructions ? If you mean interrupt, then > >>>>>>> forget it, U-Boot does not do interrupts ;-) > >>>>>> > >>>>>> Oh yes, you are right. > >>>>> > >>>>> So shall we keep the SRAM piece ? > >>>> > >>>> Although in this case the better solution would be to have watermark > >>>> interrupt/status check based on sram fill level, let us keep the > >>>> existing piece of SRAM. > >>> > >>> Good. > >>> > >>>> Can we make it configurable (SRAM Level test or not) like from DT or > >>>> #define ? > >>> > >>> How would you call such an option ? Something like CONFIG_SYS_YOLO (to > >>> indicate that life is too short to use correct, but slower code) ? :-) > >>> > >>> I don't want to have two different codepaths in the codebase, one of > >>> which is buggy. So no, I disagree we should add this option. I also > >>> don't think it would be such a performance improvement, so I only see > >>> downsides in such a code. > >> > >> I expected the same answer :-) & agree also. > > > > Heh, thanks :-) > > > >> ok, the issue is SRAM Fill Level register is not being updated on my > >> SOC, seems like design issue. Any idea how can i add this fix (to avoid > >> sram level polling) to my soc in u-boot mainline. > > > > Mask all interrupts in the controller, set watermark to N bytes > > (depending on the length of your transfer) and poll the interrupt status > > register until the watermark is reached ? Is this possible ? > > Watermark is for sram level and might not work as well. > In general also for such soc/platform issues for any driver, any idea how > to apply fixes. That's a good question . Is there any way on the ST chip to avoid such direct blocking read which can stall the CPU core indefinitelly? Can you check with the chip designers ?
Hi, On 08/13/2015 05:44 PM, Marek Vasut wrote: > On Friday, August 14, 2015 at 02:26:02 AM, vikas wrote: > > Hi! > >>>>>>>>>>>> There is no need to check for sram fill level. If sram is empty, >>>>>>>>>>>> cpu will go in the wait state till the time data is available >>>>>>>>>>>> from flash. >>>>>>>>>>> >>>>>>>>>>> Consider the following scenario: >>>>>>>>>>> - CPU core reads some memory area, but there are no data >>>>>>>>>>> available just yet >>>>>>>>>>> >>>>>>>>>>> => CPU core goes into wait state >>>>>>>>>>> >>>>>>>>>>> - The data never arrive >>>>>>>>>>> >>>>>>>>>>> Will the CPU be stuck forever ? If we checked the fill level >>>>>>>>>>> first instead, we would never enter such stuck-state. >>>>>>>>>> >>>>>>>>>> This indirect mode of reading/writing would be entered when the >>>>>>>>>> read/write addresses are in the programmed valid range of >>>>>>>>>> addresses. >>>>>>>>>> >>>>>>>>>> Even in case of "data never arrive" scenario, a simple timeout >>>>>>>>>> seems better then currently implemented read sram level with >>>>>>>>>> timeout. >>>>>>>>> >>>>>>>>> How do you implement a "simple timeout" if the CPU core is stuck >>>>>>>>> and does not execute instructions ? If you mean interrupt, then >>>>>>>>> forget it, U-Boot does not do interrupts ;-) >>>>>>>> >>>>>>>> Oh yes, you are right. >>>>>>> >>>>>>> So shall we keep the SRAM piece ? >>>>>> >>>>>> Although in this case the better solution would be to have watermark >>>>>> interrupt/status check based on sram fill level, let us keep the >>>>>> existing piece of SRAM. >>>>> >>>>> Good. >>>>> >>>>>> Can we make it configurable (SRAM Level test or not) like from DT or >>>>>> #define ? >>>>> >>>>> How would you call such an option ? Something like CONFIG_SYS_YOLO (to >>>>> indicate that life is too short to use correct, but slower code) ? :-) >>>>> >>>>> I don't want to have two different codepaths in the codebase, one of >>>>> which is buggy. So no, I disagree we should add this option. I also >>>>> don't think it would be such a performance improvement, so I only see >>>>> downsides in such a code. >>>> >>>> I expected the same answer :-) & agree also. >>> >>> Heh, thanks :-) >>> >>>> ok, the issue is SRAM Fill Level register is not being updated on my >>>> SOC, seems like design issue. Any idea how can i add this fix (to avoid >>>> sram level polling) to my soc in u-boot mainline. >>> >>> Mask all interrupts in the controller, set watermark to N bytes >>> (depending on the length of your transfer) and poll the interrupt status >>> register until the watermark is reached ? Is this possible ? >> >> Watermark is for sram level and might not work as well. >> In general also for such soc/platform issues for any driver, any idea how >> to apply fixes. > > That's a good question . Is there any way on the ST chip to avoid such direct > blocking read which can stall the CPU core indefinitelly? Can you check with > the chip designers ? I already checked & it does not exist in the hardware. Solution to that are timeout interrupts in indirect mode or use only direct mode. In case of direct mode, the u-boot driver can't be used. But again, can we add some fix for stv0991 arch to avoid sram level. Without that we will not be able to use the driver at all for stv0991. In general, again it seems not a unique problem. Any idea how to apply fixes for such peripheral design bugs. Rgds, Vikas > > . >
On Friday, August 14, 2015 at 02:46:20 AM, vikas wrote: > Hi, Hi, [...] > >>>>> I don't want to have two different codepaths in the codebase, one of > >>>>> which is buggy. So no, I disagree we should add this option. I also > >>>>> don't think it would be such a performance improvement, so I only see > >>>>> downsides in such a code. > >>>> > >>>> I expected the same answer :-) & agree also. > >>> > >>> Heh, thanks :-) > >>> > >>>> ok, the issue is SRAM Fill Level register is not being updated on my > >>>> SOC, seems like design issue. Any idea how can i add this fix (to > >>>> avoid sram level polling) to my soc in u-boot mainline. > >>> > >>> Mask all interrupts in the controller, set watermark to N bytes > >>> (depending on the length of your transfer) and poll the interrupt > >>> status register until the watermark is reached ? Is this possible ? > >> > >> Watermark is for sram level and might not work as well. > >> In general also for such soc/platform issues for any driver, any idea > >> how to apply fixes. > > > > That's a good question . Is there any way on the ST chip to avoid such > > direct blocking read which can stall the CPU core indefinitelly? Can you > > check with the chip designers ? > > I already checked & it does not exist in the hardware. Solution to that are > timeout interrupts in indirect mode or use only direct mode. In case of > direct mode, the u-boot driver can't be used. Why can't you use direct mode ? Wouldn't that solve your problem (with some performance penalty probably) ? > But again, can we add some fix for stv0991 arch to avoid sram level. > Without that we will not be able to use the driver at all for stv0991. > > In general, again it seems not a unique problem. Any idea how to apply > fixes for such peripheral design bugs. What do you mean it's not unique please ? One option would be to pull out the SRAM level check into a separate function and in that function check whether the controller is the one on ST chip (via compatible string) and if it is, return fake "full" SRAM level. You'd then fall back to the blocking read. I don't like the idea , but if there is no other way ... Best regards, Marek Vasut
Hi, On 08/13/2015 06:03 PM, Marek Vasut wrote: > On Friday, August 14, 2015 at 02:46:20 AM, vikas wrote: >> Hi, > > Hi, > > [...] > >>>>>>> I don't want to have two different codepaths in the codebase, one of >>>>>>> which is buggy. So no, I disagree we should add this option. I also >>>>>>> don't think it would be such a performance improvement, so I only see >>>>>>> downsides in such a code. >>>>>> >>>>>> I expected the same answer :-) & agree also. >>>>> >>>>> Heh, thanks :-) >>>>> >>>>>> ok, the issue is SRAM Fill Level register is not being updated on my >>>>>> SOC, seems like design issue. Any idea how can i add this fix (to >>>>>> avoid sram level polling) to my soc in u-boot mainline. >>>>> >>>>> Mask all interrupts in the controller, set watermark to N bytes >>>>> (depending on the length of your transfer) and poll the interrupt >>>>> status register until the watermark is reached ? Is this possible ? >>>> >>>> Watermark is for sram level and might not work as well. >>>> In general also for such soc/platform issues for any driver, any idea >>>> how to apply fixes. >>> >>> That's a good question . Is there any way on the ST chip to avoid such >>> direct blocking read which can stall the CPU core indefinitelly? Can you >>> check with the chip designers ? >> >> I already checked & it does not exist in the hardware. Solution to that are >> timeout interrupts in indirect mode or use only direct mode. In case of >> direct mode, the u-boot driver can't be used. > > Why can't you use direct mode ? Wouldn't that solve your problem (with some > performance penalty probably) ? u-boot driver does not support direct mode. > >> But again, can we add some fix for stv0991 arch to avoid sram level. >> Without that we will not be able to use the driver at all for stv0991. >> >> In general, again it seems not a unique problem. Any idea how to apply >> fixes for such peripheral design bugs. > > What do you mean it's not unique please ? SOCs have one or the other design bugs often & there should be some way to apply fixes/workrounds. > > One option would be to pull out the SRAM level check into a separate function > and in that function check whether the controller is the one on ST chip (via > compatible string) and if it is, return fake "full" SRAM level. You'd then > fall back to the blocking read. I don't like the idea , but if there is no > other way ... It seems ok to me, I can send the patch for the same. Please let me know. Rgds, Vikas > > Best regards, > Marek Vasut > . >
On Friday, August 14, 2015 at 03:05:57 AM, vikas wrote: > Hi, Hi! > On 08/13/2015 06:03 PM, Marek Vasut wrote: > > On Friday, August 14, 2015 at 02:46:20 AM, vikas wrote: > >> Hi, > > > > Hi, > > > > [...] > > > >>>>>>> I don't want to have two different codepaths in the codebase, one > >>>>>>> of which is buggy. So no, I disagree we should add this option. I > >>>>>>> also don't think it would be such a performance improvement, so I > >>>>>>> only see downsides in such a code. > >>>>>> > >>>>>> I expected the same answer :-) & agree also. > >>>>> > >>>>> Heh, thanks :-) > >>>>> > >>>>>> ok, the issue is SRAM Fill Level register is not being updated on my > >>>>>> SOC, seems like design issue. Any idea how can i add this fix (to > >>>>>> avoid sram level polling) to my soc in u-boot mainline. > >>>>> > >>>>> Mask all interrupts in the controller, set watermark to N bytes > >>>>> (depending on the length of your transfer) and poll the interrupt > >>>>> status register until the watermark is reached ? Is this possible ? > >>>> > >>>> Watermark is for sram level and might not work as well. > >>>> In general also for such soc/platform issues for any driver, any idea > >>>> how to apply fixes. > >>> > >>> That's a good question . Is there any way on the ST chip to avoid such > >>> direct blocking read which can stall the CPU core indefinitelly? Can > >>> you check with the chip designers ? > >> > >> I already checked & it does not exist in the hardware. Solution to that > >> are timeout interrupts in indirect mode or use only direct mode. In > >> case of direct mode, the u-boot driver can't be used. > > > > Why can't you use direct mode ? Wouldn't that solve your problem (with > > some performance penalty probably) ? > > u-boot driver does not support direct mode. What about implementing support for direct mode ? :) > >> But again, can we add some fix for stv0991 arch to avoid sram level. > >> Without that we will not be able to use the driver at all for stv0991. > >> > >> In general, again it seems not a unique problem. Any idea how to apply > >> fixes for such peripheral design bugs. > > > > What do you mean it's not unique please ? > > SOCs have one or the other design bugs often & there should be some way to > apply fixes/workrounds. They do, but unless we precisely know what the problem is, we're kept in the dark. If it's really the case that your SRAM level is not begin updated, then you might want to add DT prop, something like "cdns,broken-sram-level", pull out the sram level checking code and if this prop is set, simple report that the SRAM level is "full". But I wonder, is your SRAM level counter not updating or is your FIFO just overflowing (ie. SRAM level indicator is rolling over) ? What are the real symptoms ? > > One option would be to pull out the SRAM level check into a separate > > function and in that function check whether the controller is the one on > > ST chip (via compatible string) and if it is, return fake "full" SRAM > > level. You'd then fall back to the blocking read. I don't like the idea > > , but if there is no other way ... > > It seems ok to me, I can send the patch for the same. Please let me know. Thinking about it a bit further, adding a DT prop for this might be a better idea than the compatible string. Best regards,
diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c index 1ae7edf..487bbef 100644 --- a/drivers/spi/cadence_qspi_apb.c +++ b/drivers/spi/cadence_qspi_apb.c @@ -192,7 +192,7 @@ static unsigned int cadence_qspi_apb_cmd2addr(const unsigned char *addr_buf, return addr; } -static void cadence_qspi_apb_read_fifo_data(void *dest, +static int cadence_qspi_apb_read_fifo_data(void *dest, const void *src_ahb_addr, unsigned int bytes) { unsigned int temp; @@ -211,7 +211,7 @@ static void cadence_qspi_apb_read_fifo_data(void *dest, memcpy(dest_ptr, &temp, remaining); } - return; + return 0; } static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr, @@ -240,42 +240,6 @@ static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr, return; } -/* Read from SRAM FIFO with polling SRAM fill level. */ -static int qspi_read_sram_fifo_poll(const void *reg_base, void *dest_addr, - const void *src_addr, unsigned int num_bytes) -{ - unsigned int remaining = num_bytes; - unsigned int retry; - unsigned int sram_level = 0; - unsigned char *dest = (unsigned char *)dest_addr; - - while (remaining > 0) { - retry = CQSPI_REG_RETRY; - while (retry--) { - sram_level = CQSPI_GET_RD_SRAM_LEVEL(reg_base); - if (sram_level) - break; - udelay(1); - } - - if (!retry) { - printf("QSPI: No receive data after polling for %d times\n", - CQSPI_REG_RETRY); - return -1; - } - - sram_level *= CQSPI_FIFO_WIDTH; - sram_level = sram_level > remaining ? remaining : sram_level; - - /* Read data from FIFO. */ - cadence_qspi_apb_read_fifo_data(dest, src_addr, sram_level); - dest += sram_level; - remaining -= sram_level; - udelay(1); - } - return 0; -} - /* Write to SRAM FIFO with polling SRAM fill level. */ static int qpsi_write_sram_fifo_push(struct cadence_spi_platdata *plat, const void *src_addr, unsigned int num_bytes) @@ -750,9 +714,10 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat, /* Start the indirect read transfer */ writel(CQSPI_REG_INDIRECTRD_START_MASK, plat->regbase + CQSPI_REG_INDIRECTRD); + udelay(1); - if (qspi_read_sram_fifo_poll(plat->regbase, (void *)rxbuf, - (const void *)plat->ahbbase, rxlen)) + if (cadence_qspi_apb_read_fifo_data((void *)rxbuf, + (const void *)plat->ahbbase, rxlen)) goto failrd; /* Check flash indirect controller */
There is no need to check for sram fill level. If sram is empty, cpu will go in the wait state till the time data is available from flash. Also Relying on SRAM fill level only for deciding when the data should be fetched from the local SRAM is not most efficient approach, particularly if we are working on high data rates. It should be noticed that after one SRAM word is collected, the information is forwarded into system domain and then synchronized into register domain (apb). If we are using slow APB and AHB clks, SRAM fill level might not be up-to-dated because of latency cycles needed for synchronization. For example in case we are getting SRAM fill level equal to 10 locations but in reality there were 2 another words completed and actual level is 12 but information may not be synchronized yet because of the synchronization latency on APB domain. Signed-off-by: Vikas Manocha <vikas.manocha@st.com> --- Changes in v2: Rebased to master drivers/spi/cadence_qspi_apb.c | 45 +++++----------------------------------- 1 file changed, 5 insertions(+), 40 deletions(-)