RE: [PATCH v3 04/11] OMAP3: PM: Adding smartreflex device file.

From: "Gopinath, Thara" <thara@ti.com>
To: Kevin Hilman <khilman@deeprootsystems.com>
Cc: "linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>,
	"paul@pwsan.com" <paul@pwsan.com>,
	"Cousson, Benoit" <b-cousson@ti.com>,
	"Sripathy, Vishwanath" <vishwanath.bs@ti.com>,
	"Sawant, Anand" <sawant@ti.com>
Subject: RE: [PATCH v3 04/11] OMAP3: PM: Adding smartreflex device file.
Date: Sat, 23 Oct 2010 16:25:44 +0530	[thread overview]
Message-ID: <5A47E75E594F054BAF48C5E4FC4B92AB035EB95F70@dbde02.ent.ti.com> (raw)
In-Reply-To: <87aam6b33n.fsf@deeprootsystems.com>

>>-----Original Message-----
>>From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>>Sent: Friday, October 22, 2010 10:02 PM
>>To: Gopinath, Thara
>>Cc: linux-omap@vger.kernel.org; paul@pwsan.com; Cousson, Benoit; Sripathy,
>>Vishwanath; Sawant, Anand
>>Subject: Re: [PATCH v3 04/11] OMAP3: PM: Adding smartreflex device file.
>>
>>"Gopinath, Thara" <thara@ti.com> writes:
>>
>>>>>On Wed, 2010-09-22 at 20:15 +0530, Thara Gopinath wrote:
>>>>>> This patch adds support for device registration of various
>>>>>> smartreflex module present in the system. This patch introduces
>>>>>> the platform data for smartreflex devices which include
>>>>>> the efused and test n-target vaules, module enable/disable
>>>>>> pointers and a parameter to indicate whether smartreflex
>>>>>> autocompensation needs to be enabled on init or not.
>>>>>> Currently auocompensation is enabled on init by default
>>>>>> for OMAP3430 ES3.1 chip.
>>>>>>
>>>>>> Signed-off-by: Thara Gopinath <thara@ti.com>
>>
>>[...]
>>
>>>>>> +	sr_data->voltdm = omap_voltage_domain_get(sr_dev_data->vdd_name);
>>>>>> +	if (IS_ERR(sr_data->voltdm)) {
>>>>>> +		pr_err("%s: Unable to get voltage domain pointer for VDD
>>%s\n",
>>>>>> +			__func__, sr_dev_data->vdd_name);
>>>>>> +		goto exit;
>>>>>> +	}
>>>>>> +
>>>>>> +	sr_dev_data->volts_supported = omap_voltage_get_volttable(
>>>>>> +			sr_data->voltdm, &sr_dev_data->volt_data);
>>>>>>
>>>>>> +	if (!sr_dev_data->volts_supported) {
>>>>>> +		pr_warning("%s: No Voltage table registerd fo VDD%d."
>>>>>> +			"Something really wrong\n\n", __func__, i + 1);
>>>>>> +		goto exit;
>>>>>> +	}
>>>>>> +
>>>>>> +	sr_set_nvalues(sr_dev_data, sr_data);
>>>>>
>>>>>First question: why does this N-value init need to be done in the device
>>>>>init?  It seems better to be a part of the SR driver probe.
>>>
>>> OMAP3 and OMAP4 has different ways of reading the efuse registers. I
>>> would like it to be in device file so that we can have the necessary
>>> checks. The driver should not be bothered about getting of the
>>> n-values.
>>
>>Bothered?   The driver's job is to probe the HW.  The device code
>>can tell the driver where the N-values are located (register offsets,
>>via platform_data), but IMO, should not be reading the values from HW.

But then we will have to put  cpu_is_omap34xx() and cpu_is_omap44xx() checks in the driver. Also omap_ctrl_readl API will have to be used from the driver.

>>
>>>>>
>>>>>Second, this section took me quite some time to understand, as it seems
>>>>>to blur the lines of device and driver but also how it interacts with
>>>>>the voltage layer.
>>>>>
>>>>>sr_set_nvalues() is directly manipulating structures that are internal
>>>>>to the voltage layer.  It also makes assumptions about the ordering of
>>>>>volt_data structs in the voltage layer.
>>>>>
>>>>>Strictly speaking neither the sr_nvalue or sr_errminlimit fields of
>>>>>volt_data are not used at all in the voltage layer, but only in the SR
>>>>>layer, so ideally they should really stay in the SR layer.
>>>>>
>>>>>I think what is needed here is a cleaner way for the SR layer to connect
>>>>>the N-values to a voltage.  The current method of manipulating voltage
>>>>>layer structs inside the SR layer is not acceptable.  Since the n-values
>>>>>are fixed in HW (per SoC), what I suggest is simply having a field like
>>>>>'efuse_id' or something in volt_data.  Then, the smart reflex layer can
>>>>>lookup and index all the efuse values during probe, and when sr_enable
>>>>>is called, it looks up the nvalue based on efuse_id.
>>>>>
>>>>>I assume that the sr_errminlimit could also be set based on efuse_id,
>>>>>and therefore remain contained within the SR layer, but I'm not sure I
>>>>>like that idea any better than keeping it inside volt_data.  For now,
>>>>>I'll let you make the call on errminlimit, but I really want to see the
>>>>>N-value stuff isolated in the SR layer.
>>>
>>> I do not understand your proposal here? What kind of variable is efuse-id??
>>>
>>
>>Sorry, it was kind of rambling.  I'll try again...
>>
>>Currently, omap_volt_data (in the voltage layer) includes SR N-value
>>field:
>>
>>struct omap_volt_data {
>>	u32	volt_nominal;
>>	u32	sr_nvalue;
>>	u8	sr_errminlimit;
>>	u8	vp_errgain;
>>};
>>
>>but sr_nvalue is not used in the voltage layer at all.
>>
>>This choice has led to the SR device init code having to populate
>>internal structures of the voltage layer.  That is what I don't like.
>>(I have a similar problem with the presence of sr_errminlimit in the
>>omap_volt_data, because it's not used in the voltage layer either.)
>>
>>So my suggestion was to remove the sr_nvalue field and replace it with
>>something like an efuse_id field.  Since n-values are directly connected
>>with HW registers, this is a simple mapping.
>>
>>This way, the voltage layer does not know about the exact N-value
>>(because it doesn't care), ll the voltage layer needs to know is
>>which efuse to use for this voltage.

Yes. But then voltage layer will not know about the efuse id as well. They are passed as dev attribute from hwmod file today. Are you telling remove them and hardcode them in voltage layer?

>>
>>As for implementation, in the SR driver, probe could read the N-values
>>from HW into a table.  The enable/disable functiosn can look up the
>>N-values from the table using the efuse_id from volt_data.

You mean maintain a efuse-id n-value table per SR basis??

Regards
Thara

>>
>>Did I explain any better this time?
>>
>>Kevin
>>
>>
>>
>>
>>
>>
>>
>>