| Message ID | 20171108004449.32730-7-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
Florian Fainelli <f.fainelli@gmail.com> writes: > The interrupt masking done for Northstart Plus and Northstar (BCM5301X) > is moved from being a function pointer mapped to of_device_id::data into > a proper part of the hwrng::init callback. While at it, we also make the > of_data be a proper structure indicating the platform specifics, since > the day we need to add a second type of platform information, we would > have to do that anyway. I still think we should just unconditionally mask off the interrupt regardless of platform if we're not going to use it in the driver and some platforms need it. Looks like a fine refactor, though: Reviewed-by: Eric Anholt <eric@anholt.net>
Hi Florian, > Florian Fainelli <f.fainelli@gmail.com> hat am 8. November 2017 um 01:44 geschrieben: > > > The interrupt masking done for Northstart Plus and Northstar (BCM5301X) > is moved from being a function pointer mapped to of_device_id::data into > a proper part of the hwrng::init callback. While at it, we also make the > of_data be a proper structure indicating the platform specifics, since > the day we need to add a second type of platform information, we would > have to do that anyway. in the lack of proper documentation for bcm2835 rng, is it possible that we should mask the interrupts for bcm2835 as well?
Stefan Wahren <stefan.wahren@i2se.com> writes: > Hi Florian, > >> Florian Fainelli <f.fainelli@gmail.com> hat am 8. November 2017 um 01:44 geschrieben: >> >> >> The interrupt masking done for Northstart Plus and Northstar (BCM5301X) >> is moved from being a function pointer mapped to of_device_id::data into >> a proper part of the hwrng::init callback. While at it, we also make the >> of_data be a proper structure indicating the platform specifics, since >> the day we need to add a second type of platform information, we would >> have to do that anyway. > > in the lack of proper documentation for bcm2835 rng, is it possible > that we should mask the interrupts for bcm2835 as well? I don't have the RTL nearby and the RNG block doesn't have the usual #defines for power-on-reset values, so I'm not sure. I don't see any use of the RNG by the firmware that should impact Linux -- a driver exists and it uses IRQs, but it shouldn't have been active, and even if it was then its teardown process masks the interrupt off again. However, masking it off ourselves should be harmless at worst.
diff --git a/drivers/char/hw_random/bcm2835-rng.c b/drivers/char/hw_random/bcm2835-rng.c index 67b9bd3be28d..ed20e0b6b7ae 100644 --- a/drivers/char/hw_random/bcm2835-rng.c +++ b/drivers/char/hw_random/bcm2835-rng.c @@ -32,18 +32,9 @@ struct bcm2835_rng_priv { struct hwrng rng; void __iomem *base; + bool mask_interrupts; }; -static void __init nsp_rng_init(void __iomem *base) -{ - u32 val; - - /* mask the interrupt */ - val = readl(base + RNG_INT_MASK); - val |= RNG_INT_OFF; - writel(val, base + RNG_INT_MASK); -} - static inline struct bcm2835_rng_priv *to_rng_priv(struct hwrng *rng) { return container_of(rng, struct bcm2835_rng_priv, rng); @@ -75,6 +66,14 @@ static int bcm2835_rng_read(struct hwrng *rng, void *buf, size_t max, static int bcm2835_rng_init(struct hwrng *rng) { struct bcm2835_rng_priv *priv = to_rng_priv(rng); + u32 val; + + if (priv->mask_interrupts) { + /* mask the interrupt */ + val = readl(priv->base + RNG_INT_MASK); + val |= RNG_INT_OFF; + writel(val, priv->base + RNG_INT_MASK); + } /* set warm-up count & enable */ __raw_writel(RNG_WARMUP_COUNT, priv->base + RNG_STATUS); @@ -91,18 +90,26 @@ static void bcm2835_rng_cleanup(struct hwrng *rng) __raw_writel(0, priv->base + RNG_CTRL); } +struct bcm2835_rng_of_data { + bool mask_interrupts; +}; + +static const struct bcm2835_rng_of_data nsp_rng_of_data = { + .mask_interrupts = true, +}; + static const struct of_device_id bcm2835_rng_of_match[] = { { .compatible = "brcm,bcm2835-rng"}, - { .compatible = "brcm,bcm-nsp-rng", .data = nsp_rng_init}, - { .compatible = "brcm,bcm5301x-rng", .data = nsp_rng_init}, + { .compatible = "brcm,bcm-nsp-rng", .data = &nsp_rng_of_data }, + { .compatible = "brcm,bcm5301x-rng", .data = &nsp_rng_of_data }, {}, }; static int bcm2835_rng_probe(struct platform_device *pdev) { + const struct bcm2835_rng_of_data *of_data; struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; - void (*rng_setup)(void __iomem *base); const struct of_device_id *rng_id; struct bcm2835_rng_priv *priv; struct resource *r; @@ -133,9 +140,9 @@ static int bcm2835_rng_probe(struct platform_device *pdev) return -EINVAL; /* Check for rng init function, execute it */ - rng_setup = rng_id->data; - if (rng_setup) - rng_setup(priv->base); + of_data = rng_id->data; + if (of_data) + priv->mask_interrupts = of_data->mask_interrupts; /* register driver */ err = devm_hwrng_register(dev, &priv->rng);
The interrupt masking done for Northstart Plus and Northstar (BCM5301X) is moved from being a function pointer mapped to of_device_id::data into a proper part of the hwrng::init callback. While at it, we also make the of_data be a proper structure indicating the platform specifics, since the day we need to add a second type of platform information, we would have to do that anyway. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/char/hw_random/bcm2835-rng.c | 39 +++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 16 deletions(-)