subject:"RE\: \[PATCH v10 4\/6\] PCI\: hisi\: Add PCIe host support for HiSilicon SoC Hip05"

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-15 Thread Zhou Wang

On 2015/10/15 5:56, Arnd Bergmann wrote:
> On Wednesday 14 October 2015 17:44:11 Zhou Wang wrote:
>> On 2015/10/14 17:06, Arnd Bergmann wrote:
>>> On Wednesday 14 October 2015 16:59:03 Zhou Wang wrote:

 Hi Arnd,

 In Hip05 PCIe host, it uses GITS_TRANSLATER's address to get TLP package
 which contains MSI address and MSI data, and then combine BDF and MSI data
 to a 32 bit data which will be writen to GITS_TRANSLATER register of ITS.

 I think maybe this is a defect of our PCIe controller.
>>>
>>> I'd consider it a bug in the firmware if this is not set up correctly
>>> before boot.
>>>
> I don't think what you do here is safe because the 'reg' property
> of the MSI controller might point to the address that is used for
> the message directly.

 I see your point, however we must get address of GITS_TRANSLATER and
 set it to PCIe host. How about adding necessary comments here?
>>>
>>> This seems to just be static setup that should be done before Linux
>>> is even loaded. Any reason you can't do it that way?
>>>
>>
>> There are some ITSs in Hip05-D02 platform, in fact, we can use any of them
>> as a msi-controller,  which we can configure in dts. I am afraid that
>> hard-setting the value in BIOS would lead to restrictions in terms of 
>> flexibility,
>> as with the current implementation the same BIOS-driver can fit different
>> DTS structures.
> 
> The dtb generally should be expected to match whatever the firmware sets up,
> so if there is one reasonable setting here, I see no problem with hardcoding
> it that way. In particular on server systems, we usually expect the firmware
> to configure almost everything in advance and just tell us how it is
> configured, while on embedded systems we can't trust the bootload and
> usually set it all up in the kernel from scratch.
>

I see your point. Actually in order to support platforms without PCIe 
configuration
support BIOS we planned to have a further commit once the driver was upstreamed,
where we check if the link is already up, if not we would configure it in the 
kernel,
otherwise we would return silently.

Now about this patchset we can remove GITS_TRANSLATER address setting and do 
this
in BIOS together with link-up setup; then in the next commit for supporting 
platforms
without PCIe setup in UEFI, we can add this part back where we first check
if the link is already up (we can assume that if BIOS has configured link-up,
it has also setup the msi-parent address), if the link is up we skip reading
msi-parent address.

> What would be a reason to pick one ITS over another?

In fact, we set PCIe host and ITS binding in dts. I mean that PCIe host can
bind to any ITS nodes in system.

> 
> On a related note, don't you also need to describe in DT how PCI B/D/F
> function numbers get turned into addresses in the ITS? Does that also
> require configuration in the driver? I see this code here:

We don't need to describe this. we need only describe the relationship between
PCIe host and ITS, PCIe host will service PCIe devices which are connected to 
it.

> 
> 
> +   hisi_pcie_apb_writel(pcie, PCIE_MSI_ASID_ENABLE | PCIE_MSI_ASID_VALUE,
> +PCIE_SLV_MSI_ASID);
> +   hisi_pcie_apb_writel(pcie, PCIE_MSI_TRANS_ENABLE, PCIE_MSI_TRANS_REG);
> +   hisi_pcie_change_apb_mode(pcie, PCIE_SLV_DBI_MODE);
>

This code is to enable MSI support in PCIe host.

> plus all of hisi_pcie_config_context(). This looks like it will change
> the way the MSI is interpreted. This also seems like something that
> could be done in the firmware in advance, and just get reported in DT.

I think all hisi_pcie_config_context can be move to BIOS for this patchset,
however in order to support other BIOSs which have no PCIe setup we would plan
a future commit adding this back and working as explained above.

Thanks,
Zhou

> 
>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-15 Thread Zhou Wang

On 2015/10/15 5:56, Arnd Bergmann wrote:
> On Wednesday 14 October 2015 17:44:11 Zhou Wang wrote:
>> On 2015/10/14 17:06, Arnd Bergmann wrote:
>>> On Wednesday 14 October 2015 16:59:03 Zhou Wang wrote:

 Hi Arnd,

 In Hip05 PCIe host, it uses GITS_TRANSLATER's address to get TLP package
 which contains MSI address and MSI data, and then combine BDF and MSI data
 to a 32 bit data which will be writen to GITS_TRANSLATER register of ITS.

 I think maybe this is a defect of our PCIe controller.
>>>
>>> I'd consider it a bug in the firmware if this is not set up correctly
>>> before boot.
>>>
> I don't think what you do here is safe because the 'reg' property
> of the MSI controller might point to the address that is used for
> the message directly.

 I see your point, however we must get address of GITS_TRANSLATER and
 set it to PCIe host. How about adding necessary comments here?
>>>
>>> This seems to just be static setup that should be done before Linux
>>> is even loaded. Any reason you can't do it that way?
>>>
>>
>> There are some ITSs in Hip05-D02 platform, in fact, we can use any of them
>> as a msi-controller,  which we can configure in dts. I am afraid that
>> hard-setting the value in BIOS would lead to restrictions in terms of 
>> flexibility,
>> as with the current implementation the same BIOS-driver can fit different
>> DTS structures.
> 
> The dtb generally should be expected to match whatever the firmware sets up,
> so if there is one reasonable setting here, I see no problem with hardcoding
> it that way. In particular on server systems, we usually expect the firmware
> to configure almost everything in advance and just tell us how it is
> configured, while on embedded systems we can't trust the bootload and
> usually set it all up in the kernel from scratch.
>

I see your point. Actually in order to support platforms without PCIe 
configuration
support BIOS we planned to have a further commit once the driver was upstreamed,
where we check if the link is already up, if not we would configure it in the 
kernel,
otherwise we would return silently.

Now about this patchset we can remove GITS_TRANSLATER address setting and do 
this
in BIOS together with link-up setup; then in the next commit for supporting 
platforms
without PCIe setup in UEFI, we can add this part back where we first check
if the link is already up (we can assume that if BIOS has configured link-up,
it has also setup the msi-parent address), if the link is up we skip reading
msi-parent address.

> What would be a reason to pick one ITS over another?

In fact, we set PCIe host and ITS binding in dts. I mean that PCIe host can
bind to any ITS nodes in system.

> 
> On a related note, don't you also need to describe in DT how PCI B/D/F
> function numbers get turned into addresses in the ITS? Does that also
> require configuration in the driver? I see this code here:

We don't need to describe this. we need only describe the relationship between
PCIe host and ITS, PCIe host will service PCIe devices which are connected to 
it.

> 
> 
> +   hisi_pcie_apb_writel(pcie, PCIE_MSI_ASID_ENABLE | PCIE_MSI_ASID_VALUE,
> +PCIE_SLV_MSI_ASID);
> +   hisi_pcie_apb_writel(pcie, PCIE_MSI_TRANS_ENABLE, PCIE_MSI_TRANS_REG);
> +   hisi_pcie_change_apb_mode(pcie, PCIE_SLV_DBI_MODE);
>

This code is to enable MSI support in PCIe host.

> plus all of hisi_pcie_config_context(). This looks like it will change
> the way the MSI is interpreted. This also seems like something that
> could be done in the firmware in advance, and just get reported in DT.

I think all hisi_pcie_config_context can be move to BIOS for this patchset,
however in order to support other BIOSs which have no PCIe setup we would plan
a future commit adding this back and working as explained above.

Thanks,
Zhou

> 
>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Arnd Bergmann

On Wednesday 14 October 2015 17:44:11 Zhou Wang wrote:
> On 2015/10/14 17:06, Arnd Bergmann wrote:
> > On Wednesday 14 October 2015 16:59:03 Zhou Wang wrote:
> >>
> >> Hi Arnd,
> >>
> >> In Hip05 PCIe host, it uses GITS_TRANSLATER's address to get TLP package
> >> which contains MSI address and MSI data, and then combine BDF and MSI data
> >> to a 32 bit data which will be writen to GITS_TRANSLATER register of ITS.
> >>
> >> I think maybe this is a defect of our PCIe controller.
> > 
> > I'd consider it a bug in the firmware if this is not set up correctly
> > before boot.
> > 
> >>> I don't think what you do here is safe because the 'reg' property
> >>> of the MSI controller might point to the address that is used for
> >>> the message directly.
> >>
> >> I see your point, however we must get address of GITS_TRANSLATER and
> >> set it to PCIe host. How about adding necessary comments here?
> > 
> > This seems to just be static setup that should be done before Linux
> > is even loaded. Any reason you can't do it that way?
> >
> 
> There are some ITSs in Hip05-D02 platform, in fact, we can use any of them
> as a msi-controller,  which we can configure in dts. I am afraid that
> hard-setting the value in BIOS would lead to restrictions in terms of 
> flexibility,
> as with the current implementation the same BIOS-driver can fit different
> DTS structures.

The dtb generally should be expected to match whatever the firmware sets up,
so if there is one reasonable setting here, I see no problem with hardcoding
it that way. In particular on server systems, we usually expect the firmware
to configure almost everything in advance and just tell us how it is
configured, while on embedded systems we can't trust the bootload and
usually set it all up in the kernel from scratch.

What would be a reason to pick one ITS over another?

On a related note, don't you also need to describe in DT how PCI B/D/F
function numbers get turned into addresses in the ITS? Does that also
require configuration in the driver? I see this code here:

+   hisi_pcie_apb_writel(pcie, PCIE_MSI_ASID_ENABLE | PCIE_MSI_ASID_VALUE,
+PCIE_SLV_MSI_ASID);
+   hisi_pcie_apb_writel(pcie, PCIE_MSI_TRANS_ENABLE, PCIE_MSI_TRANS_REG);
+   hisi_pcie_change_apb_mode(pcie, PCIE_SLV_DBI_MODE);

plus all of hisi_pcie_config_context(). This looks like it will change
the way the MSI is interpreted. This also seems like something that
could be done in the firmware in advance, and just get reported in DT.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Gabriele Paoloni



> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Wednesday, October 14, 2015 10:42 AM
> To: Gabriele Paoloni
> Cc: Wangzhou (B); Bjorn Helgaas; Bjorn Helgaas; jingooh...@gmail.com;
> pratyush.an...@gmail.com; li...@arm.linux.org.uk;
> thomas.petazz...@free-electrons.com; lorenzo.pieral...@arm.com;
> james.mo...@arm.com; liviu.du...@arm.com; ja...@lakedaemon.net;
> r...@kernel.org; gabriel.fernan...@linaro.org;
> minghuan.l...@freescale.com; linux-...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; zhangjukuo; qiuzhenfa; liudongdong (C);
> qiujiang; xuwei (O); Liguozhu (Kenneth); Wangkefeng (Kevin); Rob
> Herring
> Subject: Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for
> HiSilicon SoC Hip05
> 
> On Wednesday 14 October 2015 09:31:48 Gabriele Paoloni wrote:
> > >
> > > > To me it sounds more appropriate to adopt subsys_initcall() for
> all
> > > the
> > > > PCI Host Bridge controllers rather than having them as loadable
> > > modules...
> > > >
> > > > What is your view?
> > >
> > > subsys_initcall() sounds odd because it's a driver rather than a
> > > subsystem,
> > > but I realize that most of the other levels don't fit any better.
> >
> > Yes well I was seeing for example the vgaarb
> > http://lxr.free-electrons.com/source/drivers/gpu/vga/vgaarb.c#L1357
> >
> > That in the init is calling pci_get_subsys()
> >
> > So I was wondering that the PCI devices may not be registered unless
> > we also init the PCI host bridge through subsys_initcall()...
> 
> I think this should work as is: the code first looks for devices
> that are already there and then registers a notifier for devices
> that show up later. This is meant to work for both devices that
> are hotplugged at a later point as well as PCI buses that are
> already there but not yet probed.
> 
> > But then maybe is the vgaarb to be buggy...
> 
> Possible. It may well be that the code is only tested on x86,
> which always probes its PCI very early.
> 
> > > As I said, it's not really a choice we have to make in the source
> code,
> > > we can use subsys_initcall together with module_exit(), or we can
> > > create a helper macro that is similar to module_platform_driver()
> > > specifically for PCI that uses a particular initcall level.
> >
> > Ok got it. But I guess this needs to be thought and applied to all
> > the PCI host bridge controllers...
> >
> > So maybe for this driver I can use module_platform_driver_probe()
> > and then we can see...
> 
> Sounds good. Let's focus on getting the driver merged first and
> then follow up with a patch to get this right for all PCI hosts.

Ok good let's do like this then

will change in v11

Again Many Thanks

Gab

> 
>   Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Arnd Bergmann

On Wednesday 14 October 2015 09:31:48 Gabriele Paoloni wrote:
> > 
> > > To me it sounds more appropriate to adopt subsys_initcall() for all
> > the
> > > PCI Host Bridge controllers rather than having them as loadable
> > modules...
> > >
> > > What is your view?
> > 
> > subsys_initcall() sounds odd because it's a driver rather than a
> > subsystem,
> > but I realize that most of the other levels don't fit any better.
> 
> Yes well I was seeing for example the vgaarb 
> http://lxr.free-electrons.com/source/drivers/gpu/vga/vgaarb.c#L1357
> 
> That in the init is calling pci_get_subsys()
> 
> So I was wondering that the PCI devices may not be registered unless
> we also init the PCI host bridge through subsys_initcall()...

I think this should work as is: the code first looks for devices
that are already there and then registers a notifier for devices
that show up later. This is meant to work for both devices that
are hotplugged at a later point as well as PCI buses that are
already there but not yet probed.

> But then maybe is the vgaarb to be buggy...

Possible. It may well be that the code is only tested on x86,
which always probes its PCI very early.

> > As I said, it's not really a choice we have to make in the source code,
> > we can use subsys_initcall together with module_exit(), or we can
> > create a helper macro that is similar to module_platform_driver()
> > specifically for PCI that uses a particular initcall level.
> 
> Ok got it. But I guess this needs to be thought and applied to all
> the PCI host bridge controllers...
> 
> So maybe for this driver I can use module_platform_driver_probe()
> and then we can see...

Sounds good. Let's focus on getting the driver merged first and
then follow up with a patch to get this right for all PCI hosts.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Zhou Wang

On 2015/10/14 17:06, Arnd Bergmann wrote:
> On Wednesday 14 October 2015 16:59:03 Zhou Wang wrote:
>>
>> Hi Arnd,
>>
>> In Hip05 PCIe host, it uses GITS_TRANSLATER's address to get TLP package
>> which contains MSI address and MSI data, and then combine BDF and MSI data
>> to a 32 bit data which will be writen to GITS_TRANSLATER register of ITS.
>>
>> I think maybe this is a defect of our PCIe controller.
> 
> I'd consider it a bug in the firmware if this is not set up correctly
> before boot.
> 
>>> I don't think what you do here is safe because the 'reg' property
>>> of the MSI controller might point to the address that is used for
>>> the message directly.
>>
>> I see your point, however we must get address of GITS_TRANSLATER and
>> set it to PCIe host. How about adding necessary comments here?
> 
> This seems to just be static setup that should be done before Linux
> is even loaded. Any reason you can't do it that way?
>

There are some ITSs in Hip05-D02 platform, in fact, we can use any of them
as a msi-controller,  which we can configure in dts. I am afraid that
hard-setting the value in BIOS would lead to restrictions in terms of 
flexibility,
as with the current implementation the same BIOS-driver can fit different
DTS structures.

Regards,
Zhou

>   Arnd
> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Gabriele Paoloni



> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Wednesday, October 14, 2015 10:04 AM
> To: Gabriele Paoloni
> Cc: Wangzhou (B); Bjorn Helgaas; Bjorn Helgaas; jingooh...@gmail.com;
> pratyush.an...@gmail.com; li...@arm.linux.org.uk;
> thomas.petazz...@free-electrons.com; lorenzo.pieral...@arm.com;
> james.mo...@arm.com; liviu.du...@arm.com; ja...@lakedaemon.net;
> r...@kernel.org; gabriel.fernan...@linaro.org;
> minghuan.l...@freescale.com; linux-...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; zhangjukuo; qiuzhenfa; liudongdong (C);
> qiujiang; xuwei (O); Liguozhu (Kenneth); Wangkefeng (Kevin); Rob
> Herring
> Subject: Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for
> HiSilicon SoC Hip05
> 
> On Wednesday 14 October 2015 08:34:43 Gabriele Paoloni wrote:
> > > -Original Message-
> > > From: Arnd Bergmann [mailto:a...@arndb.de]
> > > Sent: Tuesday, October 13, 2015 12:19 PM
> > > To: Gabriele Paoloni
> > > Cc: Wangzhou (B); Bjorn Helgaas; Bjorn Helgaas;
> jingooh...@gmail.com;
> > > pratyush.an...@gmail.com; li...@arm.linux.org.uk;
> > > thomas.petazz...@free-electrons.com; lorenzo.pieral...@arm.com;
> > > james.mo...@arm.com; liviu.du...@arm.com; ja...@lakedaemon.net;
> > > r...@kernel.org; gabriel.fernan...@linaro.org;
> > > minghuan.l...@freescale.com; linux-...@vger.kernel.org; linux-arm-
> > > ker...@lists.infradead.org; devicet...@vger.kernel.org; linux-
> > > ker...@vger.kernel.org; zhangjukuo; qiuzhenfa; liudongdong (C);
> > > qiujiang; xuwei (O); Liguozhu (Kenneth); Wangkefeng (Kevin); Rob
> > > Herring
> > > Subject: Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for
> > > HiSilicon SoC Hip05
> > >
> > > On Tuesday 13 October 2015 06:58:42 Gabriele Paoloni wrote:
> > > >
> > > > > >> +
> > > > > >> +static int __init hisi_pcie_init(void)
> > > > > >> +{
> > > > > >> +  return platform_driver_probe(_pcie_driver,
> > > hisi_pcie_probe);
> > > > > >> +}
> > > > > >> +subsys_initcall(hisi_pcie_init);
> > > > > >
> > > > > > Can you use module_platform_driver() or
> > > module_platform_driver_probe()
> > > > > > here instead of the subsys_initcall()?  No, I don't really
> know
> > > what
> > > > > > the difference between module_platform_driver() and
> > > > > > module_platform_driver_probe() is, sorry
> > >
> > > module_platform_driver_probe() will only call the probe function
> once
> > > (and fail in case of -EPROBE_DEFER), while module_platform_driver()
> > > installs the platform driver in a way that the device can be bound
> > > and unbound at any point.
> > >
> > > > > In fact, I used module_platform_driver_probe in previous
> version,
> > > but
> > > > > A PCIe VGA card of HiSilicon will use Hip05 PCIe host, so we
> > > modified
> > > > > module_platform_driver_probe to subsys_initcall which will be
> > > called
> > > > > before module_platform_driver_probe.
> > > > >
> > > > > We will upstream the driver of above PCIe VGA card soon.
> > >
> > > I don't see a reason why a VGA driver would need the PCI host to be
> > > probed this early, unless it is the only usable console in the
> system.
> > >
> > > > Hi Bjorn, firstly many thanks for looking at this.
> > > >
> > > > About this last bit the reason why we use subsys_initcall() is
> that
> > > > our host bridge is embedded in the SoC and as such is not hot-
> > > pluggable
> > > > for instance see:
> > > > http://lxr.free-electrons.com/source/Documentation/driver-
> > > model/platform.txt#L59
> > >
> > > We should still be able to build the driver as a loadable module,
> > > even if you don't do that on your own kernels.
> >
> > Hi Arnd, I don't see the point of having loadable KOs for platform
> > devices that are integrated into SoCs (like PCIe Host Controllers...)
> 
> Mainly we want as many drivers as possible to be loadable modules,
> and there is no reason why PCI needs to be different from other
> subsystems here.

Ok I see now. Thanks

> 
> > > This doesn't mean that it has to be module_platform_driver,
> > > subsys_initcall
> > >

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Arnd Bergmann

On Wednesday 14 October 2015 16:59:03 Zhou Wang wrote:
> 
> Hi Arnd,
> 
> In Hip05 PCIe host, it uses GITS_TRANSLATER's address to get TLP package
> which contains MSI address and MSI data, and then combine BDF and MSI data
> to a 32 bit data which will be writen to GITS_TRANSLATER register of ITS.
> 
> I think maybe this is a defect of our PCIe controller.

I'd consider it a bug in the firmware if this is not set up correctly
before boot.

> > I don't think what you do here is safe because the 'reg' property
> > of the MSI controller might point to the address that is used for
> > the message directly.
> 
> I see your point, however we must get address of GITS_TRANSLATER and
> set it to PCIe host. How about adding necessary comments here?

This seems to just be static setup that should be done before Linux
is even loaded. Any reason you can't do it that way?

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Arnd Bergmann

On Wednesday 14 October 2015 08:34:43 Gabriele Paoloni wrote:
> > -Original Message-
> > From: Arnd Bergmann [mailto:a...@arndb.de]
> > Sent: Tuesday, October 13, 2015 12:19 PM
> > To: Gabriele Paoloni
> > Cc: Wangzhou (B); Bjorn Helgaas; Bjorn Helgaas; jingooh...@gmail.com;
> > pratyush.an...@gmail.com; li...@arm.linux.org.uk;
> > thomas.petazz...@free-electrons.com; lorenzo.pieral...@arm.com;
> > james.mo...@arm.com; liviu.du...@arm.com; ja...@lakedaemon.net;
> > r...@kernel.org; gabriel.fernan...@linaro.org;
> > minghuan.l...@freescale.com; linux-...@vger.kernel.org; linux-arm-
> > ker...@lists.infradead.org; devicet...@vger.kernel.org; linux-
> > ker...@vger.kernel.org; zhangjukuo; qiuzhenfa; liudongdong (C);
> > qiujiang; xuwei (O); Liguozhu (Kenneth); Wangkefeng (Kevin); Rob
> > Herring
> > Subject: Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for
> > HiSilicon SoC Hip05
> > 
> > On Tuesday 13 October 2015 06:58:42 Gabriele Paoloni wrote:
> > >
> > > > >> +
> > > > >> +static int __init hisi_pcie_init(void)
> > > > >> +{
> > > > >> +  return platform_driver_probe(_pcie_driver,
> > hisi_pcie_probe);
> > > > >> +}
> > > > >> +subsys_initcall(hisi_pcie_init);
> > > > >
> > > > > Can you use module_platform_driver() or
> > module_platform_driver_probe()
> > > > > here instead of the subsys_initcall()?  No, I don't really know
> > what
> > > > > the difference between module_platform_driver() and
> > > > > module_platform_driver_probe() is, sorry 
> > 
> > module_platform_driver_probe() will only call the probe function once
> > (and fail in case of -EPROBE_DEFER), while module_platform_driver()
> > installs the platform driver in a way that the device can be bound
> > and unbound at any point.
> > 
> > > > In fact, I used module_platform_driver_probe in previous version,
> > but
> > > > A PCIe VGA card of HiSilicon will use Hip05 PCIe host, so we
> > modified
> > > > module_platform_driver_probe to subsys_initcall which will be
> > called
> > > > before module_platform_driver_probe.
> > > >
> > > > We will upstream the driver of above PCIe VGA card soon.
> > 
> > I don't see a reason why a VGA driver would need the PCI host to be
> > probed this early, unless it is the only usable console in the system.
> > 
> > > Hi Bjorn, firstly many thanks for looking at this.
> > >
> > > About this last bit the reason why we use subsys_initcall() is that
> > > our host bridge is embedded in the SoC and as such is not hot-
> > pluggable
> > > for instance see:
> > > http://lxr.free-electrons.com/source/Documentation/driver-
> > model/platform.txt#L59
> > 
> > We should still be able to build the driver as a loadable module,
> > even if you don't do that on your own kernels.
> 
> Hi Arnd, I don't see the point of having loadable KOs for platform
> devices that are integrated into SoCs (like PCIe Host Controllers...)

Mainly we want as many drivers as possible to be loadable modules,
and there is no reason why PCI needs to be different from other
subsystems here.

> > This doesn't mean that it has to be module_platform_driver,
> > subsys_initcall
> > will also work in a loadable module, it just won't be as early. However,
> > we should try to come up with a consistent approach for all PCI host
> > drivers,
> > I don't see any reason for hisi to be different from the others here.
> 
> To me it sounds more appropriate to adopt subsys_initcall() for all the
> PCI Host Bridge controllers rather than having them as loadable modules...
> 
> What is your view?

subsys_initcall() sounds odd because it's a driver rather than a subsystem,
but I realize that most of the other levels don't fit any better.

As I said, it's not really a choice we have to make in the source code,
we can use subsys_initcall together with module_exit(), or we can
create a helper macro that is similar to module_platform_driver()
specifically for PCI that uses a particular initcall level.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Zhou Wang

On 2015/10/13 23:00, Arnd Bergmann wrote:
> On Tuesday 13 October 2015 14:49:07 Gabriele Paoloni wrote:
>>> On Monday 12 October 2015 16:35:45 Bjorn Helgaas wrote:

> +{
> + u64 addr;
> + struct device_node *msi_node;
> + struct resource res;
> + struct device_node *np = pp->dev->of_node;
> + struct hisi_pcie *pcie = to_hisi_pcie(pp);
> +
> + msi_node = of_parse_phandle(np, "msi-parent", 0);
> + if (!msi_node) {
> + dev_err(pp->dev, "failed to find msi-parent\n");
> + return -EINVAL;
> + }
> + of_address_to_resource(msi_node, 0, );

 Does this use the "msi-parent" node in the same way as other drivers
 do?  I'm sure there must be other places where we extract struct
 resource information from an "msi-parent" node, but I don't see them.

 I'm trying to verify that this isn't some kind of incompatible
 extension of the "msi-parent" property.  I cc'd Arnd and Rob (DT
 experts).
>>>
>>> This is not ok, what this does is that it relies on a particular
>>> implementation of the MSI controller and directly accesses its
>>> registers.
>>
>> Hi Arnd, thanks for reviewing.
>>
>> What we do is to retrieve the msi-parent physical address and we store it
>> in our internal PCIe register locations...
> 
> Ah, thanks for the clarification, I missed that part.
> 
>> So we do not operate directly on the msi controller registers...
>>
>> So I wonder if the current implementation is Ok to retrieve the
>> msi-parent address
> 
> Not sure. Why do you do this? Normally the PCI host does not know
> or care about the address of the MSI controller, because the messages
> are just passed up to the parent bus as DMA transfers.
>

Hi Arnd,

In Hip05 PCIe host, it uses GITS_TRANSLATER's address to get TLP package
which contains MSI address and MSI data, and then combine BDF and MSI data
to a 32 bit data which will be writen to GITS_TRANSLATER register of ITS.

I think maybe this is a defect of our PCIe controller.

> I don't think what you do here is safe because the 'reg' property
> of the MSI controller might point to the address that is used for
> the message directly.

I see your point, however we must get address of GITS_TRANSLATER and set it to 
PCIe
host. How about adding necessary comments here?

Best Regards,
Zhou

>
>   Arnd
> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Gabriele Paoloni



> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Tuesday, October 13, 2015 12:19 PM
> To: Gabriele Paoloni
> Cc: Wangzhou (B); Bjorn Helgaas; Bjorn Helgaas; jingooh...@gmail.com;
> pratyush.an...@gmail.com; li...@arm.linux.org.uk;
> thomas.petazz...@free-electrons.com; lorenzo.pieral...@arm.com;
> james.mo...@arm.com; liviu.du...@arm.com; ja...@lakedaemon.net;
> r...@kernel.org; gabriel.fernan...@linaro.org;
> minghuan.l...@freescale.com; linux-...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; zhangjukuo; qiuzhenfa; liudongdong (C);
> qiujiang; xuwei (O); Liguozhu (Kenneth); Wangkefeng (Kevin); Rob
> Herring
> Subject: Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for
> HiSilicon SoC Hip05
> 
> On Tuesday 13 October 2015 06:58:42 Gabriele Paoloni wrote:
> >
> > > >> +
> > > >> +static int __init hisi_pcie_init(void)
> > > >> +{
> > > >> +  return platform_driver_probe(_pcie_driver,
> hisi_pcie_probe);
> > > >> +}
> > > >> +subsys_initcall(hisi_pcie_init);
> > > >
> > > > Can you use module_platform_driver() or
> module_platform_driver_probe()
> > > > here instead of the subsys_initcall()?  No, I don't really know
> what
> > > > the difference between module_platform_driver() and
> > > > module_platform_driver_probe() is, sorry :)
> 
> module_platform_driver_probe() will only call the probe function once
> (and fail in case of -EPROBE_DEFER), while module_platform_driver()
> installs the platform driver in a way that the device can be bound
> and unbound at any point.
> 
> > > In fact, I used module_platform_driver_probe in previous version,
> but
> > > A PCIe VGA card of HiSilicon will use Hip05 PCIe host, so we
> modified
> > > module_platform_driver_probe to subsys_initcall which will be
> called
> > > before module_platform_driver_probe.
> > >
> > > We will upstream the driver of above PCIe VGA card soon.
> 
> I don't see a reason why a VGA driver would need the PCI host to be
> probed this early, unless it is the only usable console in the system.
> 
> > Hi Bjorn, firstly many thanks for looking at this.
> >
> > About this last bit the reason why we use subsys_initcall() is that
> > our host bridge is embedded in the SoC and as such is not hot-
> pluggable
> > for instance see:
> > http://lxr.free-electrons.com/source/Documentation/driver-
> model/platform.txt#L59
> 
> We should still be able to build the driver as a loadable module,
> even if you don't do that on your own kernels.

Hi Arnd, I don't see the point of having loadable KOs for platform
devices that are integrated into SoCs (like PCIe Host Controllers...)

> 
> This doesn't mean that it has to be module_platform_driver,
> subsys_initcall
> will also work in a loadable module, it just won't be as early. However,
> we should try to come up with a consistent approach for all PCI host
> drivers,
> I don't see any reason for hisi to be different from the others here.

To me it sounds more appropriate to adopt subsys_initcall() for all the
PCI Host Bridge controllers rather than having them as loadable modules...

What is your view?

Thanks

Gab

> 
> > Our approach is also used by Exynos:
> > http://lxr.free-electrons.com/source/drivers/pci/host/pci-
> exynos.c#L644
> 
> Maybe we should change that too.
> 
>   Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Arnd Bergmann

On Wednesday 14 October 2015 09:31:48 Gabriele Paoloni wrote:
> > 
> > > To me it sounds more appropriate to adopt subsys_initcall() for all
> > the
> > > PCI Host Bridge controllers rather than having them as loadable
> > modules...
> > >
> > > What is your view?
> > 
> > subsys_initcall() sounds odd because it's a driver rather than a
> > subsystem,
> > but I realize that most of the other levels don't fit any better.
> 
> Yes well I was seeing for example the vgaarb 
> http://lxr.free-electrons.com/source/drivers/gpu/vga/vgaarb.c#L1357
> 
> That in the init is calling pci_get_subsys()
> 
> So I was wondering that the PCI devices may not be registered unless
> we also init the PCI host bridge through subsys_initcall()...

I think this should work as is: the code first looks for devices
that are already there and then registers a notifier for devices
that show up later. This is meant to work for both devices that
are hotplugged at a later point as well as PCI buses that are
already there but not yet probed.

> But then maybe is the vgaarb to be buggy...

Possible. It may well be that the code is only tested on x86,
which always probes its PCI very early.

> > As I said, it's not really a choice we have to make in the source code,
> > we can use subsys_initcall together with module_exit(), or we can
> > create a helper macro that is similar to module_platform_driver()
> > specifically for PCI that uses a particular initcall level.
> 
> Ok got it. But I guess this needs to be thought and applied to all
> the PCI host bridge controllers...
> 
> So maybe for this driver I can use module_platform_driver_probe()
> and then we can see...

Sounds good. Let's focus on getting the driver merged first and
then follow up with a patch to get this right for all PCI hosts.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Gabriele Paoloni



> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Tuesday, October 13, 2015 12:19 PM
> To: Gabriele Paoloni
> Cc: Wangzhou (B); Bjorn Helgaas; Bjorn Helgaas; jingooh...@gmail.com;
> pratyush.an...@gmail.com; li...@arm.linux.org.uk;
> thomas.petazz...@free-electrons.com; lorenzo.pieral...@arm.com;
> james.mo...@arm.com; liviu.du...@arm.com; ja...@lakedaemon.net;
> r...@kernel.org; gabriel.fernan...@linaro.org;
> minghuan.l...@freescale.com; linux-...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; zhangjukuo; qiuzhenfa; liudongdong (C);
> qiujiang; xuwei (O); Liguozhu (Kenneth); Wangkefeng (Kevin); Rob
> Herring
> Subject: Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for
> HiSilicon SoC Hip05
> 
> On Tuesday 13 October 2015 06:58:42 Gabriele Paoloni wrote:
> >
> > > >> +
> > > >> +static int __init hisi_pcie_init(void)
> > > >> +{
> > > >> +  return platform_driver_probe(_pcie_driver,
> hisi_pcie_probe);
> > > >> +}
> > > >> +subsys_initcall(hisi_pcie_init);
> > > >
> > > > Can you use module_platform_driver() or
> module_platform_driver_probe()
> > > > here instead of the subsys_initcall()?  No, I don't really know
> what
> > > > the difference between module_platform_driver() and
> > > > module_platform_driver_probe() is, sorry :)
> 
> module_platform_driver_probe() will only call the probe function once
> (and fail in case of -EPROBE_DEFER), while module_platform_driver()
> installs the platform driver in a way that the device can be bound
> and unbound at any point.
> 
> > > In fact, I used module_platform_driver_probe in previous version,
> but
> > > A PCIe VGA card of HiSilicon will use Hip05 PCIe host, so we
> modified
> > > module_platform_driver_probe to subsys_initcall which will be
> called
> > > before module_platform_driver_probe.
> > >
> > > We will upstream the driver of above PCIe VGA card soon.
> 
> I don't see a reason why a VGA driver would need the PCI host to be
> probed this early, unless it is the only usable console in the system.
> 
> > Hi Bjorn, firstly many thanks for looking at this.
> >
> > About this last bit the reason why we use subsys_initcall() is that
> > our host bridge is embedded in the SoC and as such is not hot-
> pluggable
> > for instance see:
> > http://lxr.free-electrons.com/source/Documentation/driver-
> model/platform.txt#L59
> 
> We should still be able to build the driver as a loadable module,
> even if you don't do that on your own kernels.

Hi Arnd, I don't see the point of having loadable KOs for platform
devices that are integrated into SoCs (like PCIe Host Controllers...)

> 
> This doesn't mean that it has to be module_platform_driver,
> subsys_initcall
> will also work in a loadable module, it just won't be as early. However,
> we should try to come up with a consistent approach for all PCI host
> drivers,
> I don't see any reason for hisi to be different from the others here.

To me it sounds more appropriate to adopt subsys_initcall() for all the
PCI Host Bridge controllers rather than having them as loadable modules...

What is your view?

Thanks

Gab

> 
> > Our approach is also used by Exynos:
> > http://lxr.free-electrons.com/source/drivers/pci/host/pci-
> exynos.c#L644
> 
> Maybe we should change that too.
> 
>   Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Arnd Bergmann

On Wednesday 14 October 2015 08:34:43 Gabriele Paoloni wrote:
> > -Original Message-
> > From: Arnd Bergmann [mailto:a...@arndb.de]
> > Sent: Tuesday, October 13, 2015 12:19 PM
> > To: Gabriele Paoloni
> > Cc: Wangzhou (B); Bjorn Helgaas; Bjorn Helgaas; jingooh...@gmail.com;
> > pratyush.an...@gmail.com; li...@arm.linux.org.uk;
> > thomas.petazz...@free-electrons.com; lorenzo.pieral...@arm.com;
> > james.mo...@arm.com; liviu.du...@arm.com; ja...@lakedaemon.net;
> > r...@kernel.org; gabriel.fernan...@linaro.org;
> > minghuan.l...@freescale.com; linux-...@vger.kernel.org; linux-arm-
> > ker...@lists.infradead.org; devicet...@vger.kernel.org; linux-
> > ker...@vger.kernel.org; zhangjukuo; qiuzhenfa; liudongdong (C);
> > qiujiang; xuwei (O); Liguozhu (Kenneth); Wangkefeng (Kevin); Rob
> > Herring
> > Subject: Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for
> > HiSilicon SoC Hip05
> > 
> > On Tuesday 13 October 2015 06:58:42 Gabriele Paoloni wrote:
> > >
> > > > >> +
> > > > >> +static int __init hisi_pcie_init(void)
> > > > >> +{
> > > > >> +  return platform_driver_probe(_pcie_driver,
> > hisi_pcie_probe);
> > > > >> +}
> > > > >> +subsys_initcall(hisi_pcie_init);
> > > > >
> > > > > Can you use module_platform_driver() or
> > module_platform_driver_probe()
> > > > > here instead of the subsys_initcall()?  No, I don't really know
> > what
> > > > > the difference between module_platform_driver() and
> > > > > module_platform_driver_probe() is, sorry 
> > 
> > module_platform_driver_probe() will only call the probe function once
> > (and fail in case of -EPROBE_DEFER), while module_platform_driver()
> > installs the platform driver in a way that the device can be bound
> > and unbound at any point.
> > 
> > > > In fact, I used module_platform_driver_probe in previous version,
> > but
> > > > A PCIe VGA card of HiSilicon will use Hip05 PCIe host, so we
> > modified
> > > > module_platform_driver_probe to subsys_initcall which will be
> > called
> > > > before module_platform_driver_probe.
> > > >
> > > > We will upstream the driver of above PCIe VGA card soon.
> > 
> > I don't see a reason why a VGA driver would need the PCI host to be
> > probed this early, unless it is the only usable console in the system.
> > 
> > > Hi Bjorn, firstly many thanks for looking at this.
> > >
> > > About this last bit the reason why we use subsys_initcall() is that
> > > our host bridge is embedded in the SoC and as such is not hot-
> > pluggable
> > > for instance see:
> > > http://lxr.free-electrons.com/source/Documentation/driver-
> > model/platform.txt#L59
> > 
> > We should still be able to build the driver as a loadable module,
> > even if you don't do that on your own kernels.
> 
> Hi Arnd, I don't see the point of having loadable KOs for platform
> devices that are integrated into SoCs (like PCIe Host Controllers...)

Mainly we want as many drivers as possible to be loadable modules,
and there is no reason why PCI needs to be different from other
subsystems here.

> > This doesn't mean that it has to be module_platform_driver,
> > subsys_initcall
> > will also work in a loadable module, it just won't be as early. However,
> > we should try to come up with a consistent approach for all PCI host
> > drivers,
> > I don't see any reason for hisi to be different from the others here.
> 
> To me it sounds more appropriate to adopt subsys_initcall() for all the
> PCI Host Bridge controllers rather than having them as loadable modules...
> 
> What is your view?

subsys_initcall() sounds odd because it's a driver rather than a subsystem,
but I realize that most of the other levels don't fit any better.

As I said, it's not really a choice we have to make in the source code,
we can use subsys_initcall together with module_exit(), or we can
create a helper macro that is similar to module_platform_driver()
specifically for PCI that uses a particular initcall level.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Gabriele Paoloni



> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Wednesday, October 14, 2015 10:42 AM
> To: Gabriele Paoloni
> Cc: Wangzhou (B); Bjorn Helgaas; Bjorn Helgaas; jingooh...@gmail.com;
> pratyush.an...@gmail.com; li...@arm.linux.org.uk;
> thomas.petazz...@free-electrons.com; lorenzo.pieral...@arm.com;
> james.mo...@arm.com; liviu.du...@arm.com; ja...@lakedaemon.net;
> r...@kernel.org; gabriel.fernan...@linaro.org;
> minghuan.l...@freescale.com; linux-...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; zhangjukuo; qiuzhenfa; liudongdong (C);
> qiujiang; xuwei (O); Liguozhu (Kenneth); Wangkefeng (Kevin); Rob
> Herring
> Subject: Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for
> HiSilicon SoC Hip05
> 
> On Wednesday 14 October 2015 09:31:48 Gabriele Paoloni wrote:
> > >
> > > > To me it sounds more appropriate to adopt subsys_initcall() for
> all
> > > the
> > > > PCI Host Bridge controllers rather than having them as loadable
> > > modules...
> > > >
> > > > What is your view?
> > >
> > > subsys_initcall() sounds odd because it's a driver rather than a
> > > subsystem,
> > > but I realize that most of the other levels don't fit any better.
> >
> > Yes well I was seeing for example the vgaarb
> > http://lxr.free-electrons.com/source/drivers/gpu/vga/vgaarb.c#L1357
> >
> > That in the init is calling pci_get_subsys()
> >
> > So I was wondering that the PCI devices may not be registered unless
> > we also init the PCI host bridge through subsys_initcall()...
> 
> I think this should work as is: the code first looks for devices
> that are already there and then registers a notifier for devices
> that show up later. This is meant to work for both devices that
> are hotplugged at a later point as well as PCI buses that are
> already there but not yet probed.
> 
> > But then maybe is the vgaarb to be buggy...
> 
> Possible. It may well be that the code is only tested on x86,
> which always probes its PCI very early.
> 
> > > As I said, it's not really a choice we have to make in the source
> code,
> > > we can use subsys_initcall together with module_exit(), or we can
> > > create a helper macro that is similar to module_platform_driver()
> > > specifically for PCI that uses a particular initcall level.
> >
> > Ok got it. But I guess this needs to be thought and applied to all
> > the PCI host bridge controllers...
> >
> > So maybe for this driver I can use module_platform_driver_probe()
> > and then we can see...
> 
> Sounds good. Let's focus on getting the driver merged first and
> then follow up with a patch to get this right for all PCI hosts.

Ok good let's do like this then

will change in v11

Again Many Thanks

Gab

> 
>   Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Zhou Wang

On 2015/10/13 23:00, Arnd Bergmann wrote:
> On Tuesday 13 October 2015 14:49:07 Gabriele Paoloni wrote:
>>> On Monday 12 October 2015 16:35:45 Bjorn Helgaas wrote:

> +{
> + u64 addr;
> + struct device_node *msi_node;
> + struct resource res;
> + struct device_node *np = pp->dev->of_node;
> + struct hisi_pcie *pcie = to_hisi_pcie(pp);
> +
> + msi_node = of_parse_phandle(np, "msi-parent", 0);
> + if (!msi_node) {
> + dev_err(pp->dev, "failed to find msi-parent\n");
> + return -EINVAL;
> + }
> + of_address_to_resource(msi_node, 0, );

 Does this use the "msi-parent" node in the same way as other drivers
 do?  I'm sure there must be other places where we extract struct
 resource information from an "msi-parent" node, but I don't see them.

 I'm trying to verify that this isn't some kind of incompatible
 extension of the "msi-parent" property.  I cc'd Arnd and Rob (DT
 experts).
>>>
>>> This is not ok, what this does is that it relies on a particular
>>> implementation of the MSI controller and directly accesses its
>>> registers.
>>
>> Hi Arnd, thanks for reviewing.
>>
>> What we do is to retrieve the msi-parent physical address and we store it
>> in our internal PCIe register locations...
> 
> Ah, thanks for the clarification, I missed that part.
> 
>> So we do not operate directly on the msi controller registers...
>>
>> So I wonder if the current implementation is Ok to retrieve the
>> msi-parent address
> 
> Not sure. Why do you do this? Normally the PCI host does not know
> or care about the address of the MSI controller, because the messages
> are just passed up to the parent bus as DMA transfers.
>

Hi Arnd,

In Hip05 PCIe host, it uses GITS_TRANSLATER's address to get TLP package
which contains MSI address and MSI data, and then combine BDF and MSI data
to a 32 bit data which will be writen to GITS_TRANSLATER register of ITS.

I think maybe this is a defect of our PCIe controller.

> I don't think what you do here is safe because the 'reg' property
> of the MSI controller might point to the address that is used for
> the message directly.

I see your point, however we must get address of GITS_TRANSLATER and set it to 
PCIe
host. How about adding necessary comments here?

Best Regards,
Zhou

>
>   Arnd
> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Arnd Bergmann

On Wednesday 14 October 2015 16:59:03 Zhou Wang wrote:
> 
> Hi Arnd,
> 
> In Hip05 PCIe host, it uses GITS_TRANSLATER's address to get TLP package
> which contains MSI address and MSI data, and then combine BDF and MSI data
> to a 32 bit data which will be writen to GITS_TRANSLATER register of ITS.
> 
> I think maybe this is a defect of our PCIe controller.

I'd consider it a bug in the firmware if this is not set up correctly
before boot.

> > I don't think what you do here is safe because the 'reg' property
> > of the MSI controller might point to the address that is used for
> > the message directly.
> 
> I see your point, however we must get address of GITS_TRANSLATER and
> set it to PCIe host. How about adding necessary comments here?

This seems to just be static setup that should be done before Linux
is even loaded. Any reason you can't do it that way?

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Gabriele Paoloni



> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Wednesday, October 14, 2015 10:04 AM
> To: Gabriele Paoloni
> Cc: Wangzhou (B); Bjorn Helgaas; Bjorn Helgaas; jingooh...@gmail.com;
> pratyush.an...@gmail.com; li...@arm.linux.org.uk;
> thomas.petazz...@free-electrons.com; lorenzo.pieral...@arm.com;
> james.mo...@arm.com; liviu.du...@arm.com; ja...@lakedaemon.net;
> r...@kernel.org; gabriel.fernan...@linaro.org;
> minghuan.l...@freescale.com; linux-...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; zhangjukuo; qiuzhenfa; liudongdong (C);
> qiujiang; xuwei (O); Liguozhu (Kenneth); Wangkefeng (Kevin); Rob
> Herring
> Subject: Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for
> HiSilicon SoC Hip05
> 
> On Wednesday 14 October 2015 08:34:43 Gabriele Paoloni wrote:
> > > -Original Message-
> > > From: Arnd Bergmann [mailto:a...@arndb.de]
> > > Sent: Tuesday, October 13, 2015 12:19 PM
> > > To: Gabriele Paoloni
> > > Cc: Wangzhou (B); Bjorn Helgaas; Bjorn Helgaas;
> jingooh...@gmail.com;
> > > pratyush.an...@gmail.com; li...@arm.linux.org.uk;
> > > thomas.petazz...@free-electrons.com; lorenzo.pieral...@arm.com;
> > > james.mo...@arm.com; liviu.du...@arm.com; ja...@lakedaemon.net;
> > > r...@kernel.org; gabriel.fernan...@linaro.org;
> > > minghuan.l...@freescale.com; linux-...@vger.kernel.org; linux-arm-
> > > ker...@lists.infradead.org; devicet...@vger.kernel.org; linux-
> > > ker...@vger.kernel.org; zhangjukuo; qiuzhenfa; liudongdong (C);
> > > qiujiang; xuwei (O); Liguozhu (Kenneth); Wangkefeng (Kevin); Rob
> > > Herring
> > > Subject: Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for
> > > HiSilicon SoC Hip05
> > >
> > > On Tuesday 13 October 2015 06:58:42 Gabriele Paoloni wrote:
> > > >
> > > > > >> +
> > > > > >> +static int __init hisi_pcie_init(void)
> > > > > >> +{
> > > > > >> +  return platform_driver_probe(_pcie_driver,
> > > hisi_pcie_probe);
> > > > > >> +}
> > > > > >> +subsys_initcall(hisi_pcie_init);
> > > > > >
> > > > > > Can you use module_platform_driver() or
> > > module_platform_driver_probe()
> > > > > > here instead of the subsys_initcall()?  No, I don't really
> know
> > > what
> > > > > > the difference between module_platform_driver() and
> > > > > > module_platform_driver_probe() is, sorry
> > >
> > > module_platform_driver_probe() will only call the probe function
> once
> > > (and fail in case of -EPROBE_DEFER), while module_platform_driver()
> > > installs the platform driver in a way that the device can be bound
> > > and unbound at any point.
> > >
> > > > > In fact, I used module_platform_driver_probe in previous
> version,
> > > but
> > > > > A PCIe VGA card of HiSilicon will use Hip05 PCIe host, so we
> > > modified
> > > > > module_platform_driver_probe to subsys_initcall which will be
> > > called
> > > > > before module_platform_driver_probe.
> > > > >
> > > > > We will upstream the driver of above PCIe VGA card soon.
> > >
> > > I don't see a reason why a VGA driver would need the PCI host to be
> > > probed this early, unless it is the only usable console in the
> system.
> > >
> > > > Hi Bjorn, firstly many thanks for looking at this.
> > > >
> > > > About this last bit the reason why we use subsys_initcall() is
> that
> > > > our host bridge is embedded in the SoC and as such is not hot-
> > > pluggable
> > > > for instance see:
> > > > http://lxr.free-electrons.com/source/Documentation/driver-
> > > model/platform.txt#L59
> > >
> > > We should still be able to build the driver as a loadable module,
> > > even if you don't do that on your own kernels.
> >
> > Hi Arnd, I don't see the point of having loadable KOs for platform
> > devices that are integrated into SoCs (like PCIe Host Controllers...)
> 
> Mainly we want as many drivers as possible to be loadable modules,
> and there is no reason why PCI needs to be different from other
> subsystems here.

Ok I see now. Thanks

> 
> > > This doesn't mean that it has to be module_platform_driver,
> > > subsys_initcall
> > >

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Zhou Wang

On 2015/10/14 17:06, Arnd Bergmann wrote:
> On Wednesday 14 October 2015 16:59:03 Zhou Wang wrote:
>>
>> Hi Arnd,
>>
>> In Hip05 PCIe host, it uses GITS_TRANSLATER's address to get TLP package
>> which contains MSI address and MSI data, and then combine BDF and MSI data
>> to a 32 bit data which will be writen to GITS_TRANSLATER register of ITS.
>>
>> I think maybe this is a defect of our PCIe controller.
> 
> I'd consider it a bug in the firmware if this is not set up correctly
> before boot.
> 
>>> I don't think what you do here is safe because the 'reg' property
>>> of the MSI controller might point to the address that is used for
>>> the message directly.
>>
>> I see your point, however we must get address of GITS_TRANSLATER and
>> set it to PCIe host. How about adding necessary comments here?
> 
> This seems to just be static setup that should be done before Linux
> is even loaded. Any reason you can't do it that way?
>

There are some ITSs in Hip05-D02 platform, in fact, we can use any of them
as a msi-controller,  which we can configure in dts. I am afraid that
hard-setting the value in BIOS would lead to restrictions in terms of 
flexibility,
as with the current implementation the same BIOS-driver can fit different
DTS structures.

Regards,
Zhou

>   Arnd
> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-14 Thread Arnd Bergmann

On Wednesday 14 October 2015 17:44:11 Zhou Wang wrote:
> On 2015/10/14 17:06, Arnd Bergmann wrote:
> > On Wednesday 14 October 2015 16:59:03 Zhou Wang wrote:
> >>
> >> Hi Arnd,
> >>
> >> In Hip05 PCIe host, it uses GITS_TRANSLATER's address to get TLP package
> >> which contains MSI address and MSI data, and then combine BDF and MSI data
> >> to a 32 bit data which will be writen to GITS_TRANSLATER register of ITS.
> >>
> >> I think maybe this is a defect of our PCIe controller.
> > 
> > I'd consider it a bug in the firmware if this is not set up correctly
> > before boot.
> > 
> >>> I don't think what you do here is safe because the 'reg' property
> >>> of the MSI controller might point to the address that is used for
> >>> the message directly.
> >>
> >> I see your point, however we must get address of GITS_TRANSLATER and
> >> set it to PCIe host. How about adding necessary comments here?
> > 
> > This seems to just be static setup that should be done before Linux
> > is even loaded. Any reason you can't do it that way?
> >
> 
> There are some ITSs in Hip05-D02 platform, in fact, we can use any of them
> as a msi-controller,  which we can configure in dts. I am afraid that
> hard-setting the value in BIOS would lead to restrictions in terms of 
> flexibility,
> as with the current implementation the same BIOS-driver can fit different
> DTS structures.

The dtb generally should be expected to match whatever the firmware sets up,
so if there is one reasonable setting here, I see no problem with hardcoding
it that way. In particular on server systems, we usually expect the firmware
to configure almost everything in advance and just tell us how it is
configured, while on embedded systems we can't trust the bootload and
usually set it all up in the kernel from scratch.

What would be a reason to pick one ITS over another?

On a related note, don't you also need to describe in DT how PCI B/D/F
function numbers get turned into addresses in the ITS? Does that also
require configuration in the driver? I see this code here:

+   hisi_pcie_apb_writel(pcie, PCIE_MSI_ASID_ENABLE | PCIE_MSI_ASID_VALUE,
+PCIE_SLV_MSI_ASID);
+   hisi_pcie_apb_writel(pcie, PCIE_MSI_TRANS_ENABLE, PCIE_MSI_TRANS_REG);
+   hisi_pcie_change_apb_mode(pcie, PCIE_SLV_DBI_MODE);

plus all of hisi_pcie_config_context(). This looks like it will change
the way the MSI is interpreted. This also seems like something that
could be done in the firmware in advance, and just get reported in DT.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-13 Thread Arnd Bergmann

On Tuesday 13 October 2015 14:49:07 Gabriele Paoloni wrote:
> > On Monday 12 October 2015 16:35:45 Bjorn Helgaas wrote:
> > >
> > > > +{
> > > > + u64 addr;
> > > > + struct device_node *msi_node;
> > > > + struct resource res;
> > > > + struct device_node *np = pp->dev->of_node;
> > > > + struct hisi_pcie *pcie = to_hisi_pcie(pp);
> > > > +
> > > > + msi_node = of_parse_phandle(np, "msi-parent", 0);
> > > > + if (!msi_node) {
> > > > + dev_err(pp->dev, "failed to find msi-parent\n");
> > > > + return -EINVAL;
> > > > + }
> > > > + of_address_to_resource(msi_node, 0, );
> > >
> > > Does this use the "msi-parent" node in the same way as other drivers
> > > do?  I'm sure there must be other places where we extract struct
> > > resource information from an "msi-parent" node, but I don't see them.
> > >
> > > I'm trying to verify that this isn't some kind of incompatible
> > > extension of the "msi-parent" property.  I cc'd Arnd and Rob (DT
> > > experts).
> > 
> > This is not ok, what this does is that it relies on a particular
> > implementation of the MSI controller and directly accesses its
> > registers.
> 
> Hi Arnd, thanks for reviewing.
> 
> What we do is to retrieve the msi-parent physical address and we store it
> in our internal PCIe register locations...

Ah, thanks for the clarification, I missed that part.

> So we do not operate directly on the msi controller registers...
> 
> So I wonder if the current implementation is Ok to retrieve the
> msi-parent address

Not sure. Why do you do this? Normally the PCI host does not know
or care about the address of the MSI controller, because the messages
are just passed up to the parent bus as DMA transfers.

I don't think what you do here is safe because the 'reg' property
of the MSI controller might point to the address that is used for
the message directly.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-13 Thread Gabriele Paoloni



> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Tuesday, October 13, 2015 12:12 PM
> To: Bjorn Helgaas
> Cc: Wangzhou (B); Bjorn Helgaas; jingooh...@gmail.com;
> pratyush.an...@gmail.com; li...@arm.linux.org.uk;
> thomas.petazz...@free-electrons.com; Gabriele Paoloni;
> lorenzo.pieral...@arm.com; james.mo...@arm.com; liviu.du...@arm.com;
> ja...@lakedaemon.net; r...@kernel.org; gabriel.fernan...@linaro.org;
> minghuan.l...@freescale.com; linux-...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; zhangjukuo; qiuzhenfa; liudongdong (C);
> qiujiang; xuwei (O); Liguozhu (Kenneth); Wangkefeng (Kevin); Rob
> Herring
> Subject: Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for
> HiSilicon SoC Hip05
> 
> On Monday 12 October 2015 16:35:45 Bjorn Helgaas wrote:
> >
> > > +{
> > > + u64 addr;
> > > + struct device_node *msi_node;
> > > + struct resource res;
> > > + struct device_node *np = pp->dev->of_node;
> > > + struct hisi_pcie *pcie = to_hisi_pcie(pp);
> > > +
> > > + msi_node = of_parse_phandle(np, "msi-parent", 0);
> > > + if (!msi_node) {
> > > + dev_err(pp->dev, "failed to find msi-parent\n");
> > > + return -EINVAL;
> > > + }
> > > + of_address_to_resource(msi_node, 0, );
> >
> > Does this use the "msi-parent" node in the same way as other drivers
> > do?  I'm sure there must be other places where we extract struct
> > resource information from an "msi-parent" node, but I don't see them.
> >
> > I'm trying to verify that this isn't some kind of incompatible
> > extension of the "msi-parent" property.  I cc'd Arnd and Rob (DT
> > experts).
> 
> This is not ok, what this does is that it relies on a particular
> implementation of the MSI controller and directly accesses its
> registers.

Hi Arnd, thanks for reviewing.

What we do is to retrieve the msi-parent physical address and we store it
in our internal PCIe register locations...

So we do not operate directly on the msi controller registers...

So I wonder if the current implementation is Ok to retrieve the
msi-parent address

Thanks

Gab


> 
> Instead, it should reference only the msi irq domain and let the
> driver for the MSI controller access the registers. Otherwise this
> code has to be rewritten once the same PCI host code appears in
> a machine that has a real GICv2m or GICv3.
> 
>   Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-13 Thread Arnd Bergmann

On Tuesday 13 October 2015 06:58:42 Gabriele Paoloni wrote:
> 
> > >> +
> > >> +static int __init hisi_pcie_init(void)
> > >> +{
> > >> +return platform_driver_probe(_pcie_driver, 
> > >> hisi_pcie_probe);
> > >> +}
> > >> +subsys_initcall(hisi_pcie_init);
> > >
> > > Can you use module_platform_driver() or module_platform_driver_probe()
> > > here instead of the subsys_initcall()?  No, I don't really know what
> > > the difference between module_platform_driver() and
> > > module_platform_driver_probe() is, sorry :)

module_platform_driver_probe() will only call the probe function once
(and fail in case of -EPROBE_DEFER), while module_platform_driver()
installs the platform driver in a way that the device can be bound
and unbound at any point.

> > In fact, I used module_platform_driver_probe in previous version, but
> > A PCIe VGA card of HiSilicon will use Hip05 PCIe host, so we modified
> > module_platform_driver_probe to subsys_initcall which will be called
> > before module_platform_driver_probe.
> > 
> > We will upstream the driver of above PCIe VGA card soon.

I don't see a reason why a VGA driver would need the PCI host to be
probed this early, unless it is the only usable console in the system.

> Hi Bjorn, firstly many thanks for looking at this.
> 
> About this last bit the reason why we use subsys_initcall() is that
> our host bridge is embedded in the SoC and as such is not hot-pluggable
> for instance see:
> http://lxr.free-electrons.com/source/Documentation/driver-model/platform.txt#L59

We should still be able to build the driver as a loadable module,
even if you don't do that on your own kernels. 

This doesn't mean that it has to be module_platform_driver, subsys_initcall
will also work in a loadable module, it just won't be as early. However,
we should try to come up with a consistent approach for all PCI host drivers,
I don't see any reason for hisi to be different from the others here.

> Our approach is also used by Exynos:
> http://lxr.free-electrons.com/source/drivers/pci/host/pci-exynos.c#L644

Maybe we should change that too.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-13 Thread Arnd Bergmann

On Monday 12 October 2015 16:35:45 Bjorn Helgaas wrote:
> 
> > +{
> > + u64 addr;
> > + struct device_node *msi_node;
> > + struct resource res;
> > + struct device_node *np = pp->dev->of_node;
> > + struct hisi_pcie *pcie = to_hisi_pcie(pp);
> > +
> > + msi_node = of_parse_phandle(np, "msi-parent", 0);
> > + if (!msi_node) {
> > + dev_err(pp->dev, "failed to find msi-parent\n");
> > + return -EINVAL;
> > + }
> > + of_address_to_resource(msi_node, 0, );
> 
> Does this use the "msi-parent" node in the same way as other drivers
> do?  I'm sure there must be other places where we extract struct
> resource information from an "msi-parent" node, but I don't see them.
> 
> I'm trying to verify that this isn't some kind of incompatible
> extension of the "msi-parent" property.  I cc'd Arnd and Rob (DT
> experts).

This is not ok, what this does is that it relies on a particular
implementation of the MSI controller and directly accesses its
registers.

Instead, it should reference only the msi irq domain and let the
driver for the MSI controller access the registers. Otherwise this
code has to be rewritten once the same PCI host code appears in
a machine that has a real GICv2m or GICv3.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-13 Thread Gabriele Paoloni


> >> +
> >> +static int __init hisi_pcie_init(void)
> >> +{
> >> +  return platform_driver_probe(_pcie_driver, hisi_pcie_probe);
> >> +}
> >> +subsys_initcall(hisi_pcie_init);
> >
> > Can you use module_platform_driver() or module_platform_driver_probe()
> > here instead of the subsys_initcall()?  No, I don't really know what
> > the difference between module_platform_driver() and
> > module_platform_driver_probe() is, sorry :)
> 
> In fact, I used module_platform_driver_probe in previous version, but
> A PCIe VGA card of HiSilicon will use Hip05 PCIe host, so we modified
> module_platform_driver_probe to subsys_initcall which will be called
> before module_platform_driver_probe.
> 
> We will upstream the driver of above PCIe VGA card soon.
> 
> Many thanks for your reviewing,
> Zhou

Hi Bjorn, firstly many thanks for looking at this.

About this last bit the reason why we use subsys_initcall() is that
our host bridge is embedded in the SoC and as such is not hot-pluggable
for instance see:
http://lxr.free-electrons.com/source/Documentation/driver-model/platform.txt#L59

Our approach is also used by Exynos:
http://lxr.free-electrons.com/source/drivers/pci/host/pci-exynos.c#L644

Cheers

Gab

> 
> >
> > Bjorn
> >
> > .
> >
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-13 Thread Zhou Wang

On 2015/10/13 5:35, Bjorn Helgaas wrote:
> [+cc Arnd, Rob]
> 
> Hi Zhou,
> 
> I have a few minor comments and two questions: one about the fact
> all the config accesses are 32 bits, and another about the use of the
> "msi-parent" node.
> 
> On Sat, Oct 10, 2015 at 10:59:15AM +0800, Zhou Wang wrote:
>> This patch adds PCIe host support for HiSilicon SoC Hip05.
>>
>> Signed-off-by: Zhou Wang 
>> Signed-off-by: Gabriele Paoloni 
>> Signed-off-by: liudongdong 
>> Signed-off-by: Kefeng Wang 
>> Signed-off-by: qiuzhenfa 
> 
> This is a pretty long Signed-off-by chain.  If each of these people
> created part of this patch, that's fine.  I'm just checking whether
> that's the case, or whether some of these should be Reviewed-by,
> Suggested-by, etc., per Documentation/SubmittingPatches.
>

Hi Bjorn,

Thanks for reminding this. I had merged patches from those colleagues,
so just add Signed-off-by from them.

>> ---
>>  drivers/pci/host/Kconfig |   8 ++
>>  drivers/pci/host/Makefile|   1 +
>>  drivers/pci/host/pcie-hisi.c | 320 
>> +++
>>  3 files changed, 329 insertions(+)
>>  create mode 100644 drivers/pci/host/pcie-hisi.c
>>
>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index d5e58ba..ae873be 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -145,4 +145,12 @@ config PCIE_IPROC_BCMA
>>Say Y here if you want to use the Broadcom iProc PCIe controller
>>through the BCMA bus interface
>>  
>> +config PCI_HISI
>> +depends on OF && ARM64
>> +bool "HiSilicon SoC HIP05 PCIe controller"
>> +select PCIEPORTBUS
>> +select PCIE_DW
>> +help
>> +  Say Y here if you want PCIe controller support on HiSilicon HIP05 SoC
>> +
>>  endmenu
>> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
>> index 140d66f..ea1dbf2 100644
>> --- a/drivers/pci/host/Makefile
>> +++ b/drivers/pci/host/Makefile
>> @@ -17,3 +17,4 @@ obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
>>  obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
>>  obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
>>  obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
>> +obj-$(CONFIG_PCI_HISI) += pcie-hisi.o
>> diff --git a/drivers/pci/host/pcie-hisi.c b/drivers/pci/host/pcie-hisi.c
>> new file mode 100644
>> index 000..26aa0d9
>> --- /dev/null
>> +++ b/drivers/pci/host/pcie-hisi.c
>> @@ -0,0 +1,320 @@
>> +/*
>> + * PCIe host controller driver for HiSilicon Hip05 SoC
>> + *
>> + * Copyright (C) 2015 HiSilicon Co., Ltd. http://www.hisilicon.com
>> + *
>> + * Author: Zhou Wang 
>> + * Dacai Zhu 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "pcie-designware.h"
>> +
>> +#define PCIE_SUBCTRL_MODE_REG   0x2800
>> +#define PCIE_SUBCTRL_SYS_STATE4_REG 0x6818
>> +#define PCIE_SLV_DBI_MODE   0x0
>> +#define PCIE_SLV_SYSCTRL_MODE   0x1
>> +#define PCIE_SLV_CONTENT_MODE   0x2
>> +#define PCIE_SLV_MSI_ASID   0x10
>> +#define PCIE_LTSSM_LINKUP_STATE 0x11
>> +#define PCIE_LTSSM_STATE_MASK   0x3F
>> +#define PCIE_MSI_ASID_ENABLE(0x1 << 12)
>> +#define PCIE_MSI_ASID_VALUE (0x1 << 16)
>> +#define PCIE_MSI_TRANS_ENABLE   (0x1 << 12)
>> +#define PCIE_MSI_TRANS_REG  0x1c8
>> +#define PCIE_MSI_LOW_ADDRESS0x1b4
>> +#define PCIE_MSI_HIGH_ADDRESS   0x1c4
>> +#define PCIE_GITS_TRANSLATER0x10040
> 
> "TRANSLATOR" is the more common spelling.  But if you're matching
> hardware documentation, you should follow that.
>

This is a copy from linux/include/linux/irqchip/arm-gic-v3.h:
#define GITS_TRANSLATER 0X10040
It indicates the offset of ITS's GITS_TRANSLATER register.

>> +#define PCIE_SYS_CTRL20_REG 0x20
>> +#define PCIE_RD_TAB_SEL BIT(31)
>> +#define PCIE_RD_TAB_EN  BIT(30)
>> +
>> +#define to_hisi_pcie(x) container_of(x, struct hisi_pcie, pp)
>> +
>> +struct hisi_pcie {
>> +struct regmap *subctrl;
>> +void __iomem *reg_base;
>> +u32 port_id;
>> +struct pcie_port pp;
>> +};
>> +
>> +static inline void hisi_pcie_apb_writel(struct hisi_pcie *pcie,
>> +u32 val, u32 reg)
>> +{
>> +writel(val, pcie->reg_base + reg);
>> +}
>> +
>> +static inline u32 hisi_pcie_apb_readl(struct hisi_pcie *pcie, u32 reg)
>> +{
>>

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-13 Thread Zhou Wang

On 2015/10/13 5:35, Bjorn Helgaas wrote:
> [+cc Arnd, Rob]
> 
> Hi Zhou,
> 
> I have a few minor comments and two questions: one about the fact
> all the config accesses are 32 bits, and another about the use of the
> "msi-parent" node.
> 
> On Sat, Oct 10, 2015 at 10:59:15AM +0800, Zhou Wang wrote:
>> This patch adds PCIe host support for HiSilicon SoC Hip05.
>>
>> Signed-off-by: Zhou Wang 
>> Signed-off-by: Gabriele Paoloni 
>> Signed-off-by: liudongdong 
>> Signed-off-by: Kefeng Wang 
>> Signed-off-by: qiuzhenfa 
> 
> This is a pretty long Signed-off-by chain.  If each of these people
> created part of this patch, that's fine.  I'm just checking whether
> that's the case, or whether some of these should be Reviewed-by,
> Suggested-by, etc., per Documentation/SubmittingPatches.
>

Hi Bjorn,

Thanks for reminding this. I had merged patches from those colleagues,
so just add Signed-off-by from them.

>> ---
>>  drivers/pci/host/Kconfig |   8 ++
>>  drivers/pci/host/Makefile|   1 +
>>  drivers/pci/host/pcie-hisi.c | 320 
>> +++
>>  3 files changed, 329 insertions(+)
>>  create mode 100644 drivers/pci/host/pcie-hisi.c
>>
>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index d5e58ba..ae873be 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -145,4 +145,12 @@ config PCIE_IPROC_BCMA
>>Say Y here if you want to use the Broadcom iProc PCIe controller
>>through the BCMA bus interface
>>  
>> +config PCI_HISI
>> +depends on OF && ARM64
>> +bool "HiSilicon SoC HIP05 PCIe controller"
>> +select PCIEPORTBUS
>> +select PCIE_DW
>> +help
>> +  Say Y here if you want PCIe controller support on HiSilicon HIP05 SoC
>> +
>>  endmenu
>> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
>> index 140d66f..ea1dbf2 100644
>> --- a/drivers/pci/host/Makefile
>> +++ b/drivers/pci/host/Makefile
>> @@ -17,3 +17,4 @@ obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
>>  obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
>>  obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
>>  obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
>> +obj-$(CONFIG_PCI_HISI) += pcie-hisi.o
>> diff --git a/drivers/pci/host/pcie-hisi.c b/drivers/pci/host/pcie-hisi.c
>> new file mode 100644
>> index 000..26aa0d9
>> --- /dev/null
>> +++ b/drivers/pci/host/pcie-hisi.c
>> @@ -0,0 +1,320 @@
>> +/*
>> + * PCIe host controller driver for HiSilicon Hip05 SoC
>> + *
>> + * Copyright (C) 2015 HiSilicon Co., Ltd. http://www.hisilicon.com
>> + *
>> + * Author: Zhou Wang 
>> + * Dacai Zhu 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "pcie-designware.h"
>> +
>> +#define PCIE_SUBCTRL_MODE_REG   0x2800
>> +#define PCIE_SUBCTRL_SYS_STATE4_REG 0x6818
>> +#define PCIE_SLV_DBI_MODE   0x0
>> +#define PCIE_SLV_SYSCTRL_MODE   0x1
>> +#define PCIE_SLV_CONTENT_MODE   0x2
>> +#define PCIE_SLV_MSI_ASID   0x10
>> +#define PCIE_LTSSM_LINKUP_STATE 0x11
>> +#define PCIE_LTSSM_STATE_MASK   0x3F
>> +#define PCIE_MSI_ASID_ENABLE(0x1 << 12)
>> +#define PCIE_MSI_ASID_VALUE (0x1 << 16)
>> +#define PCIE_MSI_TRANS_ENABLE   (0x1 << 12)
>> +#define PCIE_MSI_TRANS_REG  0x1c8
>> +#define PCIE_MSI_LOW_ADDRESS0x1b4
>> +#define PCIE_MSI_HIGH_ADDRESS   0x1c4
>> +#define PCIE_GITS_TRANSLATER0x10040
> 
> "TRANSLATOR" is the more common spelling.  But if you're matching
> hardware documentation, you should follow that.
>

This is a copy from linux/include/linux/irqchip/arm-gic-v3.h:
#define GITS_TRANSLATER 0X10040
It indicates the offset of ITS's GITS_TRANSLATER register.

>> +#define PCIE_SYS_CTRL20_REG 0x20
>> +#define PCIE_RD_TAB_SEL BIT(31)
>> +#define PCIE_RD_TAB_EN  BIT(30)
>> +
>> +#define to_hisi_pcie(x) container_of(x, struct hisi_pcie, pp)
>> +
>> +struct hisi_pcie {
>> +struct regmap *subctrl;
>> +void __iomem *reg_base;
>> +u32 port_id;
>> +struct pcie_port pp;
>> +};
>> +
>> +static inline void hisi_pcie_apb_writel(struct hisi_pcie *pcie,
>> +

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-13 Thread Gabriele Paoloni


> >> +
> >> +static int __init hisi_pcie_init(void)
> >> +{
> >> +  return platform_driver_probe(_pcie_driver, hisi_pcie_probe);
> >> +}
> >> +subsys_initcall(hisi_pcie_init);
> >
> > Can you use module_platform_driver() or module_platform_driver_probe()
> > here instead of the subsys_initcall()?  No, I don't really know what
> > the difference between module_platform_driver() and
> > module_platform_driver_probe() is, sorry :)
> 
> In fact, I used module_platform_driver_probe in previous version, but
> A PCIe VGA card of HiSilicon will use Hip05 PCIe host, so we modified
> module_platform_driver_probe to subsys_initcall which will be called
> before module_platform_driver_probe.
> 
> We will upstream the driver of above PCIe VGA card soon.
> 
> Many thanks for your reviewing,
> Zhou

Hi Bjorn, firstly many thanks for looking at this.

About this last bit the reason why we use subsys_initcall() is that
our host bridge is embedded in the SoC and as such is not hot-pluggable
for instance see:
http://lxr.free-electrons.com/source/Documentation/driver-model/platform.txt#L59

Our approach is also used by Exynos:
http://lxr.free-electrons.com/source/drivers/pci/host/pci-exynos.c#L644

Cheers

Gab

> 
> >
> > Bjorn
> >
> > .
> >
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-13 Thread Arnd Bergmann

On Tuesday 13 October 2015 06:58:42 Gabriele Paoloni wrote:
> 
> > >> +
> > >> +static int __init hisi_pcie_init(void)
> > >> +{
> > >> +return platform_driver_probe(_pcie_driver, 
> > >> hisi_pcie_probe);
> > >> +}
> > >> +subsys_initcall(hisi_pcie_init);
> > >
> > > Can you use module_platform_driver() or module_platform_driver_probe()
> > > here instead of the subsys_initcall()?  No, I don't really know what
> > > the difference between module_platform_driver() and
> > > module_platform_driver_probe() is, sorry :)

module_platform_driver_probe() will only call the probe function once
(and fail in case of -EPROBE_DEFER), while module_platform_driver()
installs the platform driver in a way that the device can be bound
and unbound at any point.

> > In fact, I used module_platform_driver_probe in previous version, but
> > A PCIe VGA card of HiSilicon will use Hip05 PCIe host, so we modified
> > module_platform_driver_probe to subsys_initcall which will be called
> > before module_platform_driver_probe.
> > 
> > We will upstream the driver of above PCIe VGA card soon.

I don't see a reason why a VGA driver would need the PCI host to be
probed this early, unless it is the only usable console in the system.

> Hi Bjorn, firstly many thanks for looking at this.
> 
> About this last bit the reason why we use subsys_initcall() is that
> our host bridge is embedded in the SoC and as such is not hot-pluggable
> for instance see:
> http://lxr.free-electrons.com/source/Documentation/driver-model/platform.txt#L59

We should still be able to build the driver as a loadable module,
even if you don't do that on your own kernels. 

This doesn't mean that it has to be module_platform_driver, subsys_initcall
will also work in a loadable module, it just won't be as early. However,
we should try to come up with a consistent approach for all PCI host drivers,
I don't see any reason for hisi to be different from the others here.

> Our approach is also used by Exynos:
> http://lxr.free-electrons.com/source/drivers/pci/host/pci-exynos.c#L644

Maybe we should change that too.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-13 Thread Arnd Bergmann

On Monday 12 October 2015 16:35:45 Bjorn Helgaas wrote:
> 
> > +{
> > + u64 addr;
> > + struct device_node *msi_node;
> > + struct resource res;
> > + struct device_node *np = pp->dev->of_node;
> > + struct hisi_pcie *pcie = to_hisi_pcie(pp);
> > +
> > + msi_node = of_parse_phandle(np, "msi-parent", 0);
> > + if (!msi_node) {
> > + dev_err(pp->dev, "failed to find msi-parent\n");
> > + return -EINVAL;
> > + }
> > + of_address_to_resource(msi_node, 0, );
> 
> Does this use the "msi-parent" node in the same way as other drivers
> do?  I'm sure there must be other places where we extract struct
> resource information from an "msi-parent" node, but I don't see them.
> 
> I'm trying to verify that this isn't some kind of incompatible
> extension of the "msi-parent" property.  I cc'd Arnd and Rob (DT
> experts).

This is not ok, what this does is that it relies on a particular
implementation of the MSI controller and directly accesses its
registers.

Instead, it should reference only the msi irq domain and let the
driver for the MSI controller access the registers. Otherwise this
code has to be rewritten once the same PCI host code appears in
a machine that has a real GICv2m or GICv3.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-13 Thread Gabriele Paoloni



> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Tuesday, October 13, 2015 12:12 PM
> To: Bjorn Helgaas
> Cc: Wangzhou (B); Bjorn Helgaas; jingooh...@gmail.com;
> pratyush.an...@gmail.com; li...@arm.linux.org.uk;
> thomas.petazz...@free-electrons.com; Gabriele Paoloni;
> lorenzo.pieral...@arm.com; james.mo...@arm.com; liviu.du...@arm.com;
> ja...@lakedaemon.net; r...@kernel.org; gabriel.fernan...@linaro.org;
> minghuan.l...@freescale.com; linux-...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; zhangjukuo; qiuzhenfa; liudongdong (C);
> qiujiang; xuwei (O); Liguozhu (Kenneth); Wangkefeng (Kevin); Rob
> Herring
> Subject: Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for
> HiSilicon SoC Hip05
> 
> On Monday 12 October 2015 16:35:45 Bjorn Helgaas wrote:
> >
> > > +{
> > > + u64 addr;
> > > + struct device_node *msi_node;
> > > + struct resource res;
> > > + struct device_node *np = pp->dev->of_node;
> > > + struct hisi_pcie *pcie = to_hisi_pcie(pp);
> > > +
> > > + msi_node = of_parse_phandle(np, "msi-parent", 0);
> > > + if (!msi_node) {
> > > + dev_err(pp->dev, "failed to find msi-parent\n");
> > > + return -EINVAL;
> > > + }
> > > + of_address_to_resource(msi_node, 0, );
> >
> > Does this use the "msi-parent" node in the same way as other drivers
> > do?  I'm sure there must be other places where we extract struct
> > resource information from an "msi-parent" node, but I don't see them.
> >
> > I'm trying to verify that this isn't some kind of incompatible
> > extension of the "msi-parent" property.  I cc'd Arnd and Rob (DT
> > experts).
> 
> This is not ok, what this does is that it relies on a particular
> implementation of the MSI controller and directly accesses its
> registers.

Hi Arnd, thanks for reviewing.

What we do is to retrieve the msi-parent physical address and we store it
in our internal PCIe register locations...

So we do not operate directly on the msi controller registers...

So I wonder if the current implementation is Ok to retrieve the
msi-parent address

Thanks

Gab


> 
> Instead, it should reference only the msi irq domain and let the
> driver for the MSI controller access the registers. Otherwise this
> code has to be rewritten once the same PCI host code appears in
> a machine that has a real GICv2m or GICv3.
> 
>   Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-13 Thread Arnd Bergmann

On Tuesday 13 October 2015 14:49:07 Gabriele Paoloni wrote:
> > On Monday 12 October 2015 16:35:45 Bjorn Helgaas wrote:
> > >
> > > > +{
> > > > + u64 addr;
> > > > + struct device_node *msi_node;
> > > > + struct resource res;
> > > > + struct device_node *np = pp->dev->of_node;
> > > > + struct hisi_pcie *pcie = to_hisi_pcie(pp);
> > > > +
> > > > + msi_node = of_parse_phandle(np, "msi-parent", 0);
> > > > + if (!msi_node) {
> > > > + dev_err(pp->dev, "failed to find msi-parent\n");
> > > > + return -EINVAL;
> > > > + }
> > > > + of_address_to_resource(msi_node, 0, );
> > >
> > > Does this use the "msi-parent" node in the same way as other drivers
> > > do?  I'm sure there must be other places where we extract struct
> > > resource information from an "msi-parent" node, but I don't see them.
> > >
> > > I'm trying to verify that this isn't some kind of incompatible
> > > extension of the "msi-parent" property.  I cc'd Arnd and Rob (DT
> > > experts).
> > 
> > This is not ok, what this does is that it relies on a particular
> > implementation of the MSI controller and directly accesses its
> > registers.
> 
> Hi Arnd, thanks for reviewing.
> 
> What we do is to retrieve the msi-parent physical address and we store it
> in our internal PCIe register locations...

Ah, thanks for the clarification, I missed that part.

> So we do not operate directly on the msi controller registers...
> 
> So I wonder if the current implementation is Ok to retrieve the
> msi-parent address

Not sure. Why do you do this? Normally the PCI host does not know
or care about the address of the MSI controller, because the messages
are just passed up to the parent bus as DMA transfers.

I don't think what you do here is safe because the 'reg' property
of the MSI controller might point to the address that is used for
the message directly.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-12 Thread Bjorn Helgaas

[+cc Arnd, Rob]

Hi Zhou,

I have a few minor comments and two questions: one about the fact
all the config accesses are 32 bits, and another about the use of the
"msi-parent" node.

On Sat, Oct 10, 2015 at 10:59:15AM +0800, Zhou Wang wrote:
> This patch adds PCIe host support for HiSilicon SoC Hip05.
> 
> Signed-off-by: Zhou Wang 
> Signed-off-by: Gabriele Paoloni 
> Signed-off-by: liudongdong 
> Signed-off-by: Kefeng Wang 
> Signed-off-by: qiuzhenfa 

This is a pretty long Signed-off-by chain.  If each of these people
created part of this patch, that's fine.  I'm just checking whether
that's the case, or whether some of these should be Reviewed-by,
Suggested-by, etc., per Documentation/SubmittingPatches.

> ---
>  drivers/pci/host/Kconfig |   8 ++
>  drivers/pci/host/Makefile|   1 +
>  drivers/pci/host/pcie-hisi.c | 320 
> +++
>  3 files changed, 329 insertions(+)
>  create mode 100644 drivers/pci/host/pcie-hisi.c
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index d5e58ba..ae873be 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -145,4 +145,12 @@ config PCIE_IPROC_BCMA
> Say Y here if you want to use the Broadcom iProc PCIe controller
> through the BCMA bus interface
>  
> +config PCI_HISI
> + depends on OF && ARM64
> + bool "HiSilicon SoC HIP05 PCIe controller"
> + select PCIEPORTBUS
> + select PCIE_DW
> + help
> +   Say Y here if you want PCIe controller support on HiSilicon HIP05 SoC
> +
>  endmenu
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 140d66f..ea1dbf2 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -17,3 +17,4 @@ obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
>  obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
>  obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
>  obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
> +obj-$(CONFIG_PCI_HISI) += pcie-hisi.o
> diff --git a/drivers/pci/host/pcie-hisi.c b/drivers/pci/host/pcie-hisi.c
> new file mode 100644
> index 000..26aa0d9
> --- /dev/null
> +++ b/drivers/pci/host/pcie-hisi.c
> @@ -0,0 +1,320 @@
> +/*
> + * PCIe host controller driver for HiSilicon Hip05 SoC
> + *
> + * Copyright (C) 2015 HiSilicon Co., Ltd. http://www.hisilicon.com
> + *
> + * Author: Zhou Wang 
> + * Dacai Zhu 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "pcie-designware.h"
> +
> +#define PCIE_SUBCTRL_MODE_REG   0x2800
> +#define PCIE_SUBCTRL_SYS_STATE4_REG 0x6818
> +#define PCIE_SLV_DBI_MODE   0x0
> +#define PCIE_SLV_SYSCTRL_MODE   0x1
> +#define PCIE_SLV_CONTENT_MODE   0x2
> +#define PCIE_SLV_MSI_ASID   0x10
> +#define PCIE_LTSSM_LINKUP_STATE 0x11
> +#define PCIE_LTSSM_STATE_MASK   0x3F
> +#define PCIE_MSI_ASID_ENABLE(0x1 << 12)
> +#define PCIE_MSI_ASID_VALUE (0x1 << 16)
> +#define PCIE_MSI_TRANS_ENABLE   (0x1 << 12)
> +#define PCIE_MSI_TRANS_REG  0x1c8
> +#define PCIE_MSI_LOW_ADDRESS0x1b4
> +#define PCIE_MSI_HIGH_ADDRESS   0x1c4
> +#define PCIE_GITS_TRANSLATER0x10040

"TRANSLATOR" is the more common spelling.  But if you're matching
hardware documentation, you should follow that.

> +#define PCIE_SYS_CTRL20_REG 0x20
> +#define PCIE_RD_TAB_SEL BIT(31)
> +#define PCIE_RD_TAB_EN  BIT(30)
> +
> +#define to_hisi_pcie(x)  container_of(x, struct hisi_pcie, pp)
> +
> +struct hisi_pcie {
> + struct regmap *subctrl;
> + void __iomem *reg_base;
> + u32 port_id;
> + struct pcie_port pp;
> +};
> +
> +static inline void hisi_pcie_apb_writel(struct hisi_pcie *pcie,
> + u32 val, u32 reg)
> +{
> + writel(val, pcie->reg_base + reg);
> +}
> +
> +static inline u32 hisi_pcie_apb_readl(struct hisi_pcie *pcie, u32 reg)
> +{
> + return readl(pcie->reg_base + reg);
> +}
> +
> +/*
> + * Change mode to indicate the same reg_base to base of PCIe host configure
> + * registers, base of RC configure space or base of vmid/asid context table

It's fine to use lower-case in the C code, but in English text, it
reads a little better to capitalize acronyms like VMID, ASID, APB, AR,
AW, etc.  

> + */
> +static void hisi_pcie_change_apb_mode(struct hisi_pcie *pcie, u32

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

2015-10-12 Thread Bjorn Helgaas

[+cc Arnd, Rob]

Hi Zhou,

I have a few minor comments and two questions: one about the fact
all the config accesses are 32 bits, and another about the use of the
"msi-parent" node.

On Sat, Oct 10, 2015 at 10:59:15AM +0800, Zhou Wang wrote:
> This patch adds PCIe host support for HiSilicon SoC Hip05.
> 
> Signed-off-by: Zhou Wang 
> Signed-off-by: Gabriele Paoloni 
> Signed-off-by: liudongdong 
> Signed-off-by: Kefeng Wang 
> Signed-off-by: qiuzhenfa 

This is a pretty long Signed-off-by chain.  If each of these people
created part of this patch, that's fine.  I'm just checking whether
that's the case, or whether some of these should be Reviewed-by,
Suggested-by, etc., per Documentation/SubmittingPatches.

> ---
>  drivers/pci/host/Kconfig |   8 ++
>  drivers/pci/host/Makefile|   1 +
>  drivers/pci/host/pcie-hisi.c | 320 
> +++
>  3 files changed, 329 insertions(+)
>  create mode 100644 drivers/pci/host/pcie-hisi.c
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index d5e58ba..ae873be 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -145,4 +145,12 @@ config PCIE_IPROC_BCMA
> Say Y here if you want to use the Broadcom iProc PCIe controller
> through the BCMA bus interface
>  
> +config PCI_HISI
> + depends on OF && ARM64
> + bool "HiSilicon SoC HIP05 PCIe controller"
> + select PCIEPORTBUS
> + select PCIE_DW
> + help
> +   Say Y here if you want PCIe controller support on HiSilicon HIP05 SoC
> +
>  endmenu
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 140d66f..ea1dbf2 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -17,3 +17,4 @@ obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
>  obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
>  obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
>  obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
> +obj-$(CONFIG_PCI_HISI) += pcie-hisi.o
> diff --git a/drivers/pci/host/pcie-hisi.c b/drivers/pci/host/pcie-hisi.c
> new file mode 100644
> index 000..26aa0d9
> --- /dev/null
> +++ b/drivers/pci/host/pcie-hisi.c
> @@ -0,0 +1,320 @@
> +/*
> + * PCIe host controller driver for HiSilicon Hip05 SoC
> + *
> + * Copyright (C) 2015 HiSilicon Co., Ltd. http://www.hisilicon.com
> + *
> + * Author: Zhou Wang 
> + * Dacai Zhu 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "pcie-designware.h"
> +
> +#define PCIE_SUBCTRL_MODE_REG   0x2800
> +#define PCIE_SUBCTRL_SYS_STATE4_REG 0x6818
> +#define PCIE_SLV_DBI_MODE   0x0
> +#define PCIE_SLV_SYSCTRL_MODE   0x1
> +#define PCIE_SLV_CONTENT_MODE   0x2
> +#define PCIE_SLV_MSI_ASID   0x10
> +#define PCIE_LTSSM_LINKUP_STATE 0x11
> +#define PCIE_LTSSM_STATE_MASK   0x3F
> +#define PCIE_MSI_ASID_ENABLE(0x1 << 12)
> +#define PCIE_MSI_ASID_VALUE (0x1 << 16)
> +#define PCIE_MSI_TRANS_ENABLE   (0x1 << 12)
> +#define PCIE_MSI_TRANS_REG  0x1c8
> +#define PCIE_MSI_LOW_ADDRESS0x1b4
> +#define PCIE_MSI_HIGH_ADDRESS   0x1c4
> +#define PCIE_GITS_TRANSLATER0x10040

"TRANSLATOR" is the more common spelling.  But if you're matching
hardware documentation, you should follow that.

> +#define PCIE_SYS_CTRL20_REG 0x20
> +#define PCIE_RD_TAB_SEL BIT(31)
> +#define PCIE_RD_TAB_EN  BIT(30)
> +
> +#define to_hisi_pcie(x)  container_of(x, struct hisi_pcie, pp)
> +
> +struct hisi_pcie {
> + struct regmap *subctrl;
> + void __iomem *reg_base;
> + u32 port_id;
> + struct pcie_port pp;
> +};
> +
> +static inline void hisi_pcie_apb_writel(struct hisi_pcie *pcie,
> + u32 val, u32 reg)
> +{
> + writel(val, pcie->reg_base + reg);
> +}
> +
> +static inline u32 hisi_pcie_apb_readl(struct hisi_pcie *pcie, u32 reg)
> +{
> + return readl(pcie->reg_base + reg);
> +}
> +
> +/*
> + * Change mode to indicate the same reg_base to base of PCIe host configure
> + * registers, base of RC configure space or base of vmid/asid context table

It's fine to use lower-case in the C code, but

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

RE: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

Re: [PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

34 matches

Site Navigation

Mail list logo

Footer information