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

[Marcin Nowakowski]

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.


>  >
>  >> + Â Â Â Â Â Â Â /* 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.

Marcin