Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Dilip,
On Tue, Sep 3, 2019 at 12:20 PM Dilip Kota wrote:
>
> Hi Martin,
>
> On 8/29/2019 10:54 AM, Chuan Hua, Lei wrote:
>
>
> On 8/29/2019 3:36 AM, Martin Blumenstingl wrote:
>
> On Wed, Aug 28, 2019 at 5:35 AM Chuan Hua, Lei
> wrote:
> [...]
>
> +static int intel_pcie_ep_rst_init(struct intel_pcie_port *lpp)
> +{
> +struct device *dev = lpp->pci->dev;
> +int ret = 0;
> +
> +lpp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +if (IS_ERR(lpp->reset_gpio)) {
> +ret = PTR_ERR(lpp->reset_gpio);
> +if (ret != -EPROBE_DEFER)
> +dev_err(dev, "failed to request PCIe GPIO: %d\n", ret);
> +return ret;
> +}
> +/* Make initial reset last for 100ms */
> +msleep(100);
>
> why is there lpp->rst_interval when you hardcode 100ms here?
>
> There are different purpose. rst_interval is purely for asserted reset
> pulse.
>
> Here 100ms is to make sure the initial state keeps at least 100ms, then we
> can reset.
>
> my interpretation is that it totally depends on the board design or
> the bootloader setup.
>
> Partially, you are right. However, we should not add some dependency
> here from
> bootloader and board. rst_interval is just to make sure the pulse (low
> active or high active)
> lasts the specified the time.
>
> +Cc Kishon
>
> he recently added support for a GPIO reset line to the
> pcie-cadence-host.c [0] and I believe he's also maintaining
> pci-keystone.c which are both using a 100uS delay (instead of 100ms).
> I don't know the PCIe spec so maybe Kishon can comment on the values
> that should be used according to the spec.
> if there's then a reason why values other than the ones from the spec
> are needed then there should be a comment explaining why different
> values are needed (what problem does it solve).
>
> spec doesn't guide this part. It is a board or SoC specific setting.
> 100us also should work. spec only requirs reset duration should last
> 100ms. The idea is that before reset assert and deassert, make sure the
> default deassert status keeps some time. We take this value from
> hardware suggestion long time back. We can reduce this value to 100us,
> but we need to test on the board.
>
> OK. I don't know how other PCI controller drivers manage this. if the
> PCI maintainers are happy with this then I am as well
> maybe it's worth changing the comment to indicate that this delay was
> suggested by the hardware team (so it's clear that this is not coming
> from the PCI spec)
>
> Dilip will change to 100us delay and run the test. I also need to run some
> tests for old boards(XRX350/550/PRX300) to confirm this has no impact on
> function.
>
> I have tested 100us on the target and it is working fine.
> Along with this change, i have validated below changes and test is successful.
> Enabling the A/B/C/D interrupts during the initialization instead of in
> map_irq()
> Calling dw_pcie_setup_rc() function during initialization.
>
> I will push these changes in the next patch version.
great, thank you for working on simplifying the code!
> And, regarding [1]:
> I have checked the code for using regmap; Helper functions especially
> update_bits() cannot be avoided(it is required while configuring pcie RC
> registers too). and LGM is little endian.
> Switching to regmap() is not bringing any gain.
OK, if it doesn't help you for LGM then no need to switch to regmap now
I can still do it afterwards when adding support for other SoCs
> Regarding [2]:
> PCIE_SPEED2STR() is quite different from the pcie_link_gen_to_str().
> PCIE_SPEED2STR() expects a encoded value defined in pcie_link_speed[] array
> in probe.c, whereas pcie_link_gen_to_str() is a direct mapping to the
> register bits value.
> pcie_link_gen_to_str() is pretty much simple and straight forward.
>
> And, any of the pcie controller drivers are using neither PCIE_SPEED2STR()
> nor pcie_link_speed[].
OK, I see - thank you for following up
the PCI maintainers need to decide whether pcie_link_status_show is
acceptable (instead of using lspci) - that's the only place where
pcie_link_gen_to_str is used
Martin
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Kishon, On Thu, Aug 29, 2019 at 7:10 AM Kishon Vijay Abraham I wrote: [...] > The PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION defines the Power > Sequencing and Reset Signal Timings in Table 2-4. Please also refer Figure > 2-10: Power Up of the CEM. > > ╔═╤══╤═╤═╤═══╗ > ║ Symbol │ Parameter│ Min │ Max │ Units ║ > ╠═╪══╪═╪═╪═══╣ > ║ T PVPERL│ Power stable to PERST# inactive │ 100 │ │ ms║ > ╟─┼──┼─┼─┼───╢ > ║ T PERST-CLK │ REFCLK stable before PERST# inactive │ 100 │ │ μs║ > ╟─┼──┼─┼─┼───╢ > ║ T PERST │ PERST# active time │ 100 │ │ μs║ > ╟─┼──┼─┼─┼───╢ > ║ T FAIL │ Power level invalid to PERST# active │ │ 500 │ ns║ > ╟─┼──┼─┼─┼───╢ > ║ T WKRF │ WAKE# rise – fall time │ │ 100 │ ns║ > ╚═╧══╧═╧═╧═══╝ > > In my code I used T PERST-CLK (i.e REFCLK stable before PERST# inactive). > REFCLK to the card is enabled as part of PHY enable and then wait for 100μs > before making PERST# inactive. > > Power to the device is given during board power up and the assumption here is > it will take more the 100ms for the probe to be invoked after board power up > (i.e after ROM, bootloaders and linux kernel). But if you have a regulator > that > is enabled in PCI probe, then T PVPERL (100ms) should also used in probe. thank you for this detailed overview and for the explanation about the assumptions you made (and why) Martin
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Martin,
On 28/08/19 2:08 AM, Martin Blumenstingl wrote:
> Hello,
>
> On Tue, Aug 27, 2019 at 5:09 AM Chuan Hua, Lei
> wrote:
>>
>> Hi Martin,
>>
>> Thanks for your feedback. Please check the comments below.
>>
>> On 8/27/2019 5:15 AM, Martin Blumenstingl wrote:
>>> Hello,
>>>
>>> On Mon, Aug 26, 2019 at 5:31 AM Chuan Hua, Lei
>>> wrote:
Hi Martin,
Thanks for your valuable comments. I reply some of them as below.
>>> you're welcome
>>>
>>> [...]
>> +config PCIE_INTEL_AXI
>> +bool "Intel AHB/AXI PCIe host controller support"
> I believe that this is mostly the same IP block as it's used in Lantiq
> (xDSL) VRX200 SoCs (with MIPS cores) which was introduced in 2010
> (before Intel acquired Lantiq).
> This is why I would have personally called the driver PCIE_LANTIQ
VRX200 SoC(internally called VR9) was the first PCIe SoC product which
was using synopsys
controller v3.30a. It only supports PCIe Gen1.1/1.0. The phy is internal
phy from infineon.
>>> thank you for these details
>>> I wasn't aware that the PCIe PHY on these SoCs was developed by
>>> Infineon nor is the DWC version documented anywhere
>>
>> VRX200/ARX300 PHY is internal value. There are a lot of hardcode which was
>> from hardware people. From XRX500, we switch to synopsis PHY. However, later
>> comboPHY is coming to the picture. Even though we have one same controller
>> with different versions, we most likely will have three different phy
>> drivers.
> that is a good argument for using a separate PHY driver and
> integrating that using the PHY subsystem (which is already the case in
> this patch revision)
>
.
.
>> +static int intel_pcie_ep_rst_init(struct intel_pcie_port *lpp)
>> +{
>> +struct device *dev = lpp->pci->dev;
>> +int ret = 0;
>> +
>> +lpp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>> +if (IS_ERR(lpp->reset_gpio)) {
>> +ret = PTR_ERR(lpp->reset_gpio);
>> +if (ret != -EPROBE_DEFER)
>> +dev_err(dev, "failed to request PCIe GPIO: %d\n",
>> ret);
>> +return ret;
>> +}
>> +/* Make initial reset last for 100ms */
>> +msleep(100);
> why is there lpp->rst_interval when you hardcode 100ms here?
There are different purpose. rst_interval is purely for asserted reset
pulse.
Here 100ms is to make sure the initial state keeps at least 100ms, then we
can reset.
>>> my interpretation is that it totally depends on the board design or
>>> the bootloader setup.
>>
>> Partially, you are right. However, we should not add some dependency
>> here from
>> bootloader and board. rst_interval is just to make sure the pulse (low
>> active or high active)
>> lasts the specified the time.
> +Cc Kishon
>
> he recently added support for a GPIO reset line to the
> pcie-cadence-host.c [0] and I believe he's also maintaining
> pci-keystone.c which are both using a 100uS delay (instead of 100ms).
> I don't know the PCIe spec so maybe Kishon can comment on the values
> that should be used according to the spec.
> if there's then a reason why values other than the ones from the spec
> are needed then there should be a comment explaining why different
> values are needed (what problem does it solve).
The PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION defines the Power
Sequencing and Reset Signal Timings in Table 2-4. Please also refer Figure
2-10: Power Up of the CEM.
╔═╤══╤═╤═╤═══╗
║ Symbol │ Parameter│ Min │ Max │ Units ║
╠═╪══╪═╪═╪═══╣
║ T PVPERL│ Power stable to PERST# inactive │ 100 │ │ ms║
╟─┼──┼─┼─┼───╢
║ T PERST-CLK │ REFCLK stable before PERST# inactive │ 100 │ │ μs║
╟─┼──┼─┼─┼───╢
║ T PERST │ PERST# active time │ 100 │ │ μs║
╟─┼──┼─┼─┼───╢
║ T FAIL │ Power level invalid to PERST# active │ │ 500 │ ns║
╟─┼──┼─┼─┼───╢
║ T WKRF │ WAKE# rise – fall time │ │ 100 │ ns║
╚═╧══╧═╧═╧═══╝
In my code I used T PERST-CLK (i.e REFCLK stable before PERST# inactive).
REFCLK to the card is enabled as part of PHY enable and then wait for 100μs
before making PERST# inactive.
Power to the device is given during board power up and the assumption here is
it will take more the 100ms for the probe to be invoked after board power up
(i.e after ROM, bootloaders and linux kernel). But if you have a regulator that
is enabled in PCI probe, then T PVPERL (100ms) should also used in
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
On 8/29/2019 3:36 AM, Martin Blumenstingl wrote:
On Wed, Aug 28, 2019 at 5:35 AM Chuan Hua, Lei
wrote:
[...]
+static int intel_pcie_ep_rst_init(struct intel_pcie_port *lpp)
+{
+struct device *dev = lpp->pci->dev;
+int ret = 0;
+
+lpp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+if (IS_ERR(lpp->reset_gpio)) {
+ret = PTR_ERR(lpp->reset_gpio);
+if (ret != -EPROBE_DEFER)
+dev_err(dev, "failed to request PCIe GPIO: %d\n", ret);
+return ret;
+}
+/* Make initial reset last for 100ms */
+msleep(100);
why is there lpp->rst_interval when you hardcode 100ms here?
There are different purpose. rst_interval is purely for asserted reset
pulse.
Here 100ms is to make sure the initial state keeps at least 100ms, then we
can reset.
my interpretation is that it totally depends on the board design or
the bootloader setup.
Partially, you are right. However, we should not add some dependency
here from
bootloader and board. rst_interval is just to make sure the pulse (low
active or high active)
lasts the specified the time.
+Cc Kishon
he recently added support for a GPIO reset line to the
pcie-cadence-host.c [0] and I believe he's also maintaining
pci-keystone.c which are both using a 100uS delay (instead of 100ms).
I don't know the PCIe spec so maybe Kishon can comment on the values
that should be used according to the spec.
if there's then a reason why values other than the ones from the spec
are needed then there should be a comment explaining why different
values are needed (what problem does it solve).
spec doesn't guide this part. It is a board or SoC specific setting.
100us also should work. spec only requirs reset duration should last
100ms. The idea is that before reset assert and deassert, make sure the
default deassert status keeps some time. We take this value from
hardware suggestion long time back. We can reduce this value to 100us,
but we need to test on the board.
OK. I don't know how other PCI controller drivers manage this. if the
PCI maintainers are happy with this then I am as well
maybe it's worth changing the comment to indicate that this delay was
suggested by the hardware team (so it's clear that this is not coming
from the PCI spec)
Dilip will change to 100us delay and run the test. I also need to run
some tests for old boards(XRX350/550/PRX300) to confirm this has no
impact on function.
[...]
+static void __intel_pcie_remove(struct intel_pcie_port *lpp)
+{
+pcie_rc_cfg_wr_mask(lpp, PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER,
+0, PCI_COMMAND);
I expect logic like this to be part of the PCI subsystem in Linux.
why is this needed?
[...]
bind/unbind case we use this. For extreme cases, we use unbind and bind
to reset
PCI instead of rebooting.
OK, but this does not seem Intel/Lantiq specific at all
why isn't this managed by either pcie-designware-host.c or the generic
PCI/PCIe subsystem in Linux?
I doubt if other RC driver will support bind/unbind. We do have this
requirement due to power management from WiFi devices.
pcie-designware-host.c will gain .remove() support in Linux 5.4
I don't understand how .remove() and then .probe() again is different
from .unbind() followed by a .bind()
Good. If this is the case, bind/unbind eventually goes to probe/remove,
so we can remove this.
OK. as far as I understand you need to call dw_pcie_host_deinit from
the .remove() callback (which is missing in this version)
(I'm using drivers/pci/controller/dwc/pcie-tegra194.c as an example,
this driver is in linux-next and thus will appear in Linux 5.4)
Thanks for your information. We should adapt this in next version.
Martin
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
On Wed, Aug 28, 2019 at 5:35 AM Chuan Hua, Lei
wrote:
[...]
> >> +static int intel_pcie_ep_rst_init(struct intel_pcie_port *lpp)
> >> +{
> >> +struct device *dev = lpp->pci->dev;
> >> +int ret = 0;
> >> +
> >> +lpp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> >> +if (IS_ERR(lpp->reset_gpio)) {
> >> +ret = PTR_ERR(lpp->reset_gpio);
> >> +if (ret != -EPROBE_DEFER)
> >> +dev_err(dev, "failed to request PCIe GPIO: %d\n",
> >> ret);
> >> +return ret;
> >> +}
> >> +/* Make initial reset last for 100ms */
> >> +msleep(100);
> > why is there lpp->rst_interval when you hardcode 100ms here?
> There are different purpose. rst_interval is purely for asserted reset
> pulse.
>
> Here 100ms is to make sure the initial state keeps at least 100ms, then
> we
> can reset.
> >>> my interpretation is that it totally depends on the board design or
> >>> the bootloader setup.
> >> Partially, you are right. However, we should not add some dependency
> >> here from
> >> bootloader and board. rst_interval is just to make sure the pulse (low
> >> active or high active)
> >> lasts the specified the time.
> > +Cc Kishon
> >
> > he recently added support for a GPIO reset line to the
> > pcie-cadence-host.c [0] and I believe he's also maintaining
> > pci-keystone.c which are both using a 100uS delay (instead of 100ms).
> > I don't know the PCIe spec so maybe Kishon can comment on the values
> > that should be used according to the spec.
> > if there's then a reason why values other than the ones from the spec
> > are needed then there should be a comment explaining why different
> > values are needed (what problem does it solve).
>
> spec doesn't guide this part. It is a board or SoC specific setting.
> 100us also should work. spec only requirs reset duration should last
> 100ms. The idea is that before reset assert and deassert, make sure the
> default deassert status keeps some time. We take this value from
> hardware suggestion long time back. We can reduce this value to 100us,
> but we need to test on the board.
OK. I don't know how other PCI controller drivers manage this. if the
PCI maintainers are happy with this then I am as well
maybe it's worth changing the comment to indicate that this delay was
suggested by the hardware team (so it's clear that this is not coming
from the PCI spec)
[...]
> >> +static void __intel_pcie_remove(struct intel_pcie_port *lpp)
> >> +{
> >> +pcie_rc_cfg_wr_mask(lpp, PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER,
> >> +0, PCI_COMMAND);
> > I expect logic like this to be part of the PCI subsystem in Linux.
> > why is this needed?
> >
> > [...]
> bind/unbind case we use this. For extreme cases, we use unbind and bind
> to reset
> PCI instead of rebooting.
> >>> OK, but this does not seem Intel/Lantiq specific at all
> >>> why isn't this managed by either pcie-designware-host.c or the generic
> >>> PCI/PCIe subsystem in Linux?
> >> I doubt if other RC driver will support bind/unbind. We do have this
> >> requirement due to power management from WiFi devices.
> > pcie-designware-host.c will gain .remove() support in Linux 5.4
> > I don't understand how .remove() and then .probe() again is different
> > from .unbind() followed by a .bind()
> Good. If this is the case, bind/unbind eventually goes to probe/remove,
> so we can remove this.
OK. as far as I understand you need to call dw_pcie_host_deinit from
the .remove() callback (which is missing in this version)
(I'm using drivers/pci/controller/dwc/pcie-tegra194.c as an example,
this driver is in linux-next and thus will appear in Linux 5.4)
Martin
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Martin, Thanks for your comment. On 8/28/2019 4:38 AM, Martin Blumenstingl wrote: Hello, On Tue, Aug 27, 2019 at 5:09 AM Chuan Hua, Lei wrote: Hi Martin, Thanks for your feedback. Please check the comments below. On 8/27/2019 5:15 AM, Martin Blumenstingl wrote: Hello, On Mon, Aug 26, 2019 at 5:31 AM Chuan Hua, Lei wrote: Hi Martin, Thanks for your valuable comments. I reply some of them as below. you're welcome [...] +config PCIE_INTEL_AXI +bool "Intel AHB/AXI PCIe host controller support" I believe that this is mostly the same IP block as it's used in Lantiq (xDSL) VRX200 SoCs (with MIPS cores) which was introduced in 2010 (before Intel acquired Lantiq). This is why I would have personally called the driver PCIE_LANTIQ VRX200 SoC(internally called VR9) was the first PCIe SoC product which was using synopsys controller v3.30a. It only supports PCIe Gen1.1/1.0. The phy is internal phy from infineon. thank you for these details I wasn't aware that the PCIe PHY on these SoCs was developed by Infineon nor is the DWC version documented anywhere VRX200/ARX300 PHY is internal value. There are a lot of hardcode which was from hardware people. From XRX500, we switch to synopsis PHY. However, later comboPHY is coming to the picture. Even though we have one same controller with different versions, we most likely will have three different phy drivers. that is a good argument for using a separate PHY driver and integrating that using the PHY subsystem (which is already the case in this patch revision) Right. CombonPHY(PRX300/Lighting Mountain) and SlimPHY driver(XRX350/550) are in the way to upstream. [...] +#define PCIE_CCRID 0x8 + +#define PCIE_LCAP 0x7C +#define PCIE_LCAP_MAX_LINK_SPEEDGENMASK(3, 0) +#define PCIE_LCAP_MAX_LENGTH_WIDTH GENMASK(9, 4) + +/* Link Control and Status Register */ +#define PCIE_LCTLSTS0x80 +#define PCIE_LCTLSTS_ASPM_ENABLEGENMASK(1, 0) +#define PCIE_LCTLSTS_RCB128 BIT(3) +#define PCIE_LCTLSTS_LINK_DISABLE BIT(4) +#define PCIE_LCTLSTS_COM_CLK_CFGBIT(6) +#define PCIE_LCTLSTS_HW_AW_DIS BIT(9) +#define PCIE_LCTLSTS_LINK_SPEED GENMASK(19, 16) +#define PCIE_LCTLSTS_NEGOTIATED_LINK_WIDTH GENMASK(25, 20) +#define PCIE_LCTLSTS_SLOT_CLK_CFG BIT(28) + +#define PCIE_LCTLSTS2 0xA0 +#define PCIE_LCTLSTS2_TGT_LINK_SPEEDGENMASK(3, 0) +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_25GT 0x1 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_5GT0x2 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_8GT0x3 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_16GT 0x4 +#define PCIE_LCTLSTS2_HW_AUTO_DIS BIT(5) + +/* Ack Frequency Register */ +#define PCIE_AFR0x70C +#define PCIE_AFR_FTS_NUMGENMASK(15, 8) +#define PCIE_AFR_COM_FTS_NUMGENMASK(23, 16) +#define PCIE_AFR_GEN12_FTS_NUM_DFT (SZ_128 - 1) +#define PCIE_AFR_GEN3_FTS_NUM_DFT 180 +#define PCIE_AFR_GEN4_FTS_NUM_DFT 196 + +#define PCIE_PLCR_DLL_LINK_EN BIT(5) +#define PCIE_PORT_LOGIC_FTS GENMASK(7, 0) +#define PCIE_PORT_LOGIC_DFT_FTS_NUM (SZ_128 - 1) + +#define PCIE_MISC_CTRL 0x8BC +#define PCIE_MISC_CTRL_DBI_RO_WR_EN BIT(0) + +#define PCIE_MULTI_LANE_CTRL0x8C0 +#define PCIE_UPCONFIG_SUPPORT BIT(7) +#define PCIE_DIRECT_LINK_WIDTH_CHANGE BIT(6) +#define PCIE_TARGET_LINK_WIDTH GENMASK(5, 0) + +#define PCIE_IOP_CTRL 0x8C4 +#define PCIE_IOP_RX_STANDBY_CTRLGENMASK(6, 0) no need for IOP with "are you sure that you need any of the registers above?" I really meant all registers above (including, but not limited to IOP) [...] As I mentioned, VRX200 was a very old PCIe Gen1.1 product. In our latest SoC Lightning Mountain, we are using synopsys controller 5.20/5.50a. We support Gen2(XRX350/550), Gen3(PRX300) and GEN4(X86 based SoC). We also supported dual lane and single lane. Some of the above registers are needed to control FTS, link width and link speed. only now I noticed that I didn't explain why I was asking whether all these registers are needed my understanding of the DWC PCIe controller driver "library" is that: - all functionality which is provided by the DesignWare PCIe core should be added to drivers/pci/controller/dwc/pcie-designware* - functionality which is built on top/around the DWC PCIe core should be added to the link width and link speed settings (I don't know about "FTS") don't seem Intel/Lantiq controller specific to me so the register setup for these bits should be moved to drivers/pci/controller/dwc/pcie-designware* FTS means fast training sequence. Different generations will
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Dilip,
On Tue, Aug 27, 2019 at 10:47 AM Dilip Kota wrote:
[...]
> >
> >
> >> now I am wondering:
> >> - if we don't have to disable the interrupt line (once it is enabled),
> >> why can't we enable all of these interrupts at initialization time
> >> (instead of doing it on-demand)?
> > Good point! we even can remote map_irq patch, directly call
> >
> > of_irq_parse_and_map_pci as other drivers do.
>
> Irrespective of disabling, imo interrupts(A/B/C/D) should be enabled
> when they are requested; which happens during map_irq() call.
with an integrated interrupt controller (which I decided to use in my
experiment because I could not find a .unmap_irq callback) this would
be the case:
- of_irq_parse_and_map_pci finds our APP interrupt
- this will enable the interrupt in the PCIe controller's APP register first
- then it will enable the interrupt in the parent interrupt controller
- when the PCIe card then frees the IRQ line again we will disable
both interrupts (the APP interrupt as well as the parent interrupt)
I don't see why of_irq_parse_and_map_pci + custom code to enable the
APP interrupt in .map_irq (without any .unmap_irq callback) is better
than always enabling it
[...]
> >>> This is needed. In the old driver, we fixed this by fixup. The original
> >>> comment as follows,
> >>>
> >>> /*
> >>>* The root complex has a hardwired class of
> >>> PCI_CLASS_NETWORK_OTHER or
> >>>* PCI_CLASS_BRIDGE_HOST, when it is operating as a root complex this
> >>>* needs to be switched to * PCI_CLASS_BRIDGE_PCI
> >>>*/
> >> that would be a good comment to add if you really need it
> >> can you please look at dw_pcie_setup_rc (from
> >> pcie-designware-host.c), it does:
> >>/* Enable write permission for the DBI read-only register */
> >>dw_pcie_dbi_ro_wr_en(pci);
> >>/* Program correct class for RC */
> >>dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
> >>/* Better disable write permission right after the update */
> >>dw_pcie_dbi_ro_wr_dis(pci);
> >>
> >> so my understanding is that there is a functional requirement to set
> >> the class to PCI_CLASS_BRIDGE_PCI
> >> however, that requirement is already covered by pcie-designware-host.c
> > I will task Dilip to check if we can use dwc one.
> dw_pcie_setup_rc () cannot be called here because, it is not doing
> PCI_CLASS_BRIDGE_PCI set alone, it is configuring many other things.
I am surprised to see that dw_pcie_setup_rc() is not used at all in your driver
it sets up BARs, bus numbers, interrupt pins, the command register, etc.
with my limited knowledge I assumed that these ("many other things")
are all mandatory so everything works correctly
what problems do you experience when you use dw_pcie_setup_rc()?
also it seems that virtually every PCIe driver based on the DWC
controller uses it:
$ grep -R dw_pcie_setup_rc drivers/pci/controller/dwc/ | wc -l
20
Martin
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hello, On Tue, Aug 27, 2019 at 5:09 AM Chuan Hua, Lei wrote: > > Hi Martin, > > Thanks for your feedback. Please check the comments below. > > On 8/27/2019 5:15 AM, Martin Blumenstingl wrote: > > Hello, > > > > On Mon, Aug 26, 2019 at 5:31 AM Chuan Hua, Lei > > wrote: > >> Hi Martin, > >> > >> Thanks for your valuable comments. I reply some of them as below. > > you're welcome > > > > [...] > +config PCIE_INTEL_AXI > +bool "Intel AHB/AXI PCIe host controller support" > >>> I believe that this is mostly the same IP block as it's used in Lantiq > >>> (xDSL) VRX200 SoCs (with MIPS cores) which was introduced in 2010 > >>> (before Intel acquired Lantiq). > >>> This is why I would have personally called the driver PCIE_LANTIQ > >> VRX200 SoC(internally called VR9) was the first PCIe SoC product which > >> was using synopsys > >> > >> controller v3.30a. It only supports PCIe Gen1.1/1.0. The phy is internal > >> phy from infineon. > > thank you for these details > > I wasn't aware that the PCIe PHY on these SoCs was developed by > > Infineon nor is the DWC version documented anywhere > > VRX200/ARX300 PHY is internal value. There are a lot of hardcode which was > from hardware people. From XRX500, we switch to synopsis PHY. However, later > comboPHY is coming to the picture. Even though we have one same controller > with different versions, we most likely will have three different phy > drivers. that is a good argument for using a separate PHY driver and integrating that using the PHY subsystem (which is already the case in this patch revision) > > [...] > +#define PCIE_CCRID 0x8 > + > +#define PCIE_LCAP 0x7C > +#define PCIE_LCAP_MAX_LINK_SPEEDGENMASK(3, 0) > +#define PCIE_LCAP_MAX_LENGTH_WIDTH GENMASK(9, 4) > + > +/* Link Control and Status Register */ > +#define PCIE_LCTLSTS0x80 > +#define PCIE_LCTLSTS_ASPM_ENABLEGENMASK(1, 0) > +#define PCIE_LCTLSTS_RCB128 BIT(3) > +#define PCIE_LCTLSTS_LINK_DISABLE BIT(4) > +#define PCIE_LCTLSTS_COM_CLK_CFGBIT(6) > +#define PCIE_LCTLSTS_HW_AW_DIS BIT(9) > +#define PCIE_LCTLSTS_LINK_SPEED GENMASK(19, 16) > +#define PCIE_LCTLSTS_NEGOTIATED_LINK_WIDTH GENMASK(25, 20) > +#define PCIE_LCTLSTS_SLOT_CLK_CFG BIT(28) > + > +#define PCIE_LCTLSTS2 0xA0 > +#define PCIE_LCTLSTS2_TGT_LINK_SPEEDGENMASK(3, 0) > +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_25GT 0x1 > +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_5GT0x2 > +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_8GT0x3 > +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_16GT 0x4 > +#define PCIE_LCTLSTS2_HW_AUTO_DIS BIT(5) > + > +/* Ack Frequency Register */ > +#define PCIE_AFR0x70C > +#define PCIE_AFR_FTS_NUMGENMASK(15, 8) > +#define PCIE_AFR_COM_FTS_NUMGENMASK(23, 16) > +#define PCIE_AFR_GEN12_FTS_NUM_DFT (SZ_128 - 1) > +#define PCIE_AFR_GEN3_FTS_NUM_DFT 180 > +#define PCIE_AFR_GEN4_FTS_NUM_DFT 196 > + > +#define PCIE_PLCR_DLL_LINK_EN BIT(5) > +#define PCIE_PORT_LOGIC_FTS GENMASK(7, 0) > +#define PCIE_PORT_LOGIC_DFT_FTS_NUM (SZ_128 - 1) > + > +#define PCIE_MISC_CTRL 0x8BC > +#define PCIE_MISC_CTRL_DBI_RO_WR_EN BIT(0) > + > +#define PCIE_MULTI_LANE_CTRL0x8C0 > +#define PCIE_UPCONFIG_SUPPORT BIT(7) > +#define PCIE_DIRECT_LINK_WIDTH_CHANGE BIT(6) > +#define PCIE_TARGET_LINK_WIDTH GENMASK(5, 0) > + > +#define PCIE_IOP_CTRL 0x8C4 > +#define PCIE_IOP_RX_STANDBY_CTRLGENMASK(6, 0) > >> no need for IOP > > with "are you sure that you need any of the registers above?" I really > > meant all registers above (including, but not limited to IOP) > > > > [...] > >> As I mentioned, VRX200 was a very old PCIe Gen1.1 product. In our latest > >> SoC Lightning > >> > >> Mountain, we are using synopsys controller 5.20/5.50a. We support > >> Gen2(XRX350/550), > >> > >> Gen3(PRX300) and GEN4(X86 based SoC). We also supported dual lane and > >> single lane. > >> > >> Some of the above registers are needed to control FTS, link width and > >> link speed. > > only now I noticed that I didn't explain why I was asking whether all > > these registers are needed > > my understanding of the DWC PCIe controller driver "library" is that: > > - all functionality which is provided by the DesignWare PCIe core > > should be added to drivers/pci/controller/dwc/pcie-designware* > > -
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
On Mon, Aug 26, 2019 at 11:15:48PM +0200, Martin Blumenstingl wrote:
> On Mon, Aug 26, 2019 at 5:31 AM Chuan Hua, Lei
> wrote:
> > As I mentioned, VRX200 was a very old PCIe Gen1.1 product. In our latest
> > SoC Lightning
> >
> > Mountain, we are using synopsys controller 5.20/5.50a. We support
> > Gen2(XRX350/550),
> >
> > Gen3(PRX300) and GEN4(X86 based SoC). We also supported dual lane and
> > single lane.
> >
> > Some of the above registers are needed to control FTS, link width and
> > link speed.
> only now I noticed that I didn't explain why I was asking whether all
> these registers are needed
> my understanding of the DWC PCIe controller driver "library" is that:
> - all functionality which is provided by the DesignWare PCIe core
> should be added to drivers/pci/controller/dwc/pcie-designware*
> - functionality which is built on top/around the DWC PCIe core should
> be added to
>
> the link width and link speed settings (I don't know about "FTS")
> don't seem Intel/Lantiq controller specific to me
> so the register setup for these bits should be moved to
> drivers/pci/controller/dwc/pcie-designware*
I think it may be done this way. We have already example with stmmac
(DWC network card IP) driver which split in similar way.
> > We can support up to XRX350/XRX500/PRX300 for MIPS SoC since we still
> > sell these products.
> OK, I understand this.
> switching to regmap will give you two benefits:
> - big endian registers writes (without additional code) on the MIPS SoCs
> - you can drop the pcie_app_* helper functions and use
> regmap_{read,write,update_bits} instead
Actually one more, i.e. dump of the registers by request via debugfs, which
I found very helpful.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
On 8/27/2019 4:14 AM, Martin Blumenstingl wrote:
second example: pcie-tegra194 (only in -next, will be part of v5.4)
struct tegra_pcie_dw {
...
struct dw_pcie pci;
...
};
so some drivers store a pointer pointer to the dw_pcie struct vs.
embedding the dw_pcie struct directly.
as far as I know the result will be equal, except that you don't have
to use a second devm_kzalloc for struct dw_pcie (and thus reducing
memory fragmentation).
Okay, i will change it to "struct dw_pcie pci;"
Thanks for the feedback.
Regards,
Dilip
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Martin, On 8/27/2019 11:09 AM, Chuan Hua, Lei wrote: [...] now I am wondering: - if we don't have to disable the interrupt line (once it is enabled), why can't we enable all of these interrupts at initialization time (instead of doing it on-demand)? Good point! we even can remote map_irq patch, directly call of_irq_parse_and_map_pci as other drivers do. Irrespective of disabling, imo interrupts(A/B/C/D) should be enabled when they are requested; which happens during map_irq() call. - if the interrupts do have to be disabled again (that is what I assumed so far) then where is this supposed to happen? (my solution for this was to implement a simple interrupt controller within the PCIe driver which only supports enable/disable. disclaimer: I didn't ask the PCI or interrupt maintainers for feedback on this yet) [...] We can implement one interrupt controller, but personally, it has too much overhead. If we follow this way, almost all modules of all old lantiq SoCs can implement one interrupt controller. Maybe you can check with PCI maintainer for their comments. [...] This is needed. In the old driver, we fixed this by fixup. The original comment as follows, /* * The root complex has a hardwired class of PCI_CLASS_NETWORK_OTHER or * PCI_CLASS_BRIDGE_HOST, when it is operating as a root complex this * needs to be switched to * PCI_CLASS_BRIDGE_PCI */ that would be a good comment to add if you really need it can you please look at dw_pcie_setup_rc (from pcie-designware-host.c), it does: /* Enable write permission for the DBI read-only register */ dw_pcie_dbi_ro_wr_en(pci); /* Program correct class for RC */ dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); /* Better disable write permission right after the update */ dw_pcie_dbi_ro_wr_dis(pci); so my understanding is that there is a functional requirement to set the class to PCI_CLASS_BRIDGE_PCI however, that requirement is already covered by pcie-designware-host.c I will task Dilip to check if we can use dwc one. dw_pcie_setup_rc () cannot be called here because, it is not doing PCI_CLASS_BRIDGE_PCI set alone, it is configuring many other things. [...] Regards, Dilip
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Martin, Thanks for your feedback. Please check the comments below. On 8/27/2019 5:15 AM, Martin Blumenstingl wrote: Hello, On Mon, Aug 26, 2019 at 5:31 AM Chuan Hua, Lei wrote: Hi Martin, Thanks for your valuable comments. I reply some of them as below. you're welcome [...] +config PCIE_INTEL_AXI +bool "Intel AHB/AXI PCIe host controller support" I believe that this is mostly the same IP block as it's used in Lantiq (xDSL) VRX200 SoCs (with MIPS cores) which was introduced in 2010 (before Intel acquired Lantiq). This is why I would have personally called the driver PCIE_LANTIQ VRX200 SoC(internally called VR9) was the first PCIe SoC product which was using synopsys controller v3.30a. It only supports PCIe Gen1.1/1.0. The phy is internal phy from infineon. thank you for these details I wasn't aware that the PCIe PHY on these SoCs was developed by Infineon nor is the DWC version documented anywhere VRX200/ARX300 PHY is internal value. There are a lot of hardcode which was from hardware people. From XRX500, we switch to synopsis PHY. However, later comboPHY is coming to the picture. Even though we have one same controller with different versions, we most likely will have three different phy drivers. [...] +#define PCIE_CCRID 0x8 + +#define PCIE_LCAP 0x7C +#define PCIE_LCAP_MAX_LINK_SPEEDGENMASK(3, 0) +#define PCIE_LCAP_MAX_LENGTH_WIDTH GENMASK(9, 4) + +/* Link Control and Status Register */ +#define PCIE_LCTLSTS0x80 +#define PCIE_LCTLSTS_ASPM_ENABLEGENMASK(1, 0) +#define PCIE_LCTLSTS_RCB128 BIT(3) +#define PCIE_LCTLSTS_LINK_DISABLE BIT(4) +#define PCIE_LCTLSTS_COM_CLK_CFGBIT(6) +#define PCIE_LCTLSTS_HW_AW_DIS BIT(9) +#define PCIE_LCTLSTS_LINK_SPEED GENMASK(19, 16) +#define PCIE_LCTLSTS_NEGOTIATED_LINK_WIDTH GENMASK(25, 20) +#define PCIE_LCTLSTS_SLOT_CLK_CFG BIT(28) + +#define PCIE_LCTLSTS2 0xA0 +#define PCIE_LCTLSTS2_TGT_LINK_SPEEDGENMASK(3, 0) +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_25GT 0x1 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_5GT0x2 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_8GT0x3 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_16GT 0x4 +#define PCIE_LCTLSTS2_HW_AUTO_DIS BIT(5) + +/* Ack Frequency Register */ +#define PCIE_AFR0x70C +#define PCIE_AFR_FTS_NUMGENMASK(15, 8) +#define PCIE_AFR_COM_FTS_NUMGENMASK(23, 16) +#define PCIE_AFR_GEN12_FTS_NUM_DFT (SZ_128 - 1) +#define PCIE_AFR_GEN3_FTS_NUM_DFT 180 +#define PCIE_AFR_GEN4_FTS_NUM_DFT 196 + +#define PCIE_PLCR_DLL_LINK_EN BIT(5) +#define PCIE_PORT_LOGIC_FTS GENMASK(7, 0) +#define PCIE_PORT_LOGIC_DFT_FTS_NUM (SZ_128 - 1) + +#define PCIE_MISC_CTRL 0x8BC +#define PCIE_MISC_CTRL_DBI_RO_WR_EN BIT(0) + +#define PCIE_MULTI_LANE_CTRL0x8C0 +#define PCIE_UPCONFIG_SUPPORT BIT(7) +#define PCIE_DIRECT_LINK_WIDTH_CHANGE BIT(6) +#define PCIE_TARGET_LINK_WIDTH GENMASK(5, 0) + +#define PCIE_IOP_CTRL 0x8C4 +#define PCIE_IOP_RX_STANDBY_CTRLGENMASK(6, 0) no need for IOP with "are you sure that you need any of the registers above?" I really meant all registers above (including, but not limited to IOP) [...] As I mentioned, VRX200 was a very old PCIe Gen1.1 product. In our latest SoC Lightning Mountain, we are using synopsys controller 5.20/5.50a. We support Gen2(XRX350/550), Gen3(PRX300) and GEN4(X86 based SoC). We also supported dual lane and single lane. Some of the above registers are needed to control FTS, link width and link speed. only now I noticed that I didn't explain why I was asking whether all these registers are needed my understanding of the DWC PCIe controller driver "library" is that: - all functionality which is provided by the DesignWare PCIe core should be added to drivers/pci/controller/dwc/pcie-designware* - functionality which is built on top/around the DWC PCIe core should be added to the link width and link speed settings (I don't know about "FTS") don't seem Intel/Lantiq controller specific to me so the register setup for these bits should be moved to drivers/pci/controller/dwc/pcie-designware* FTS means fast training sequence. Different generations will have different FTS. Common DWC drivers have default number for all generations which are not optimized. DWC driver handles link speed and link width during the initialization. Then left link speed change and link width to device (EP) according to PCIe spec. Not sure if other vendors or customers have this kind of requirement. We implemented this due to customer's requirement. We can check
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hello, On Mon, Aug 26, 2019 at 5:31 AM Chuan Hua, Lei wrote: > > Hi Martin, > > Thanks for your valuable comments. I reply some of them as below. you're welcome [...] > >> +config PCIE_INTEL_AXI > >> +bool "Intel AHB/AXI PCIe host controller support" > > I believe that this is mostly the same IP block as it's used in Lantiq > > (xDSL) VRX200 SoCs (with MIPS cores) which was introduced in 2010 > > (before Intel acquired Lantiq). > > This is why I would have personally called the driver PCIE_LANTIQ > > VRX200 SoC(internally called VR9) was the first PCIe SoC product which > was using synopsys > > controller v3.30a. It only supports PCIe Gen1.1/1.0. The phy is internal > phy from infineon. thank you for these details I wasn't aware that the PCIe PHY on these SoCs was developed by Infineon nor is the DWC version documented anywhere [...] > >> +#define PCIE_CCRID 0x8 > >> + > >> +#define PCIE_LCAP 0x7C > >> +#define PCIE_LCAP_MAX_LINK_SPEEDGENMASK(3, 0) > >> +#define PCIE_LCAP_MAX_LENGTH_WIDTH GENMASK(9, 4) > >> + > >> +/* Link Control and Status Register */ > >> +#define PCIE_LCTLSTS0x80 > >> +#define PCIE_LCTLSTS_ASPM_ENABLEGENMASK(1, 0) > >> +#define PCIE_LCTLSTS_RCB128 BIT(3) > >> +#define PCIE_LCTLSTS_LINK_DISABLE BIT(4) > >> +#define PCIE_LCTLSTS_COM_CLK_CFGBIT(6) > >> +#define PCIE_LCTLSTS_HW_AW_DIS BIT(9) > >> +#define PCIE_LCTLSTS_LINK_SPEED GENMASK(19, 16) > >> +#define PCIE_LCTLSTS_NEGOTIATED_LINK_WIDTH GENMASK(25, 20) > >> +#define PCIE_LCTLSTS_SLOT_CLK_CFG BIT(28) > >> + > >> +#define PCIE_LCTLSTS2 0xA0 > >> +#define PCIE_LCTLSTS2_TGT_LINK_SPEEDGENMASK(3, 0) > >> +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_25GT 0x1 > >> +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_5GT0x2 > >> +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_8GT0x3 > >> +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_16GT 0x4 > >> +#define PCIE_LCTLSTS2_HW_AUTO_DIS BIT(5) > >> + > >> +/* Ack Frequency Register */ > >> +#define PCIE_AFR0x70C > >> +#define PCIE_AFR_FTS_NUMGENMASK(15, 8) > >> +#define PCIE_AFR_COM_FTS_NUMGENMASK(23, 16) > >> +#define PCIE_AFR_GEN12_FTS_NUM_DFT (SZ_128 - 1) > >> +#define PCIE_AFR_GEN3_FTS_NUM_DFT 180 > >> +#define PCIE_AFR_GEN4_FTS_NUM_DFT 196 > >> + > >> +#define PCIE_PLCR_DLL_LINK_EN BIT(5) > >> +#define PCIE_PORT_LOGIC_FTS GENMASK(7, 0) > >> +#define PCIE_PORT_LOGIC_DFT_FTS_NUM (SZ_128 - 1) > >> + > >> +#define PCIE_MISC_CTRL 0x8BC > >> +#define PCIE_MISC_CTRL_DBI_RO_WR_EN BIT(0) > >> + > >> +#define PCIE_MULTI_LANE_CTRL0x8C0 > >> +#define PCIE_UPCONFIG_SUPPORT BIT(7) > >> +#define PCIE_DIRECT_LINK_WIDTH_CHANGE BIT(6) > >> +#define PCIE_TARGET_LINK_WIDTH GENMASK(5, 0) > >> + > >> +#define PCIE_IOP_CTRL 0x8C4 > >> +#define PCIE_IOP_RX_STANDBY_CTRLGENMASK(6, 0) > no need for IOP with "are you sure that you need any of the registers above?" I really meant all registers above (including, but not limited to IOP) [...] > As I mentioned, VRX200 was a very old PCIe Gen1.1 product. In our latest > SoC Lightning > > Mountain, we are using synopsys controller 5.20/5.50a. We support > Gen2(XRX350/550), > > Gen3(PRX300) and GEN4(X86 based SoC). We also supported dual lane and > single lane. > > Some of the above registers are needed to control FTS, link width and > link speed. only now I noticed that I didn't explain why I was asking whether all these registers are needed my understanding of the DWC PCIe controller driver "library" is that: - all functionality which is provided by the DesignWare PCIe core should be added to drivers/pci/controller/dwc/pcie-designware* - functionality which is built on top/around the DWC PCIe core should be added to the link width and link speed settings (I don't know about "FTS") don't seem Intel/Lantiq controller specific to me so the register setup for these bits should be moved to drivers/pci/controller/dwc/pcie-designware* > > this also makes me wonder if various functions below like > > intel_pcie_link_setup() and intel_pcie_max_speed_setup() (and probably > > more) are really needed or if it's just cargo cult / copy paste from > > an out-of-tree driver). > > intel_pcie_link_setup is to record maximum speed and and link width. We need > these > to change speed and link width on the fly which is not supported by dwc > driver common > source. > There are two major purposes. > 1. For cable applications, they have battery support mode. In this case, it > has to > switch from x2 and gen4 to x1 and gen1 on the fly > 2. Some
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Dilip,
On Mon, Aug 26, 2019 at 8:42 AM Dilip Kota wrote:
[...]
> intel_pcie_port structure is having "struct dw_pcie" as mentioned below:
>
> struct intel_pcie_port {
> struct dw_pcie *pci;
> unsigned intid; /* Physical RC Index */
> void __iomem*app_base;
> struct gpio_desc*reset_gpio;
> [...]
> };
>
> Almost all the drivers are following the same way. I don't see any issue in
> this way.
> Please help me with more description if you see an issue here.
>
> struct qcom_pcie {
> struct dw_pcie *pci;
> Ref:
> https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/pci/controller/dwc/pcie-qcom.c
>
> struct armada8k_pcie {
> struct dw_pcie *pci;
> Ref:
> https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/pci/controller/dwc/pcie-armada8k.c
>
> struct artpec6_pcie {
> struct dw_pcie *pci;
> Ref:
> https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/pci/controller/dwc/pcie-artpec6.c
>
> struct kirin_pcie {
> struct dw_pcie *pci;
> Ref:
> https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/pci/controller/dwc/pcie-kirin.c
>
> struct spear13xx_pcie {
> struct dw_pcie *pci;
> Ref:
> https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/pci/controller/dwc/pcie-spear13xx.c
thank you for this detailed list.
it seems that I picked the minority of drivers as "reference" where
it's implemented differently:
first example: pci-meson
struct meson_pcie {
struct dw_pcie pci;
...
};
second example: pcie-tegra194 (only in -next, will be part of v5.4)
struct tegra_pcie_dw {
...
struct dw_pcie pci;
...
};
so some drivers store a pointer pointer to the dw_pcie struct vs.
embedding the dw_pcie struct directly.
as far as I know the result will be equal, except that you don't have
to use a second devm_kzalloc for struct dw_pcie (and thus reducing
memory fragmentation).
Martin
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
[Got delivery failure mail , so re-sending the mail] Hi Martin, Thanks for review comments, please find my response inline. On 8/26/2019 11:30 AM, Chuan Hua, Lei wrote: Hi Martin, Thanks for your valuable comments. I reply some of them as below. Regards, Chuanhua On 8/25/2019 5:03 AM, Martin Blumenstingl wrote: Hi Dilip, first of all: thank you for submitting this upstream! I hope that we can use this driver to replace the out-of-tree PCIe driver that's used in OpenWrt for the Lantiq VRX200 SoCs. a small disclaimer: I don't have access to any Lantiq, Intel or DesignWare datasheets. so everything I write below is based on my own understanding of the Tegra public datasheet (which describes the DesignWare PCIe registers) as well as the public Lantiq UGW code drops with out-of-tree drivers for an older version of this PCIe IP. thus some of my statements below may be wrong - in this case I would appreciate an explanation of how the hardware really works. please keep me CC'ed on further revisions of this series. I am highly interested in making this driver work on the Lantiq VRX200 SoCs once the initial version has landed in linux-next. +config PCIE_INTEL_AXI + bool "Intel AHB/AXI PCIe host controller support" I believe that this is mostly the same IP block as it's used in Lantiq (xDSL) VRX200 SoCs (with MIPS cores) which was introduced in 2010 (before Intel acquired Lantiq). This is why I would have personally called the driver PCIE_LANTIQ VRX200 SoC(internally called VR9) was the first PCIe SoC product which was using synopsys controller v3.30a. It only supports PCIe Gen1.1/1.0. The phy is internal phy from infineon. After that, we have other PCe 1.1/10. products such as ARX300/390. However, these products are EOL, that is why we don't put effort to support VRX200/ARX300/390. PCIE_LANTIQ was also a name used internally in the product as in linux-3.10.x. [...] +#define PCIE_CCRID 0x8 + +#define PCIE_LCAP 0x7C +#define PCIE_LCAP_MAX_LINK_SPEED GENMASK(3, 0) +#define PCIE_LCAP_MAX_LENGTH_WIDTH GENMASK(9, 4) + +/* Link Control and Status Register */ +#define PCIE_LCTLSTS 0x80 +#define PCIE_LCTLSTS_ASPM_ENABLE GENMASK(1, 0) +#define PCIE_LCTLSTS_RCB128 BIT(3) +#define PCIE_LCTLSTS_LINK_DISABLE BIT(4) +#define PCIE_LCTLSTS_COM_CLK_CFG BIT(6) +#define PCIE_LCTLSTS_HW_AW_DIS BIT(9) +#define PCIE_LCTLSTS_LINK_SPEED GENMASK(19, 16) +#define PCIE_LCTLSTS_NEGOTIATED_LINK_WIDTH GENMASK(25, 20) +#define PCIE_LCTLSTS_SLOT_CLK_CFG BIT(28) + +#define PCIE_LCTLSTS2 0xA0 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED GENMASK(3, 0) +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_25GT 0x1 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_5GT 0x2 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_8GT 0x3 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_16GT 0x4 +#define PCIE_LCTLSTS2_HW_AUTO_DIS BIT(5) + +/* Ack Frequency Register */ +#define PCIE_AFR 0x70C +#define PCIE_AFR_FTS_NUM GENMASK(15, 8) +#define PCIE_AFR_COM_FTS_NUM GENMASK(23, 16) +#define PCIE_AFR_GEN12_FTS_NUM_DFT (SZ_128 - 1) +#define PCIE_AFR_GEN3_FTS_NUM_DFT 180 +#define PCIE_AFR_GEN4_FTS_NUM_DFT 196 + +#define PCIE_PLCR_DLL_LINK_EN BIT(5) +#define PCIE_PORT_LOGIC_FTS GENMASK(7, 0) +#define PCIE_PORT_LOGIC_DFT_FTS_NUM (SZ_128 - 1) + +#define PCIE_MISC_CTRL 0x8BC +#define PCIE_MISC_CTRL_DBI_RO_WR_EN BIT(0) + +#define PCIE_MULTI_LANE_CTRL 0x8C0 +#define PCIE_UPCONFIG_SUPPORT BIT(7) +#define PCIE_DIRECT_LINK_WIDTH_CHANGE BIT(6) +#define PCIE_TARGET_LINK_WIDTH GENMASK(5, 0) + +#define PCIE_IOP_CTRL 0x8C4 +#define PCIE_IOP_RX_STANDBY_CTRL GENMASK(6, 0) no need for IOP are you sure that you need any of the registers above? as far as I can tell most (all?) of them are part of the DesignWare register set. why doesn't pcie-designware-host initialize these? I don't have any datasheet or register documentation for the DesignWare PCIe core. In my own experiment from a month ago on the Lantiq VRX200 SoC I didn't need any of this: [0] As I mentioned, VRX200 was a very old PCIe Gen1.1 product. In our latest SoC Lightning Mountain, we are using synopsys controller 5.20/5.50a. We support Gen2(XRX350/550), Gen3(PRX300) and GEN4(X86 based SoC). We also supported dual lane and single lane. Some of the above registers are needed to control FTS, link width and link speed. this also makes me wonder if various functions below like intel_pcie_link_setup() and intel_pcie_max_speed_setup() (and probably more) are really needed or if it's just cargo cult / copy paste from an out-of-tree driver). intel_pcie_link_setup is to record maximum speed and and link width. We need these to change speed and link width on the fly which is not supported by dwc
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Martin, Thanks for your valuable comments. I reply some of them as below. Regards, Chuanhua On 8/25/2019 5:03 AM, Martin Blumenstingl wrote: Hi Dilip, first of all: thank you for submitting this upstream! I hope that we can use this driver to replace the out-of-tree PCIe driver that's used in OpenWrt for the Lantiq VRX200 SoCs. a small disclaimer: I don't have access to any Lantiq, Intel or DesignWare datasheets. so everything I write below is based on my own understanding of the Tegra public datasheet (which describes the DesignWare PCIe registers) as well as the public Lantiq UGW code drops with out-of-tree drivers for an older version of this PCIe IP. thus some of my statements below may be wrong - in this case I would appreciate an explanation of how the hardware really works. please keep me CC'ed on further revisions of this series. I am highly interested in making this driver work on the Lantiq VRX200 SoCs once the initial version has landed in linux-next. +config PCIE_INTEL_AXI +bool "Intel AHB/AXI PCIe host controller support" I believe that this is mostly the same IP block as it's used in Lantiq (xDSL) VRX200 SoCs (with MIPS cores) which was introduced in 2010 (before Intel acquired Lantiq). This is why I would have personally called the driver PCIE_LANTIQ VRX200 SoC(internally called VR9) was the first PCIe SoC product which was using synopsys controller v3.30a. It only supports PCIe Gen1.1/1.0. The phy is internal phy from infineon. After that, we have other PCe 1.1/10. products such as ARX300/390. However, these products are EOL, that is why we don't put effort to support VRX200/ARX300/390. PCIE_LANTIQ was also a name used internally in the product as in linux-3.10.x. [...] +#define PCIE_CCRID 0x8 + +#define PCIE_LCAP 0x7C +#define PCIE_LCAP_MAX_LINK_SPEED GENMASK(3, 0) +#define PCIE_LCAP_MAX_LENGTH_WIDTH GENMASK(9, 4) + +/* Link Control and Status Register */ +#define PCIE_LCTLSTS 0x80 +#define PCIE_LCTLSTS_ASPM_ENABLE GENMASK(1, 0) +#define PCIE_LCTLSTS_RCB128BIT(3) +#define PCIE_LCTLSTS_LINK_DISABLE BIT(4) +#define PCIE_LCTLSTS_COM_CLK_CFG BIT(6) +#define PCIE_LCTLSTS_HW_AW_DIS BIT(9) +#define PCIE_LCTLSTS_LINK_SPEEDGENMASK(19, 16) +#define PCIE_LCTLSTS_NEGOTIATED_LINK_WIDTH GENMASK(25, 20) +#define PCIE_LCTLSTS_SLOT_CLK_CFG BIT(28) + +#define PCIE_LCTLSTS2 0xA0 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED GENMASK(3, 0) +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_25GT 0x1 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_5GT 0x2 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_8GT 0x3 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_16GT 0x4 +#define PCIE_LCTLSTS2_HW_AUTO_DIS BIT(5) + +/* Ack Frequency Register */ +#define PCIE_AFR 0x70C +#define PCIE_AFR_FTS_NUM GENMASK(15, 8) +#define PCIE_AFR_COM_FTS_NUM GENMASK(23, 16) +#define PCIE_AFR_GEN12_FTS_NUM_DFT (SZ_128 - 1) +#define PCIE_AFR_GEN3_FTS_NUM_DFT 180 +#define PCIE_AFR_GEN4_FTS_NUM_DFT 196 + +#define PCIE_PLCR_DLL_LINK_EN BIT(5) +#define PCIE_PORT_LOGIC_FTSGENMASK(7, 0) +#define PCIE_PORT_LOGIC_DFT_FTS_NUM(SZ_128 - 1) + +#define PCIE_MISC_CTRL 0x8BC +#define PCIE_MISC_CTRL_DBI_RO_WR_ENBIT(0) + +#define PCIE_MULTI_LANE_CTRL 0x8C0 +#define PCIE_UPCONFIG_SUPPORT BIT(7) +#define PCIE_DIRECT_LINK_WIDTH_CHANGE BIT(6) +#define PCIE_TARGET_LINK_WIDTH GENMASK(5, 0) + +#define PCIE_IOP_CTRL 0x8C4 +#define PCIE_IOP_RX_STANDBY_CTRL GENMASK(6, 0) no need for IOP are you sure that you need any of the registers above? as far as I can tell most (all?) of them are part of the DesignWare register set. why doesn't pcie-designware-host initialize these? I don't have any datasheet or register documentation for the DesignWare PCIe core. In my own experiment from a month ago on the Lantiq VRX200 SoC I didn't need any of this: [0] As I mentioned, VRX200 was a very old PCIe Gen1.1 product. In our latest SoC Lightning Mountain, we are using synopsys controller 5.20/5.50a. We support Gen2(XRX350/550), Gen3(PRX300) and GEN4(X86 based SoC). We also supported dual lane and single lane. Some of the above registers are needed to control FTS, link width and link speed. this also makes me wonder if various functions below like intel_pcie_link_setup() and intel_pcie_max_speed_setup() (and probably more) are really needed or if it's just cargo cult / copy paste from an out-of-tree driver). intel_pcie_link_setup is to record maximum speed and and link width. We need these to
RE: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Dilip, first of all: thank you for submitting this upstream! I hope that we can use this driver to replace the out-of-tree PCIe driver that's used in OpenWrt for the Lantiq VRX200 SoCs. a small disclaimer: I don't have access to any Lantiq, Intel or DesignWare datasheets. so everything I write below is based on my own understanding of the Tegra public datasheet (which describes the DesignWare PCIe registers) as well as the public Lantiq UGW code drops with out-of-tree drivers for an older version of this PCIe IP. thus some of my statements below may be wrong - in this case I would appreciate an explanation of how the hardware really works. please keep me CC'ed on further revisions of this series. I am highly interested in making this driver work on the Lantiq VRX200 SoCs once the initial version has landed in linux-next. > +config PCIE_INTEL_AXI > +bool "Intel AHB/AXI PCIe host controller support" I believe that this is mostly the same IP block as it's used in Lantiq (xDSL) VRX200 SoCs (with MIPS cores) which was introduced in 2010 (before Intel acquired Lantiq). This is why I would have personally called the driver PCIE_LANTIQ [...] > +#define PCIE_CCRID 0x8 > + > +#define PCIE_LCAP0x7C > +#define PCIE_LCAP_MAX_LINK_SPEED GENMASK(3, 0) > +#define PCIE_LCAP_MAX_LENGTH_WIDTH GENMASK(9, 4) > + > +/* Link Control and Status Register */ > +#define PCIE_LCTLSTS 0x80 > +#define PCIE_LCTLSTS_ASPM_ENABLE GENMASK(1, 0) > +#define PCIE_LCTLSTS_RCB128 BIT(3) > +#define PCIE_LCTLSTS_LINK_DISABLEBIT(4) > +#define PCIE_LCTLSTS_COM_CLK_CFG BIT(6) > +#define PCIE_LCTLSTS_HW_AW_DIS BIT(9) > +#define PCIE_LCTLSTS_LINK_SPEED GENMASK(19, 16) > +#define PCIE_LCTLSTS_NEGOTIATED_LINK_WIDTH GENMASK(25, 20) > +#define PCIE_LCTLSTS_SLOT_CLK_CFGBIT(28) > + > +#define PCIE_LCTLSTS20xA0 > +#define PCIE_LCTLSTS2_TGT_LINK_SPEED GENMASK(3, 0) > +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_25GT0x1 > +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_5GT 0x2 > +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_8GT 0x3 > +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_16GT0x4 > +#define PCIE_LCTLSTS2_HW_AUTO_DISBIT(5) > + > +/* Ack Frequency Register */ > +#define PCIE_AFR 0x70C > +#define PCIE_AFR_FTS_NUM GENMASK(15, 8) > +#define PCIE_AFR_COM_FTS_NUM GENMASK(23, 16) > +#define PCIE_AFR_GEN12_FTS_NUM_DFT (SZ_128 - 1) > +#define PCIE_AFR_GEN3_FTS_NUM_DFT180 > +#define PCIE_AFR_GEN4_FTS_NUM_DFT196 > + > +#define PCIE_PLCR_DLL_LINK_ENBIT(5) > +#define PCIE_PORT_LOGIC_FTS GENMASK(7, 0) > +#define PCIE_PORT_LOGIC_DFT_FTS_NUM (SZ_128 - 1) > + > +#define PCIE_MISC_CTRL 0x8BC > +#define PCIE_MISC_CTRL_DBI_RO_WR_EN BIT(0) > + > +#define PCIE_MULTI_LANE_CTRL 0x8C0 > +#define PCIE_UPCONFIG_SUPPORTBIT(7) > +#define PCIE_DIRECT_LINK_WIDTH_CHANGEBIT(6) > +#define PCIE_TARGET_LINK_WIDTH GENMASK(5, 0) > + > +#define PCIE_IOP_CTRL0x8C4 > +#define PCIE_IOP_RX_STANDBY_CTRL GENMASK(6, 0) are you sure that you need any of the registers above? as far as I can tell most (all?) of them are part of the DesignWare register set. why doesn't pcie-designware-host initialize these? I don't have any datasheet or register documentation for the DesignWare PCIe core. In my own experiment from a month ago on the Lantiq VRX200 SoC I didn't need any of this: [0] this also makes me wonder if various functions below like intel_pcie_link_setup() and intel_pcie_max_speed_setup() (and probably more) are really needed or if it's just cargo cult / copy paste from an out-of-tree driver). > +/* APP RC Core Control Register */ > +#define PCIE_RC_CCR 0x10 > +#define PCIE_RC_CCR_LTSSM_ENABLE BIT(0) > +#define PCIE_DEVICE_TYPE GENMASK(7, 4) > +#define PCIE_RC_CCR_RC_MODE BIT(2) > + > +/* PCIe Message Control */ > +#define PCIE_MSG_CR 0x30 > +#define PCIE_XMT_PM_TURNOFF BIT(0) > + > +/* PCIe Power Management Control */ > +#define PCIE_PMC 0x44 > +#define PCIE_PM_IN_L2BIT(20) > + > +/* Interrupt Enable Register */ > +#define PCIE_IRNEN 0xF4 > +#define PCIE_IRNCR 0xF8 > +#define PCIE_IRN_AER_REPORT BIT(0) > +#define PCIE_IRN_PME BIT(2) > +#define PCIE_IRN_HOTPLUG BIT(3) > +#define PCIE_IRN_RX_VDM_MSG BIT(4) > +#define PCIE_IRN_PM_TO_ACK
