Date:   Sat, 6 Jan 2018 17:22:34 +0530
From:   PrasannaKumar Muralidharan <prasannatsmkumar@...il.com>
To:     Marcin Nowakowski <marcin.nowakowski@...s.com>
Cc:     Mathieu Malaterre <malat@...ian.org>, Zubair.Kakakhel@...s.com,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Ralf Baechle <ralf@...ux-mips.org>,
        "David S. Miller" <davem@...emloft.net>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Paul Cercueil <paul@...pouillou.net>,
        Linus Walleij <linus.walleij@...aro.org>,
        Harvey Hunt <harvey.hunt@...tec.com>,
        James Hogan <jhogan@...nel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        open list <linux-kernel@...r.kernel.org>,
        devicetree@...r.kernel.org, linux-mips@...ux-mips.org
Subject: Re: [PATCH 1/2] nvmem: add driver for JZ4780 efuse

Hi Marcin,

On 28 December 2017 at 13:35, Marcin Nowakowski
<marcin.nowakowski@...s.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@...s.com <mailto:marcin.nowakowski@...s.com>> wrote:
>>  > Hi Mathieu, PrasannaKumar,
>>  >
>>  > On 27.12.2017 13:27, Mathieu Malaterre wrote:
>>  >>
>>  >> From: PrasannaKumar Muralidharan <prasannatsmkumar@...il.com
>> <mailto:prasannatsmkumar@...il.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@...ian.org
>> <mailto:malat@...ian.org>>
>>  >> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@...il.com
>> <mailto:prasannatsmkumar@...il.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

Powered by blists - more mailing lists