Hi Sato-san,
On Wed, Sep 13, 2023 at 11:35 AM Yoshinori Sato
<ysato@xxxxxxxxxxxxxxxxxxxx> wrote:
> Signed-off-by: Yoshinori Sato <ysato@xxxxxxxxxxxxxxxxxxxx>
Thanks for your patch!
> --- /dev/null
> +++ b/drivers/pci/controller/pci-sh7751.c
> @@ -0,0 +1,338 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SH7751 PCI driver
> + * Copyright (C) 2023 Yoshinori Sato
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_pci.h>
> +#include <linux/of_platform.h>
> +#include <linux/pci-ecam.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <asm-generic/pci.h>
> +#include "pci-sh7751.h"
> +
> +#define pcic_writel(val, reg) __raw_writel(val, pci_reg_base + (reg))
> +#define pcic_readl(reg) __raw_readl(pci_reg_base + (reg))
> +
> +DEFINE_RAW_SPINLOCK(pci_config_lock);
> +
> +/*
> + * PCIC fixups
> + */
> +
> +#define PCIMCR_MRSET 0x40000000
> +#define PCIMCR_RFSH 0x00000004
> +
> +/* board depend PCI bus fixups */
> +static void __init julian_fixup(void __iomem *pci_reg_base, void __iomem *bcr)
Please drop all the __init* annotations.
Although I no longer see invalid section warnings, all symbols tagged
with __init* are still referenced from sh7751_pci_probe(), eventually.
> +{
> + unsigned long bcr1, mcr;
> +
> + bcr1 = __raw_readl(bcr + SH7751_BCR1);
> + bcr1 |= 0x00080000; /* Enable Bit 19 BREQEN, set PCIC to slave */
> + pcic_writel(bcr1, SH4_PCIBCR1);
> +
> + mcr = __raw_readl(bcr + SH7751_MCR);
> + mcr &= (~PCIMCR_MRSET) & (~PCIMCR_RFSH);
> + pcic_writel(mcr, SH4_PCIMCR);
> +
> + pcic_writel(0x0c000000, SH7751_PCICONF5);
> + pcic_writel(0xd0000000, SH7751_PCICONF6);
> + pcic_writel(0x0c000000, SH4_PCILAR0);
> + pcic_writel(0x00000000, SH4_PCILAR1);
> +}
> +
> +static void __init r2d_fixup(void __iomem *pci_reg_base, void __iomem *bcr)
> +{
> + unsigned long bcr1, mcr;
> +
> + bcr1 = ioread32(bcr + SH7751_BCR1);
> + bcr1 |= 0x40080000; /* Enable Bit 19 BREQEN, set PCIC to slave */
> + pcic_writel(bcr1, SH4_PCIBCR1);
> +
> + /* Enable all interrupts, so we known what to fix */
> + pcic_writel(0x0000c3ff, SH4_PCIINTM);
> + pcic_writel(0x0000380f, SH4_PCIAINTM);
> +
> + pcic_writel(0xfb900047, SH7751_PCICONF1);
> + pcic_writel(0xab000001, SH7751_PCICONF4);
> +
> + mcr = ioread32(bcr + SH7751_MCR);
> + mcr &= (~PCIMCR_MRSET) & (~PCIMCR_RFSH);
> + pcic_writel(mcr, SH4_PCIMCR);
> +
> + pcic_writel(0x0c000000, SH7751_PCICONF5);
> + pcic_writel(0xd0000000, SH7751_PCICONF6);
> + pcic_writel(0x0c000000, SH4_PCILAR0);
> + pcic_writel(0x00000000, SH4_PCILAR1);
> +}
> +
> +static const __initconst struct fixups {
> + char *compatible;
> + void (*fixup)(void __iomem *pci_reg_base, void __iomem *bcr);
> +} fixup_list[] = {
> + {
> + .compatible = "iodata,julian-pci",
> + .fixup = julian_fixup,
> + },
> + {
> + .compatible = "renesas,r2d-pci",
> + .fixup = r2d_fixup,
> + },
> +};
These fixups seem to be board-specific instead of specific to the
PCI block in the SoCs on these boards.
I see three options to handle this in a more appropriate way:
1. Handle this in the bootloader.
Not an attractive solution, as not everyone can/wants to update
the bootloader,
2. Use of_machine_is_compatible() in a platform-specific quirk
handler, outside the PCI driver,
3. Move the common parts into sh7751_pci_probe(), and the
handle the differences through DT topology analysis and/or
properties in DT.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
