Re: [PATCH 1/2] nvmem: add driver for JZ4780 efuse

[PrasannaKumar Muralidharan]

Hi Marcin,

On 28 December 2017 at 13:35, Marcin Nowakowski
<marcin.nowakowski@xxxxxxxx> 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@xxxxxxxx <mailto:marcin.nowakowski@xxxxxxxx>> wrote:
>>  > Hi Mathieu, PrasannaKumar,
>>  >
>>  > On 27.12.2017 13:27, Mathieu Malaterre wrote:
>>  >>
>>  >> From: PrasannaKumar Muralidharan <prasannatsmkumar@xxxxxxxxx
>> <mailto:prasannatsmkumar@xxxxxxxxx>>
>>  >>
>>  >> 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@xxxxxxxxxx
>> <mailto:malat@xxxxxxxxxx>>
>>  >> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@xxxxxxxxx
>> <mailto:prasannatsmkumar@xxxxxxxxx>>
>>  >> ---
>>  >
>>  >
>>  >> +
>>  >> +/* 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