by Srinivas Kandagatla

[permalink] [raw]

Subject: Re: [RFC PATCH 1/3] eeprom: Add a simple EEPROM framework

On 20/02/15 22:01, Rob Herring wrote:
> On Fri, Feb 20, 2015 at 1:25 PM, Srinivas Kandagatla
> <[email protected]> wrote:
>>
>>
>> On 20/02/15 17:21, Rob Herring wrote:
>>>
>>> On Thu, Feb 19, 2015 at 11:08 AM, Srinivas Kandagatla
>>> <[email protected]> wrote:
>>>>
>>>> From: Maxime Ripard <[email protected]>
>>>>
>>>> Up until now, EEPROM drivers were stored in drivers/misc, where they all
>>>> had to
>>>> duplicate pretty much the same code to register a sysfs file, allow
>>>> in-kernel
>>>> users to access the content of the devices they were driving, etc.
>>>>
>>>> This was also a problem as far as other in-kernel users were involved,
>>>> since
>>>> the solutions used were pretty much different from on driver to another,
>>>> there
>>>> was a rather big abstraction leak.
>>>>
>>>> This introduction of this framework aims at solving this. It also
>>>> introduces DT
>>>> representation for consumer devices to go get the data they require (MAC
>>>> Addresses, SoC/Revision ID, part numbers, and so on) from the EEPROMs.
>>>>
>>>> Having regmap interface to this framework would give much better
>>>> abstraction for eeproms on different buses.
>>>>
>>>> Signed-off-by: Maxime Ripard <[email protected]>
>>>> [srinivas.kandagatla: Moved to regmap based and cleanedup apis]
>>>> Signed-off-by: Srinivas Kandagatla <[email protected]>
>>>> ---
>>>> .../devicetree/bindings/eeprom/eeprom.txt | 48 ++++
>>>> drivers/Kconfig | 2 +
>>>> drivers/Makefile | 1 +
>>>> drivers/eeprom/Kconfig | 19 ++
>>>> drivers/eeprom/Makefile | 9 +
>>>> drivers/eeprom/core.c | 290
>>>> +++++++++++++++++++++
>>>> include/linux/eeprom-consumer.h | 73 ++++++
>>>> include/linux/eeprom-provider.h | 51 ++++
>>>
>>>
>>> Who is going to be the maintainer for this?
>>
>>
>> Am happy to be one.
>
> So please add a MAINTAINERS entry.
Yep, I will do that in next version.

>
> [...]
>
>>>> += Data consumers =
>>>> +
>>>> +Required properties:
>>>> +
>>>> +eeproms: List of phandle and data cell specifier triplet, one triplet
>>>> + for each data cell the device might be interested in. The
>>>> + triplet consists of the phandle to the eeprom provider, then
>>>> + the offset in byte within that storage device, and the length
>>>> + in byte of the data we care about.
>>>
>>>
>>> The problem with this is it assumes you know who the consumer is and
>>> that it is a DT node. For example, how would you describe a serial
>>> number?
>>
>> Correct me if I miss understood.
>> Is serial number any different?
>> Am hoping that the eeprom consumer would be aware of offset and size of
>> serial number in the eeprom
>>
>> Cant the consumer do:
>>
>> eeprom-consumer {
>> eeproms = <&at24 0 4>;
>> eeprom-names = "device-serial-number";
>
> Yes, but who is "eeprom-consumer"? DT nodes generally describe a h/w
> block, but it this case, the consumer depends on the OS, not the h/w.

Consumer could be any driver for the IP on the SOC, for example an
ethernet driver which needs Mac Address from eeprom or an thermal sensor
which requires cablibration values or an cpufreq driver which requires
OPP settings. Am not sure who could be the consumer for serial number, I
guess it should some soc specific driver.

> I'm not saying you can't describe where things are, but I don't think
> you should imply who is the consumer and doing so is unnecessarily
> complicated.
>
> Also, the layout of EEPROM is likely very much platform specific. Some
> could have a more complex structure perhaps with key ids and linked
> list structure.
I agree, the data layout is very specific to platform and could vary in
complexity.
This simple framework is attempting to solve most common usecase where
in the consumer drivers like thermal-sensor/network/cpufreq needs to
read an location in the eeprom. Am sure we can find a way to accommodate
the complex layout as well.
>
> I would do something more simple that is just a list of keys and their
> location like this:
>
> device-serial-number = <start size>;
> key1 = <start size>;
> key2 = <start size>;
>
There are pros and cons doing it as list of keys.

One reason for doing it as fixed properties("eeproms", "eemprom-names")
is "consistency and familiarity" like interrupts, regs, dmas, clocks,
pinctrl, reset, pwm have fixed property names, trying to get most
subsystems to do it the same way makes it easier for people to write dts
files.

--srini

> Rob
>

2015-02-22 14:35:14

by Maxime Ripard

[permalink] [raw]

Subject: Re: [RFC PATCH 1/3] eeprom: Add a simple EEPROM framework

On Fri, Feb 20, 2015 at 04:01:55PM -0600, Rob Herring wrote:
> [...]
>
> >>> += Data consumers =
> >>> +
> >>> +Required properties:
> >>> +
> >>> +eeproms: List of phandle and data cell specifier triplet, one triplet
> >>> + for each data cell the device might be interested in. The
> >>> + triplet consists of the phandle to the eeprom provider, then
> >>> + the offset in byte within that storage device, and the length
> >>> + in byte of the data we care about.
> >>
> >>
> >> The problem with this is it assumes you know who the consumer is and
> >> that it is a DT node. For example, how would you describe a serial
> >> number?
> >
> > Correct me if I miss understood.
> > Is serial number any different?
> > Am hoping that the eeprom consumer would be aware of offset and size of
> > serial number in the eeprom
> >
> > Cant the consumer do:
> >
> > eeprom-consumer {
> > eeproms = <&at24 0 4>;
> > eeprom-names = "device-serial-number";
>
> Yes, but who is "eeprom-consumer"?

Any device that should lookup values in one of the EEPROM.

> DT nodes generally describe a h/w block, but it this case, the
> consumer depends on the OS, not the h/w.

Not really, or at least, not more than any similar binding we
currently have.

The fact that a MAC-address for the first ethernet chip is stored at a
given offset in a given eeprom has nothing to do with the OS.

> I'm not saying you can't describe where things are, but I don't
> think you should imply who is the consumer and doing so is
> unnecessarily complicated.

If you only take the case of a serial number, indeed. If you take
other usage into account, you can't really do without it.

> Also, the layout of EEPROM is likely very much platform specific.

Indeed, which is why it should be in the DT.

> Some could have a more complex structure perhaps with key ids and
> linked list structure.

Then just request the size of the whole list, and parse it afterwards
in your driver?

> I would do something more simple that is just a list of keys and their
> location like this:
>
> device-serial-number = <start size>;
> key1 = <start size>;
> key2 = <start size>;

I'm sorry, but what's the difference?

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (2.28 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-02-22 14:35:10

On Thu, Feb 19, 2015 at 05:08:28PM +0000, Srinivas Kandagatla wrote:

> .../devicetree/bindings/eeprom/eeprom.txt | 48 ++++
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/eeprom/Kconfig | 19 ++
> drivers/eeprom/Makefile | 9 +
> drivers/eeprom/core.c | 290 +++++++++++++++++++++
> include/linux/eeprom-consumer.h | 73 ++++++
> include/linux/eeprom-provider.h | 51 ++++

This seems to have a bunch of different things in it - there's some
binding, there's some framework code, there's some user code for the
framework... splitting it up more would probably help with review.
I'd at least make sure the framework is split from the DT code, that
will cut down on the bulk and help make sure the framework isn't too DT
tied.

> + if (read)
> + rc = regmap_bulk_read(eeprom->regmap, offset,
> + buf, count/eeprom->stride);
> + else
> + rc = regmap_bulk_write(eeprom->regmap, offset,
> + buf, count/eeprom->stride);
> +
> + if (IS_ERR_VALUE(rc))
> + return 0;
> +
> + return count;
> +}

> +static ssize_t bin_attr_eeprom_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *attr,
> + char *buf, loff_t offset, size_t count)
> +{
> + return bin_attr_eeprom_read_write(kobj, buf, offset, count, true);
> +}

I'm not sure the factoring out is actually helping the clarity here TBH.

> +int eeprom_unregister(struct eeprom_device *eeprom)
> +{
> + device_del(&eeprom->edev);
> +
> + mutex_lock(&eeprom_list_mutex);
> + list_del(&eeprom->list);
> + mutex_unlock(&eeprom_list_mutex);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(eeprom_unregister);

Here we return having dropped the lock without doing anything else with
the eeprom, meaning the caller could delete it.

> + mutex_lock(&eeprom_list_mutex);
> +
> + list_for_each_entry(e, &eeprom_list, list) {
> + if (args.np == e->edev.of_node) {
> + eeprom = e;
> + break;
> + }
> + }
> + mutex_unlock(&eeprom_list_mutex);

Here we iterate the list, find the relevant eeprom and drop the lock
while still holding a reference to the eeprom we just found. This means
that a removal could race with us and free the eeprom underneath us.
I'm also not seeing anything stopping or even noticing a device being
removed with a cell active on it.

> +/**
> + * eeprom_cell_get(): Get eeprom cell of device form a given index.
> + *
> + * @dev: Device that will be interacted with
> + * @index: Index of the eeprom cell.
> + *
> + * The return value will be an ERR_PTR() on error or a valid pointer
> + * to a struct eeprom_cell. The eeprom_cell will be freed by the
> + * eeprom_cell_put().
> + */
> +struct eeprom_cell *eeprom_cell_get(struct device *dev, int index);

Normally the kerneldoc goes with the function definition, not the
prototype.

Attachments:

(No filename) (2.90 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-02-23 15:38:37

by Srinivas Kandagatla

[permalink] [raw]

Subject: Re: [RFC PATCH 1/3] eeprom: Add a simple EEPROM framework

Thankyou for the comments.

On 23/02/15 15:04, Mark Brown wrote:
> On Thu, Feb 19, 2015 at 05:08:28PM +0000, Srinivas Kandagatla wrote:
>
>> .../devicetree/bindings/eeprom/eeprom.txt | 48 ++++
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 1 +
>> drivers/eeprom/Kconfig | 19 ++
>> drivers/eeprom/Makefile | 9 +
>> drivers/eeprom/core.c | 290 +++++++++++++++++++++
>> include/linux/eeprom-consumer.h | 73 ++++++
>> include/linux/eeprom-provider.h | 51 ++++
>
> This seems to have a bunch of different things in it - there's some
> binding, there's some framework code, there's some user code for the
> framework... splitting it up more would probably help with review.
> I'd at least make sure the framework is split from the DT code, that
> will cut down on the bulk and help make sure the framework isn't too DT
> tied.

Yes I agree, will make sure that I split it into different patches in
next version.
>
>> + if (read)
>> + rc = regmap_bulk_read(eeprom->regmap, offset,
>> + buf, count/eeprom->stride);
>> + else
>> + rc = regmap_bulk_write(eeprom->regmap, offset,
>> + buf, count/eeprom->stride);
>> +
>> + if (IS_ERR_VALUE(rc))
>> + return 0;
>> +
>> + return count;
>> +}
>
>> +static ssize_t bin_attr_eeprom_read(struct file *filp, struct kobject *kobj,
>> + struct bin_attribute *attr,
>> + char *buf, loff_t offset, size_t count)
>> +{
>> + return bin_attr_eeprom_read_write(kobj, buf, offset, count, true);
>> +}
>
> I'm not sure the factoring out is actually helping the clarity here TBH.
>
ok.

>> +int eeprom_unregister(struct eeprom_device *eeprom)
>> +{
>> + device_del(&eeprom->edev);
>> +
>> + mutex_lock(&eeprom_list_mutex);
>> + list_del(&eeprom->list);
>> + mutex_unlock(&eeprom_list_mutex);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(eeprom_unregister);
>
> Here we return having dropped the lock without doing anything else with
> the eeprom, meaning the caller could delete it.
>
>> + mutex_lock(&eeprom_list_mutex);
>> +
>> + list_for_each_entry(e, &eeprom_list, list) {
>> + if (args.np == e->edev.of_node) {
>> + eeprom = e;
>> + break;
>> + }
>> + }
>> + mutex_unlock(&eeprom_list_mutex);
>
> Here we iterate the list, find the relevant eeprom and drop the lock
> while still holding a reference to the eeprom we just found. This means
> that a removal could race with us and free the eeprom underneath us.
> I'm also not seeing anything stopping or even noticing a device being
> removed with a cell active on it.
>
As suggested by Stephen Boyd reference counting on eeprom should ensure
safe removal of eeprom.

>> +/**
>> + * eeprom_cell_get(): Get eeprom cell of device form a given index.
>> + *
>> + * @dev: Device that will be interacted with
>> + * @index: Index of the eeprom cell.
>> + *
>> + * The return value will be an ERR_PTR() on error or a valid pointer
>> + * to a struct eeprom_cell. The eeprom_cell will be freed by the
>> + * eeprom_cell_put().
>> + */
>> +struct eeprom_cell *eeprom_cell_get(struct device *dev, int index);
>
> Normally the kerneldoc goes with the function definition, not the
> prototype.
Thats true, will fix it in next version.
>

2015-02-23 23:11:45

by Stephen Boyd

[permalink] [raw]

Subject: Re: [RFC PATCH 1/3] eeprom: Add a simple EEPROM framework

On 02/22/15 16:57, Rob Herring wrote:
> On Sun, Feb 22, 2015 at 8:32 AM, Maxime Ripard
> <[email protected]> wrote:
>> On Fri, Feb 20, 2015 at 04:01:55PM -0600, Rob Herring wrote:
>>> [...]
>>>
>>>>>> += Data consumers =
>>>>>> +
>>>>>> +Required properties:
>>>>>> +
>>>>>> +eeproms: List of phandle and data cell specifier triplet, one triplet
>>>>>> + for each data cell the device might be interested in. The
>>>>>> + triplet consists of the phandle to the eeprom provider, then
>>>>>> + the offset in byte within that storage device, and the length
>>>>>> + in byte of the data we care about.
>>>>>
>>>>> The problem with this is it assumes you know who the consumer is and
>>>>> that it is a DT node. For example, how would you describe a serial
>>>>> number?
>>>> Correct me if I miss understood.
>>>> Is serial number any different?
>>>> Am hoping that the eeprom consumer would be aware of offset and size of
>>>> serial number in the eeprom
>>>>
>>>> Cant the consumer do:
>>>>
>>>> eeprom-consumer {
>>>> eeproms = <&at24 0 4>;
>>>> eeprom-names = "device-serial-number";
>>> Yes, but who is "eeprom-consumer"?
>> Any device that should lookup values in one of the EEPROM.
>>
>>> DT nodes generally describe a h/w block, but it this case, the
>>> consumer depends on the OS, not the h/w.
>> Not really, or at least, not more than any similar binding we
>> currently have.
>>
>> The fact that a MAC-address for the first ethernet chip is stored at a
>> given offset in a given eeprom has nothing to do with the OS.
> So MAC address would be a valid use to link to a h/w device, but there
> are certainly cases that don't correspond to a device.
>
>>> I'm not saying you can't describe where things are, but I don't
>>> think you should imply who is the consumer and doing so is
>>> unnecessarily complicated.
>> If you only take the case of a serial number, indeed. If you take
>> other usage into account, you can't really do without it.
>>
>>> Also, the layout of EEPROM is likely very much platform specific.
>> Indeed, which is why it should be in the DT.
> Agreed. I'm not saying the layout should not be.
>
>>> Some could have a more complex structure perhaps with key ids and
>>> linked list structure.
>> Then just request the size of the whole list, and parse it afterwards
>> in your driver?
>>
>>> I would do something more simple that is just a list of keys and their
>>> location like this:
>>>
>>> device-serial-number = <start size>;
>>> key1 = <start size>;
>>> key2 = <start size>;
>> I'm sorry, but what's the difference?
> It can describe the layout completely whether the fields are tied to a
> h/w device or not.
>
> What I would like to see here is the entire layout described covering
> both types of fields.
>

I was thinking the DT might be like this on the provider side:

qfprom@1000000 {
reg = <0x1000000 0x1000>;
ranges = <0 0x1000000 0x1000>;
compatible = "qcom,qfprom-msm8960"

pvs-data: pvs-data@40 {
compatible = "qcom,pvs-a";
reg = <0x40 0x20>,
#eeprom-cells = <0>;
};

tsens-data: tmdata@10 {
compatible = "qcom,tsens-data-msm8960";
reg = <0x10 4>, <0x16 4>;
#eeprom-cells = <0>;

};

serial-number: serial@50 {
compatible = "qcom,serial-msm8960";
reg = <0x50 4>, <0x60 4>;
#eeprom-cells = <0>;

};
};

and then on the consumer side:

device {
eeproms = <&serial-number>;
eeprom-names = "soc-rev-id";
};

This would solve a problem where the consumer device is some standard
off-the-shelf IP block that needs to get some SoC specific calibration
data from the eeprom. I may want to interpret the bits differently
depending on which eeprom is connected to my SoC. Sometimes that data
format may be the same across many variations of the SoC (e.g. the
qcom,pvs-a node) or it may be completely different for a given SoC (e.g.
qcom,serial-msm8960 node). I imagine for other SoCs out there it could
be different depending on which eeprom the board manufacturer decides to
wire onto their board and how they choose to program the data.

So this is where I think the eeprom-cells and offset + length starts to
fall apart. It forces us to make up a bunch of different compatible
strings for our consumer device just so that we can parse the eeprom
that we decided to use for some SoC/board specific data. Instead I'd
like to see some framework that expresses exactly which eeprom is on my
board and how to interpret the bits in a way that doesn't require me to
keep refining the compatible string for my generic IP block.

I worry that if we put all those details in DT we'll end up having to
describe individual bits like serial-number-bit-2, serial-number-bit-3,
etc. because sometimes these pieces of data are scattered all around the
eeprom and aren't contiguous or aligned on a byte boundary. It may be
easier to just have a way to express that this is an eeprom with this
specific layout and my device has data stored in there. Then the driver
can be told what layout it is (via compatible or some other string based
means if we're not using DT?) and match that up with some driver data if
it needs to know how to understand the bits it can read with the
eeprom_read() API.

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2015-02-24 07:08:47

by Srinivas Kandagatla

[permalink] [raw]

Subject: Re: [RFC PATCH 1/3] eeprom: Add a simple EEPROM framework

Thanks Stephen for the comments.

On 23/02/15 23:11, Stephen Boyd wrote:
> On 02/22/15 16:57, Rob Herring wrote:
>> On Sun, Feb 22, 2015 at 8:32 AM, Maxime Ripard
>> <[email protected]> wrote:
>>> On Fri, Feb 20, 2015 at 04:01:55PM -0600, Rob Herring wrote:
>>>> [...]
>>>>
>>>>>>> += Data consumers =
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +
>>>>>>> +eeproms: List of phandle and data cell specifier triplet, one triplet
>>>>>>> + for each data cell the device might be interested in. The
>>>>>>> + triplet consists of the phandle to the eeprom provider, then
>>>>>>> + the offset in byte within that storage device, and the length
>>>>>>> + in byte of the data we care about.
>>>>>>
>>>>>> The problem with this is it assumes you know who the consumer is and
>>>>>> that it is a DT node. For example, how would you describe a serial
>>>>>> number?
>>>>> Correct me if I miss understood.
>>>>> Is serial number any different?
>>>>> Am hoping that the eeprom consumer would be aware of offset and size of
>>>>> serial number in the eeprom
>>>>>
>>>>> Cant the consumer do:
>>>>>
>>>>> eeprom-consumer {
>>>>> eeproms = <&at24 0 4>;
>>>>> eeprom-names = "device-serial-number";
>>>> Yes, but who is "eeprom-consumer"?
>>> Any device that should lookup values in one of the EEPROM.
>>>
>>>> DT nodes generally describe a h/w block, but it this case, the
>>>> consumer depends on the OS, not the h/w.
>>> Not really, or at least, not more than any similar binding we
>>> currently have.
>>>
>>> The fact that a MAC-address for the first ethernet chip is stored at a
>>> given offset in a given eeprom has nothing to do with the OS.
>> So MAC address would be a valid use to link to a h/w device, but there
>> are certainly cases that don't correspond to a device.
>>
>>>> I'm not saying you can't describe where things are, but I don't
>>>> think you should imply who is the consumer and doing so is
>>>> unnecessarily complicated.
>>> If you only take the case of a serial number, indeed. If you take
>>> other usage into account, you can't really do without it.
>>>
>>>> Also, the layout of EEPROM is likely very much platform specific.
>>> Indeed, which is why it should be in the DT.
>> Agreed. I'm not saying the layout should not be.
>>
>>>> Some could have a more complex structure perhaps with key ids and
>>>> linked list structure.
>>> Then just request the size of the whole list, and parse it afterwards
>>> in your driver?
>>>
>>>> I would do something more simple that is just a list of keys and their
>>>> location like this:
>>>>
>>>> device-serial-number = <start size>;
>>>> key1 = <start size>;
>>>> key2 = <start size>;
>>> I'm sorry, but what's the difference?
>> It can describe the layout completely whether the fields are tied to a
>> h/w device or not.
>>
>> What I would like to see here is the entire layout described covering
>> both types of fields.
>>
>
> I was thinking the DT might be like this on the provider side:
>
> qfprom@1000000 {
> reg = <0x1000000 0x1000>;
> ranges = <0 0x1000000 0x1000>;
> compatible = "qcom,qfprom-msm8960"
>
> pvs-data: pvs-data@40 {
> compatible = "qcom,pvs-a";

These compatibles could be optional. As it might not be required for
devices which are simple and do not require any special interpretation
of eeprom data.

> reg = <0x40 0x20>,
> #eeprom-cells = <0>;

Do we still need eeprom-cells if we are moving to use reg property here?

> };
>
> tsens-data: tmdata@10 {
> compatible = "qcom,tsens-data-msm8960";
> reg = <0x10 4>, <0x16 4>;
> #eeprom-cells = <0>;
>
> };
>
> serial-number: serial@50 {
> compatible = "qcom,serial-msm8960";
> reg = <0x50 4>, <0x60 4>;
> #eeprom-cells = <0>;
>
> };
> };
>
> and then on the consumer side:
>
> device {
> eeproms = <&serial-number>;
> eeprom-names = "soc-rev-id";
> };
>

Yes, this looks good, and Sasha was also recommending something on these
lines too. Also this addresses the use cases like serial number too.

>
> This would solve a problem where the consumer device is some standard
> off-the-shelf IP block that needs to get some SoC specific calibration
> data from the eeprom. I may want to interpret the bits differently
> depending on which eeprom is connected to my SoC. Sometimes that data
> format may be the same across many variations of the SoC (e.g. the
> qcom,pvs-a node) or it may be completely different for a given SoC (e.g.
> qcom,serial-msm8960 node). I imagine for other SoCs out there it could
> be different depending on which eeprom the board manufacturer decides to
> wire onto their board and how they choose to program the data.
>
> So this is where I think the eeprom-cells and offset + length starts to
> fall apart. It forces us to make up a bunch of different compatible
> strings for our consumer device just so that we can parse the eeprom
> that we decided to use for some SoC/board specific data. Instead I'd
> like to see some framework that expresses exactly which eeprom is on my
> board and how to interpret the bits in a way that doesn't require me to
> keep refining the compatible string for my generic IP block.
>
> I worry that if we put all those details in DT we'll end up having to
> describe individual bits like serial-number-bit-2, serial-number-bit-3,
> etc. because sometimes these pieces of data are scattered all around the
> eeprom and aren't contiguous or aligned on a byte boundary. It may be
> easier to just have a way to express that this is an eeprom with this
> specific layout and my device has data stored in there. Then the driver
> can be told what layout it is (via compatible or some other string based
> means if we're not using DT?) and match that up with some driver data if
> it needs to know how to understand the bits it can read with the
> eeprom_read() API.
Yes this sounds more sensible to let the consumer driver interpret the
eeprom data than the eeprom framework itself.

--srini
>

2015-02-24 09:25:09

On Tue, Feb 24, 2015 at 05:30:49PM -0800, Stephen Boyd wrote:
> On 02/24, Maxime Ripard wrote:
> > On Mon, Feb 23, 2015 at 03:11:40PM -0800, Stephen Boyd wrote:
> > > >>> I would do something more simple that is just a list of keys and
> > > >>> their location like this:
> > > >>>
> > > >>> device-serial-number = <start size>;
> > > >>> key1 = <start size>;
> > > >>> key2 = <start size>;
> > > >> I'm sorry, but what's the difference?
> > > > It can describe the layout completely whether the fields are tied to a
> > > > h/w device or not.
> > > >
> > > > What I would like to see here is the entire layout described covering
> > > > both types of fields.
> > > >
> > >
> > > I was thinking the DT might be like this on the provider side:
> > >
> > > qfprom@1000000 {
> > > reg = <0x1000000 0x1000>;
> > > ranges = <0 0x1000000 0x1000>;
> > > compatible = "qcom,qfprom-msm8960"
> > >
> > > pvs-data: pvs-data@40 {
> > > compatible = "qcom,pvs-a";
> > > reg = <0x40 0x20>,
> > > #eeprom-cells = <0>;
> > > };
> > >
> > > tsens-data: tmdata@10 {
> > > compatible = "qcom,tsens-data-msm8960";
> > > reg = <0x10 4>, <0x16 4>;
> > > #eeprom-cells = <0>;
> > >
> > > };
> > >
> > > serial-number: serial@50 {
> > > compatible = "qcom,serial-msm8960";
> > > reg = <0x50 4>, <0x60 4>;
> > > #eeprom-cells = <0>;
> > >
> > > };
> > > };
> >
> > I'm not sure the compatible is really needed.
> >
> > A label of some sort, just like the MTD partitions do would do just
> > fine, and wouldn't have the implicit expectation that a driver will be
> > probed from that node.
>
> I wasn't aware that compatible meant driver probe. I thought
> compatible just meant some software entity can understand what
> I've described within this node. For example, compatible for
> reserved-memory nodes doesn't mean we're going to probe a device.

Maybe it's just me then :)

> > > and then on the consumer side:
> > >
> > > device {
> > > eeproms = <&serial-number>;
> > > eeprom-names = "soc-rev-id";
> > > };
> > >
> > >
> > > This would solve a problem where the consumer device is some standard
> > > off-the-shelf IP block that needs to get some SoC specific calibration
> > > data from the eeprom. I may want to interpret the bits differently
> > > depending on which eeprom is connected to my SoC. Sometimes that data
> > > format may be the same across many variations of the SoC (e.g. the
> > > qcom,pvs-a node) or it may be completely different for a given SoC (e.g.
> > > qcom,serial-msm8960 node). I imagine for other SoCs out there it could
> > > be different depending on which eeprom the board manufacturer decides to
> > > wire onto their board and how they choose to program the data.
> >
> > Oh, so you'd like to infer the data format it's stored in from the
> > compatible?
> >
> > AFAICT, this format will be highly depending on the board itself,
> > rather than on the SoC, do you think it will scale enough?
> >
> > > So this is where I think the eeprom-cells and offset + length starts to
> > > fall apart. It forces us to make up a bunch of different compatible
> > > strings for our consumer device just so that we can parse the eeprom
> > > that we decided to use for some SoC/board specific data. Instead I'd
> > > like to see some framework that expresses exactly which eeprom is on my
> > > board and how to interpret the bits in a way that doesn't require me to
> > > keep refining the compatible string for my generic IP block.
> >
> > Hmmmm, apparently you don't :)
> >
> > > I worry that if we put all those details in DT we'll end up having to
> > > describe individual bits like serial-number-bit-2, serial-number-bit-3,
> > > etc. because sometimes these pieces of data are scattered all around the
> > > eeprom and aren't contiguous or aligned on a byte boundary. It may be
> > > easier to just have a way to express that this is an eeprom with this
> > > specific layout and my device has data stored in there. Then the driver
> > > can be told what layout it is (via compatible or some other string based
> > > means if we're not using DT?) and match that up with some driver data if
> > > it needs to know how to understand the bits it can read with the
> > > eeprom_read() API.
> >
> > I'm half convinced that the layout information will actually work for
> > more complex cases, like the linked list Rob described.
> >
> > If such a structure is ever to be found, it would feel wrong to have
> > that in the EEPROM driver, but it would feel just as wrong to put that
> > in the client driver, that would have to handle the parsing of raw
> > data coming flashed by one single crazy board vendor.
> >
> > Maybe we can have each cell carry a property that defines the format
> > it's stored in, and match that to some parsers plugins, starting with
> > the generic and trivial cases but still allowing for custom parsers to
> > be defined?
> >
> > Something like
> >
> > eeprom@42 {
> > compatible = "atmel,at24something";
> > reg = <0x42>;
> >
> > serial@0 {
> > label = "board serial";
> > reg = <0x0 0x10>;
> > format = "packed";
> > };
> >
> > opps@10 {
> > label = "board serial";
> > reg = <0x10 0x10>, <0x40 0x10>, <0x80 0x10>;
> > format = "random-vendor,opp-linked-list";
> > };
> > };
> >
> > That would make eeprom_read always return the same format of data to
> > the client drivers, without cripling the generic EEPROM drivers
> > either.
> >
>
> Is the goal here to make eeprom_read() figure out how to return
> the next byte of data and hide the parsing logic behind the
> eeprom APIs? I imagine "random-vendor,opp-linked-list" would be
> handled by the eeprom driver and that would return OPPs byte by
> byte across the different reg properties to the eeprom consumer?
>
> This approach concerns me because every eeprom_read() call needs
> to fit the format that the client driver is expecting. How do we
> validate that? What do we do if we have a random OPP client #1
> that expects to get the data from eeprom_read() with OPPs in
> ascending order and random OPP client #2 that expects to get the
> data from eeprom_read() with OPPs in descending order?

Without going that far, we could have the little/big endian topic here
as well.

But I guess it all boils down to wether we should support only the
trivial cases, or not. Generally speaking, and not just about the OPPs
above, we could really well end up with a "generic" (not necessarily a
really generic driver, but also IPs used across several SoCs, like the
Mentor/Synopsis ones) driver, requiring to read some data from an
EEPROM for some reason.

Where would you fit the raw data parsing code? In that generic
driver. It would end up being just as messy, if not more.

So yeah, it really depends on wether we just want to support reading a
contiguous block of data, or if we want to cover all cases. And in
that case, we should indeed support the cases you mentioned above.

> It feels like we're making the eeprom framework too smart without
> a well defined abstraction. If we were to make it so that
> eeprom_get_opps() knew what to do and parsed/populated the OPPs,
> it might work. But if we're just exporting raw data across a
> read/write API with some implementation specific mangling it
> sounds like it's going to get messy fast. And if the API is well
> defined, it would start to become rather large with many
> different types of data that need to be parsed and sometimes data
> that's only specific to a single SoC.

Or even a single board. Most of the drivers are in that case. That
doesn't mean that the frameworks should just ignore them entirely
because of that fact.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (7.72 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-02-26 13:25:09

by Maxime Ripard

[permalink] [raw]

Subject: Re: [RFC PATCH 1/3] eeprom: Add a simple EEPROM framework

On Thu, Feb 26, 2015 at 09:16:27AM +0000, Srinivas Kandagatla wrote:
> I think we are making simple eeprom framework too smart which will
> break in future.
>
> IMHO, Anything on top of eeprom interface that interprets the data should
> not go into the eeprom framework itself, it can either live some parsers/SOC
> specific drivers/interfaces.

True, but that doesn't mean that this parser support can't be built
within the framework itself.

> As Stephen pointed out earlier lets start with something like this, which
> would provide a better abstraction to the discussed use cases like
> serial-number and packed data in eeprom.
>
> qfprom@1000000 {
> reg = <0x1000000 0x1000>;
> ranges = <0 0x1000000 0x1000>;
> compatible = "qcom,qfprom-msm8960"
>
> pvs-data: pvs-data@40 {
> compatible = "qcom,pvs-a";
> reg = <0x40 0x20>,
> };
>
> tsens-data: tmdata@10 {
> reg = <0x10 40>;
> };
>
> serial-number: serial@50 {
> compatible = "qcom,serial-msm8960";
> reg = <0x50 4>, <0x60 4>;
> };
>
> };

And I'm sorry, but I still don't get why the compatibles are needed
here.

> and then on the consumer side:
>
> device {
> eeproms = <&serial-number>;
> eeprom-names = "soc-rev-id";
> };
>
> driver side:
>
> eeprom_get_cell()
> eeprom_read();

Looks good otherwise.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (1.48 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-02-26 14:56:58

by Srinivas Kandagatla

[permalink] [raw]

Subject: Re: [RFC PATCH 1/3] eeprom: Add a simple EEPROM framework

On 26/02/15 13:21, Maxime Ripard wrote:
> On Thu, Feb 26, 2015 at 09:16:27AM +0000, Srinivas Kandagatla wrote:
>> I think we are making simple eeprom framework too smart which will
>> break in future.
>>
>> IMHO, Anything on top of eeprom interface that interprets the data should
>> not go into the eeprom framework itself, it can either live some parsers/SOC
>> specific drivers/interfaces.
>
> True, but that doesn't mean that this parser support can't be built
> within the framework itself.
I was more of thinking parsers/interpreters as a layer on top of this
framework. Allowing the simplest users direct access to framework. Also
just eeprom_read() apis would not cater for all the parsers and I think
it would be very difficult to come up with abstracted consumer apis for
all the parsers which could be iterator or link-lists parsers or
something very different.

>
>> As Stephen pointed out earlier lets start with something like this, which
>> would provide a better abstraction to the discussed use cases like
>> serial-number and packed data in eeprom.
>>
>> qfprom@1000000 {
>> reg = <0x1000000 0x1000>;
>> ranges = <0 0x1000000 0x1000>;
>> compatible = "qcom,qfprom-msm8960"
>>
>> pvs-data: pvs-data@40 {
>> compatible = "qcom,pvs-a";
>> reg = <0x40 0x20>,
>> };
>>
>> tsens-data: tmdata@10 {
>> reg = <0x10 40>;
>> };
>>
>> serial-number: serial@50 {
>> compatible = "qcom,serial-msm8960";
>> reg = <0x50 4>, <0x60 4>;
>> };
>>
>> };
>
> And I'm sorry, but I still don't get why the compatibles are needed
> here.
This is an optional property, only purpose of this would be to serve as
parser/soc-specific identifier.

>
>> and then on the consumer side:
>>
>> device {
>> eeproms = <&serial-number>;
>> eeprom-names = "soc-rev-id";
>> };
>>
>> driver side:
>>
>> eeprom_get_cell()
>> eeprom_read();
>
> Looks good otherwise.
thanks
--srini
>
> Maxime
>