Re: [PATCH v3 1/3] nvmem: sunxi-sid: read NVMEM size from device compatible

Re: [PATCH v3 1/3] nvmem: sunxi-sid: read NVMEM size from device compatible

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]

On Thu, Feb 02, 2017 at 09:13:36PM +0800, Icenowy Zheng wrote:
> Sometimes the SID device have more memory address space than the real
> NVMEM size (for the registers used to read/write the SID).
> 
> Fetch the NVMEM size from device compatible, rather than the memory
> address space's length, in order to prepare for adding some
> registers-based read support.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4415 bytes --]

On Thu, Feb 02, 2017 at 09:13:37PM +0800, Icenowy Zheng wrote:
> The H3 SoC have a bigger SID controller, which has its direct read
> address at 0x200 position in the SID block, not 0x0.
> 
> Also, H3 SID controller has some silicon bug that makes the direct read
> value wrong at cold boot, add code to workaround the bug. (This bug has
> already been fixed on A64 and later SoCs)
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> ---
> This patch is the part of [PATCH v2 1/1] that adds support for H3 SID
> controller.
> 
>  .../bindings/nvmem/allwinner,sunxi-sid.txt         | 12 +++-
>  drivers/nvmem/sunxi_sid.c                          | 72 +++++++++++++++++++++-
>  2 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> index d543ed3f5363..9ab9e75a6351 100644
> --- a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> +++ b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> @@ -1,7 +1,11 @@
>  Allwinner sunxi-sid
>  
>  Required properties:
> -- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
> +- compatible: Should be one of the following (depending on your SoC):
> +  "allwinner,sun4i-a10-sid"
> +  "allwinner,sun7i-a20-sid"
> +  "allwinner,sun8i-h3-sid"
> +
>  - reg: Should contain registers location and length
>  
>  = Data cells =
> @@ -19,3 +23,9 @@ Example for sun7i:
>  		compatible = "allwinner,sun7i-a20-sid";
>  		reg = <0x01c23800 0x200>
>  	};
> +
> +Example for sun8i-h3:
> +	sid@01c14000 {
> +		compatible = "allwinner,sun8i-h3-sid";
> +		reg = <0x01c14000 0x400>;
> +	};
> diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
> index 69524b67007f..476a161ff23a 100644
> --- a/drivers/nvmem/sunxi_sid.c
> +++ b/drivers/nvmem/sunxi_sid.c
> @@ -25,6 +25,16 @@
>  #include <linux/slab.h>
>  #include <linux/random.h>
>  
> +/* Registers and special values for doing register-based SID readout on H3 */
> +#define SUN8I_SID_PRCTL		0x40
> +#define SUN8I_SID_RDKEY		0x60
> +
> +#define SUN8I_SID_OP_LOCK	0xAC
> +#define SUN8I_SID_OFFSET_MASK	0x1FF
> +#define SUN8I_SID_OFFSET_SHIFT	16
> +#define SUN8I_SID_LOCK_SHIFT	8
> +#define SUN8I_SID_READ		BIT(1)
> +
>  static struct nvmem_config econfig = {
>  	.name = "sunxi-sid",
>  	.read_only = true,
> @@ -34,11 +44,14 @@ static struct nvmem_config econfig = {
>  };
>  
>  struct sunxi_sid_cfg {
> +	u32	value_offset;
>  	u32	size;
> +	bool	need_register_readout;
>  };
>  
>  struct sunxi_sid {
>  	void __iomem		*base;
> +	u32			value_offset;
>  };
>  
>  /* We read the entire key, due to a 32 bit read alignment requirement. Since we
> @@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
>  {
>  	u32 sid_key;
>  
> -	sid_key = ioread32be(sid->base + round_down(offset, 4));
> +	sid_key = ioread32be(sid->base + sid->value_offset +
> +			     round_down(offset, 4));

This would probably be more logical to have this in sunxi_sid_read.

>  	sid_key >>= (offset % 4) * 8;
>  
>  	return sid_key; /* Only return the last byte */
> @@ -69,6 +83,33 @@ static int sunxi_sid_read(void *context, unsigned int offset,
>  	return 0;
>  }
>  
> +static int sun8i_sid_register_readout(const struct sunxi_sid *sid,
> +				      const unsigned int word,
> +				      u32 *out)
> +{
> +	u32 reg_val;
> +	unsigned long expire = jiffies + msecs_to_jiffies(250);
> +
> +	/* Set word, lock access, and set read command */
> +	reg_val = (word & SUN8I_SID_OFFSET_MASK)
> +		  << SUN8I_SID_OFFSET_SHIFT;
> +	reg_val |= SUN8I_SID_OP_LOCK << SUN8I_SID_LOCK_SHIFT;

You're not using those mask and shifts anywhere else, why not just
define the value / macro you need directly?

> +	reg_val |= SUN8I_SID_READ;
> +	writel(reg_val, sid->base + SUN8I_SID_PRCTL);
> +
> +	do {
> +		reg_val = readl(sid->base + SUN8I_SID_PRCTL);
> +	} while (time_before(jiffies, expire) && (reg_val & SUN8I_SID_READ));

readl_poll_timeout?

> +	if (reg_val & SUN8I_SID_READ)
> +		return -EIO;
> +
> +	if (out)
> +		*out = readl(sid->base + SUN8I_SID_RDKEY);

Why do you need that out parameter?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4303 bytes --]

On Mon, Feb 06, 2017 at 04:56:55PM +0800, Icenowy Zheng wrote:
> 06.02.2017, 16:54, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> > On Thu, Feb 02, 2017 at 09:13:37PM +0800, Icenowy Zheng wrote:
> >>  The H3 SoC have a bigger SID controller, which has its direct read
> >>  address at 0x200 position in the SID block, not 0x0.
> >>
> >>  Also, H3 SID controller has some silicon bug that makes the direct read
> >>  value wrong at cold boot, add code to workaround the bug. (This bug has
> >>  already been fixed on A64 and later SoCs)
> >>
> >>  Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> >>  ---
> >>  This patch is the part of [PATCH v2 1/1] that adds support for H3 SID
> >>  controller.
> >>
> >>   .../bindings/nvmem/allwinner,sunxi-sid.txt | 12 +++-
> >>   drivers/nvmem/sunxi_sid.c | 72 +++++++++++++++++++++-
> >>   2 files changed, 82 insertions(+), 2 deletions(-)
> >>
> >>  diff --git a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> >>  index d543ed3f5363..9ab9e75a6351 100644
> >>  --- a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> >>  +++ b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> >>  @@ -1,7 +1,11 @@
> >>   Allwinner sunxi-sid
> >>
> >>   Required properties:
> >>  -- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
> >>  +- compatible: Should be one of the following (depending on your SoC):
> >>  + "allwinner,sun4i-a10-sid"
> >>  + "allwinner,sun7i-a20-sid"
> >>  + "allwinner,sun8i-h3-sid"
> >>  +
> >>   - reg: Should contain registers location and length
> >>
> >>   = Data cells =
> >>  @@ -19,3 +23,9 @@ Example for sun7i:
> >>                   compatible = "allwinner,sun7i-a20-sid";
> >>                   reg = <0x01c23800 0x200>
> >>           };
> >>  +
> >>  +Example for sun8i-h3:
> >>  + sid@01c14000 {
> >>  + compatible = "allwinner,sun8i-h3-sid";
> >>  + reg = <0x01c14000 0x400>;
> >>  + };
> >>  diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
> >>  index 69524b67007f..476a161ff23a 100644
> >>  --- a/drivers/nvmem/sunxi_sid.c
> >>  +++ b/drivers/nvmem/sunxi_sid.c
> >>  @@ -25,6 +25,16 @@
> >>   #include <linux/slab.h>
> >>   #include <linux/random.h>
> >>
> >>  +/* Registers and special values for doing register-based SID readout on H3 */
> >>  +#define SUN8I_SID_PRCTL 0x40
> >>  +#define SUN8I_SID_RDKEY 0x60
> >>  +
> >>  +#define SUN8I_SID_OP_LOCK 0xAC
> >>  +#define SUN8I_SID_OFFSET_MASK 0x1FF
> >>  +#define SUN8I_SID_OFFSET_SHIFT 16
> >>  +#define SUN8I_SID_LOCK_SHIFT 8
> >>  +#define SUN8I_SID_READ BIT(1)
> >>  +
> >>   static struct nvmem_config econfig = {
> >>           .name = "sunxi-sid",
> >>           .read_only = true,
> >>  @@ -34,11 +44,14 @@ static struct nvmem_config econfig = {
> >>   };
> >>
> >>   struct sunxi_sid_cfg {
> >>  + u32 value_offset;
> >>           u32 size;
> >>  + bool need_register_readout;
> >>   };
> >>
> >>   struct sunxi_sid {
> >>           void __iomem *base;
> >>  + u32 value_offset;
> >>   };
> >>
> >>   /* We read the entire key, due to a 32 bit read alignment requirement. Since we
> >>  @@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
> >>   {
> >>           u32 sid_key;
> >>
> >>  - sid_key = ioread32be(sid->base + round_down(offset, 4));
> >>  + sid_key = ioread32be(sid->base + sid->value_offset +
> >>  + round_down(offset, 4));
> >
> > This would probably be more logical to have this in sunxi_sid_read.
> 
> But it's here which really access the memory...

This function is made to read a single register. What you want is to
offset all reads, and all the reads are made in sunxi_sid_read.

> >>  + if (reg_val & SUN8I_SID_READ)
> >>  + return -EIO;
> >>  +
> >>  + if (out)
> >>  + *out = readl(sid->base + SUN8I_SID_RDKEY);
> >
> > Why do you need that out parameter?
> 
> The read operation by registers can really return a value --
> in fact, the fix to the pre-read value is a side effect.

Yet, you're not using it at all, so this is dead code.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]

On Tue, Feb 07, 2017 at 09:36:35PM +0800, Icenowy Zheng wrote:
> >>  >>  @@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
> >>  >>   {
> >>  >>           u32 sid_key;
> >>  >>
> >>  >>  - sid_key = ioread32be(sid->base + round_down(offset, 4));
> >>  >>  + sid_key = ioread32be(sid->base + sid->value_offset +
> >>  >>  + round_down(offset, 4));
> >>  >
> >>  > This would probably be more logical to have this in sunxi_sid_read.
> >>
> >>  But it's here which really access the memory...
> >
> > This function is made to read a single register. What you want is to
> > offset all reads, and all the reads are made in sunxi_sid_read.
> 
> I think the semantic of this function is to read out one byte from SID,
> not read out a single register from SID; the parameter passed into it is
> also a const struct *sunxi_sid, so I think make the offset here is right.

You need to offset *all* register reads, it makes much more sense to
do that offset in the function that reads all the registers, and not
just one.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]