Re: [PATCH] PCI: rcar: Add PCIE_CONF_REGNO macro for PCIECAR register

[Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>]

Hi,

Thanks for your review.

(2015/03/21 7:44), Bjorn Helgaas wrote:
> On Wed, Mar 11, 2015 at 01:23:50PM +0900, Nobuhiro Iwamatsu wrote:
>> The reg variable used when setting PCIECAR register need to be masked by 0xFC
>> by restriction of the corresponding register.
>> This adds PCIE_CONF_REGNO are macros for masking changes that PCIE_CONF_REGNO
>> is used when setting PCIECAR register.
>>
>> Signed-off-by: Nobuhiro Iwamatsu<nobuhiro.iwamatsu.yj@renesas.com>
>> ---
>>   drivers/pci/host/pcie-rcar.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
>> index c57bd0a..1f493ef 100644
>> --- a/drivers/pci/host/pcie-rcar.c
>> +++ b/drivers/pci/host/pcie-rcar.c
>> @@ -104,6 +104,7 @@
>>   #define  PCIE_CONF_BUS(b)	(((b)&  0xff)<<  24)
>>   #define  PCIE_CONF_DEV(d)	(((d)&  0x1f)<<  19)
>>   #define  PCIE_CONF_FUNC(f)	(((f)&  0x7)<<  16)
>> +#define  PCIE_CONF_REGNO(r)	(r&  0xfc)
>>
>>   #define RCAR_PCI_MAX_RESOURCES 4
>>   #define MAX_NR_INBOUND_MAPS 6
>> @@ -227,7 +228,8 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie,
>>
>>   	/* Set the PIO address */
>>   	rcar_pci_write_reg(pcie, PCIE_CONF_BUS(bus->number) |
>> -		PCIE_CONF_DEV(dev) | PCIE_CONF_FUNC(func) | reg, PCIECAR);
>> +		PCIE_CONF_DEV(dev) | PCIE_CONF_FUNC(func) |
>> +		PCIE_CONF_REGNO(reg), PCIECAR);
>
> What about the other rcar_pci_read_reg()/rcar_pci_write_reg() calls (the
> ones for "if (pci_is_root_bus())"?  Do those need to be limited to the
> first 256 bytes also?
>
> It seems a little strange to mask "reg = where&  ~3" at the top (making it
> 4-byte aligned), then mask "r&  0xfc" here (limiting to first 256 bytes and
> also making it 4-byte aligned).
>
> Seems like those two operations should either be combined or completely
> split.

This depends on the PCIECAR register of Rcar PCIE.
2bit from 7bit is REGNO, and 8bit from 11bit is EREGNO (Extended Register No) field.
If we do not want to mask, you might unnecessary data is written to the EREGNO.
This depends on 'where' value, but more safely value is written by the mask.


>
>>   	/* Enable the configuration access */
>>   	if (bus->parent->number == pcie->root_bus_nr)
>> --
>> 2.1.3
>>

Best regards,
   Nobuhiro