MPC8349ea Random Device Generator driver

MPC8349ea Random Device Generator driver

[Philippe Lachenal]

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

Hello !

I've made a driver for the MPC8349ea Random Device Generator, and I 
therefore submit it to your impartial judgment.. ;)
thanks a lot !

Lachenal Philippe

_________________________________________________________________
Windows Live Spaces : créez votre blog à votre image ! 
http://www.windowslive.fr/spaces

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: talitos-rng.diff --]
[-- Type: text/x-patch; name="talitos-rng.diff", Size: 7049 bytes --]

diff --git a/arch/powerpc/platforms/83xx/mpc834x_itx.c b/arch/powerpc/platforms/83xx/mpc834x_itx.c
old mode 100644
new mode 100755
index 40a0194..b05980b
--- a/arch/powerpc/platforms/83xx/mpc834x_itx.c
+++ b/arch/powerpc/platforms/83xx/mpc834x_itx.c
@@ -48,6 +48,24 @@ unsigned long isa_mem_base = 0;
  * Setup the architecture
  *
  */
+
+static struct of_device_id mpc834x_itx[] = {
+	{ .type = "soc", },
+	{ .type ="crypto", },
+	{},
+};
+
+static int __init mpc834x_itx_declare_of_platform_devices(void)
+{
+	if (!machine_is(mpc834x_itx)){
+		printk("__init mpc834x_itx_declare_of_platform_devices error\n");
+		return 0;
+	}
+	of_platform_bus_probe(NULL, mpc834x_itx, NULL);
+	return 0;
+}
+device_initcall(mpc834x_itx_declare_of_platform_devices);
+
 static void __init mpc834x_itx_setup_arch(void)
 {
 #ifdef CONFIG_PCI
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 7cda04b..187164e 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -105,3 +105,9 @@ config HW_RANDOM_PASEMI
 
 	  If unsure, say Y.
 
+config HW_RANDOM_MPC834X
+	tristate "MPC 834x Random Number Generator support"
+	depends on HW_RANDOM && MPC834x
+	default HW_RANDOM
+	---help---
+		blah
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index c8b7300..4feb8ca 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
 obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
 obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
 obj-$(CONFIG_HW_RANDOM_PASEMI) += pasemi-rng.o
+obj-$(CONFIG_HW_RANDOM_MPC834X) += talitos-rng.o
--- a/drivers/char/hw_random/talitos-rng.c	2007-06-05 16:58:09.780048750 +0200
+++ b/drivers/char/hw_random/talitos-rng.c	2007-06-05 17:01:52.000000000 +0200
@@ -0,0 +1,223 @@
+/*
+ * Driver for mpc83xx Random Number Generator
+ *
+ * inspired by Pasemi RNG Driver and Talitos driver for OCF Linux
+ *
+ * Copyright (C) 2007 Philippe Lachenal, PowerLinux
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/hw_random.h>
+
+#include <asm/of_platform.h>
+#include <asm/io.h>
+
+#define TALITOS_RNGSR		0x028	/* RNG status register */
+#define TALITOS_RNGSR_HI	0x02c	/* RNG status register */
+#define TALITOS_RNGSR_HI_RD	0x1	/* RNG Reset done */
+#define TALITOS_RNGSR_HI_OFL	0xff0000/* number of dwords in RNG output FIFO*/
+#define TALITOS_RNGDSR		0x010	/* RNG data size register */
+#define TALITOS_RNGDSR_HI	0x014	/* RNG data size register */
+#define TALITOS_RNG_FIFO	0x800	/* RNG FIFO - pool of random numbers */
+#define TALITOS_RNGISR		0x030	/* RNG Interrupt status register */
+#define TALITOS_RNGISR_HI	0x034	/* RNG Interrupt status register */
+#define TALITOS_RNGRCR		0x018	/* RNG Reset control register */
+#define TALITOS_RNGRCR_HI	0x01c	/* RNG Reset control register */
+#define TALITOS_RNGRCR_HI_SR	0x1	/* RNG RNGRCR:Software Reset */
+
+#define TALITOS_RNGRCR_HI_MI	0x00000002
+
+#define TALITOS_HDR_DONE_BITS	0xff000000
+
+#define TALITOS_HAS_EU_RNG		(1<<4)
+#define TALITOS_HAS_EUS_SEC_2_01	0x7f
+
+
+
+
+
+
+static int talitos_hwrng_data_present(struct hwrng *rng)
+{
+	void __iomem *rng_regs = (void __iomem *)rng->priv;
+
+
+	/* check for things like FIFO underflow */
+	
+	u32 v;
+
+	v = in_be32(rng_regs + TALITOS_RNGISR_HI);
+	if (unlikely(v)) {
+		printk(KERN_ERR "talitos_hwrng_data_present : RNGISR_HI error %08x\n", v);
+		 return 0;
+	}
+
+	/*
+	 * OFL is number of available 64-bit words, 
+	 * shift and convert to a 32-bit word count
+	 */
+	v = in_be32(rng_regs + TALITOS_RNGSR_HI);
+
+	v = (v & TALITOS_RNGSR_HI_OFL) >> (16 - 1);
+
+	return v;
+}
+
+
+static int talitos_hwrng_data_read(struct hwrng *rng, u32 *data)
+{
+	void __iomem *rng_regs = (void __iomem *)rng->priv;
+
+	data = in_be32(rng_regs + TALITOS_RNG_FIFO);
+	data = in_be32(rng_regs + TALITOS_RNG_FIFO + sizeof(u32));
+	
+	return 4;
+}
+
+static int talitos_hwrng_init(struct hwrng *rng)
+{
+
+	void __iomem *rng_regs = (void __iomem *)rng->priv;
+	u32 v;
+
+	/* reset RNG EU */
+	setbits32(rng_regs + TALITOS_RNGRCR_HI, TALITOS_RNGRCR_HI_SR); 
+
+	while ((in_be32(rng_regs + TALITOS_RNGSR_HI) 
+		& TALITOS_RNGSR_HI_RD) == 0)
+			cpu_relax();
+	/*
+	 * we tell the RNG to start filling the RNG FIFO
+	 * by writing the RNGDSR 
+	 */
+	v = in_be32(rng_regs + TALITOS_RNGDSR_HI);
+	out_be32(rng_regs + TALITOS_RNGDSR_HI, v);
+	/*
+	 * 64 bits of data will be pushed onto the FIFO every 
+	 * 256 SEC cycles until the FIFO is full.  The RNG then 
+	 * attempts to keep the FIFO full.
+	 */
+	v = in_be32(rng_regs + TALITOS_RNGISR_HI);
+	if (v) {
+		printk(KERN_ERR "TALITOS INIT : RNGISR_HI error %08x\n", v);
+		return 1;
+	}
+	/*
+	 * n.b. we need to add a FIPS test here - if the RNG is going 
+	 * to fail, it's going to fail at reset time
+	 */
+
+	return 0;
+}
+
+static void talitos_hwrng_cleanup(struct hwrng *rng)
+{
+	void __iomem *rng_regs = (void __iomem *)rng->priv;
+	
+	/* reset RNG EU */
+	setbits32(rng_regs + TALITOS_RNGRCR_HI, TALITOS_RNGRCR_HI_SR); 
+
+}
+
+
+
+
+static struct hwrng talitos_rng = {
+	.name		= "talitos_rng",
+	.init		= talitos_hwrng_init,
+	.cleanup	= talitos_hwrng_cleanup,
+	.data_present	= talitos_hwrng_data_present,
+	.data_read	= talitos_hwrng_data_read,
+};
+
+
+
+
+
+
+
+
+static int __devinit
+talitos_probe(struct of_device *ofdev, const struct of_device_id *match)
+
+{	
+	void __iomem *rng_regs;
+	struct device_node *rng_np = ofdev->node;
+	struct resource res;
+	int err = 0;
+
+	err = of_address_to_resource(rng_np, 0, &res);
+	if (err)
+		return -ENODEV;	
+
+
+	rng_regs = ioremap(res.start, (res.end - res.start));
+
+	if (!rng_regs)
+		return -ENOMEM;
+
+	talitos_rng.priv = (unsigned long) rng_regs ;	
+
+	printk(KERN_INFO "Registering Talitos RNG\n");
+
+	err = hwrng_register(&talitos_rng);
+	if (err){
+		printk(".............. failure\n");
+		iounmap(rng_regs);
+	}
+	return err;
+}
+
+
+
+static int
+talitos_remove(struct of_device *op)
+{
+	void __iomem *rng_regs = (void __iomem *)talitos_rng.priv;
+
+	hwrng_unregister(&talitos_rng);
+	iounmap(rng_regs);
+	return 0;
+}
+
+
+
+
+
+static struct of_device_id talitos_of_match[] = {
+        { .compatible = "talitos-rng", },
+        {},
+};
+
+
+static struct of_platform_driver talitos_driver = {
+	.name		= "talitos-rng",
+        .match_table    = talitos_of_match,
+	.probe		= talitos_probe,
+	.remove		= talitos_remove,
+};
+
+
+
+
+static int __init talitos_init(void)
+{
+	return of_register_platform_driver(&talitos_driver);
+}
+module_init(talitos_init);
+
+static void __exit talitos_exit(void)
+{
+	of_unregister_platform_driver(&talitos_driver);
+}
+module_exit(talitos_exit);
+
+MODULE_DESCRIPTION(" Hardware CRYPTO driver");
+MODULE_AUTHOR("Philippe LACHENAL, PowerLinux");
+MODULE_LICENSE("GPL");

Re: MPC8349ea Random Device Generator driver

[Arnd Bergmann]

On Tuesday 05 June 2007, Philippe Lachenal wrote:
> I've made a driver for the MPC8349ea Random Device Generator, and I=20
> therefore submit it to your impartial judgment.. ;)

Looks good from the point of integration into the driver framework.
I can't judge the hwrng specific parts, but don't see anything
fundamentally wrong there.

> +static int __init mpc834x_itx_declare_of_platform_devices(void)
> +{
> +=A0=A0=A0=A0=A0=A0=A0if (!machine_is(mpc834x_itx)){
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0printk("__init mpc834x_itx_=
declare_of_platform_devices error\n");
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0return 0;
> +=A0=A0=A0=A0=A0=A0=A0}

I would think that it's not strictly an error to be running on some other
hardware than yours ;-)

Just remove the printk here.

> +=A0=A0=A0=A0=A0=A0=A0err =3D of_address_to_resource(rng_np, 0, &res);
> +=A0=A0=A0=A0=A0=A0=A0if (err)
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0return -ENODEV;=A0
> +=A0=A0=A0=A0=A0=A0=A0rng_regs =3D ioremap(res.start, (res.end - res.star=
t));

This can be done in one step with the new 'of_iomap' function.

> +=A0=A0=A0=A0=A0=A0=A0printk(KERN_INFO "Registering Talitos RNG\n");
> +
> +=A0=A0=A0=A0=A0=A0=A0err =3D hwrng_register(&talitos_rng);
> +=A0=A0=A0=A0=A0=A0=A0if (err){
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0printk(".............. fail=
ure\n");
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0iounmap(rng_regs);
> +=A0=A0=A0=A0=A0=A0=A0}

This printk should have a KERN_ERROR or similar level.
Ideally, replace all instances of printk in your driver with 'dev_err',
'dev_info' or similar calls from <linux/device.h>.

	Arnd <><

Re: MPC8349ea Random Number Generator driver

[Philippe Lachenal]

Hi !

Thank you for your answer :)


>Looks good from the point of integration into the driver framework.
>I can't judge the hwrng specific parts, but don't see anything
>fundamentally wrong there.
>
> > +static int __init mpc834x_itx_declare_of_platform_devices(void)
> > +{
> > +       if (!machine_is(mpc834x_itx)){
> > +               printk("__init mpc834x_itx_declare_of_platform_devices 
>error\n");
> > +               return 0;
> > +       }
>
>I would think that it's not strictly an error to be running on some other
>hardware than yours ;-)
>
>Just remove the printk here.

I would think that you're right.. ;)





> > +       err = of_address_to_resource(rng_np, 0, &res);
> > +       if (err)
> > +               return -ENODEV; > +       rng_regs = ioremap(res.start, 
>(res.end - res.start));
>
>This can be done in one step with the new 'of_iomap' function.


nice :)

... I've checked this function, (yours ? ;) ) and  there's a very small 
difference between it and what I've done :

+-static void __iomem *of_iomap(struct device_node *np)
+-{
+-	struct resource res;
+-
+-	if (of_address_to_resource(np, 0, &res))
+-		return NULL;
+-
+-	pr_debug("Resource start: 0x%lx\n", res.start);
+-	pr_debug("Resource end: 0x%lx\n", res.end);
+-
+-	return ioremap(res.start, 1 + res.end - res.start);
+-}


why is there a  1+ res.end-res.start ? Am I wrong in not adding one in my 
code ?








> > +       printk(KERN_INFO "Registering Talitos RNG\n");
> > +
> > +       err = hwrng_register(&talitos_rng);
> > +       if (err){
> > +               printk(".............. failure\n");
> > +               iounmap(rng_regs);
> > +       }
>
>This printk should have a KERN_ERROR or similar level.
>Ideally, replace all instances of printk in your driver with 'dev_err',
>'dev_info' or similar calls from <linux/device.h>.
>
>	Arnd <><


ok, I will remove those ugly printks ;)

Phil

_________________________________________________________________
Personnalisez votre Messenger avec Live.com 
http://www.windowslive.fr/livecom/

Re: MPC8349ea Random Number Generator driver

[Arnd Bergmann]

On Thursday 07 June 2007, Philippe Lachenal wrote:
> +-
> +-=A0=A0=A0=A0=A0=A0return ioremap(res.start, 1 + res.end - res.start);
> +-}
>=20
>=20
> why is there a =A01+ res.end-res.start ? Am I wrong in not adding one in =
my=20
> code ?
>=20

Yes. The res.start is the first byte in the area, res.end is the last one.
=46or example, in a two byte range, start could be 100, and end 101.
res.end - res.start would therefore be one, not two as ioremap expects.

	Arnd <><

Re: MPC8349ea Random Device Generator driver

[Timur Tabi]

Philippe Lachenal wrote:
> Hello !
> 
> I've made a driver for the MPC8349ea Random Device Generator, and I 
> therefore submit it to your impartial judgment.. ;)
> thanks a lot !

First, please don't post your patch as an attachment.  I recommend you use git-send-email.

> diff --git a/arch/powerpc/platforms/83xx/mpc834x_itx.c b/arch/powerpc/platforms/83xx/mpc834x_itx.c
> old mode 100644
> new mode 100755
> index 40a0194..b05980b
> --- a/arch/powerpc/platforms/83xx/mpc834x_itx.c
> +++ b/arch/powerpc/platforms/83xx/mpc834x_itx.c
> @@ -48,6 +48,24 @@ unsigned long isa_mem_base = 0;
>   * Setup the architecture
>   *
>   */
> +
> +static struct of_device_id mpc834x_itx[] = {
> +	{ .type = "soc", },
> +	{ .type ="crypto", },

You have a lot of minor spacing problems, like this one.  There should be a blank space 
before '"crypto"'.


> +
> +#include <asm/of_platform.h>
> +#include <asm/io.h>
> +
> +#define TALITOS_RNGSR		0x028	/* RNG status register */
> +#define TALITOS_RNGSR_HI	0x02c	/* RNG status register */
> +#define TALITOS_RNGSR_HI_RD	0x1	/* RNG Reset done */
> +#define TALITOS_RNGSR_HI_OFL	0xff0000/* number of dwords in RNG output FIFO*/
> +#define TALITOS_RNGDSR		0x010	/* RNG data size register */
> +#define TALITOS_RNGDSR_HI	0x014	/* RNG data size register */
> +#define TALITOS_RNG_FIFO	0x800	/* RNG FIFO - pool of random numbers */
> +#define TALITOS_RNGISR		0x030	/* RNG Interrupt status register */
> +#define TALITOS_RNGISR_HI	0x034	/* RNG Interrupt status register */
> +#define TALITOS_RNGRCR		0x018	/* RNG Reset control register */
> +#define TALITOS_RNGRCR_HI	0x01c	/* RNG Reset control register */

Please create a structure instead of using macros like this.  Example:

struct sec_rng {
	__be64	rngmr;
	u8 res1[8];
	__be64 rngdsr;
	__be64 rngrcr;
...
};

and then ...	

> +static int talitos_hwrng_data_present(struct hwrng *rng)
> +{
> +	void __iomem *rng_regs = (void __iomem *)rng->priv;

	struct sec_rng __iomem *rng = (struct sec_rng __iomem *) rng->priv;

> +
> +
> +	/* check for things like FIFO underflow */
> +	
> +	u32 v;
> +
> +	v = in_be32(rng_regs + TALITOS_RNGISR_HI);

	u64 v;
	v = rng->rngisr;

or something like that.  Try to use the built-in support for 64-bit data types when possible.

I get this confused easily, but I don't think you should be using the in/out_be macros 
either.  Just do regular reads and writes.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: MPC8349ea Random Device Generator driver

[Olof Johansson]

On Wed, Jun 06, 2007 at 04:57:08PM -0500, Timur Tabi wrote:

> > +#include <asm/of_platform.h>
> > +#include <asm/io.h>
> > +
> > +#define TALITOS_RNGSR		0x028	/* RNG status register */
> > +#define TALITOS_RNGSR_HI	0x02c	/* RNG status register */
> > +#define TALITOS_RNGSR_HI_RD	0x1	/* RNG Reset done */
> > +#define TALITOS_RNGSR_HI_OFL	0xff0000/* number of dwords in RNG output FIFO*/
> > +#define TALITOS_RNGDSR		0x010	/* RNG data size register */
> > +#define TALITOS_RNGDSR_HI	0x014	/* RNG data size register */
> > +#define TALITOS_RNG_FIFO	0x800	/* RNG FIFO - pool of random numbers */
> > +#define TALITOS_RNGISR		0x030	/* RNG Interrupt status register */
> > +#define TALITOS_RNGISR_HI	0x034	/* RNG Interrupt status register */
> > +#define TALITOS_RNGRCR		0x018	/* RNG Reset control register */
> > +#define TALITOS_RNGRCR_HI	0x01c	/* RNG Reset control register */
> 
> Please create a structure instead of using macros like this.  Example:
> 
> struct sec_rng {
> 	__be64	rngmr;
> 	u8 res1[8];
> 	__be64 rngdsr;
> 	__be64 rngrcr;
> ...
> };
> 
> and then ...	

There's nothing wrong with the way he coded that up. Lots of drivers
are written that way (all of mine are). It's at least as clear as any
structure, and it doesn't cause temptation to do...

> > +static int talitos_hwrng_data_present(struct hwrng *rng)
> > +{
> > +	void __iomem *rng_regs = (void __iomem *)rng->priv;
> 
> 	struct sec_rng __iomem *rng = (struct sec_rng __iomem *) rng->priv;
> 
> > +
> > +
> > +	/* check for things like FIFO underflow */
> > +	
> > +	u32 v;
> > +
> > +	v = in_be32(rng_regs + TALITOS_RNGISR_HI);
> 
> 	u64 v;
> 	v = rng->rngisr;
> 
> or something like that.  Try to use the built-in support for 64-bit data types when possible.

...this. NO! Don't reference ioremapped memory from regular code like
that. The way he's doing it is the preferred way.


-Olof

Re: MPC8349ea Random Device Generator driver

[Timur Tabi]

Olof Johansson wrote:

> There's nothing wrong with the way he coded that up. Lots of drivers
> are written that way (all of mine are). It's at least as clear as any
> structure, and it doesn't cause temptation to do...

That vast majority of Freescale SOC device register maps are handled via a structure. 
He's doing everything via 32-bit operations, even though the registers are 64 bits, and 
therefore he has twice as much macros as he needs.

>>> +
>>> +
>>> +	/* check for things like FIFO underflow */
>>> +	
>>> +	u32 v;
>>> +
>>> +	v = in_be32(rng_regs + TALITOS_RNGISR_HI);
>> 	u64 v;
>> 	v = rng->rngisr;
>>
>> or something like that.  Try to use the built-in support for 64-bit data types when possible.
> 
> ...this. NO! Don't reference ioremapped memory from regular code like
> that. The way he's doing it is the preferred way.

Can you explain that better?  What is "regular code"?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: MPC8349ea Random Device Generator driver

[Arnd Bergmann]

On Thursday 07 June 2007, Timur Tabi wrote:
> >> =A0=A0=A0=A0=A0u64 v;
> >> =A0=A0=A0=A0=A0v =3D rng->rngisr;
> >>
> >> or something like that. =A0Try to use the built-in support for 64-bit =
data types when possible.
> >=20
> > ...this. NO! Don't reference ioremapped memory from regular code like
> > that. The way he's doing it is the preferred way.
>=20
> Can you explain that better? =A0What is "regular code"?

The only code that is allowed to dereference an __iomem pointer is the
implementation of readl/writel and similar functions.
The code you gave as an example gives you a warning when building with
make C=3D1 using sparse, and missed barriers.

	Arnd <><

Re: MPC8349ea Random Device Generator driver

[Timur Tabi]

Arnd Bergmann wrote:

> The only code that is allowed to dereference an __iomem pointer is the
> implementation of readl/writel and similar functions.
> The code you gave as an example gives you a warning when building with
> make C=1 using sparse, and missed barriers.

Well, that's why I said "or something like that".

Anyway, there's a more fundamental problem.  I can't find any 64-bit versions of the 
in_be/out_be/read/write functions.  Because the registers are big-endian, then we have a 
few choices:

1) Create 64-bit versions of these functions.  Can someone tell me why they don't exist 
already?

2) Do something stupid like u32 v = in_be32(((void *) &rng->rngisr) + 4);

3) Change the struct like this:

struct sec_rng {
	__be32	rngmr_hi;
	__be32	rngmr_lo;
	u8 res1[8];
	__be32 rngdsr_hi;
	__be32 rngdsr_lo;
	__be32 rngrcr_hi;
	__be32 rngrcr_lo;
...

Comments?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: MPC8349ea Random Device Generator driver

[Arnd Bergmann]

On Thursday 07 June 2007, Timur Tabi wrote:
> Anyway, there's a more fundamental problem. =A0I can't find any 64-bit ve=
rsions of the=20
> in_be/out_be/read/write functions. =A0Because the registers are big-endia=
n, then we have a=20
> few choices:

> 1) Create 64-bit versions of these functions.  Can someone tell me why th=
ey don't exist=20
> already?

There are there on my system, see in_be64/out_be64/readq/writeq. note that =
the code
for those is created by macros, so they don't show up in ctags.

	Arnd <><

Re: MPC8349ea Random Device Generator driver

[Timur Tabi]

Arnd Bergmann wrote:

> There are there on my system, see in_be64/out_be64/readq/writeq. note that the code
> for those is created by macros, so they don't show up in ctags.

Ah, that explains it.  In that case, I think the code should be changed to reference 
everything as 64-bit registers and ints.


-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: MPC8349ea Random Device Generator driver

[Olof Johansson]

On Wed, Jun 06, 2007 at 05:07:05PM -0500, Timur Tabi wrote:
> Olof Johansson wrote:
> 
> >There's nothing wrong with the way he coded that up. Lots of drivers
> >are written that way (all of mine are). It's at least as clear as any
> >structure, and it doesn't cause temptation to do...
> 
> That vast majority of Freescale SOC device register maps are handled via a 
> structure. He's doing everything via 32-bit operations, even though the 
> registers are 64 bits, and therefore he has twice as much macros as he 
> needs.

I see only one double access back to back, not a big deal. I do however
fail to see how a couple of constructs are working, separate email with
those questions later though.

> >>>+
> >>>+
> >>>+	/* check for things like FIFO underflow */
> >>>+	
> >>>+	u32 v;
> >>>+
> >>>+	v = in_be32(rng_regs + TALITOS_RNGISR_HI);
> >>	u64 v;
> >>	v = rng->rngisr;
> >>
> >>or something like that.  Try to use the built-in support for 64-bit data 
> >>types when possible.
> >
> >...this. NO! Don't reference ioremapped memory from regular code like
> >that. The way he's doing it is the preferred way.
> 
> Can you explain that better?  What is "regular code"?

>From http://kerneltrap.org/node/3848:

---
In the byte-order case, what we have is:

	typedef __u16 __bitwise __le16;
	typedef __u16 __bitwise __be16;
	typedef __u32 __bitwise __le32;
	typedef __u32 __bitwise __be32;
	typedef __u64 __bitwise __le64;
	typedef __u64 __bitwise __be64;

and if you think about the above rules about what is acceptable for 
bitwise types, you'll likely immediately notivce that it automatically 
means

 - you can never assign a __le16 type to any other integer type or any 
   other bitwise type. You'd get a warnign about incompatible types. Makes 
   sense, no?
...
---

Note that rngisr is __be64.  Sparse should complain loudly over your code.


-Olof

Re: MPC8349ea Random Device Generator driver

[Timur Tabi]

Olof Johansson wrote:

>  - you can never assign a __le16 type to any other integer type or any 
>    other bitwise type. You'd get a warnign about incompatible types. Makes 
>    sense, no?

Then why do the in_be functions return an unsigned int instead of a __be type?  Isn't that 
effectively removing the endian-ness from the type?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: MPC8349ea Random Device Generator driver

[Olof Johansson]

On Wed, Jun 06, 2007 at 05:32:19PM -0500, Timur Tabi wrote:
> Olof Johansson wrote:
> 
> >  - you can never assign a __le16 type to any other integer type or any 
> >    other bitwise type. You'd get a warnign about incompatible types. Makes 
> >    sense, no?
> 
> Then why do the in_be functions return an unsigned int instead of a __be type?  Isn't that 
> effectively removing the endian-ness from the type?

Because they read a big endian register and returns the contents in the
native byte order for the machine.

-Olof

Re: MPC8349ea Random Device Generator driver

[Timur Tabi]

Olof Johansson wrote:
> On Wed, Jun 06, 2007 at 05:32:19PM -0500, Timur Tabi wrote:
>> Olof Johansson wrote:
>>
>>>  - you can never assign a __le16 type to any other integer type or any 
>>>    other bitwise type. You'd get a warnign about incompatible types. Makes 
>>>    sense, no?
>> Then why do the in_be functions return an unsigned int instead of a __be type?  Isn't that 
>> effectively removing the endian-ness from the type?
> 
> Because they read a big endian register and returns the contents in the
> native byte order for the machine.

In other words, the in_le16() function exists so that we can "assign a __le16 type to any 
other integer type or any other bitwise type."

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: MPC8349ea Random Device Generator driver

[Olof Johansson]

On Thu, Jun 07, 2007 at 09:23:35AM -0500, Timur Tabi wrote:
> Olof Johansson wrote:
> >On Wed, Jun 06, 2007 at 05:32:19PM -0500, Timur Tabi wrote:
> >>Olof Johansson wrote:
> >>
> >>> - you can never assign a __le16 type to any other integer type or any 
> >>>   other bitwise type. You'd get a warnign about incompatible types. 
> >>>   Makes sense, no?
> >>Then why do the in_be functions return an unsigned int instead of a __be 
> >>type?  Isn't that effectively removing the endian-ness from the type?
> >
> >Because they read a big endian register and returns the contents in the
> >native byte order for the machine.
> 
> In other words, the in_le16() function exists so that we can "assign a 
> __le16 type to any other integer type or any other bitwise type."

No. in_le16 exists so you can read a little-endian 16 bit register on
a device, and store it to a regular 16-bit integer variable. It
has nothing to do with assignment.

__le16 is just a type with hints for the programmer: "Hey, this is a
16-bit variable, but we're storing it in little-endian format. So if you
need to use it as a native data type, you need to swap it by hand first",
and sparse is able to help you find places where you didn't.

If you use the accessors to read/write hardware (as you are supposed to
do), then you don't need special types in your structs, just make sure
you use the right accessors.

-Olof

Re: MPC8349ea Random Device Generator driver

[Timur Tabi]

Olof Johansson wrote:

> No. in_le16 exists so you can read a little-endian 16 bit register on
> a device, and store it to a regular 16-bit integer variable. It
> has nothing to do with assignment.

What?  Reading from a register and storing the value into a variable *is* assignment.  If 
I have code like this:

	A = B;

I'm assigning B to A.

For the record, I understand the concepts you're talking about, I'm only confused about 
about some of things I'm reading.

> __le16 is just a type with hints for the programmer: "Hey, this is a
> 16-bit variable, but we're storing it in little-endian format. So if you
> need to use it as a native data type, you need to swap it by hand first",
> and sparse is able to help you find places where you didn't.

I understand that.

> If you use the accessors to read/write hardware (as you are supposed to
> do), then you don't need special types in your structs, just make sure
> you use the right accessors.

Ok, I understand that, too.  I just don't understand absolutist statements like "you can 
never assign a __le16 type to any other integer type" when our code is full of example of 
doing just that.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: MPC8349ea Random Device Generator driver

[Olof Johansson]

On Thu, Jun 07, 2007 at 10:20:19AM -0500, Timur Tabi wrote:

> Ok, I understand that, too.  I just don't understand absolutist statements like "you can 
> never assign a __le16 type to any other integer type" when our code is full of example of 
> doing just that.

Because you're doing it wrong, and your code is broken. Try running
it through sparse and watch the spew. I get 68 sparse warnings on
arch/powerpc/sysdev/*

You have some work to do!


-Olof

Re: MPC8349ea Random Device Generator driver

[Timur Tabi]

Olof Johansson wrote:

> 	typedef __u16 __bitwise __le16;
> 	typedef __u16 __bitwise __be16;
> 	typedef __u32 __bitwise __le32;
> 	typedef __u32 __bitwise __be32;
> 	typedef __u64 __bitwise __le64;
> 	typedef __u64 __bitwise __be64;

Wait a minute - why are all the endian-specific types __bitwise?

What if I have a 32-bit MMIO register that just contains a number, but it has to be 
written in big-endian?  I can't use __be32 to designate it.

For instance, I was going to do this for a new memory-mapped device I'm working with:

struct ccsr_dma {
	u8 res0[0x100];
	struct {
		__be32 mr;	/* x00 Mode register */
		__be32 sr;	/* x04 Status register */
		__be32 eclndar; /* x08 Current link descriptor extended address register */
		__be32 clndar;	/* x0C Current link descriptor address register */
		__be32 satr;	/* x10 Source attributes register */
		__be32 sar;	/* x14 Source address register */
...
		u8 res2[0x38];
	} channel[4];
}

The SAR register is defined as:

Bits	Name	Description
0–31	SAD	Source address. This register contains the low-order bits of the 36-bit source 
address of the DMA transfer.  The contents are updated after every DMA write operation 
unless the final stride of a striding operation is less than the stride size, in which 
case it remains equal to the address from which the last stride began.

In other words, the SAR register is just one number, but it must be written as a single 
big-endian number.  So is __be32 the wrong type?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: MPC8349ea Random Device Generator driver

[Olof Johansson]

On Wed, Jun 06, 2007 at 05:48:03PM -0500, Timur Tabi wrote:
> Olof Johansson wrote:
> 
> > 	typedef __u16 __bitwise __le16;
> > 	typedef __u16 __bitwise __be16;
> > 	typedef __u32 __bitwise __le32;
> > 	typedef __u32 __bitwise __be32;
> > 	typedef __u64 __bitwise __le64;
> > 	typedef __u64 __bitwise __be64;
> 
> Wait a minute - why are all the endian-specific types __bitwise?
> 
> What if I have a 32-bit MMIO register that just contains a number, but it has to be 
> written in big-endian?  I can't use __be32 to designate it.
> 
> For instance, I was going to do this for a new memory-mapped device I'm working with:
> 
> struct ccsr_dma {
> 	u8 res0[0x100];
> 	struct {
> 		__be32 mr;	/* x00 Mode register */
> 		__be32 sr;	/* x04 Status register */
> 		__be32 eclndar; /* x08 Current link descriptor extended address register */
> 		__be32 clndar;	/* x0C Current link descriptor address register */
> 		__be32 satr;	/* x10 Source attributes register */
> 		__be32 sar;	/* x14 Source address register */
> ...
> 		u8 res2[0x38];
> 	} channel[4];
> }
> 
> The SAR register is defined as:
> 
> Bits	Name	Description
> 0?31	SAD	Source address. This register contains the low-order bits of the 36-bit source 
> address of the DMA transfer.  The contents are updated after every DMA write operation 
> unless the final stride of a striding operation is less than the stride size, in which 
> case it remains equal to the address from which the last stride began.
> 
> In other words, the SAR register is just one number, but it must be written as a single 
> big-endian number.  So is __be32 the wrong type?

It doesn't matter a whole lot what the type is, but you should only ever
read and write registers with the MMIO ops, never directly as variable
references. You should never ever dereference an __iomem pointer (which
is what you get back from ioremap and friends)[1].

If you use in_be32/out_be32() on it, you'll be fine. It will use
appropriate barriers, and it will read and write the whole word at once.

Look at it as if anything ioremap():ed is in a different address space,
and there's only a few valid ways to access that address space. Direct
derefs in code is not one of them.

If in doubt, run sparse on your code (make C=1).


-Olof

[1] there are a few, and very very few exceptions.

Re: MPC8349ea Random Device Generator driver

[Kim Phillips]

On Tue, 05 Jun 2007 17:42:08 +0200
"Philippe Lachenal" <philippe.lachenal@hotmail.fr> wrote:

> Hello !
> 
> I've made a driver for the MPC8349ea Random Device Generator, and I 
> therefore submit it to your impartial judgment.. ;)
> thanks a lot !
> 
> Lachenal Philippe
> 

the Random Device Generator, eh?  didn't know we had one of those. ;)

The RNG h/w block in question shares registers, initialization and
configuration with what's known as the Freescale SEC device (a.k.a.
talitos).  Talitos does crypto and public key ops, and some
implementations don't have an RNG EU.  There needs to be one driver for
the SEC, and it's not an rng driver.

Kim