From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jon Hunter Subject: Re: [PATCH v6 07/10] ARM: OMAP2+: gpmc: generic timing calculation Date: Mon, 27 Aug 2012 15:30:13 -0500 Message-ID: <503BD8D5.5060400@ti.com> References: <83b963e4ebcc1009a26a8c6419c785ac6d742c0b.1345524670.git.afzal@ti.com> <50359C64.40603@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Return-path: Received: from bear.ext.ti.com ([192.94.94.41]:45644 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754286Ab2H0UaI (ORCPT ); Mon, 27 Aug 2012 16:30:08 -0400 In-Reply-To: Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: "Mohammed, Afzal" Cc: "tony@atomide.com" , "paul@pwsan.com" , "linux-omap@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" Hi Afzal, On 08/27/2012 05:37 AM, Mohammed, Afzal wrote: > On Thu, Aug 23, 2012 at 08:28:44, Hunter, Jon wrote: [snip] >> I understand that this is based upon how timings for onenand and tusb >> are being calculated today, but I am not sure that this is the way to go >> for all devices. Personally, I would like to see us get away from how >> those devices are calculating timings for any new device. > > You cannot do away with many of the those, as logically they > are right. Eg. read data should be available at access time, > assuming a zero data hold time, we can very well derive an > expression as, I am not saying that we do away with them for current devices, just maintain them as is. > read de-assertion time (oe_off) = access time plus 1 gpmc clock, > > and this is what the existing calculations do, and also the > generic routine. There could be other constraints, but this > certainly should be one (I do realize that oe_off could be > optimized to be less than access time, by relying on read > hold time, then again effect of it would be in similar way > for different peripherals, but let's forget about > optimization in the beginning) > >> In general, I am concerned that we are abstracting the timings further >> from the actual register fields. For example, the timings parameter >> "t_ceasu" is described "address setup to CS valid" which is not >> incorrect but this value is really just programming the CSONTIME field >> and so why not call this cs_on? > > Timing fields of struct gpmc_device_timings are selected such > that they should be bindable by DT. At least one of the peripheral > datasheet has these fields. Right, but these are not applicable to every device and so I worry this could be confusing. However, more documentation may help clear this up. > If user knows timings in terms of > gpmc values, he can directly use struct gpmc_timings, but > then all the values should be updated in struct gpmc_timings. > User should not update some of the values in terms of > peripheral timings, others in terms of gpmc timings, that > would make things complex and does not seem the right way > to me. > > cs_on and other gpmc aware timings could be binded by DT, not as > peripheral timing, but as gpmc timing. > > Bindings for peripheral DT timings should be something that > can be obtained from peripheral datasheet, here accidentally > it is same as gpmc timings, but not most timings are this way. > Also there could be other constraints that can come for cs_on, > even though I have not come across any till now. > >> >> So although this may consolidate how the timings are calculated today, I >> am concerned it will be confusing to add timings for a new device. At >> least if I am calculating timings, I am taking the timing information >> for the device and translating that to the how I need to program the >> gpmc register fields. > > If I am not wrong, GPMC IP has been present for around a > decade, and so far I have not come across any generic time > calculation method that can be applied to all peripherals. Yes not an easy problem to solve :-( > Getting to work same peripheral for a different gpmc > frequency is another problem. > > Here we are trying to generalize based on the understanding of > gpmc & peripheral timings, as well as by a kind of reverse > engineering without most of the hardware or datasheet. Think of > this as an attempt to create one, let it evolve and become a > robust one. If a new user want to add a peripheral, let him add > support, input timings are selected such that it is found in > peripheral datasheet, if that does not work, let him try > to add any new timing field or alter existing expression > as he deems to be proper and verify that it does not break > others. And I can help provided I am not heavily loaded > with other works. So long as it is maintainable ;-) > And at least for initial users, they are expected to have > some grasp on how to calculate timings, such a user will > not be much worried about your 3 concerns above, anyway as > of now they need to have a good grasp on it. I would consider myself to be an initial user and I am concerned, doesn't that count? An example, would be the following where you have 4 timing parameters for access time. You need to dig through the code to understand how these are being used. + u32 t_aa; /* access time from ADV assertion */ + u32 t_iaa; /* initial access time */ + u32 t_oe; /* access time from OE assertion */ + u32 t_ce; /* access time from CS asertion */ > Meanwhile I will try to document more. Yes more documentation is definitely needed. Cheers Jon From mboxrd@z Thu Jan 1 00:00:00 1970 From: jon-hunter@ti.com (Jon Hunter) Date: Mon, 27 Aug 2012 15:30:13 -0500 Subject: [PATCH v6 07/10] ARM: OMAP2+: gpmc: generic timing calculation In-Reply-To: References: <83b963e4ebcc1009a26a8c6419c785ac6d742c0b.1345524670.git.afzal@ti.com> <50359C64.40603@ti.com> Message-ID: <503BD8D5.5060400@ti.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Afzal, On 08/27/2012 05:37 AM, Mohammed, Afzal wrote: > On Thu, Aug 23, 2012 at 08:28:44, Hunter, Jon wrote: [snip] >> I understand that this is based upon how timings for onenand and tusb >> are being calculated today, but I am not sure that this is the way to go >> for all devices. Personally, I would like to see us get away from how >> those devices are calculating timings for any new device. > > You cannot do away with many of the those, as logically they > are right. Eg. read data should be available at access time, > assuming a zero data hold time, we can very well derive an > expression as, I am not saying that we do away with them for current devices, just maintain them as is. > read de-assertion time (oe_off) = access time plus 1 gpmc clock, > > and this is what the existing calculations do, and also the > generic routine. There could be other constraints, but this > certainly should be one (I do realize that oe_off could be > optimized to be less than access time, by relying on read > hold time, then again effect of it would be in similar way > for different peripherals, but let's forget about > optimization in the beginning) > >> In general, I am concerned that we are abstracting the timings further >> from the actual register fields. For example, the timings parameter >> "t_ceasu" is described "address setup to CS valid" which is not >> incorrect but this value is really just programming the CSONTIME field >> and so why not call this cs_on? > > Timing fields of struct gpmc_device_timings are selected such > that they should be bindable by DT. At least one of the peripheral > datasheet has these fields. Right, but these are not applicable to every device and so I worry this could be confusing. However, more documentation may help clear this up. > If user knows timings in terms of > gpmc values, he can directly use struct gpmc_timings, but > then all the values should be updated in struct gpmc_timings. > User should not update some of the values in terms of > peripheral timings, others in terms of gpmc timings, that > would make things complex and does not seem the right way > to me. > > cs_on and other gpmc aware timings could be binded by DT, not as > peripheral timing, but as gpmc timing. > > Bindings for peripheral DT timings should be something that > can be obtained from peripheral datasheet, here accidentally > it is same as gpmc timings, but not most timings are this way. > Also there could be other constraints that can come for cs_on, > even though I have not come across any till now. > >> >> So although this may consolidate how the timings are calculated today, I >> am concerned it will be confusing to add timings for a new device. At >> least if I am calculating timings, I am taking the timing information >> for the device and translating that to the how I need to program the >> gpmc register fields. > > If I am not wrong, GPMC IP has been present for around a > decade, and so far I have not come across any generic time > calculation method that can be applied to all peripherals. Yes not an easy problem to solve :-( > Getting to work same peripheral for a different gpmc > frequency is another problem. > > Here we are trying to generalize based on the understanding of > gpmc & peripheral timings, as well as by a kind of reverse > engineering without most of the hardware or datasheet. Think of > this as an attempt to create one, let it evolve and become a > robust one. If a new user want to add a peripheral, let him add > support, input timings are selected such that it is found in > peripheral datasheet, if that does not work, let him try > to add any new timing field or alter existing expression > as he deems to be proper and verify that it does not break > others. And I can help provided I am not heavily loaded > with other works. So long as it is maintainable ;-) > And at least for initial users, they are expected to have > some grasp on how to calculate timings, such a user will > not be much worried about your 3 concerns above, anyway as > of now they need to have a good grasp on it. I would consider myself to be an initial user and I am concerned, doesn't that count? An example, would be the following where you have 4 timing parameters for access time. You need to dig through the code to understand how these are being used. + u32 t_aa; /* access time from ADV assertion */ + u32 t_iaa; /* initial access time */ + u32 t_oe; /* access time from OE assertion */ + u32 t_ce; /* access time from CS asertion */ > Meanwhile I will try to document more. Yes more documentation is definitely needed. Cheers Jon