diff mbox series

[4/6] nvmem: sunxi_sid: Read out data in native format

Message ID 20190318073354.12151-5-wens@kernel.org (mailing list archive)
State Mainlined, archived
Headers show
Series nvmem: sunxi_sid: native format and A83T/H5 support | expand

Commit Message

Chen-Yu Tsai March 18, 2019, 7:33 a.m. UTC
From: Chen-Yu Tsai <wens@csie.org>

Originally the SID e-fuses were thought to be in big-endian format.
Later sources show that they are in fact native or little-endian.
The most compelling evidence is the thermal sensor calibration data,
which is a set of one to three 16-bit values. In native-endian they
are in 16-bit cells with increasing offsets, whereas with big-endian
they are in the wrong order, and a gap with no data will show if there
are one or three cells.

Switch to a native endian representation for the nvmem device. For the
H3, the register read-out method was already returning data in native
endian. This only affects the other SoCs.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/nvmem/sunxi_sid.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

Comments

Maxime Ripard March 18, 2019, 8:42 a.m. UTC | #1
Hi,

On Mon, Mar 18, 2019 at 03:33:52PM +0800, Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai <wens@csie.org>
>
> Originally the SID e-fuses were thought to be in big-endian format.
> Later sources show that they are in fact native or little-endian.
> The most compelling evidence is the thermal sensor calibration data,
> which is a set of one to three 16-bit values. In native-endian they
> are in 16-bit cells with increasing offsets, whereas with big-endian
> they are in the wrong order, and a gap with no data will show if there
> are one or three cells.
>
> Switch to a native endian representation for the nvmem device. For the
> H3, the register read-out method was already returning data in native
> endian. This only affects the other SoCs.
>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

I thought only the newer SoCs were impacted by this issue?

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Chen-Yu Tsai March 18, 2019, 8:45 a.m. UTC | #2
On Mon, Mar 18, 2019 at 4:42 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> Hi,
>
> On Mon, Mar 18, 2019 at 03:33:52PM +0800, Chen-Yu Tsai wrote:
> > From: Chen-Yu Tsai <wens@csie.org>
> >
> > Originally the SID e-fuses were thought to be in big-endian format.
> > Later sources show that they are in fact native or little-endian.
> > The most compelling evidence is the thermal sensor calibration data,
> > which is a set of one to three 16-bit values. In native-endian they
> > are in 16-bit cells with increasing offsets, whereas with big-endian
> > they are in the wrong order, and a gap with no data will show if there
> > are one or three cells.
> >
> > Switch to a native endian representation for the nvmem device. For the
> > H3, the register read-out method was already returning data in native
> > endian. This only affects the other SoCs.
> >
> > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>
> I thought only the newer SoCs were impacted by this issue?

It is noticable on the newer SoCs. The old ones only have the 128-bit SID,
which could be read either way, as AFAIK it's just a serial number.

If you think we should leave the old ones alone I can factor that in.

ChenYu
Maxime Ripard March 18, 2019, 8:57 a.m. UTC | #3
On Mon, Mar 18, 2019 at 04:45:19PM +0800, Chen-Yu Tsai wrote:
> On Mon, Mar 18, 2019 at 4:42 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > Hi,
> >
> > On Mon, Mar 18, 2019 at 03:33:52PM +0800, Chen-Yu Tsai wrote:
> > > From: Chen-Yu Tsai <wens@csie.org>
> > >
> > > Originally the SID e-fuses were thought to be in big-endian format.
> > > Later sources show that they are in fact native or little-endian.
> > > The most compelling evidence is the thermal sensor calibration data,
> > > which is a set of one to three 16-bit values. In native-endian they
> > > are in 16-bit cells with increasing offsets, whereas with big-endian
> > > they are in the wrong order, and a gap with no data will show if there
> > > are one or three cells.
> > >
> > > Switch to a native endian representation for the nvmem device. For the
> > > H3, the register read-out method was already returning data in native
> > > endian. This only affects the other SoCs.
> > >
> > > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >
> > I thought only the newer SoCs were impacted by this issue?
>
> It is noticable on the newer SoCs. The old ones only have the 128-bit SID,
> which could be read either way, as AFAIK it's just a serial number.
>
> If you think we should leave the old ones alone I can factor that in.

IIRC, there was also the SoC ID in the SID on those SoCs as well,
which we might have to use in the future so we'll want to make sure it
is correct.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Chen-Yu Tsai March 18, 2019, 9:09 a.m. UTC | #4
On Mon, Mar 18, 2019 at 4:57 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Mon, Mar 18, 2019 at 04:45:19PM +0800, Chen-Yu Tsai wrote:
> > On Mon, Mar 18, 2019 at 4:42 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Mar 18, 2019 at 03:33:52PM +0800, Chen-Yu Tsai wrote:
> > > > From: Chen-Yu Tsai <wens@csie.org>
> > > >
> > > > Originally the SID e-fuses were thought to be in big-endian format.
> > > > Later sources show that they are in fact native or little-endian.
> > > > The most compelling evidence is the thermal sensor calibration data,
> > > > which is a set of one to three 16-bit values. In native-endian they
> > > > are in 16-bit cells with increasing offsets, whereas with big-endian
> > > > they are in the wrong order, and a gap with no data will show if there
> > > > are one or three cells.
> > > >
> > > > Switch to a native endian representation for the nvmem device. For the
> > > > H3, the register read-out method was already returning data in native
> > > > endian. This only affects the other SoCs.
> > > >
> > > > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > >
> > > I thought only the newer SoCs were impacted by this issue?
> >
> > It is noticable on the newer SoCs. The old ones only have the 128-bit SID,
> > which could be read either way, as AFAIK it's just a serial number.
> >
> > If you think we should leave the old ones alone I can factor that in.
>
> IIRC, there was also the SoC ID in the SID on those SoCs as well,
> which we might have to use in the future so we'll want to make sure it
> is correct.

We'll need to ask Allwinner about this then.

FWIW, the fel command in sunxi-tools reads them out in little endian. I
believe this and the SID page on the linux-sunxi wiki predate the sunxi_sid
driver.

ChenYu
Maxime Ripard March 18, 2019, 9:25 a.m. UTC | #5
On Mon, Mar 18, 2019 at 05:09:44PM +0800, Chen-Yu Tsai wrote:
> On Mon, Mar 18, 2019 at 4:57 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Mon, Mar 18, 2019 at 04:45:19PM +0800, Chen-Yu Tsai wrote:
> > > On Mon, Mar 18, 2019 at 4:42 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Mon, Mar 18, 2019 at 03:33:52PM +0800, Chen-Yu Tsai wrote:
> > > > > From: Chen-Yu Tsai <wens@csie.org>
> > > > >
> > > > > Originally the SID e-fuses were thought to be in big-endian format.
> > > > > Later sources show that they are in fact native or little-endian.
> > > > > The most compelling evidence is the thermal sensor calibration data,
> > > > > which is a set of one to three 16-bit values. In native-endian they
> > > > > are in 16-bit cells with increasing offsets, whereas with big-endian
> > > > > they are in the wrong order, and a gap with no data will show if there
> > > > > are one or three cells.
> > > > >
> > > > > Switch to a native endian representation for the nvmem device. For the
> > > > > H3, the register read-out method was already returning data in native
> > > > > endian. This only affects the other SoCs.
> > > > >
> > > > > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > > >
> > > > I thought only the newer SoCs were impacted by this issue?
> > >
> > > It is noticable on the newer SoCs. The old ones only have the 128-bit SID,
> > > which could be read either way, as AFAIK it's just a serial number.
> > >
> > > If you think we should leave the old ones alone I can factor that in.
> >
> > IIRC, there was also the SoC ID in the SID on those SoCs as well,
> > which we might have to use in the future so we'll want to make sure it
> > is correct.
>
> We'll need to ask Allwinner about this then.
>
> FWIW, the fel command in sunxi-tools reads them out in little endian. I
> believe this and the SID page on the linux-sunxi wiki predate the sunxi_sid
> driver.

Yeah, and the driver has a readl as well:
https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/arch/arm/plat-sunxi/soc-detect.c#L92

For the whole series,
Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
diff mbox series

Patch

diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
index 75c1f48cb3d0..14c114620ed6 100644
--- a/drivers/nvmem/sunxi_sid.c
+++ b/drivers/nvmem/sunxi_sid.c
@@ -46,33 +46,12 @@  struct sunxi_sid {
 	u32			value_offset;
 };
 
-/* We read the entire key, due to a 32 bit read alignment requirement. Since we
- * want to return the requested byte, this results in somewhat slower code and
- * uses 4 times more reads as needed but keeps code simpler. Since the SID is
- * only very rarely probed, this is not really an issue.
- */
-static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
-			      const unsigned int offset)
-{
-	u32 sid_key;
-
-	sid_key = ioread32be(sid->base + round_down(offset, 4));
-	sid_key >>= (offset % 4) * 8;
-
-	return sid_key; /* Only return the last byte */
-}
-
 static int sunxi_sid_read(void *context, unsigned int offset,
 			  void *val, size_t bytes)
 {
 	struct sunxi_sid *sid = context;
-	u8 *buf = val;
-
-	/* Offset the read operation to the real position of SID */
-	offset += sid->value_offset;
 
-	while (bytes--)
-		*buf++ = sunxi_sid_read_byte(sid, offset++);
+	memcpy_fromio(val, sid->base + sid->value_offset + offset, bytes);
 
 	return 0;
 }