Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752256AbbBXHIr (ORCPT ); Tue, 24 Feb 2015 02:08:47 -0500 Received: from mail-wi0-f174.google.com ([209.85.212.174]:43813 "EHLO mail-wi0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751147AbbBXHIp (ORCPT ); Tue, 24 Feb 2015 02:08:45 -0500 Message-ID: <54EC2378.5040202@linaro.org> Date: Tue, 24 Feb 2015 07:08:40 +0000 From: Srinivas Kandagatla User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Stephen Boyd , Rob Herring , Maxime Ripard CC: "linux-arm-kernel@lists.infradead.org" , Rob Herring , Pawel Moll , Kumar Gala , "linux-api@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , Arnd Bergmann , Mark Brown , Greg Kroah-Hartman Subject: Re: [RFC PATCH 1/3] eeprom: Add a simple EEPROM framework References: <1424365639-26634-1-git-send-email-srinivas.kandagatla@linaro.org> <1424365708-26681-1-git-send-email-srinivas.kandagatla@linaro.org> <54E78A31.9020306@linaro.org> <20150222143211.GX25269@lukather> <54EBB3AC.30000@codeaurora.org> In-Reply-To: <54EBB3AC.30000@codeaurora.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6366 Lines: 161 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 >> 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 = ; >>>> key1 = ; >>>> key2 = ; >>> 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/