| Message ID | 1427752679-17261-1-git-send-email-srinivas.kandagatla@linaro.org |
|---|---|
| State | New |
| Headers | show |
Thanks Matt and Rob for review, On 06/04/15 16:04, Matt Porter wrote: > On Mon, Apr 06, 2015 at 09:11:05AM -0500, Rob Herring wrote: >> On Mon, Apr 6, 2015 at 8:32 AM, Matt Porter <mporter@konsulko.com> wrote: >>> On Mon, Mar 30, 2015 at 10:57:59PM +0100, Srinivas Kandagatla wrote: >>>> This patch adds bindings for simple eeprom framework which allows eeprom >>>> consumers to talk to eeprom providers to get access to eeprom cell data. >>>> >>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >>>> [Maxime Ripard: intial version of eeprom framework] >>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >>>> --- >>>> .../devicetree/bindings/eeprom/eeprom.txt | 58 ++++++++++++++++++++++ >>>> 1 file changed, 58 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/eeprom/eeprom.txt >>>> >>>> diff --git a/Documentation/devicetree/bindings/eeprom/eeprom.txt b/Documentation/devicetree/bindings/eeprom/eeprom.txt >>>> new file mode 100644 >>>> index 0000000..fb71d46 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/eeprom/eeprom.txt >>>> @@ -0,0 +1,58 @@ >>>> += EEPROM Data Device Tree Bindings = >>>> + >>>> +This binding is intended to represent the location of hardware >>>> +configuration data stored in EEPROMs. >>>> + >>>> +On a significant proportion of boards, the manufacturer has stored >>>> +some data on an EEPROM-like device, for the OS to be able to retrieve >>>> +these information and act upon it. Obviously, the OS has to know >>>> +about where to retrieve these data from, and where they are stored on >>>> +the storage device. >>> >>> Since this binding (and the kernel framework supporting it) describes >>> non-volatile memory devices other than EEPROMs (e.g. EFuses) it should >>> be named more generically like "nvmem". >>> nvmem sounds sensible name, I will rename framework to nvmem in next version. >>>> + >>>> +This document is here to document this. >>>> + >>>> += Data providers = >>>> +Contains bindings specific to provider drivers and data cells as children >>>> +to this node. >>>> + >>>> += Data cells = >>>> +These are the child nodes of the provider which contain data cell >>>> +information like offset and size in eeprom provider. >>>> + >>>> +Required properties: >>>> +reg: specifies the offset in byte within that storage device, and the length >>>> + in bytes of the data we care about. >>>> + There could be more then one offset-length pairs in this property. >>>> + >>>> +Optional properties: >>>> +As required by specific data parsers/interpreters. >>> >>> The generic binding could really use a "read-only" property here as this >>> is a common hardware attribute for many nvmem devices. A serial EEPROM >>> commonly has a write protect pin which may be hard-wired such that the >>> hardware description should reflect that. An EFuse is typically blown with >>> the required information at manufacturing time (for an end product case) >>> and would be marked with the "read-only" flag. >>> >>> Having this optional flag in the generic binding would allow the >>> framework to hint to consumers about the inability to write to the >>> provided region. >> >> This could get fairly complex if you wanted to describe grouping of WP >> regions which could have different layout than the fields here. This >> may be better left as a device property listing addr & size pairs. >> However, there is the notion of s/w "read-only" which means the OS >> should not allow write access. The MTD partition binding supports this >> with the "read-only" property. > > Yes, if the backing device has the capability to hw write protect > regions the exported fields overlap those then it does get ugly. > The MTD partition property was the inspiration here so perhaps it's > best to term this as a property indicating how the data region is > used in an implementation. > Correct me If am wrong. Regarding write protection/read-only, regmap already has provisions to support this feature. regmap would bail out with errors if any attempt to write to non-writable regions. It all depends on the data providers how they setup the regmap and the bindings for those are specific individual data providers I think. This would protect the user space side and kernel side consumers as well. This should address your original query, I guess :-) Thanks, srini > If it's left as a device property, then a binding with this property > would need to be defined for the Efuse, etc. cases..or a simple-nvmem > binding to handle the various OTP technologies. > >>> The framework sysfs attributes provide a userspace EEPROM consumer where >>> it would be useful information to know that a data provider region is >>> read only rather than having the exported writeable attribute simply >>> fail a write cycle. This would allow the consumer to be aware that a >>> failed write (if even attempted) is expected if the data provider >>> advertised itself as read-only. >> >> You could distinguish with RW versus RO file attributes. > > Right, that would be preferred, based on the what the data provider > advertises. > > -Matt > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/04/15 18:46, Mark Brown wrote: > On Tue, Apr 07, 2015 at 06:35:49PM +0100, Srinivas Kandagatla wrote: >> On 06/04/15 16:04, Matt Porter wrote: >>> On Mon, Apr 06, 2015 at 09:11:05AM -0500, Rob Herring wrote: > >>>>> The generic binding could really use a "read-only" property here as this >>>>> is a common hardware attribute for many nvmem devices. A serial EEPROM > >> Correct me If am wrong. > >> Regarding write protection/read-only, regmap already has provisions to >> support this feature. regmap would bail out with errors if any attempt to >> write to non-writable regions. It all depends on the data providers how they >> setup the regmap and the bindings for those are specific individual data >> providers I think. > > There is the ability to flag read/write permissions in regmap but I > think there's some suggestion that this should be exposed to userspace > so that it's easier for it to handle things rather than just writing > then coping with any errors. Yes, That's possible if the data provider use the "read-only" generic binding like MTD partitions which the eeprom framwork could use to set the binary file mode appropriately. "read-only" property seems to be more generic for all types of data providers. I will give it a try and document this in the bindings too in next version. --srini > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/04/15 20:46, Matt Porter wrote: > On Tue, Apr 07, 2015 at 07:03:30PM +0100, Srinivas Kandagatla wrote: >> >> >> On 07/04/15 18:46, Mark Brown wrote: >>> On Tue, Apr 07, 2015 at 06:35:49PM +0100, Srinivas Kandagatla wrote: >>>> On 06/04/15 16:04, Matt Porter wrote: >>>>> On Mon, Apr 06, 2015 at 09:11:05AM -0500, Rob Herring wrote: >>> >>>>>>> The generic binding could really use a "read-only" property here as this >>>>>>> is a common hardware attribute for many nvmem devices. A serial EEPROM >>> >>>> Correct me If am wrong. >>> >>>> Regarding write protection/read-only, regmap already has provisions to >>>> support this feature. regmap would bail out with errors if any attempt to >>>> write to non-writable regions. It all depends on the data providers how they >>>> setup the regmap and the bindings for those are specific individual data >>>> providers I think. >>> >>> There is the ability to flag read/write permissions in regmap but I >>> think there's some suggestion that this should be exposed to userspace >>> so that it's easier for it to handle things rather than just writing >>> then coping with any errors. >> >> Yes, That's possible if the data provider use the "read-only" generic >> binding like MTD partitions which the eeprom framwork could use to set the >> binary file mode appropriately. > > Right, that's what Rob suggested as to how it should be exposed to > userspace. I think Mark is suggesting that it can also be done by > returning appropriately fine-grained error codes from a writeable > attribute. > We are taking about two things here. 1: "Setting the correct mode for the user space binary file." This is only possible if the entire eeprom/nvmem has write protection. We could get that info from 2 places. One having a new DT property bindings like "read-only" as Rob suggested. or Second, scan the regmap for writeable attributes and the correct file mode. For this option AFAIK, with existing regmap code we can only do this by scanning the entire range, which is bad I guess. 2: "Returning appropriate error codes to user space." This is already addressed in the existing code, regmap would return an (-EIO) I/O error Or error codes from providers in cases where an write attempt to non writable register/or something wrong is made and the *same error* code is passed back to user too. All the error codes from regmap/provider layer are always passed back to the user space. These error code, I supposed are fine grained originating from the low layer. I think with "read-only" property and returning same error codes from regmap/provider layer to user-space would address the points raised by Matt. > Just to clarify here, I brought this up because if we only allow > the writes to fail, there's not necessarily not enough information > available to know if they failed due to a real error (perhaps write > cycles for the underlying nvmem device have been exhausted) or is > it simply that the underlying device has been hardware write protected > (such as as the write protect pin hardwired that way or it's an OTP > device). The client, whether userspace or otherwise is going to need > to know this information to make informed policy decisions. > Thanks for the inputs. The exiting regmap writeable_register api will only return true or false. But if the provider implements its own read/writes functions, the error-codes from these apis would be passed back to user anyway, so the user can make informed policy decisions. This logic already present in with the exiting eeprom/regmap code. --srini > -Matt > >> "read-only" property seems to be more generic for all types of data >> providers. >> >> I will give it a try and document this in the bindings too in next version. >> >> --srini >>> -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/eeprom/eeprom.txt b/Documentation/devicetree/bindings/eeprom/eeprom.txt new file mode 100644 index 0000000..fb71d46 --- /dev/null +++ b/Documentation/devicetree/bindings/eeprom/eeprom.txt @@ -0,0 +1,58 @@ += EEPROM Data Device Tree Bindings = + +This binding is intended to represent the location of hardware +configuration data stored in EEPROMs. + +On a significant proportion of boards, the manufacturer has stored +some data on an EEPROM-like device, for the OS to be able to retrieve +these information and act upon it. Obviously, the OS has to know +about where to retrieve these data from, and where they are stored on +the storage device. + +This document is here to document this. + += Data providers = +Contains bindings specific to provider drivers and data cells as children +to this node. + += Data cells = +These are the child nodes of the provider which contain data cell +information like offset and size in eeprom provider. + +Required properties: +reg: specifies the offset in byte within that storage device, and the length + in bytes of the data we care about. + There could be more then one offset-length pairs in this property. + +Optional properties: +As required by specific data parsers/interpreters. + +For example: + + /* Provider */ + qfprom: qfprom@00700000 { + compatible = "qcom,qfprom"; + reg = <0x00700000 0x8000>; + ... + + /* Data cells */ + tsens_calibration: calib@4404 { + reg = <0x4404 0x10>; + }; + + serial_number: sn { + reg = <0x104 0x4>, <0x204 0x4>, <0x30c 0x4>; + + }; + ... + }; + += Data consumers = +Are device nodes which consume eeprom data cells. + +For example: + + tsens { + ... + calibration = <&tsens_calibration>; + };