Re: [RESEND RFC PATCH 06/12] drivers/pci: Add SH7751 PCI Host bridge driver.

Hi Sato-san,

On Thu, Aug 31, 2023 at 7:38 AM Yoshinori Sato
<[email protected]> wrote:
> Use Common framework host bridge driver.
>
> Signed-off-by: Yoshinori Sato <[email protected]>

Thanks for your patch!

> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -343,6 +343,15 @@ config PCIE_XILINX_CPM
>           Say 'Y' here if you want kernel support for the
>           Xilinx Versal CPM host bridge.
>
> +config PCI_SH7751
> +       bool "Renesas SH7751 PCI controller"
> +       depends on OF
> +       depends on CPU_SUBTYPE_SH7751 || CPU_SUBTYPE_SH7751R

|| COMPILE_TEST ?

> +       select PCI_HOST_COMMON
> +       help
> +         Say 'Y' here if you want kernel to support the Renesas SH7751 PCI
> +         Host Bridge driver.
> +
>  source "drivers/pci/controller/cadence/Kconfig"
>  source "drivers/pci/controller/dwc/Kconfig"
>  source "drivers/pci/controller/mobiveil/Kconfig"

> --- /dev/null
> +++ b/drivers/pci/controller/pci-sh7751.c
> @@ -0,0 +1,382 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas SH7751 PCI Host bridge 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
> +
> +static void __init julian_fixup(void __iomem *pci_reg_base, void __iomem *bcr)

Please drop all the __init* annotations from this driver:

    WARNING: modpost: vmlinux: section mismatch in reference:
sh7751_pci_driver+0x0 (section: .data) -> set_reset_devices (section:
.init.text)

> +{
> +       unsigned long bcr1, mcr;

u32

> +
> +       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 r2dplus_fixup(void __iomem *pci_reg_base, void __iomem *bcr)
> +{
> +       unsigned long bcr1, mcr;

u32

> +static __init void pcic_fixups(struct device_node *np,
> +                      void __iomem *pcic, void __iomem *bcr)
> +{
> +       int i;

unsigned int

> +       const struct fixups *f = fixup_list;
> +
> +       for (i = 0; i < ARRAY_SIZE(fixup_list); i++) {
> +               if (of_device_is_compatible(np, f->compatible)) {
> +                       f->fixup(pcic, bcr);
> +                       break;
> +               }
> +       }
> +}

> +/*
> + * We need to avoid collisions with `mirrored' VGA ports
> + * and other strange ISA hardware, so we always want the
> + * addresses to be allocated in the 0x000-0x0ff region
> + * modulo 0x400.
> + */
> +resource_size_t pcibios_align_resource(void *data, const struct resource *res,
> +                                             resource_size_t size, resource_size_t align)
> +{
> +       resource_size_t start = res->start;
> +
> +       return start;

return res->start

> +}

> +static int __init area_sdram_check(void __iomem *pci_reg_base,
> +                                  void __iomem *bcr,
> +                                  unsigned int area)
> +{
> +       unsigned long word;

u32

> +
> +       word = __raw_readl(bcr + SH7751_BCR1);

> +static __init int sh7751_pci_probe(struct platform_device *pdev)
> +{
> +       struct resource *res, *wres;
> +       u32 id;
> +       u32 reg, word;
> +       void __iomem *pci_reg_base;
> +       void __iomem *bcr;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       pci_reg_base = (void __iomem *)res->start;
> +       if (IS_ERR(pci_reg_base))
> +               return PTR_ERR(pci_reg_base);
> +
> +       wres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +       if (IS_ERR(wres))
> +               return PTR_ERR(wres);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +       bcr = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(pci_reg_base))
> +               return PTR_ERR(bcr);
> +
> +       /* check for SH7751/SH7751R hardware */
> +       id = pcic_readl(SH7751_PCICONF0);
> +       if (id != ((SH7751_DEVICE_ID << 16) | SH7751_VENDOR_ID) &&
> +           id != ((SH7751R_DEVICE_ID << 16) | SH7751_VENDOR_ID)) {
> +               pr_warn("PCI: This is not an SH7751(R)\n");
> +               return -ENODEV;
> +       }
> +       dev_info(&pdev->dev, "PCI core found at %p\n",

%p prints an obfuscated pointer
If you would keep the original resource, you could print it with %pR.

> +               pci_reg_base);

> +static const struct of_device_id sh7751_pci_of_match[] = {
> +       { .compatible = "renesas,sh7751-pci",
> +         .data = &pci_sh7751_bus_ops },
> +
> +       { },

Please no comma after a sentinel.

> +};
> +MODULE_DEVICE_TABLE(of, sh7751_pci_of_match);

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