On Tue, 19 Sep 2023 00:32:46 +0900,
Geert Uytterhoeven wrote:
>
> 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.
I think the bootloader is not initialized on targets that do not use
a PCI device for booting.
I think it's better to use option 2 or 3.
I looked at the current fixup, but the only difference is the PCIC setting,
so I will try plan 3.
> 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
--
Yosinori Sato
