Re: [RFC PATCH V2 1/3] PCI: hisi: re-architect Hip05/Hip06 controllers driver to preapare for ACPI

Dongdong Liu
2016-09-01T12:45:30Z

Hi Arnd

在 2016/9/1 15:41, Arnd Bergmann 写道:
> On Thursday, September 1, 2016 10:05:29 AM CEST Dongdong Liu wrote:
>>
>> 在 2016/8/31 19:45, Arnd Bergmann 写道:
>>> On Wednesday, August 31, 2016 7:48:12 PM CEST Dongdong Liu wrote:
>>>> +
>>>> +/* HipXX PCIe host only supports 32-bit config access */
>>>> +int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size,
>>>> + u32 *val)
>>>> +{
>>>> + u32 reg;
>>>> + u32 reg_val;
>>>> + void *walker = &reg_val;
>>>> +
>>>> + walker += (where & 0x3);
>>>> + reg = where & ~0x3;
>>>> + reg_val = readl(reg_base + reg);
>>>> +
>>>> + if (size == 1)
>>>> + *val = *(u8 __force *) walker;
>>>> + else if (size == 2)
>>>> + *val = *(u16 __force *) walker;
>>>
>>> What is the __force for?
>>
>> Hi Arnd, thanks for replying.
>>
>> __force is used to, well, force a conversion, like casting from or to a bitwise type, else the Sparse checker will throw a warning.
>
> I know what it's for in general, but in this case there is no __bitwise
> or __iomem or any other annotation on either side of the assignment.

Thanks for you point that.

>
>>>
>>>> + else if (size == 4)
>>>> + *val = reg_val;
>>>> + else
>>>> + return PCIBIOS_BAD_REGISTER_NUMBER;
>>>> +
>>>> + return PCIBIOS_SUCCESSFUL;
>>>
>>> It looks like you are reimplementing pci_generic_config_read32/pci_generic_config_write32
>>> read here, better use them directly.
>>>
>>
>> For our host bridge, access RC and EP config space are not the same way.
>> Our host bridge is non ECAM only for the RC bus config space;
>> for any other bus underneath the root bus we support ECAM access.
>>
>> hisi_pcie_common_cfg_read is used to read RC config space, only supports 32-bit config access.
>> hisi_pcie_common_cfg_read/hisi_pcie_common_cfg_write may change as below will be better.
>>
>> /* HipXX PCIe host only supports 32-bit config access */
>> int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size,
>> u32 *val)
>> {
>> void __iomem *addr;
>>
>> addr = reg_base + (where & ~0x3);
>> *val = readl(addr);
>>
>> if (size <= 2)
>> *val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
>>
>> return PCIBIOS_SUCCESSFUL;
>> }
>
> My point was: why not call pci_generic_config_read32() when accessing
> the RC config space instead of duplicating the code from it?

I know your point.

1. For our host bridge , ".map_bus = pci_ecam_map_bus" is only suitable for
accessing the EP config space.
pci_generic_config_read32() need to call "addr = bus->ops->map_bus(bus, devfn, where & ~0x3);",

drivers/pci/host/pcie-hisi-acpi.c
static struct pci_ops hisi_pcie_ops = {
.map_bus = pci_ecam_map_bus,
.read = hisi_pcie_acpi_rd_conf,
.write = hisi_pcie_acpi_wr_conf,
};

Yes, we can change ".map_bus = pci_ecam_map_bus" to ".map_bus = hisi_pci_map_bus", and implentment hisi_pci_map_bus as below,
then we will not need to call hisi_pcie_common_cfg_read().

void __iomem *hisi_pci_map_bus(struct pci_bus *bus, unsigned int devfn, int where)
{
struct pci_config_window *cfg = bus->sysdata;
void __iomem *reg_base = cfg->priv;

/* for RC config access*/
if (bus->number == cfg->busr.start)
return reg_base + (where & ~0x3);
else
/* for EP config access */
return pci_ecam_map_bus(bus, devfn, where);
}

and hisi_pcie_acpi_rd_conf() need to change as below.
static int hisi_pcie_acpi_rd_conf(struct pci_bus *bus, u32 devfn, int where,
int size, u32 *val)
{
struct pci_config_window *cfg = bus->sysdata;

if (hisi_pcie_acpi_valid_config(cfg, bus, PCI_SLOT(devfn)) == 0)
return PCIBIOS_DEVICE_NOT_FOUND;

/* access RC config space */
if (bus->number == cfg->busr.start)
return pci_generic_config_read32(bus, devfn, where, size, val);

/* access EP config space */
return pci_generic_config_read(bus, devfn, where, size, val);
}

2. We need to backward compatible with the old dt way config access as below code,
so we have to call hisi_pcie_common_cfg_read() when accessing the RC config space.
For this, we have to call hisi_pcie_common_cfg_read().

drivers/pci/host/pcie-hisi.c
static inline int hisi_pcie_cfg_read(struct pcie_port *pp, int where,
int size, u32 *val)
{
struct hisi_pcie *pcie = to_hisi_pcie(pp);

return hisi_pcie_common_cfg_read(pcie->reg_base, where, size, val);
}

static struct pcie_host_ops hisi_pcie_host_ops = {
.rd_own_conf = hisi_pcie_cfg_read,
.wr_own_conf = hisi_pcie_cfg_write,
.link_up = hisi_pcie_link_up,
};

Thanks
Dongdong
>
> Arnd
>
> .
>