Re: [PATCH v2 5/5] hwrng: amd: Rework of the amd768-hwrng driver

2016-08-26 Thread Jason Cooper 
Hi Corentin,

On Fri, Aug 26, 2016 at 10:38:02AM +0200, LABBE Corentin wrote:
> On Thu, Aug 25, 2016 at 02:56:38PM +, Jason Cooper wrote:
> > On Thu, Aug 25, 2016 at 02:16:35PM +0200, LABBE Corentin wrote:
> > > This patch convert the hwrng interface used by amd768-rng to its new API
> > > by replacing data_read()/data_present() by read().
> > > 
> > > Furthermore, Instead of having two global variable, it's better to use a
> > > private struct. This will permit to remove amd_pdev variable.
> > > 
> > > Finally, Instead of accessing hw directly via pmbase, it's better to
> > > access after ioport_map() via ioread32/iowrite32.
> > 
> > I was going to recommend a better $subject line, but now I see why it's
> > vague. :(  I would recommend breaking this patch up into three:
> > 
> >   hwrng: amd - Access hardware via ioread32/iowrite32
> >   hwrng: amd - Replace global variable with private struct
> >   hwrng: amd - Convert to new hwrng read() API
> > 
> 
> That was my first idea, but believed that it wasnt worth it.

When working with crypto/rng code, I'm a firm believer in moving
cautiously and deliberately. :-)

> Anyway I will do it.

Thanks!

thx,

Jason.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] hwrng: amd: Rework of the amd768-hwrng driver

2016-08-26 Thread LABBE Corentin 
On Thu, Aug 25, 2016 at 02:56:38PM +, Jason Cooper wrote:
> Hi Corentin,
> 
> On Thu, Aug 25, 2016 at 02:16:35PM +0200, LABBE Corentin wrote:
> > This patch convert the hwrng interface used by amd768-rng to its new API
> > by replacing data_read()/data_present() by read().
> > 
> > Furthermore, Instead of having two global variable, it's better to use a
> > private struct. This will permit to remove amd_pdev variable.
> > 
> > Finally, Instead of accessing hw directly via pmbase, it's better to
> > access after ioport_map() via ioread32/iowrite32.
> 
> I was going to recommend a better $subject line, but now I see why it's
> vague. :(  I would recommend breaking this patch up into three:
> 
>   hwrng: amd - Access hardware via ioread32/iowrite32
>   hwrng: amd - Replace global variable with private struct
>   hwrng: amd - Convert to new hwrng read() API
> 

That was my first idea, but believed that it wasnt worth it.

Anyway I will do it.

Regards
LABBE Corentin
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] hwrng: amd: Rework of the amd768-hwrng driver

2016-08-25 Thread Jason Cooper 
Hi Corentin,

On Thu, Aug 25, 2016 at 02:16:35PM +0200, LABBE Corentin wrote:
> This patch convert the hwrng interface used by amd768-rng to its new API
> by replacing data_read()/data_present() by read().
> 
> Furthermore, Instead of having two global variable, it's better to use a
> private struct. This will permit to remove amd_pdev variable.
> 
> Finally, Instead of accessing hw directly via pmbase, it's better to
> access after ioport_map() via ioread32/iowrite32.

I was going to recommend a better $subject line, but now I see why it's
vague. :(  I would recommend breaking this patch up into three:

  hwrng: amd - Access hardware via ioread32/iowrite32
  hwrng: amd - Replace global variable with private struct
  hwrng: amd - Convert to new hwrng read() API

thx,

Jason.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/5] hwrng: amd: Rework of the amd768-hwrng driver

2016-08-25 Thread LABBE Corentin 
This patch convert the hwrng interface used by amd768-rng to its new API
by replacing data_read()/data_present() by read().

Furthermore, Instead of having two global variable, it's better to use a
private struct. This will permit to remove amd_pdev variable.

Finally, Instead of accessing hw directly via pmbase, it's better to
access after ioport_map() via ioread32/iowrite32.

Signed-off-by: LABBE Corentin 
---
 drivers/char/hw_random/amd-rng.c | 151 +--
 1 file changed, 99 insertions(+), 52 deletions(-)

diff --git a/drivers/char/hw_random/amd-rng.c b/drivers/char/hw_random/amd-rng.c
index de82fe3..785bc3e 100644
--- a/drivers/char/hw_random/amd-rng.c
+++ b/drivers/char/hw_random/amd-rng.c
@@ -32,6 +32,14 @@
 
 #define DRV_NAME "AMD768-HWRNG"
 
+#define GEN_CFG_REG1   0x40
+#define GEN_CFG_REG2   0x41
+#define SMIO_SPACEPTR  0x58
+#define RNGDATA0x00
+#define RNGDONE0x04
+#define PMBASE_OFFSET  0xF0
+#define PMBASE_SIZE8
+
 /*
  * Data for PCI driver interface
  *
@@ -39,6 +47,7 @@
  * PCI ids via MODULE_DEVICE_TABLE.  We do not actually
  * register a pci_driver, because someone else might one day
  * want to register another driver on the same PCI id.
+ * Like i2c-amd756
  */
 static const struct pci_device_id pci_tbl[] = {
{ PCI_VDEVICE(AMD, 0x7443), 0, },
@@ -47,112 +56,150 @@ static const struct pci_device_id pci_tbl[] = {
 };
 MODULE_DEVICE_TABLE(pci, pci_tbl);
 
-static struct pci_dev *amd_pdev;
+struct amd768_priv {
+   void __iomem *iobase;
+   struct pci_dev *pcidev;
+   u32 pmbase;
+};
 
-static int amd_rng_data_present(struct hwrng *rng, int wait)
+static int amd_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
 {
-   u32 pmbase = (u32)rng->priv;
-   int data, i;
-
-   for (i = 0; i < 20; i++) {
-   data = !!(inl(pmbase + 0xF4) & 1);
-   if (data || !wait)
-   break;
-   udelay(10);
+   u32 *data = buf;
+   struct amd768_priv *priv = (struct amd768_priv *)rng->priv;
+   size_t read = 0;
+   /* We will wait at maximum one time per read */
+   int timeout = max / 4 + 1;
+
+   /*
+* RNG data is available when RNGDONE is set to 1
+* New random numbers are generated approximately 128 microseconds
+* after RNGDATA is read
+*/
+   while (read < max) {
+   if (ioread32(priv->iobase + RNGDONE) == 0) {
+   if (wait) {
+   /* Delay given by datasheet */
+   usleep_range(128, 196);
+   if (timeout-- == 0)
+   return read;
+   } else {
+   return 0;
+   }
+   } else {
+   *data = ioread32(priv->iobase + RNGDATA);
+   data++;
+   read += 4;
+   }
}
-   return data;
-}
-
-static int amd_rng_data_read(struct hwrng *rng, u32 *data)
-{
-   u32 pmbase = (u32)rng->priv;
 
-   *data = inl(pmbase + 0xF0);
-
-   return 4;
+   return read;
 }
 
 static int amd_rng_init(struct hwrng *rng)
 {
u8 rnen;
+   struct amd768_priv *priv = (struct amd768_priv *)rng->priv;
 
-   pci_read_config_byte(amd_pdev, 0x40, );
+   pci_read_config_byte(priv->pcidev, GEN_CFG_REG1, );
rnen |= BIT(7); /* RNG on */
-   pci_write_config_byte(amd_pdev, 0x40, rnen);
+   pci_write_config_byte(priv->pcidev, GEN_CFG_REG1, rnen);
 
-   pci_read_config_byte(amd_pdev, 0x41, );
+   pci_read_config_byte(priv->pcidev, GEN_CFG_REG2, );
rnen |= BIT(7); /* PMIO enable */
-   pci_write_config_byte(amd_pdev, 0x41, rnen);
-
+   pci_write_config_byte(priv->pcidev, GEN_CFG_REG2, rnen);
return 0;
 }
 
 static void amd_rng_cleanup(struct hwrng *rng)
 {
u8 rnen;
+   struct amd768_priv *priv = (struct amd768_priv *)rng->priv;
 
-   pci_read_config_byte(amd_pdev, 0x40, );
+   pci_read_config_byte(priv->pcidev, GEN_CFG_REG1, );
rnen &= ~BIT(7);/* RNG off */
-   pci_write_config_byte(amd_pdev, 0x40, rnen);
+   pci_write_config_byte(priv->pcidev, GEN_CFG_REG1, rnen);
 }
 
 static struct hwrng amd_rng = {
.name   = "amd",
+   .read   = amd_rng_read,
.init   = amd_rng_init,
.cleanup= amd_rng_cleanup,
-   .data_present   = amd_rng_data_present,
-   .data_read  = amd_rng_data_read,
 };
 
-static int __init mod_init(void)
+static int mod_init(void)
 {
-   int err = -ENODEV;
+   int err;
struct pci_dev *pdev = NULL;
const struct pci_device_id *ent;
u32 pmbase;
+   struct amd768_priv *priv;
 
for_each_pci_dev(pdev) {
ent = pci_match_id(pci_tbl, pdev);