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
