RE: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0
> -Original Message- > ow...@vger.kernel.org] On Behalf Of Paul Walmsley > Sent: Wednesday, October 28, 2009 1:37 AM > [..] > a comment on cpu_relax() and udelay(). > > On Mon, 26 Oct 2009, Menon, Nishanth wrote: > > > > -Original Message- > > > From: G, Manjunath Kondaiah > > > > > > As per your comments for other patches when ever there is udelay usage, > > > cpu_relax is the better option. I see there are lot of udelay(...) > calls > > > used in this patch. Why can't you use cpu_relax() or schedule(). > > > Any specific reason? > > > > > Don’t really want to do cpu_relax in irq_locked context.. > > There are no restrictions on the calling context for cpu_relax(). > [ schedule(), of course, does have restrictions. ] Thanks.. > > I don't understand the use of the udelay(10). Could you please explain > this further? Is it the case that if the register bit doesn't change > within 50 reads, that it is then not likely to change for 10 microseconds? The delay of 10 used is in vcbypass command -> the loop actually waits for the i2c command to get thru.. depending on the speed of the bus configured and cpu speed, 50 iterations might not be enough, but yes, a udelay(1) loop in addition to cpu_relax might be better.. > > If that isn't how the device works, then using udelay(10) will just waste > power busy-looping in the CPU. I would rather see this code using > udelay(1) between each register read if you want to ensure that you read > the register for at least 50 microseconds. This would also simplify your > timeout loops considerably, which seem unnecessarily complicated. Ack. Regards, Nishanth Menon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0
> ow...@vger.kernel.org] On Behalf Of Kalle Jokiniemi
> Sent: Tuesday, October 27, 2009 5:14 PM
>
> Hi Nishanth,
>
> Found some dead code, see below:
[...]
> > +/* T2 SMART REFLEX */
> > +#define R_SRI2C_SLAVE_ADDR 0x12
> > +#define R_VDD1_SR_CONTROL 0x00
> > +#define R_VDD2_SR_CONTROL 0x01
> > +
> > +/* VDDs*/
> > +#define PRCM_VDD1 1
> > +#define PRCM_VDD2 2
> > +
>
> This comment below can be removed, provided that you apply my other
> comments.
>
> > +/*
> > + * XXX: These should be removed/moved from here once we have a working
> DVFS
> > + * implementation in place
> > + */
> > +#define PHY_TO_OFF_PM_MASTER(p)(p - 0x36)
>
> No need for PHY_TO_OFF_PM_MASTER definition. Remove that.
>
>
> > +#define PHY_TO_OFF_PM_RECIEVER(p) (p - 0x5b)
> > +#define PHY_TO_OFF_PM_INT(p) (p - 0x2e)
>
> No need for PHY_TO_OFF_PM_INT def. Remove.
>
> > +
> > +/* Vmode control */
> > +#define R_DCDC_GLOBAL_CFG PHY_TO_OFF_PM_RECIEVER(0x61)
> > +/* R_DCDC_GLOBAL_CFG register, SMARTREFLEX_ENABLE values */
> > +#define DCDC_GLOBAL_CFG_ENABLE_SRFLX 0x08
>
>
> As we now have other ways of determining opp, remove code from here...
>
> > +
> > +/* DEVICE ID/DPLL ID/CLOCK ID: bits 28-31 for OMAP type */
> > +#define OMAP_TYPE_SHIFT28
> > +#define OMAP_TYPE_MASK 0xF
> > +/* OPP ID: bits: 0-4 for OPP number */
> > +#define OPP_NO_POS 0
> > +#define OPP_NO_MASK0x1F
> > +/* OPP ID: bits: 5-6 for VDD */
> > +#define VDD_NO_POS 5
> > +#define VDD_NO_MASK0x3
> > +/* Other IDs: bits 20-27 for ID type */
> > +/* These IDs have bits 25,26,27 as 1 */
> > +#define OTHER_ID_TYPE_SHIFT20
> > +#define OTHER_ID_TYPE_MASK 0xFF
> > +
> > +#define OTHER_ID_TYPE(X) ((X & OTHER_ID_TYPE_MASK) <<
> OTHER_ID_TYPE_SHIFT)
> > +#define ID_OPP_NO(X)((X & OPP_NO_MASK) << OPP_NO_POS)
> > +#define ID_VDD(X) ((X & VDD_NO_MASK) << VDD_NO_POS)
> > +#define OMAP(X) ((X >> OMAP_TYPE_SHIFT) &
> OMAP_TYPE_MASK)
> > +#define get_opp_no(X) ((X >> OPP_NO_POS) & OPP_NO_MASK)
> > +#define get_vdd(X) ((X >> VDD_NO_POS) & VDD_NO_MASK)
> > +
> > +/* XXX: end remove/move */
> > +
> > +/* XXX: find more appropriate place for these once DVFS is in place */
> > +extern u32 current_vdd1_opp;
> > +extern u32 current_vdd2_opp;
>
> ...to here.
>
> At least with a quick glance, it seems the above defs are not in use.
> Your compiler should tell if I missed something ;)
Thanks... will fix in my rev 5.
Regards,
Nishanth Menon
N�r��yb�X��ǧv�^�){.n�+{��f��{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
RE: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0
Hello Nishanth, Manjunath, a comment on cpu_relax() and udelay(). On Mon, 26 Oct 2009, Menon, Nishanth wrote: > > -Original Message- > > From: G, Manjunath Kondaiah > > > > As per your comments for other patches when ever there is udelay usage, > > cpu_relax is the better option. I see there are lot of udelay(...) calls > > used in this patch. Why can't you use cpu_relax() or schedule(). > > Any specific reason? > > > Don’t really want to do cpu_relax in irq_locked context.. There are no restrictions on the calling context for cpu_relax(). [ schedule(), of course, does have restrictions. ] I don't understand the use of the udelay(10). Could you please explain this further? Is it the case that if the register bit doesn't change within 50 reads, that it is then not likely to change for 10 microseconds? If that isn't how the device works, then using udelay(10) will just waste power busy-looping in the CPU. I would rather see this code using udelay(1) between each register read if you want to ensure that you read the register for at least 50 microseconds. This would also simplify your timeout loops considerably, which seem unnecessarily complicated. But if the scenario above is how the device is expected to respond, then these loops should have some comments to explain why the extra complexity and power consumption of a 10 microsecond busy-loop after every 50 reads is the right thing to do. regards, - Paul
RE: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0
Hi Nishanth, After some testing I have found a little problem with the init > From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- > ow...@vger.kernel.org] On Behalf Of Menon, Nishanth > > Refactor the smart reflex implementation. [snip] > + return ret; > +} > +late_initcall(omap_sr_init); It seem that with late_initcall we are initializing SR after SGX init. This SGX init call DVFS to change opp and we are not yet initialized this cause the device to hang (Panic Joined) One solution will be to change from late_initcall to device_initcall so SR will be initialized before SGX + return ret; +} +device_initcall(omap_sr_init); Is there any objections or constraint with this ? Or maybe there is another way to fix this ? BR Guilhem -[Panic]-- [8.128723] Unable to handle kernel NULL pointer dereference at virtual address [8.137329] pgd = c0004000 [8.140289] [] *pgd= [8.144042] Internal error: Oops: 5 [#1] PREEMPT [8.148895] Modules linked in: [8.152099] CPU: 0Not tainted (2.6.29-omap1 #43) [8.157409] PC is at sr_vp_disable_both+0x18/0x80 [8.162322] LR is at program_opp+0x80/0x1d0 [8.166717] pc : []lr : []psr: 6013 [8.166717] sp : cec1fce0 ip : cec1fcf8 fp : cec1fcf4 [8.178741] r10: c050d3d8 r9 : 0003 r8 : [8.184204] r7 : r6 : 0010 r5 : 0002 r4 : cec1e000 [8.191040] r3 : r2 : 0042 r1 : 0043 r0 : 0002 [8.197875] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel [8.205535] Control: 10c5387d Table: 80c04019 DAC: 0017 [8.211578] [8.211578] PC: 0xc004e1bc: [8.216033] e1bc eb0df5ab ea11 ebfffe8f e2504000 15d51000 11a02004 159f003c 1a06 [8.224639] e1dc e1a5 eb8f e2504000 0a07 e5d51000 e1a02004 e59f0020 eb0df59c [8.233245] e1fc e59f001c e30014bd e3a02000 eb0083d7 e1a4 e89da830 c04632b6 c04632e7 [8.241851] e21c c046330b c046316a e1a0c00d e92dd830 e24cb004 e59f3064 e7e102d0 e5933000 [8.250457] e23c e5d32000 e152 2a02 e3a03000 e5833000 eafd e7935100 e5d53021 [8.259033] e25c e353 0a0a e5d53020 e353 1a07 e1a5 ebc4 e2504000 [8.267639] e27c 0a04 e5d51000 e59f0014 eb0df578 ea00 e3a04000 e1a4 e89da830 [8.276245] e29c c050d3a0 c046332f e1a0c00d e92dddf0 e24cb004 e2535000 e1a06000 e1a07001 [8.284851] [8.284851] LR: 0xc004f4c4: [8.289306] f4c4 e1a0c00d e92ddff0 e24cb004 e24dd024 e50b0040 e1a06182 e0810183 e0811006 [8.297912] f4e4 e50b103c e1a05002 e50b0038 e1a09003 e5d01004 e1a0c00d e51b003c e3cc4d7f [8.306518] f504 e51b2040 e3a08000 e59fa168 e201101f e2023003 e5d02004 e3c4403f e1a07008 [8.315124] f524 e1a03283 e202201f e1832002 e1831001 e1a2 e50b2034 e50b1030 ebfffb37 [8.323730] f544 e1550009 d3a02000 c3a02001 e50b2044 e51b3044 e1570003 1a36 e51bc040 [8.332336] f564 e35c0001 1a0c e59f010c e5903000 e59a0210 e7931006 eb000a65 e59f20fc [8.340942] f584 e59a0214 e5923000 e7931006 eb000a60 ebffdd91 e59f30e8 ea23 e3a01040 [8.349517] f5a4 e3a00c02 ebffdf20 e10f8000 f10c0080 e5943004 e2833001 e5843004 e50b004c [8.358123] [8.358123] SP: 0xcec1fc60: [8.362609] fc60 02c7 cedc0720 cec1fc94 cec1fc80 cec1fccc [8.371215] fc80 0010 cec1fcf4 cec1fc98 c003c9ac c003c200 0002 0043 [8.379821] fca0 0042 cec1e000 0002 0010 0003 [8.388397] fcc0 c050d3d8 cec1fcf4 cec1fcf8 cec1fce0 c004f544 c004e23c 6013 [8.397003] fce0 cec1e000 0002 cec1fd44 cec1fcf8 c004f544 c004e230 [8.405609] fd00 0002 c04db200 c04db208 0042 0043 c04da0a8 [8.414215] fd20 0002 0002 0001 0003 cec1fd7c cec1fd48 [8.422821] fd40 c004f7d4 c004f4d0 c04da0cc 6013 cec1fda4 cec1fd60 c03ccc74 [8.431427] [8.431427] IP: 0xcec1fc78: [8.435882] fc78 cec1fccc 0010 cec1fcf4 cec1fc98 c003c9ac c003c200 [8.88] fc98 0002 0043 0042 cec1e000 0002 0010 [8.453094] fcb8 0003 c050d3d8 cec1fcf4 cec1fcf8 cec1fce0 c004f544 c004e23c [8.461700] fcd8 6013 cec1e000 0002 cec1fd44 cec1fcf8 c004f544 c004e230 [8.470275] fcf8 0002 c04db200 c04db208 0042 0043 [8.478881] fd18 c04da0a8 0002 0002 0001 0003 [8.487487] fd38 cec1fd7c cec1fd48 c004f7d4 c004f4d0 c04da0cc 6013 cec1fda4 cec1fd60 [8.496093] fd58 c03ccc74 c04da0a8 0002 0001 cec1fd94 cec1fd80 [8.504699] [8.504699] FP: 0xcec1fc74: [8.509155] fc74 cec1fc80 cec1fccc 0010 cec1fcf4 cec1fc98 c003c9ac [8.517761] fc94 c003c200 0002 004
RE: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0
> From: Nishanth Menon [mailto:menon.nisha...@gmail.com] On Behalf Of Menon,
> Nishanth
>
> Benoit,
> thanks.. overall good discussion that is nice to take in.. more comments
You're welcome! My pleasure.
> follow:
>
> Cousson, Benoit said the following on 10/26/2009 08:05 AM:
> >> From: Nishanth Menon [mailto:menon.nisha...@gmail.com]
> >>
> >> Cousson, Benoit said the following on 10/25/2009 05:12 PM:
> >>
> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-
>
>
> >> [...]
> >>
> + },
> + /* *INDENT-ON* */
> +};
> +
> +/* SR list for 3430 */
> +static __initdata struct omap_sr_list omap34xx_srlist = {
> + .num_sr = 2,
> + .sr_list = {&omap34xx_sr1, &omap34xx_sr2}
> +};
> +
> +/* The final SR list */
> +static struct omap_sr_list *omap_srlist;
>
>
> >>> I have a couple a suggestions regarding the code partitioning:
> >>>
> >>> - SmartReflex is one IP with several instances; it means that only the
> >>>
> >> base address will change between SR1 and SR2. There is no need to
> >> duplicate the mask and shift defines per SR.
> >>
> >> might have been easier with a standard OCP wrapper and standard
> >> INT/INTSTATUS regs for SR instead of the current integration but
> >> yeah, I had been thinking of that- if O4/beyond could have the same IP
> >> wrapped in a different register mapping, i should handle it then, not
> >> dream about it now.. I get your point here.. there are a few places we
> >> can kick it out and some places your are stuck with them!
> >>
> >
> > SR is a regular IP, with a dedicated PD, dedicated fclk and bound to the
> L4...
> > VP/VC is not, that why I was proposing to separated them.
> >
> I am not denying it, but VP/VC functions are already separated out where
> I could do it (note the seperation of function names), only they dont
> have a new file perse.. but that is not needed at the moment.
> >
> >>> Moreover, SR being an IP, I think we can encode the one IP / several
> >>>
> >> instance in a platform_device / platform_driver code. It will allow the
> >> support of several drivers for the same devices in order to implement
> for
> >> example class3, class2 or class1 drivers. SR can even be represented by
> an
> >> HWMOD.
> >>
> >> what is your intention in misusing platform_device for a SOC specific
> >> code? I think you have an idea here that I am completely missing. can
> >> you care to elaborate?
> >>
> >
> > AFAIK, platform_device is there in order to deal with SoC devices???
> > Considering that SR is an IP that can be in several SoC, it looks to me
> the perfect candidate for platform_device/platform_driver.
> > Am I missing something?
> >
> Everything abstracted is a perfect candidate for device-driver model,
> but that is beside the point. platform_device traditionally has been
> platform aka board specific info, not SOC specific info..
Are you sure about that? The definition does not seem to be that strict.
Extract from Documentation/driver-model/platform.txt
Platform Devices and Drivers
[snip]
This pseudo-bus is used to connect devices on busses with minimal
infrastructure, like those used to integrate peripherals on many system-on-chip
processors, or some "legacy" PC interconnects; as opposed to large
formally specified ones like PCI or USB.
[snip]
Moreover, in the current l-o tree, the platform_device is used for DSP, MCBSP,
MMC, uWIRE, UART, USB, ISP, bangap sensor, toaster...
> >
> >>> - The smartreflex.c file contains 34xx specifics code; it should not
> be
> >>>
> >> there, only SR specific code should be there.
> >>
> >> read the TODO. which specific 3430 code does it have? other than using
> >> macros which have 34xx prefix - that should be killed -yep.
> >>
> >
> > omap34xx_sr1, omap34xx_sr2, omap34xx_srlist... seem to be quite OMAP34xx
> specifics...
> >
> please read the code carefully next time, look at how it is copied based
> on cpu_is_ api in in __init??
I did, and... My opinion is that smartreflex.c should contain the code for the
smartreflex IP... that's all. The omap34xx implementation with two SR IP is SoC
specific. And the next time, I will still think the same.
> yes, there are specific paramaters to SOC, you cannot avoid that. but
> you can make the handling independent of the SOC. that is what I have
> done.. let me know if you have a better idea.. :D..
I think I already did. Please read again.
> >
> >>> - If we want to go further, I think that the VP/VC code should not be
> in
> >>>
> >> SR code either. It is located in the PRM, and can be used independently
> of
> >> SR.
> >>
> >>> - In a SR class 2 mode, the smartreflex driver will not use the
> direct
> >>> connection to the VP.
> >>> - If we don't want SR we can still use the VP/VC for device voltage
> >>> control.
> >>>
> >>>
> >> - How about adding - Smart reflex should actually fit in a
> >> voltage control
Re: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0
Trying not to top post.. Apologies before hand on my client restrictions. Anyways.. Manjunath, Yes, the call sequences are common. We may consider using cpu_relax() in the context of dvfs calls.. Except it could result in race connditions not in intrest. Do me a favor and flag the udelays u'd like to convert and send a patch for fix pls. Regards, Nishanth Menon - Original Message - From: G, Manjunath Kondaiah To: Menon, Nishanth; linux-omap@vger.kernel.org Cc: Imberton Guilhem ; Mike Chan ; Nayak, Rajendra; Roger Quadros ; Kalle Jokiniemi ; Reddy, Teerth; Kevin Hilman ; Paul Walmsley ; Hogander Jouni Sent: Mon Oct 26 10:09:07 2009 Subject: RE: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0 > -Original Message- > From: Menon, Nishanth > Sent: Monday, October 26, 2009 3:48 PM > To: G, Manjunath Kondaiah; linux-omap@vger.kernel.org > Cc: Imberton Guilhem; Mike Chan; Nayak, Rajendra; Roger > Quadros; Kalle Jokiniemi; Reddy, Teerth; Kevin Hilman; Paul > Walmsley; Hogander Jouni > Subject: RE: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0 > > > -Original Message- > > From: G, Manjunath Kondaiah > > Sent: Monday, October 26, 2009 3:40 AM > > To: Menon, Nishanth; linux-omap@vger.kernel.org > > Cc: Imberton Guilhem; Mike Chan; Nayak, Rajendra; Roger > Quadros; Kalle > > Jokiniemi; Reddy, Teerth; Kevin Hilman; Paul Walmsley; > Hogander Jouni > > Subject: RE: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex > Refactor Rev4.0 > > > > > > As per your comments for other patches when ever there is > udelay usage, > > cpu_relax is the better option. I see there are lot of > udelay(...) calls > > used in this patch. Why can't you use cpu_relax() or schedule(). > > Any specific reason? > > > Don’t really want to do cpu_relax in irq_locked context.. if > you look at the code flow, the call from cpu_idle is in > irq_locked.. Further this delay is part of bring up form > saved context where there is nothing else scheduled + we > don’t want anything else scheduled (and causing a change in > scheduling decision). So unfortunately, unlike standard > drivers, this cannot use the same reasoning. > NAK. My understanding is that, SR functions will be called during voltage scaling also. Voltage scaling will happen in non IRQ locked context. Please clarify if I am wrong. -Manjunath
RE: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0
> -Original Message- > From: Menon, Nishanth > Sent: Monday, October 26, 2009 3:48 PM > To: G, Manjunath Kondaiah; linux-omap@vger.kernel.org > Cc: Imberton Guilhem; Mike Chan; Nayak, Rajendra; Roger > Quadros; Kalle Jokiniemi; Reddy, Teerth; Kevin Hilman; Paul > Walmsley; Hogander Jouni > Subject: RE: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0 > > > -Original Message- > > From: G, Manjunath Kondaiah > > Sent: Monday, October 26, 2009 3:40 AM > > To: Menon, Nishanth; linux-omap@vger.kernel.org > > Cc: Imberton Guilhem; Mike Chan; Nayak, Rajendra; Roger > Quadros; Kalle > > Jokiniemi; Reddy, Teerth; Kevin Hilman; Paul Walmsley; > Hogander Jouni > > Subject: RE: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex > Refactor Rev4.0 > > > > > > As per your comments for other patches when ever there is > udelay usage, > > cpu_relax is the better option. I see there are lot of > udelay(...) calls > > used in this patch. Why can't you use cpu_relax() or schedule(). > > Any specific reason? > > > Don’t really want to do cpu_relax in irq_locked context.. if > you look at the code flow, the call from cpu_idle is in > irq_locked.. Further this delay is part of bring up form > saved context where there is nothing else scheduled + we > don’t want anything else scheduled (and causing a change in > scheduling decision). So unfortunately, unlike standard > drivers, this cannot use the same reasoning. > NAK. My understanding is that, SR functions will be called during voltage scaling also. Voltage scaling will happen in non IRQ locked context. Please clarify if I am wrong. -Manjunath
Re: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0
Benoit,
thanks.. overall good discussion that is nice to take in.. more comments
follow:
Cousson, Benoit said the following on 10/26/2009 08:05 AM:
>> From: Nishanth Menon [mailto:menon.nisha...@gmail.com]
>>
>> Cousson, Benoit said the following on 10/25/2009 05:12 PM:
>>
diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-
>> [...]
>>
+ },
+ /* *INDENT-ON* */
+};
+
+/* SR list for 3430 */
+static __initdata struct omap_sr_list omap34xx_srlist = {
+ .num_sr = 2,
+ .sr_list = {&omap34xx_sr1, &omap34xx_sr2}
+};
+
+/* The final SR list */
+static struct omap_sr_list *omap_srlist;
>>> I have a couple a suggestions regarding the code partitioning:
>>>
>>> - SmartReflex is one IP with several instances; it means that only the
>>>
>> base address will change between SR1 and SR2. There is no need to
>> duplicate the mask and shift defines per SR.
>>
>> might have been easier with a standard OCP wrapper and standard
>> INT/INTSTATUS regs for SR instead of the current integration but
>> yeah, I had been thinking of that- if O4/beyond could have the same IP
>> wrapped in a different register mapping, i should handle it then, not
>> dream about it now.. I get your point here.. there are a few places we
>> can kick it out and some places your are stuck with them!
>>
>
> SR is a regular IP, with a dedicated PD, dedicated fclk and bound to the L4...
> VP/VC is not, that why I was proposing to separated them.
>
I am not denying it, but VP/VC functions are already separated out where
I could do it (note the seperation of function names), only they dont
have a new file perse.. but that is not needed at the moment.
>
>>> Moreover, SR being an IP, I think we can encode the one IP / several
>>>
>> instance in a platform_device / platform_driver code. It will allow the
>> support of several drivers for the same devices in order to implement for
>> example class3, class2 or class1 drivers. SR can even be represented by an
>> HWMOD.
>>
>> what is your intention in misusing platform_device for a SOC specific
>> code? I think you have an idea here that I am completely missing. can
>> you care to elaborate?
>>
>
> AFAIK, platform_device is there in order to deal with SoC devices???
> Considering that SR is an IP that can be in several SoC, it looks to me the
> perfect candidate for platform_device/platform_driver.
> Am I missing something?
>
Everything abstracted is a perfect candidate for device-driver model,
but that is beside the point. platform_device traditionally has been
platform aka board specific info, not SOC specific info..
>
>>> - The smartreflex.c file contains 34xx specifics code; it should not be
>>>
>> there, only SR specific code should be there.
>>
>> read the TODO. which specific 3430 code does it have? other than using
>> macros which have 34xx prefix - that should be killed -yep.
>>
>
> omap34xx_sr1, omap34xx_sr2, omap34xx_srlist... seem to be quite OMAP34xx
> specifics...
>
please read the code carefully next time, look at how it is copied based
on cpu_is_ api in in __init??
yes, there are specific paramaters to SOC, you cannot avoid that. but
you can make the handling independent of the SOC. that is what I have
done.. let me know if you have a better idea.. :D..
>
>>> - If we want to go further, I think that the VP/VC code should not be in
>>>
>> SR code either. It is located in the PRM, and can be used independently of
>> SR.
>>
>>> - In a SR class 2 mode, the smartreflex driver will not use the direct
>>> connection to the VP.
>>> - If we don't want SR we can still use the VP/VC for device voltage
>>> control.
>>>
>>>
>> - How about adding - Smart reflex should actually fit in a
>> voltage control infrastructure so that the system can manipulate and set
>> voltage with and without SR..
>>
>
> Fully agree, that was one of the proposals I had in mind, but since it will
> require some more works, it should be done in a second pass.
>
>
>> that is what is really missing in our code base today -> SR and VP comes
>> plugged in close as buddies in all silicons, so if your recommendation
>> has an silicon example where OMAP SR talks not to VP, let me know and I
>> will second splitting the code :D. till then sorry.. you dont have a case.
>>
>
> We had the case in several chips that unfortunately are not there anymore
> :-(
> The SR IP was done to handle several config, having SR tied to VP/VC is a
> chip dependant implementation.
>
lets bring in a proper voltage control infrastructure then worry about
SR class2/class3 implementation. class2 is inferior to class 3 and the
overhead of cpu intervention should be balanced with a proper latency
heuristics, the infrastructure is sadly missing at this point of time..
>
>> now, it is a differe
RE: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0
> From: Nishanth Menon [mailto:menon.nisha...@gmail.com]
>
> Cousson, Benoit said the following on 10/25/2009 05:12 PM:
> >> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-
> >>
> [...]
> >> + },
> >> + /* *INDENT-ON* */
> >> +};
> >> +
> >> +/* SR list for 3430 */
> >> +static __initdata struct omap_sr_list omap34xx_srlist = {
> >> + .num_sr = 2,
> >> + .sr_list = {&omap34xx_sr1, &omap34xx_sr2}
> >> +};
> >> +
> >> +/* The final SR list */
> >> +static struct omap_sr_list *omap_srlist;
> >>
> >
> > I have a couple a suggestions regarding the code partitioning:
> >
> > - SmartReflex is one IP with several instances; it means that only the
> base address will change between SR1 and SR2. There is no need to
> duplicate the mask and shift defines per SR.
> >
> might have been easier with a standard OCP wrapper and standard
> INT/INTSTATUS regs for SR instead of the current integration but
> yeah, I had been thinking of that- if O4/beyond could have the same IP
> wrapped in a different register mapping, i should handle it then, not
> dream about it now.. I get your point here.. there are a few places we
> can kick it out and some places your are stuck with them!
SR is a regular IP, with a dedicated PD, dedicated fclk and bound to the L4...
VP/VC is not, that why I was proposing to separated them.
> > Moreover, SR being an IP, I think we can encode the one IP / several
> instance in a platform_device / platform_driver code. It will allow the
> support of several drivers for the same devices in order to implement for
> example class3, class2 or class1 drivers. SR can even be represented by an
> HWMOD.
> >
> what is your intention in misusing platform_device for a SOC specific
> code? I think you have an idea here that I am completely missing. can
> you care to elaborate?
AFAIK, platform_device is there in order to deal with SoC devices???
Considering that SR is an IP that can be in several SoC, it looks to me the
perfect candidate for platform_device/platform_driver.
Am I missing something?
> > - The smartreflex.c file contains 34xx specifics code; it should not be
> there, only SR specific code should be there.
> >
> read the TODO. which specific 3430 code does it have? other than using
> macros which have 34xx prefix - that should be killed -yep.
omap34xx_sr1, omap34xx_sr2, omap34xx_srlist... seem to be quite OMAP34xx
specifics...
> > - If we want to go further, I think that the VP/VC code should not be in
> SR code either. It is located in the PRM, and can be used independently of
> SR.
> > - In a SR class 2 mode, the smartreflex driver will not use the direct
> > connection to the VP.
> > - If we don't want SR we can still use the VP/VC for device voltage
> > control.
> >
> - How about adding - Smart reflex should actually fit in a
> voltage control infrastructure so that the system can manipulate and set
> voltage with and without SR..
Fully agree, that was one of the proposals I had in mind, but since it will
require some more works, it should be done in a second pass.
> that is what is really missing in our code base today -> SR and VP comes
> plugged in close as buddies in all silicons, so if your recommendation
> has an silicon example where OMAP SR talks not to VP, let me know and I
> will second splitting the code :D. till then sorry.. you dont have a case.
We had the case in several chips that unfortunately are not there anymore
:-(
The SR IP was done to handle several config, having SR tied to VP/VC is a chip
dependant implementation.
> now, it is a different case where you want to use SR as a pure
> monitoring engine -> that is not an implementation that is exactly
> better than class 3 operation.. why support it at all?
It is not a matter or being better, it is just having HW vs. SW implementation.
The point is Class 3 is not usable with non SR compliant power IC. In that case
you should rely on a Class 2 implementation.
> > [snip]
> >
> >
> >> +/** PMIC WEAK FUNCTIONS FOR TWL4030 derivatives
> >> **/
> >> +
> >> +/**
> >> + * @brief pmic_srinit - Power management IC initialization
> >> + * for smart reflex. The current code is written for TWL4030
> >> + * derivatives, replace in board file if PMIC requires
> >> + * a different sequence
> >> + *
> >> + * @return result of operation
> >> + */
> >> +int __weak __init omap_pmic_srinit(void)
> >> +{
> >> + int ret = -ENODEV;
> >> +#ifdef CONFIG_TWL4030_CORE
> >> + u8 reg;
> >> + /* Enable SR on T2 */
> >> + ret = twl4030_i2c_read_u8(TWL4030_MODULE_PM_RECEIVER, ®,
> >> + R_DCDC_GLOBAL_CFG);
> >> +
> >> + reg |= DCDC_GLOBAL_CFG_ENABLE_SRFLX;
> >> + ret |= twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, reg,
> >> + R_DCDC_GLOBAL_CFG);
> >> +#endif /* End of CONFIG_TWL4030_CORE */
> >> + return ret;
> >> +}
> >> +
> >> +/**
> >> + *
RE: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0
> -Original Message- > From: G, Manjunath Kondaiah > Sent: Monday, October 26, 2009 3:40 AM > To: Menon, Nishanth; linux-omap@vger.kernel.org > Cc: Imberton Guilhem; Mike Chan; Nayak, Rajendra; Roger Quadros; Kalle > Jokiniemi; Reddy, Teerth; Kevin Hilman; Paul Walmsley; Hogander Jouni > Subject: RE: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0 > > > As per your comments for other patches when ever there is udelay usage, > cpu_relax is the better option. I see there are lot of udelay(...) calls > used in this patch. Why can't you use cpu_relax() or schedule(). > Any specific reason? > Don’t really want to do cpu_relax in irq_locked context.. if you look at the code flow, the call from cpu_idle is in irq_locked.. Further this delay is part of bring up form saved context where there is nothing else scheduled + we don’t want anything else scheduled (and causing a change in scheduling decision). So unfortunately, unlike standard drivers, this cannot use the same reasoning. Regards, Nishanth Menon
Re: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0
Cousson, Benoit said the following on 10/25/2009 05:12 PM:
>> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-
>>
[...]
>> + },
>> + /* *INDENT-ON* */
>> +};
>> +
>> +/* SR list for 3430 */
>> +static __initdata struct omap_sr_list omap34xx_srlist = {
>> + .num_sr = 2,
>> + .sr_list = {&omap34xx_sr1, &omap34xx_sr2}
>> +};
>> +
>> +/* The final SR list */
>> +static struct omap_sr_list *omap_srlist;
>>
>
> I have a couple a suggestions regarding the code partitioning:
>
> - SmartReflex is one IP with several instances; it means that only the base
> address will change between SR1 and SR2. There is no need to duplicate the
> mask and shift defines per SR.
>
might have been easier with a standard OCP wrapper and standard
INT/INTSTATUS regs for SR instead of the current integration but
yeah, I had been thinking of that- if O4/beyond could have the same IP
wrapped in a different register mapping, i should handle it then, not
dream about it now.. I get your point here.. there are a few places we
can kick it out and some places your are stuck with them!
> Moreover, SR being an IP, I think we can encode the one IP / several instance
> in a platform_device / platform_driver code. It will allow the support of
> several drivers for the same devices in order to implement for example
> class3, class2 or class1 drivers. SR can even be represented by an HWMOD.
>
what is your intention in misusing platform_device for a SOC specific
code? I think you have an idea here that I am completely missing. can
you care to elaborate?
> - The smartreflex.c file contains 34xx specifics code; it should not be
> there, only SR specific code should be there.
>
read the TODO. which specific 3430 code does it have? other than using
macros which have 34xx prefix - that should be killed -yep.
> - If we want to go further, I think that the VP/VC code should not be in SR
> code either. It is located in the PRM, and can be used independently of SR.
> - In a SR class 2 mode, the smartreflex driver will not use the direct
> connection to the VP.
> - If we don't want SR we can still use the VP/VC for device voltage
> control.
>
- How about adding - Smart reflex should actually fit in a
voltage control infrastructure so that the system can manipulate and set
voltage with and without SR..
that is what is really missing in our code base today -> SR and VP comes
plugged in close as buddies in all silicons, so if your recommendation
has an silicon example where OMAP SR talks not to VP, let me know and I
will second splitting the code :D. till then sorry.. you dont have a case.
now, it is a different case where you want to use SR as a pure
monitoring engine -> that is not an implementation that is exactly
better than class 3 operation.. why support it at all?
> [snip]
>
>
>> +/** PMIC WEAK FUNCTIONS FOR TWL4030 derivatives
>> **/
>> +
>> +/**
>> + * @brief pmic_srinit - Power management IC initialization
>> + * for smart reflex. The current code is written for TWL4030
>> + * derivatives, replace in board file if PMIC requires
>> + * a different sequence
>> + *
>> + * @return result of operation
>> + */
>> +int __weak __init omap_pmic_srinit(void)
>> +{
>> + int ret = -ENODEV;
>> +#ifdef CONFIG_TWL4030_CORE
>> + u8 reg;
>> + /* Enable SR on T2 */
>> + ret = twl4030_i2c_read_u8(TWL4030_MODULE_PM_RECEIVER, ®,
>> + R_DCDC_GLOBAL_CFG);
>> +
>> + reg |= DCDC_GLOBAL_CFG_ENABLE_SRFLX;
>> + ret |= twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, reg,
>> + R_DCDC_GLOBAL_CFG);
>> +#endif /* End of CONFIG_TWL4030_CORE */
>> + return ret;
>> +}
>> +
>> +/**
>> + * @brief omap_pmic_voltage_ramp_delay - how much should this pmic ramp
>> delay
>> + * Various PMICs have different ramp up and down delays. choose to
>> implement
>> + * in required pmic file to override this function.
>> + * On TWL4030 derivatives:
>> + * T2 SMPS slew rate (min) 4mV/uS, step size 12.5mV,
>> + * 2us added as buffer.
>> + *
>> + * @param srid - which SR is this for?
>> + * @param target_vsel - targetted voltage selction
>> + * @param current_vsel - current voltage selection
>> + *
>> + * @return delay in uSeconds
>> + */
>> +u32 __weak omap_pmic_voltage_ramp_delay(u8 srid, u8 target_vsel,
>> + u8 current_vsel)
>> +{
>> + u32 t2_smps_steps = abs(target_vsel - current_vsel);
>> + u32 t2_smps_delay = ((t2_smps_steps * 125) / 40) + 2;
>> + return t2_smps_delay;
>> +}
>> +
>> +#ifdef CONFIG_OMAP_VC_BYPASS_UPDATE
>> +/**
>> + * @brief omap_pmic_voltage_cmds - hook for pmic command sequence
>> + * to be send out which are specific to pmic to set a specific voltage.
>> + * this should inturn call vc_send_command with the required sequence
>> + * The current implementation is for TWL4030 derivatives
>> + *
>> + *
RE: [PATCH 2/2 v3] OMAP3: PM: SR: SmartReflex Refactor Rev4.0
Hi Nishanth,
> From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> ow...@vger.kernel.org] On Behalf Of Menon, Nishanth
>
> Refactor the smart reflex implementation.
[snip]
> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-
> omap2/smartreflex.c
> new file mode 100644
> index 000..d506896
> --- /dev/null
> +++ b/arch/arm/mach-omap2/smartreflex.c
> @@ -0,0 +1,1604 @@
> +/*
> + * linux/arch/arm/mach-omap3/smartreflex.c
> + *
> + * OMAP34XX SmartReflex Voltage Control
> + *
> + * Copyright (C) 2009 Texas Instruments, Inc.
> + * Nishanth Menon
> + *
> + * Copyright (C) 2008 Nokia Corporation
> + * Kalle Jokiniemi
> + *
> + * Copyright (C) 2007 Texas Instruments, Inc.
> + * Lesly A M
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "prm.h"
> +#include "smartreflex.h"
> +#include "prm-regbits-34xx.h"
> +
> +/* MCUDISACK is expected to happen within 1uSec. */
> +#define COUNT_TIMEOUT_MCUDISACK 200
> +
> +/* VPINIDLE is expected to happen within 100uSec. Typical is 2uSec */
> +#define COUNT_TIMEOUT_VPINIDLE 200
> +
> +/* Time taken for setting the device - worst case as FS I2C
> + * Depends on SMPSWAITIME MIN/MAX Typical is 200uSec
> + */
> +#define COUNT_TIMEOUT_TRANSDONE_SET 400
> +
> +/* Time to clear out multiple transdone events typical is 3uSec */
> +#define COUNT_TIMEOUT_TRANSDONE_CLR 50
> +
> +/* Time For VCBypass mode for TWL4030 derivative chip. */
> +#define COUNT_TIMEOUT_TWL4030_VCBYPASS 500
> +
> +/* How many retries to do for I2C errors seen on bus for Forceupdate? */
> +#define COUNT_RETRY_SMPSNOACK4
> +
> +#define SR_REGADDR(offset) (sr->srbase_addr + (offset))
> +
> +/* Which function to use for setting voltage */
> +#ifdef CONFIG_OMAP_VC_BYPASS_UPDATE
> +#define SR_CHOSEN_VOLTAGE_UPDATE_MECH sr_vc_bypass
> +#else
> +#define SR_CHOSEN_VOLTAGE_UPDATE_MECH sr_vp_forceupdate
> +#endif
> +
> +#ifdef CONFIG_OMAP_PM_NONE
> +struct omap_opp *mpu_opps;
> +struct omap_opp *dsp_opps;
> +struct omap_opp *l3_opps;
> +#endif
> +
> +static ssize_t omap_sr_vdd_autocomp_show(struct kobject *kobj,
> + struct kobj_attribute *attr,
> + char *buf);
> +static ssize_t omap_sr_vdd_autocomp_store(struct kobject *kobj,
> + struct kobj_attribute *attr,
> + const char *buf, size_t n);
> +/* Structure for Voltage processor */
> +struct omap_sr_vp {
> + /* Store the commonly used register offsets.
> + * this saves a if condition decision
> + */
> + u16 prm_vpx_status_offset;
> + u16 prm_vpx_config_offset;
> + u16 prm_vpx_stepmin_offset;
> + u16 prm_vpx_stepmax_offset;
> + u16 prm_vpx_limito_offset;
> + u32 prm_vpx_vlimito_timeout;
> + u8 prm_vpx_vlimito_shift;
> + u16 prm_vpx_voltage_offset;
> + u16 prm_vc_cmd_val_offset;
> + /* Store the defaults
> + * allowing us to save OCP read
> + * operation
> + */
> + u32 vpconfig_value;
> + u32 vpstepmin_value;
> + u32 vpstepmax_value;
> + u32 vplimito_value;
> + u32 vpenable_mask;
> + u32 irqmask_trans_done;
> + u32 irqmask_smps_noack;
> +};
> +
> +/* Structure for Smart Reflex */
> +struct omap_sr {
> + u8 srid;
> + u8 prcm_vdd;
> + char *vdd_name;
> + struct kobj_attribute autocom_attr;
> + struct omap_opp **omap_opp;
> + /* SR activity marker */
> + u8 is_sr_reset;
> + u8 is_autocomp_active;
> + u32 req_opp_no;
> + u32 sr_config_value;
> + u32 sr_errconfig_value;
> + u32 sr_n_mod_mask;
> + u8 sr_n_mod_shift;
> + u32 sr_p_mod_mask;
> + u8 sr_p_mod_shift;
> + struct clk *fclk;
> + struct clk *iclk;
> + void __iomem *srbase_addr;
> + char *iclk_name;
> + char *fclk_name;
> + /* Voltage processor for the specific SR module */
> + struct omap_sr_vp vp;
> + /* This will contain the register offset on
> + * boot, replaced with the actual value
> + * as part of init routine
> + */
> + u8 num_opp;
> + u8 opp_boundary;
> + u32 opp_nvalue[];
> +};
> +
> +/* A superset of all SRs in the system ordered by SRID */
> +struct omap_sr_list {
> + u8 num_sr;
> + struct omap_sr *sr_list[];
> +};
> +
> +/* Definitions for 3430 Silicon */
> +/* Smart Reflex 1 structure */
> +static __initdata struct omap_sr omap34xx_sr1 = {
> + /* *INDENT-OFF* */
> + .srid = SR1,
> + .prcm_vdd = PRCM_VDD1,
> +
