On Wed, Oct 05, 2022 at 03:55:17PM +0200, Emil Renner Berthing wrote: > On Wed, 5 Oct 2022 at 15:48, Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx> wrote: > > > > On 05/10/2022 14:44, Emil Renner Berthing wrote: > > > On Thu, 29 Sept 2022 at 19:59, Ben Dooks <ben.dooks@xxxxxxxxxx> wrote: > > >> > > >> On 29/09/2022 15:32, Hal Feng wrote: > > >>> From: Emil Renner Berthing <kernel@xxxxxxxx> > > >>> > > >>> This converts the driver to use the builtin_platform_driver_probe macro > > >>> to initialize the driver. This macro ends up calling device_initcall as > > >>> was used previously, but also allocates a platform device which gives us > > >>> access to much nicer APIs such as platform_ioremap_resource, > > >>> platform_get_irq and dev_err_probe. > > >> > > >> This is useful, but also there are other changes currently being sorted > > >> out by Zong Li (cc'd into this message) which have already been reviewed > > >> and are hopefully queued for the next kernel release. > > >> > > >>> Signed-off-by: Emil Renner Berthing <kernel@xxxxxxxx> > > >>> Signed-off-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxxxxxxxx> > > > > > > I'm ok with something like this being merged, but please note that if > > > we ever want to support the JH7100 which uses registers in this > > > peripheral to flush the cache for its non-coherent DMAs then this > > > driver needs to be loaded before other peripherals or we will trigger > > > the 2nd warning in arch/riscv/mm/dma-noncoherent.c. I'm not sure we > > > can do that when it's a platform driver. See this patch for an > > > alternative to support the JH71x0s: > > > https://github.com/esmil/linux/commit/9c5b29da56ae29159c9572c5bb195fe3a1b535c5 > > > > > > /Emil > > > > Are you replying to your own patch that does the conversion to > > platform driver and then saying that it could actually cause > > issues? > > Yes, I can see it seems odd, but this patch lived for a while in the > kernel repo for the JH7100 until I rebased on 6.0-rc1 and realized the > above. > Hal Feng must have based his patches on a version of the code before > that when preparing this series. > > > I'm all for dropping this for the moment and keeping the old > > early init for the ccache. > > Cool. FWIW, if converting to a platform driver will inhibit using the driver for doing non-coherent stuff I would like to NAK the patch :) > > /Emil > > > >>> drivers/soc/sifive/sifive_l2_cache.c | 79 ++++++++++++++-------------- > > >>> 1 file changed, 40 insertions(+), 39 deletions(-) > > >>> > > >>> diff --git a/drivers/soc/sifive/sifive_l2_cache.c b/drivers/soc/sifive/sifive_l2_cache.c > > >>> index 59640a1d0b28..010d612f7420 100644 > > >>> --- a/drivers/soc/sifive/sifive_l2_cache.c > > >>> +++ b/drivers/soc/sifive/sifive_l2_cache.c > > >>> @@ -7,9 +7,9 @@ > > >>> */ > > >>> #include <linux/debugfs.h> > > >>> #include <linux/interrupt.h> > > >>> -#include <linux/of_irq.h> > > >>> -#include <linux/of_address.h> > > >>> -#include <linux/device.h> > > >>> +#include <linux/io.h> > > >>> +#include <linux/mod_devicetable.h> > > >>> +#include <linux/platform_device.h> > > >>> #include <asm/cacheinfo.h> > > >>> #include <soc/sifive/sifive_l2_cache.h> > > >>> > > >>> @@ -96,12 +96,6 @@ static void l2_config_read(void) > > >>> pr_info("L2CACHE: Index of the largest way enabled: %d\n", regval); > > >>> } > > >>> > > >>> -static const struct of_device_id sifive_l2_ids[] = { > > >>> - { .compatible = "sifive,fu540-c000-ccache" }, > > >>> - { .compatible = "sifive,fu740-c000-ccache" }, > > >>> - { /* end of table */ }, > > >>> -}; > > >>> - > > >>> static ATOMIC_NOTIFIER_HEAD(l2_err_chain); > > >>> > > >>> int register_sifive_l2_error_notifier(struct notifier_block *nb) > > >>> @@ -192,36 +186,29 @@ static irqreturn_t l2_int_handler(int irq, void *device) > > >>> return IRQ_HANDLED; > > >>> } > > >>> > > >>> -static int __init sifive_l2_init(void) > > >>> +static int __init sifive_l2_probe(struct platform_device *pdev) > > >>> { > > >>> - struct device_node *np; > > >>> - struct resource res; > > >>> - int i, rc, intr_num; > > >>> - > > >>> - np = of_find_matching_node(NULL, sifive_l2_ids); > > >>> - if (!np) > > >>> - return -ENODEV; > > >>> - > > >>> - if (of_address_to_resource(np, 0, &res)) > > >>> - return -ENODEV; > > >>> - > > >>> - l2_base = ioremap(res.start, resource_size(&res)); > > >>> - if (!l2_base) > > >>> - return -ENOMEM; > > >>> - > > >>> - intr_num = of_property_count_u32_elems(np, "interrupts"); > > >>> - if (!intr_num) { > > >>> - pr_err("L2CACHE: no interrupts property\n"); > > >>> - return -ENODEV; > > >>> - } > > >>> - > > >>> - for (i = 0; i < intr_num; i++) { > > >>> - g_irq[i] = irq_of_parse_and_map(np, i); > > >>> - rc = request_irq(g_irq[i], l2_int_handler, 0, "l2_ecc", NULL); > > >>> - if (rc) { > > >>> - pr_err("L2CACHE: Could not request IRQ %d\n", g_irq[i]); > > >>> - return rc; > > >>> - } > > >>> + struct device *dev = &pdev->dev; > > >>> + int nirqs; > > >>> + int ret; > > >>> + int i; > > >>> + > > >>> + l2_base = devm_platform_ioremap_resource(pdev, 0); > > >>> + if (IS_ERR(l2_base)) > > >>> + return PTR_ERR(l2_base); > > >>> + > > >>> + nirqs = platform_irq_count(pdev); > > >>> + if (nirqs <= 0) > > >>> + return dev_err_probe(dev, -ENODEV, "no interrupts\n"); > > >> > > >> I wonder if zero irqs is an actual issue here? > > >> > > >>> + for (i = 0; i < nirqs; i++) { > > >>> + g_irq[i] = platform_get_irq(pdev, i); > > >> > > >> I wonder if we need to keep g_irq[] around now? Is it going to be useful > > >> in the future? > > >> > > >>> + if (g_irq[i] < 0) > > >>> + return g_irq[i]; > > >>> + > > >>> + ret = devm_request_irq(dev, g_irq[i], l2_int_handler, 0, pdev->name, NULL); > > >>> + if (ret) > > >>> + return dev_err_probe(dev, ret, "Could not request IRQ %d\n", g_irq[i]); > > >>> } > > >>> > > >>> l2_config_read(); > > >>> @@ -234,4 +221,18 @@ static int __init sifive_l2_init(void) > > >>> #endif > > >>> return 0; > > >>> } > > >>> -device_initcall(sifive_l2_init); > > >>> + > > >>> +static const struct of_device_id sifive_l2_match[] = { > > >>> + { .compatible = "sifive,fu540-c000-ccache" }, > > >>> + { .compatible = "sifive,fu740-c000-ccache" }, > > >>> + { /* sentinel */ } > > >>> +}; > > >>> + > > >>> +static struct platform_driver sifive_l2_driver = { > > >>> + .driver = { > > >>> + .name = "sifive_l2_cache", > > >>> + .of_match_table = sifive_l2_match, > > >>> + .suppress_bind_attrs = true, > > >>> + }, > > >>> +}; > > >>> +builtin_platform_driver_probe(sifive_l2_driver, sifive_l2_probe); > > >> > > >> > > >> _______________________________________________ > > >> linux-riscv mailing list > > >> linux-riscv@xxxxxxxxxxxxxxxxxxx > > >> http://lists.infradead.org/mailman/listinfo/linux-riscv > > > > > > _______________________________________________ > > > linux-riscv mailing list > > > linux-riscv@xxxxxxxxxxxxxxxxxxx > > > http://lists.infradead.org/mailman/listinfo/linux-riscv > > > > > > > -- > > Ben Dooks http://www.codethink.co.uk/ > > Senior Engineer Codethink - Providing Genius > > > > https://www.codethink.co.uk/privacy.html > >
