lkml.org 
[lkml]   [2018]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 1/2] nvmem: add driver for JZ4780 efuse
Hi Marcin,

On 28 December 2017 at 13:35, Marcin Nowakowski
<marcin.nowakowski@mips.com> wrote:
> Hi Mathieu,
>
> On 28.12.2017 08:26, Mathieu Malaterre wrote:
>>
>> Hi Marcin,
>>
>> On Thu, Dec 28, 2017 at 8:13 AM, Marcin Nowakowski
>> <marcin.nowakowski@mips.com <mailto:marcin.nowakowski@mips.com>> wrote:
>> > Hi Mathieu, PrasannaKumar,
>> >
>> > On 27.12.2017 13:27, Mathieu Malaterre wrote:
>> >>
>> >> From: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com
>> <mailto:prasannatsmkumar@gmail.com>>
>> >>
>> >> This patch brings support for the JZ4780 efuse. Currently it only
>> expose
>> >> a read only access to the entire 8K bits efuse memory.
>> >>
>> >> Tested-by: Mathieu Malaterre <malat@debian.org
>> <mailto:malat@debian.org>>
>> >> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com
>> <mailto:prasannatsmkumar@gmail.com>>
>> >> ---
>> >
>> >
>> >> +
>> >> +/* main entry point */
>> >> +static int jz4780_efuse_read(void *context, unsigned int offset,
>> >> + void *val, size_t bytes)
>> >> +{
>> >> + static const int nsegments = sizeof(segments) /
>> sizeof(*segments);
>> >> + struct jz4780_efuse *efuse = context;
>> >> + char buf[32];
>> >> + char *cur = val;
>> >> + int i;
>> >> + /* PM recommends read/write each segment separately */
>> >> + for (i = 0; i < nsegments; ++i) {
>> >> + unsigned int *segment = segments[i];
>> >> + unsigned int lpos = segment[0];
>> >> + unsigned int buflen = segment[1] / 8;
>> >> + unsigned int ncount = buflen / 32;
>> >> + unsigned int remain = buflen % 32;
>> >> + int j;
>> >
>> >
>> > This doesn't look right, as offset & bytes are completely ignored. This
>> > means it will return data from an offset other than requested and may
>> also
>> > overrun the provided output buffer?
>>
>>
>> Thanks for the review ! That was the part of nvmem framework I was not
>> totally clear. Let say I want to expose only a portion of efuse space, eg:
>
>
> Do you need to expose this to the userspace or to other drivers only?
> For the second case have a look at the description of nvmem cell interface.
>
>
>> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> index 2f26922718559..44d97c06a6d15 100644
>> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> @@ -299,6 +299,15 @@
>> clocks = <&cgu JZ4780_CLK_AHB2>;
>> clock-names = "bus_clk";
>> +
>> +#address-cells = <1>;
>> +#size-cells = <1>;
>> +
>> +eth_mac: eth_mac@12 {
>> +/* six byte/48bit MAC address stored as 8-bit integers */
>> +reg = <0x12 0x6>;
>> +};
>> +
>> };
>> };
>> What should I do to expose that chunk only in the user space ?
>
>
> The nvmem interface's userspace interface (via /sys/.../nvmem) provides
> access to the complete device raw memory so the only way to achieve that
> would be to parse the devicetree description in your driver and only
> register part of the memory with the nvmem driver - but that would be a
> slight abuse of the interface.
> The nvmem devicetree binding document shows clearly how to define the cell
> interface that can later be used by any consumer - that way you could have
> the ethernet driver access the cell directly. However, as the dm9000 driver
> isn't designed to do that and this is a SoC-specific extention, I don't know
> how it fits with the general eth driver design ...
>
> Potentially a good and useful compromise would be to have all of the cell
> regs exposed via /sys/.../nvmem-cellname file (or something similar), but
> this is not currently supported and I don't know what the view of nvmem
> maintainers on adding such extension would be.

Currently exposing MAC address is necessary. No need to worry about
user space stuff for now.

>> >
>> >> + /* EFUSE can read or write maximum 256bit in each time
>> */
>> >> + for (j = 0; j < ncount ; ++j) {
>> >> + jz4780_efuse_read_32bytes(efuse, buf, lpos);
>> >> + memcpy(cur, buf, sizeof(buf));
>> >> + cur += sizeof(buf);
>> >> + lpos += sizeof(buf);
>> >> + }
>> >> + if (remain) {
>> >> + jz4780_efuse_read_32bytes(efuse, buf, lpos);
>> >> + memcpy(cur, buf, remain);
>> >> + cur += remain;
>> >> + }
>> >> + }
>> >> +
>> >> + return 0;
>> >> +}
>
>
> Regardless of the choices above, you still always have to make sure in your
> reg_read method that you only read from the offset specified in method
> arguments and never return more than 'bytes' of data requested.

Sure, will do that.

Regards,
PrasannaKumar

\
 
 \ /
  Last update: 2018-01-06 12:53    [W:0.064 / U:2.664 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site