Re: [RFC PATCH v2 07/30] drivers/pci: SH7751 PCI Host bridge controller driver.

Hi Sato-san,

On Wed, Sep 13, 2023 at 11:35 AM Yoshinori Sato
<[email protected]> wrote:
> Signed-off-by: Yoshinori Sato <[email protected]>

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 -- [email protected]

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