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

[Yoshinori Sato <ysato@users.sourceforge.jp>]

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@users.sourceforge.jp> wrote:
> > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> 
> 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@linux-m68k.org
> 
> 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