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

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Sep 13, 2023 at 06:23:32PM +0900, Yoshinori Sato wrote:

See subject line comments at
https://lore.kernel.org/r/20230918191602.GA201859@bhelgaas

Please add a commit log.  Repeating the subject line is fine, but
there's a little more detail that could be included here, e.g., a hint
about what platforms this is useful for.

> + * SH7751 PCI driver
> + * Copyright (C) 2023 Yoshinori Sato
> + *

Spurious blank line.

> +#define pcic_writel(val, reg) __raw_writel(val, pci_reg_base + (reg))
> +#define pcic_readl(reg) __raw_readl(pci_reg_base + (reg))

Best to include a pointer to a struct in the macro arguments so this
doesn't depend on local variable names in the users.  See advk_readl()
and advk_writel(), for example.

> +DEFINE_RAW_SPINLOCK(pci_config_lock);

Should be static.

> +	pcic_writel(0x0c000000, SH7751_PCICONF5);
> +	pcic_writel(0xd0000000, SH7751_PCICONF6);
> +	pcic_writel(0x0c000000, SH4_PCILAR0);
> +	pcic_writel(0x00000000, SH4_PCILAR1);

The header file uses upper-case hex, but this file looks like mostly
lower-case.  Maybe make them consistent?

> + * Since SH4 only does 32bit access we'll have to do a read,
> + * mask,write operation.

This RMW corrupts some registers; see comments and warning in
pci_generic_config_write32().  A comment here is probably enough.

> + * We'll allow an odd byte offset, though it should be illegal.
> + */
> +static int sh4_pci_write(struct pci_bus *bus, unsigned int devfn,
> +			 int where, int size, u32 val)

> +static int area_sdram_check(void __iomem *pci_reg_base,
> +			    void __iomem *bcr,
> +			    unsigned int area)
> +{
> +	unsigned long word;
> +
> +	word = __raw_readl(bcr + SH7751_BCR1);
> +	/* check BCR for SDRAM in area */
> +	if (((word >> area) & 1) == 0) {
> +		pr_info("PCI: Area %d is not configured for SDRAM. BCR1=0x%lx\n",

Use dev_info().  Will require passing a controller struct around,
which is common style in drivers/pci/controller/.

> +	dev_info(&pdev->dev, "PCI core found at %p\n",
> +		pci_reg_base);

Use %pR.

> +	/* use round robin mode to stop a device starving/overruning */

overrunning

Bjorn