| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 27 Apr 2011 12:09:47 +0530
From: "Subhasish Ghosh" <subhasish@...tralsolutions.com>
To: "Marc Kleine-Budde" <mkl@...gutronix.de>
Cc: <davinci-linux-open-source@...ux.davincidsp.com>,
<sachi@...tralsolutions.com>,
"Samuel Ortiz" <sameo@...ux.intel.com>, <nsekhar@...com>,
"open list" <linux-kernel@...r.kernel.org>, <m-watkins@...com>,
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v4 01/11] mfd: add pruss mfd driver.
Hi Mark,
- Is it ok to have u32 etc for __iomem cookie ?
> +
> +s32 pruss_disable(struct device *dev, u8 pruss_num)
make it a int function
SG -- Ok will do
> +
> + /* Reset PRU */
> + iowrite32(PRUCORE_CONTROL_RESETVAL,
> + &h_pruss->control);
> + spin_unlock(&pruss->lock);
> +
> + return 0;
make it a void function?
SG -- This should be int, in case of invalid pru num, we ret an error.
> +}
> +EXPORT_SYMBOL_GPL(pruss_disable);
> +
> +s32 pruss_enable(struct device *dev, u8 pruss_num)
int?
SG -- Ok will do
> +
> + if ((pruss_num != PRUCORE_0) && (pruss_num != PRUCORE_1))
> + return -EINVAL;
> +
> + h_pruss = &pruss_mmap->core[pruss_num];
> +
> + /* Reset PRU */
> + spin_lock(&pruss->lock);
> + iowrite32(PRUCORE_CONTROL_RESETVAL, &h_pruss->control);
no need to lock the ram reset below?
SG -- I don't think its required. We just reset the RAM during init and
since each pru can only be attached to only one device, the access
will be already serialized based upon the pru num.
> + /* Reset any garbage in the ram */
> + if (pruss_num == PRUCORE_0)
> + for (i = 0; i < PRUSS_PRU0_RAM_SZ; i++)
> + iowrite32(0x0, &pruss_mmap->dram0[i]);
> + else if (pruss_num == PRUCORE_1)
> + for (i = 0; i < PRUSS_PRU1_RAM_SZ; i++)
> + iowrite32(0x0, &pruss_mmap->dram1[i]);
if you make a array for these
+struct pruss_map {
+ u8 dram0[512];
+ u8 res1[7680];
+ u8 dram1[512];
+ u8 res2[7680];
..}
you don't need the if..else..
SG - The dram/iram is not contiguous, there is a reserved space in between,
how do I declare an array for it.
> +/* Load the specified PRU with code */
> +s32 pruss_load(struct device *dev, u8 pruss_num,
> + u32 *pruss_code, u32 code_size_in_words)
> +{
> + struct pruss_priv *pruss = dev_get_drvdata(dev->parent);
> + struct pruss_map __iomem *pruss_mmap = pruss->ioaddr;
> + u32 __iomem *pruss_iram;
> + u32 i;
> +
> + if (pruss_num == PRUCORE_0)
> + pruss_iram = (u32 __iomem *)&pruss_mmap->iram0;
> + else if (pruss_num == PRUCORE_1)
> + pruss_iram = (u32 __iomem *)&pruss_mmap->iram1;
> + else
same here
SG - same here.
> +s32 pruss_run(struct device *dev, u8 pruss_num)
int?
SG - Ok, Will do.
> + while (cnt--) {
> + temp_reg = ioread32(&h_pruss->control);
> + if (((temp_reg & PRUCORE_CONTROL_RUNSTATE_MASK) >>
> + PRUCORE_CONTROL_RUNSTATE_SHIFT) ==
> + PRUCORE_CONTROL_RUNSTATE_HALT)
> + break;
how long might this take? what about some delay, sleep, or reschedule?
SG - This does not take more than 10 to 20 counts,
> +s32 pruss_writeb(struct device *dev, u32 offset, u8 pdatatowrite)
> +{
> + struct pruss_priv *pruss = dev_get_drvdata(dev->parent);
> + void __iomem *paddresstowrite;
we usually don't use "p" variable names for pointers
SG - Ok, will remove.
> +s32 pruss_rmwb(struct device *dev, u32 offset, u8 mask, u8 val)
void function?
SG - Ok, will do.
> +
> +s32 pruss_readb(struct device *dev, u32 offset, u8 *pdatatoread)
void?
SG - Ok, will do.
> +{
> + struct pruss_priv *pruss = dev_get_drvdata(dev->parent);
> + void __iomem *paddresstoread;
> +
> + paddresstoread = pruss->ioaddr + offset ;
> + *pdatatoread = ioread8(paddresstoread);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(pruss_readb);
> +
> +s32 pruss_readb_multi(struct device *dev, u32 offset,
> + u8 *pdatatoread, u16 bytestoread)
viod?
SG - Ok, will do.
> +{
> + struct pruss_priv *pruss = dev_get_drvdata(dev->parent);
> + u8 __iomem *paddresstoread;
> + u16 i;
int?
SG - Ok, will do.
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(pruss_readb_multi);
> +
> +s32 pruss_writel(struct device *dev, u32 offset,
> + u32 pdatatowrite)
void?
SG - Ok, will do.
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(pruss_writel);
> +
> +s32 pruss_writel_multi(struct device *dev, u32 offset,
> + u32 *pdatatowrite, u16 wordstowrite)
void?
SG - Ok, will do.
> +{
> + struct pruss_priv *pruss = dev_get_drvdata(dev->parent);
> + u32 __iomem *paddresstowrite;
> + u16 i;
> +
> + paddresstowrite = pruss->ioaddr + offset;
> +
> + for (i = 0; i < wordstowrite; i++)
> + iowrite32(*pdatatowrite++, paddresstowrite++);
memcopy_to_iomem?
SG -- I did not understand, could you please elaborate.
> +
> +s32 pruss_rmwl(struct device *dev, u32 offset, u32 mask, u32 val)
void?
SG - Ok, will do.
> +}
> +EXPORT_SYMBOL_GPL(pruss_rmwl);
> +
> +s32 pruss_readl(struct device *dev, u32 offset, u32 *pdatatoread)
void? or return the read value
SG - Ok, will do.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists