Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Shawn Lin <shawn.lin@rock-chips.com>]

Hi Arnd,

On 2016/6/1 16:24, Arnd Bergmann wrote:
> On Friday, May 27, 2016 6:31:28 PM CEST Wenrui Li wrote:
>> Hi,
>>
>> On 2016/5/27 15:13, Bharat Kumar Gogada wrote:
>>>>>
>>>>>> +
>>>>>> +static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp,
>>>>>> +                                 struct pci_bus *bus, u32 devfn,
>>>>>> +                                 int where, int size, u32 *val)
>>>>>> +{
>>>>>> +  u32 busdev;
>>>>>> +
>>>>>> +  busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn),
>>>>>> +                          PCI_FUNC(devfn), where);
>>>>>> +
>>>>>> +  if (busdev & (size - 1)) {
>>>>>> +          *val = 0;
>>>>>> +          return PCIBIOS_BAD_REGISTER_NUMBER;
>>>>>> +  }
>>>>>> +
>>>>>> +  if (size == 4) {
>>>>>> +          *val = readl(pp->reg_base + busdev);
>>>>>> +  } else if (size == 2) {
>>>>>> +          *val = readw(pp->reg_base + busdev);
>>>>>> +  } else if (size == 1) {
>>>>>> +          *val = readb(pp->reg_base + busdev);
>>>>>> +  } else {
>>>>>> +          *val = 0;
>>>>>> +          return PCIBIOS_BAD_REGISTER_NUMBER;
>>>>>> +  }
>>>>>> +  return PCIBIOS_SUCCESSFUL;
>>>>>> +}
>>>>>> +
>>>>>
>>>>> This looks like the normal ECAM operations, you could just call those.
>>>>
>>>> I read ECAM reference code, I found it not support ioremap config space
>>>> for each bus individually on 64-bit systems. Our soc is 64-bit system,
>>>> and bus0 config space base address is 0xfda00000, bus1 base address is
>>>> 0xf8100000. So I think it is not normal ECAM operations, I do not know
>>>> if I have understood correctly?
>>>>
>>> Hi,
>>>
>>> I think Arnd was suggesting to use generic config read/write calls, pci_generic_config_read/pci_generic_config_write
>>> which does above functionality.
>>
>> Yeah, I seem the pci_generic_config_write use writew/writeb for byte and
>> word write. but our SOC not support byte and word write in RC config
>> spcace. So I redefine the the pci_ops.write
>
> Rihgt, that bug should be documented somewhere. You can also use
> the pci_generic_config_read32/pci_generic_config_write32 functions
> for this.
>
> I wonder if we should add a more general way of treating type 1
> config space accesses differently, as a lot of host bridges have
> similar requirements, accessing the registers in a different place,
> different layout, or other constraints.

yes, that is what we need and we believe other host bridges have
this requirements.  With your patch applied, we only need to
implement our map0 callback here. Thanks for improving it.

BTW, would you mind that we pack your patch into my patchset for
the next version if possible, or maybe you wanna upstream it
by yourself? :)


Thanks.

>
> The patch below would make that very easy to do. Suggestions for
> better naming welcome.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index d11cdbb8fba3..2f7dcaa63e1d 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -34,9 +34,12 @@ int pci_bus_read_config_##size \
>  	u32 data = 0;							\
>  	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
>  	raw_spin_lock_irqsave(&pci_lock, flags);			\
> -	res = bus->ops->read(bus, devfn, pos, len, &data);		\
> +	if (bus->number == 0 && bus->ops->read0)			\
> +		res = bus->ops->read0(bus, devfn, pos, len, &data);	\
> +	else								\
> +		res = bus->ops->read(bus, devfn, pos, len, &data);	\
>  	*value = (type)data;						\
> -	raw_spin_unlock_irqrestore(&pci_lock, flags);		\
> +	raw_spin_unlock_irqrestore(&pci_lock, flags);			\
>  	return res;							\
>  }
>
> @@ -48,8 +51,11 @@ int pci_bus_write_config_##size \
>  	unsigned long flags;						\
>  	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
>  	raw_spin_lock_irqsave(&pci_lock, flags);			\
> -	res = bus->ops->write(bus, devfn, pos, len, value);		\
> -	raw_spin_unlock_irqrestore(&pci_lock, flags);		\
> +	if (bus->number == 0 && bus->ops->write0)			\
> +		res = bus->ops->write0(bus, devfn, pos, len, value);	\
> +	else								\
> +		res = bus->ops->write(bus, devfn, pos, len, value);	\
> +	raw_spin_unlock_irqrestore(&pci_lock, flags);			\
>  	return res;							\
>  }
>
> @@ -72,7 +78,11 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
>  {
>  	void __iomem *addr;
>
> -	addr = bus->ops->map_bus(bus, devfn, where);
> +	if (bus->number == 0 && bus->ops->map_bus0)
> +		addr = bus->ops->map_bus0(bus, devfn, where);
> +	else
> +		addr = bus->ops->map_bus(bus, devfn, where);
> +
>  	if (!addr) {
>  		*val = ~0;
>  		return PCIBIOS_DEVICE_NOT_FOUND;
> @@ -94,7 +104,10 @@ int pci_generic_config_write(struct pci_bus *bus, unsigned int devfn,
>  {
>  	void __iomem *addr;
>
> -	addr = bus->ops->map_bus(bus, devfn, where);
> +	if (bus->number == 0 && bus->ops->map_bus0)
> +		addr = bus->ops->map_bus0(bus, devfn, where);
> +	else
> +		addr = bus->ops->map_bus(bus, devfn, where);
>  	if (!addr)
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>
> @@ -114,7 +127,10 @@ int pci_generic_config_read32(struct pci_bus *bus, unsigned int devfn,
>  {
>  	void __iomem *addr;
>
> -	addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
> +	if (bus->number == 0 && bus->ops->map_bus0)
> +		addr = bus->ops->map_bus0(bus, devfn, where);
> +	else
> +		addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
>  	if (!addr) {
>  		*val = ~0;
>  		return PCIBIOS_DEVICE_NOT_FOUND;
> @@ -135,7 +151,10 @@ int pci_generic_config_write32(struct pci_bus *bus, unsigned int devfn,
>  	void __iomem *addr;
>  	u32 mask, tmp;
>
> -	addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
> +	if (bus->number == 0 && bus->ops->map_bus0)
> +		addr = bus->ops->map_bus0(bus, devfn, where);
> +	else
> +		addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
>  	if (!addr)
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index df41c4645911..1caae3bd7115 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -580,6 +580,9 @@ struct pci_ops {
>  	void __iomem *(*map_bus)(struct pci_bus *bus, unsigned int devfn, int where);
>  	int (*read)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val);
>  	int (*write)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val);
> +	void __iomem *(*map_bus0)(struct pci_bus *bus, unsigned int devfn, int where);
> +	int (*read0)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val);
> +	int (*write0)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val);
>  };
>
>  /*
>
>
>
>


-- 
Best Regards
Shawn Lin