Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Monday 27 January 2014, Tanmay Inamdar wrote: > On Sat, Jan 25, 2014 at 12:11 PM, Arnd Bergmann wrote: > > On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote: > >> On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar wrote: > >> > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann wrote: > >> >> On Wednesday 15 January 2014, Tanmay Inamdar wrote: > >> >> > >> >> Maybe another msleep() in the loop? It seems weird to first do an > >> >> unconditional sleep but then busy-wait for the result. > >> > > >> > ok. > >> > >> This loop can execute for maximum 4 msec. So putting msleep(1) won't > >> get us much. > > > > 4 msec is still quite a long time for a busy loop that can be spent doing > > useful work in another thread. > > > > Right. If 'msleep(1)' is used, then 'checkpatch' throws a warning > saying that it can actually sleep for 20ms in some cases. I will check > if 'usleep_range' is useful here. Sound good. This is really a false positive from checkpatch though, with the timeout handling in place, everything's fine even with msleep(1). > >> >> > >> >> Another general note: Your "compatible" strings are rather unspecific. > >> >> Do you have a version number for this IP block? I suppose that it's > >> >> related > >> >> to one that has been used in other chips before, or will be used in > >> >> future > >> >> chips, if it's not actually licensed from some other company. > >> > > >> > I will have to check this. > >> > > >> > >> We have decided to stick with current compatible string for now. > > > > Can you elaborate on your reasoning? Does this mean X-Gene is a one-off > > product and you won't be doing any new chips based on the same hardware > > components? > > The current convention is to key upon the family name - X-Gene. Future > chips will also be a part of X-Gene family. Right now it is unclear if > there are any obvious feature additions to be done in Linux PCIe > driver. Until then same driver is expected to work as is in future > chips. This is not enough for me. Of course you hope that things keep working, but experience shows that sometimes hardware has slight differences that you need to work around later. It's better to always be specific and at least as a secondary identifier list the exact model of the component, or if that is not know, the model of the SoC. The driver can bind to the most generic string, but in DT you should have a specific one as well. You could for instance have something like compatible = "apm,xgene-1234w78-pcie", "thirdparty,pcie-1.23", "apm,xgene-pcie", "thirdparty,pcie"; as an example where you licensed the pcie block version 1.23 from a company named thirdparty and integrated it into the xgene variant with product code 1234w78. 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: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Monday 27 January 2014, Tanmay Inamdar wrote: On Sat, Jan 25, 2014 at 12:11 PM, Arnd Bergmann a...@arndb.de wrote: On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote: On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar tinam...@apm.com wrote: On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann a...@arndb.de wrote: On Wednesday 15 January 2014, Tanmay Inamdar wrote: Maybe another msleep() in the loop? It seems weird to first do an unconditional sleep but then busy-wait for the result. ok. This loop can execute for maximum 4 msec. So putting msleep(1) won't get us much. 4 msec is still quite a long time for a busy loop that can be spent doing useful work in another thread. Right. If 'msleep(1)' is used, then 'checkpatch' throws a warning saying that it can actually sleep for 20ms in some cases. I will check if 'usleep_range' is useful here. Sound good. This is really a false positive from checkpatch though, with the timeout handling in place, everything's fine even with msleep(1). Another general note: Your compatible strings are rather unspecific. Do you have a version number for this IP block? I suppose that it's related to one that has been used in other chips before, or will be used in future chips, if it's not actually licensed from some other company. I will have to check this. We have decided to stick with current compatible string for now. Can you elaborate on your reasoning? Does this mean X-Gene is a one-off product and you won't be doing any new chips based on the same hardware components? The current convention is to key upon the family name - X-Gene. Future chips will also be a part of X-Gene family. Right now it is unclear if there are any obvious feature additions to be done in Linux PCIe driver. Until then same driver is expected to work as is in future chips. This is not enough for me. Of course you hope that things keep working, but experience shows that sometimes hardware has slight differences that you need to work around later. It's better to always be specific and at least as a secondary identifier list the exact model of the component, or if that is not know, the model of the SoC. The driver can bind to the most generic string, but in DT you should have a specific one as well. You could for instance have something like compatible = apm,xgene-1234w78-pcie, thirdparty,pcie-1.23, apm,xgene-pcie, thirdparty,pcie; as an example where you licensed the pcie block version 1.23 from a company named thirdparty and integrated it into the xgene variant with product code 1234w78. 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: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Mon, Jan 27, 2014 at 4:55 PM, Bjorn Helgaas wrote:
> We're only seeing Arnd's side of the conversation on linux-pci.
> Tanmay, are your messages being rejected because they're too "fancy",
> per the definition here: http://vger.kernel.org/majordomo-info.html ?
>
Thanks for pointing out. I am not sure though what's being detected as
fancy. I checked that my emails are received as plaintext on
majordomo. They are also displayed fine on LKML.
In this email, I have tried to keep the format of to: and cc: same as
first email in the thread. Not sure if this fixes the problem.
Again.. sorry for spamming.
> On Sat, Jan 25, 2014 at 1:11 PM, Arnd Bergmann wrote:
>> On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote:
>>> On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar wrote:
>>> > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann wrote:
>>> >> On Wednesday 15 January 2014, Tanmay Inamdar wrote:
>>
>>> >>
>>> >>> +static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32
>>> >>> *lanes)
>>> >>> +{
>>> >>> + void *csr_base = port->csr_base;
>>> >>> + u32 val32;
>>> >>> + u64 start_time, time;
>>> >>> +
>>> >>> + /*
>>> >>> + * A component enters the LTSSM Detect state within
>>> >>> + * 20ms of the end of fundamental core reset.
>>> >>> + */
>>> >>> + msleep(XGENE_LTSSM_DETECT_WAIT);
>>> >>> + port->link_up = 0;
>>> >>> + start_time = jiffies;
>>> >>> + do {
>>> >>> + val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
>>> >>> + if (val32 & LINK_UP_MASK) {
>>> >>> + port->link_up = 1;
>>> >>> + port->link_speed = PIPE_PHY_RATE_RD(val32);
>>> >>> + val32 = readl(csr_base + BRIDGE_STATUS_0);
>>> >>> + *lanes = val32 >> 26;
>>> >>> + }
>>> >>> + time = jiffies_to_msecs(jiffies - start_time);
>>> >>> + } while ((!port->link_up) || (time <= XGENE_LTSSM_L0_WAIT));
>>> >>> +}
>>> >>
>>> >> Maybe another msleep() in the loop? It seems weird to first do an
>>> >> unconditional sleep but then busy-wait for the result.
>>> >
>>> > ok.
>>>
>>> This loop can execute for maximum 4 msec. So putting msleep(1) won't
>>> get us much.
>>
>> 4 msec is still quite a long time for a busy loop that can be spent doing
>> useful work in another thread.
>>
>>> >>
>>> >> Another general note: Your "compatible" strings are rather unspecific.
>>> >> Do you have a version number for this IP block? I suppose that it's
>>> >> related
>>> >> to one that has been used in other chips before, or will be used in
>>> >> future
>>> >> chips, if it's not actually licensed from some other company.
>>> >
>>> > I will have to check this.
>>> >
>>>
>>> We have decided to stick with current compatible string for now.
>>
>> Can you elaborate on your reasoning? Does this mean X-Gene is a one-off
>> product and you won't be doing any new chips based on the same hardware
>> components?
>>
>> 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: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
We're only seeing Arnd's side of the conversation on linux-pci.
Tanmay, are your messages being rejected because they're too "fancy",
per the definition here: http://vger.kernel.org/majordomo-info.html ?
On Sat, Jan 25, 2014 at 1:11 PM, Arnd Bergmann wrote:
> On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote:
>> On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar wrote:
>> > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann wrote:
>> >> On Wednesday 15 January 2014, Tanmay Inamdar wrote:
>
>> >>
>> >>> +static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32
>> >>> *lanes)
>> >>> +{
>> >>> + void *csr_base = port->csr_base;
>> >>> + u32 val32;
>> >>> + u64 start_time, time;
>> >>> +
>> >>> + /*
>> >>> + * A component enters the LTSSM Detect state within
>> >>> + * 20ms of the end of fundamental core reset.
>> >>> + */
>> >>> + msleep(XGENE_LTSSM_DETECT_WAIT);
>> >>> + port->link_up = 0;
>> >>> + start_time = jiffies;
>> >>> + do {
>> >>> + val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
>> >>> + if (val32 & LINK_UP_MASK) {
>> >>> + port->link_up = 1;
>> >>> + port->link_speed = PIPE_PHY_RATE_RD(val32);
>> >>> + val32 = readl(csr_base + BRIDGE_STATUS_0);
>> >>> + *lanes = val32 >> 26;
>> >>> + }
>> >>> + time = jiffies_to_msecs(jiffies - start_time);
>> >>> + } while ((!port->link_up) || (time <= XGENE_LTSSM_L0_WAIT));
>> >>> +}
>> >>
>> >> Maybe another msleep() in the loop? It seems weird to first do an
>> >> unconditional sleep but then busy-wait for the result.
>> >
>> > ok.
>>
>> This loop can execute for maximum 4 msec. So putting msleep(1) won't
>> get us much.
>
> 4 msec is still quite a long time for a busy loop that can be spent doing
> useful work in another thread.
>
>> >>
>> >> Another general note: Your "compatible" strings are rather unspecific.
>> >> Do you have a version number for this IP block? I suppose that it's
>> >> related
>> >> to one that has been used in other chips before, or will be used in future
>> >> chips, if it's not actually licensed from some other company.
>> >
>> > I will have to check this.
>> >
>>
>> We have decided to stick with current compatible string for now.
>
> Can you elaborate on your reasoning? Does this mean X-Gene is a one-off
> product and you won't be doing any new chips based on the same hardware
> components?
>
> 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: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Sat, Jan 25, 2014 at 12:11 PM, Arnd Bergmann wrote:
> On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote:
>> On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar wrote:
>> > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann wrote:
>> >> On Wednesday 15 January 2014, Tanmay Inamdar wrote:
>
>> >>
>> >>> +static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32
>> >>> *lanes)
>> >>> +{
>> >>> + void *csr_base = port->csr_base;
>> >>> + u32 val32;
>> >>> + u64 start_time, time;
>> >>> +
>> >>> + /*
>> >>> + * A component enters the LTSSM Detect state within
>> >>> + * 20ms of the end of fundamental core reset.
>> >>> + */
>> >>> + msleep(XGENE_LTSSM_DETECT_WAIT);
>> >>> + port->link_up = 0;
>> >>> + start_time = jiffies;
>> >>> + do {
>> >>> + val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
>> >>> + if (val32 & LINK_UP_MASK) {
>> >>> + port->link_up = 1;
>> >>> + port->link_speed = PIPE_PHY_RATE_RD(val32);
>> >>> + val32 = readl(csr_base + BRIDGE_STATUS_0);
>> >>> + *lanes = val32 >> 26;
>> >>> + }
>> >>> + time = jiffies_to_msecs(jiffies - start_time);
>> >>> + } while ((!port->link_up) || (time <= XGENE_LTSSM_L0_WAIT));
>> >>> +}
>> >>
>> >> Maybe another msleep() in the loop? It seems weird to first do an
>> >> unconditional sleep but then busy-wait for the result.
>> >
>> > ok.
>>
>> This loop can execute for maximum 4 msec. So putting msleep(1) won't
>> get us much.
>
> 4 msec is still quite a long time for a busy loop that can be spent doing
> useful work in another thread.
>
Right. If 'msleep(1)' is used, then 'checkpatch' throws a warning
saying that it can actually sleep for 20ms in some cases. I will check
if 'usleep_range' is useful here.
>> >>
>> >> Another general note: Your "compatible" strings are rather unspecific.
>> >> Do you have a version number for this IP block? I suppose that it's
>> >> related
>> >> to one that has been used in other chips before, or will be used in future
>> >> chips, if it's not actually licensed from some other company.
>> >
>> > I will have to check this.
>> >
>>
>> We have decided to stick with current compatible string for now.
>
> Can you elaborate on your reasoning? Does this mean X-Gene is a one-off
> product and you won't be doing any new chips based on the same hardware
> components?
The current convention is to key upon the family name - X-Gene. Future
chips will also be a part of X-Gene family. Right now it is unclear if
there are any obvious feature additions to be done in Linux PCIe
driver. Until then same driver is expected to work as is in future
chips.
>
> 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: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Sat, Jan 25, 2014 at 12:11 PM, Arnd Bergmann a...@arndb.de wrote:
On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote:
On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar tinam...@apm.com wrote:
On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann a...@arndb.de wrote:
On Wednesday 15 January 2014, Tanmay Inamdar wrote:
+static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32
*lanes)
+{
+ void *csr_base = port-csr_base;
+ u32 val32;
+ u64 start_time, time;
+
+ /*
+ * A component enters the LTSSM Detect state within
+ * 20ms of the end of fundamental core reset.
+ */
+ msleep(XGENE_LTSSM_DETECT_WAIT);
+ port-link_up = 0;
+ start_time = jiffies;
+ do {
+ val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
+ if (val32 LINK_UP_MASK) {
+ port-link_up = 1;
+ port-link_speed = PIPE_PHY_RATE_RD(val32);
+ val32 = readl(csr_base + BRIDGE_STATUS_0);
+ *lanes = val32 26;
+ }
+ time = jiffies_to_msecs(jiffies - start_time);
+ } while ((!port-link_up) || (time = XGENE_LTSSM_L0_WAIT));
+}
Maybe another msleep() in the loop? It seems weird to first do an
unconditional sleep but then busy-wait for the result.
ok.
This loop can execute for maximum 4 msec. So putting msleep(1) won't
get us much.
4 msec is still quite a long time for a busy loop that can be spent doing
useful work in another thread.
Right. If 'msleep(1)' is used, then 'checkpatch' throws a warning
saying that it can actually sleep for 20ms in some cases. I will check
if 'usleep_range' is useful here.
Another general note: Your compatible strings are rather unspecific.
Do you have a version number for this IP block? I suppose that it's
related
to one that has been used in other chips before, or will be used in future
chips, if it's not actually licensed from some other company.
I will have to check this.
We have decided to stick with current compatible string for now.
Can you elaborate on your reasoning? Does this mean X-Gene is a one-off
product and you won't be doing any new chips based on the same hardware
components?
The current convention is to key upon the family name - X-Gene. Future
chips will also be a part of X-Gene family. Right now it is unclear if
there are any obvious feature additions to be done in Linux PCIe
driver. Until then same driver is expected to work as is in future
chips.
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: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
We're only seeing Arnd's side of the conversation on linux-pci.
Tanmay, are your messages being rejected because they're too fancy,
per the definition here: http://vger.kernel.org/majordomo-info.html ?
On Sat, Jan 25, 2014 at 1:11 PM, Arnd Bergmann a...@arndb.de wrote:
On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote:
On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar tinam...@apm.com wrote:
On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann a...@arndb.de wrote:
On Wednesday 15 January 2014, Tanmay Inamdar wrote:
+static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32
*lanes)
+{
+ void *csr_base = port-csr_base;
+ u32 val32;
+ u64 start_time, time;
+
+ /*
+ * A component enters the LTSSM Detect state within
+ * 20ms of the end of fundamental core reset.
+ */
+ msleep(XGENE_LTSSM_DETECT_WAIT);
+ port-link_up = 0;
+ start_time = jiffies;
+ do {
+ val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
+ if (val32 LINK_UP_MASK) {
+ port-link_up = 1;
+ port-link_speed = PIPE_PHY_RATE_RD(val32);
+ val32 = readl(csr_base + BRIDGE_STATUS_0);
+ *lanes = val32 26;
+ }
+ time = jiffies_to_msecs(jiffies - start_time);
+ } while ((!port-link_up) || (time = XGENE_LTSSM_L0_WAIT));
+}
Maybe another msleep() in the loop? It seems weird to first do an
unconditional sleep but then busy-wait for the result.
ok.
This loop can execute for maximum 4 msec. So putting msleep(1) won't
get us much.
4 msec is still quite a long time for a busy loop that can be spent doing
useful work in another thread.
Another general note: Your compatible strings are rather unspecific.
Do you have a version number for this IP block? I suppose that it's
related
to one that has been used in other chips before, or will be used in future
chips, if it's not actually licensed from some other company.
I will have to check this.
We have decided to stick with current compatible string for now.
Can you elaborate on your reasoning? Does this mean X-Gene is a one-off
product and you won't be doing any new chips based on the same hardware
components?
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: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Mon, Jan 27, 2014 at 4:55 PM, Bjorn Helgaas bhelg...@google.com wrote:
We're only seeing Arnd's side of the conversation on linux-pci.
Tanmay, are your messages being rejected because they're too fancy,
per the definition here: http://vger.kernel.org/majordomo-info.html ?
Thanks for pointing out. I am not sure though what's being detected as
fancy. I checked that my emails are received as plaintext on
majordomo. They are also displayed fine on LKML.
In this email, I have tried to keep the format of to: and cc: same as
first email in the thread. Not sure if this fixes the problem.
Again.. sorry for spamming.
On Sat, Jan 25, 2014 at 1:11 PM, Arnd Bergmann a...@arndb.de wrote:
On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote:
On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar tinam...@apm.com wrote:
On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann a...@arndb.de wrote:
On Wednesday 15 January 2014, Tanmay Inamdar wrote:
+static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32
*lanes)
+{
+ void *csr_base = port-csr_base;
+ u32 val32;
+ u64 start_time, time;
+
+ /*
+ * A component enters the LTSSM Detect state within
+ * 20ms of the end of fundamental core reset.
+ */
+ msleep(XGENE_LTSSM_DETECT_WAIT);
+ port-link_up = 0;
+ start_time = jiffies;
+ do {
+ val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
+ if (val32 LINK_UP_MASK) {
+ port-link_up = 1;
+ port-link_speed = PIPE_PHY_RATE_RD(val32);
+ val32 = readl(csr_base + BRIDGE_STATUS_0);
+ *lanes = val32 26;
+ }
+ time = jiffies_to_msecs(jiffies - start_time);
+ } while ((!port-link_up) || (time = XGENE_LTSSM_L0_WAIT));
+}
Maybe another msleep() in the loop? It seems weird to first do an
unconditional sleep but then busy-wait for the result.
ok.
This loop can execute for maximum 4 msec. So putting msleep(1) won't
get us much.
4 msec is still quite a long time for a busy loop that can be spent doing
useful work in another thread.
Another general note: Your compatible strings are rather unspecific.
Do you have a version number for this IP block? I suppose that it's
related
to one that has been used in other chips before, or will be used in
future
chips, if it's not actually licensed from some other company.
I will have to check this.
We have decided to stick with current compatible string for now.
Can you elaborate on your reasoning? Does this mean X-Gene is a one-off
product and you won't be doing any new chips based on the same hardware
components?
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: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote:
> On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar wrote:
> > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann wrote:
> >> On Wednesday 15 January 2014, Tanmay Inamdar wrote:
> >>
> >>> +static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32
> >>> *lanes)
> >>> +{
> >>> + void *csr_base = port->csr_base;
> >>> + u32 val32;
> >>> + u64 start_time, time;
> >>> +
> >>> + /*
> >>> + * A component enters the LTSSM Detect state within
> >>> + * 20ms of the end of fundamental core reset.
> >>> + */
> >>> + msleep(XGENE_LTSSM_DETECT_WAIT);
> >>> + port->link_up = 0;
> >>> + start_time = jiffies;
> >>> + do {
> >>> + val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
> >>> + if (val32 & LINK_UP_MASK) {
> >>> + port->link_up = 1;
> >>> + port->link_speed = PIPE_PHY_RATE_RD(val32);
> >>> + val32 = readl(csr_base + BRIDGE_STATUS_0);
> >>> + *lanes = val32 >> 26;
> >>> + }
> >>> + time = jiffies_to_msecs(jiffies - start_time);
> >>> + } while ((!port->link_up) || (time <= XGENE_LTSSM_L0_WAIT));
> >>> +}
> >>
> >> Maybe another msleep() in the loop? It seems weird to first do an
> >> unconditional sleep but then busy-wait for the result.
> >
> > ok.
>
> This loop can execute for maximum 4 msec. So putting msleep(1) won't
> get us much.
4 msec is still quite a long time for a busy loop that can be spent doing
useful work in another thread.
> >>
> >> Another general note: Your "compatible" strings are rather unspecific.
> >> Do you have a version number for this IP block? I suppose that it's related
> >> to one that has been used in other chips before, or will be used in future
> >> chips, if it's not actually licensed from some other company.
> >
> > I will have to check this.
> >
>
> We have decided to stick with current compatible string for now.
Can you elaborate on your reasoning? Does this mean X-Gene is a one-off
product and you won't be doing any new chips based on the same hardware
components?
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: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote:
On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar tinam...@apm.com wrote:
On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann a...@arndb.de wrote:
On Wednesday 15 January 2014, Tanmay Inamdar wrote:
+static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32
*lanes)
+{
+ void *csr_base = port-csr_base;
+ u32 val32;
+ u64 start_time, time;
+
+ /*
+ * A component enters the LTSSM Detect state within
+ * 20ms of the end of fundamental core reset.
+ */
+ msleep(XGENE_LTSSM_DETECT_WAIT);
+ port-link_up = 0;
+ start_time = jiffies;
+ do {
+ val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
+ if (val32 LINK_UP_MASK) {
+ port-link_up = 1;
+ port-link_speed = PIPE_PHY_RATE_RD(val32);
+ val32 = readl(csr_base + BRIDGE_STATUS_0);
+ *lanes = val32 26;
+ }
+ time = jiffies_to_msecs(jiffies - start_time);
+ } while ((!port-link_up) || (time = XGENE_LTSSM_L0_WAIT));
+}
Maybe another msleep() in the loop? It seems weird to first do an
unconditional sleep but then busy-wait for the result.
ok.
This loop can execute for maximum 4 msec. So putting msleep(1) won't
get us much.
4 msec is still quite a long time for a busy loop that can be spent doing
useful work in another thread.
Another general note: Your compatible strings are rather unspecific.
Do you have a version number for this IP block? I suppose that it's related
to one that has been used in other chips before, or will be used in future
chips, if it's not actually licensed from some other company.
I will have to check this.
We have decided to stick with current compatible string for now.
Can you elaborate on your reasoning? Does this mean X-Gene is a one-off
product and you won't be doing any new chips based on the same hardware
components?
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: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar wrote:
> On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann wrote:
>> On Wednesday 15 January 2014, Tanmay Inamdar wrote:
>>> This patch adds the AppliedMicro X-Gene SOC PCIe controller driver.
>>> X-Gene PCIe controller supports maxmum upto 8 lanes and GEN3 speed.
>>> X-Gene has maximum 5 PCIe ports supported.
>>>
>>> Signed-off-by: Tanmay Inamdar
>>
>> This already looks much better than the first version, but I have a more
>> comments. Most importantly, it would help to know how the root ports
>> are structured. Is this a standard root complex and multiple ports,
>> multiple root complexes with one port each, or a nonstandard organization
>> that is a mix of those two models?
>
> This is multiple root complexes with one port each.
>
>>
>>> +
>>> +/* When the address bit [17:16] is 2'b01, the Configuration access will be
>>> + * treated as Type 1 and it will be forwarded to external PCIe device.
>>> + */
>>> +static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
>>> +{
>>> + struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
>>> + u64 addr = (u64)port->cfg_base;
>>> +
>>> + if (bus->number >= (port->first_busno + 1))
>>> + addr |= AXI_EP_CFG_ACCESS;
>>> +
>>> + return (void *)addr;
>>> +}
>>
>> Wrong type, it should be 'void __iomem *'. Also you can't assume that
>> bit operations work on virtual __iomem addresses, so it should be better
>> to just add a constant integer to the pointer, which is a valid
>> operation.
>
> ok.
>
>>
>> I also wonder why you need to do this at all. If there isn't a global
>> config space for all ports, but rather a separate type0/type1 config
>> cycle based on the bus number, I see that as an indication that the
>> ports are in fact separate domains and should each start with bus 0.
>
> It is not a standard ECAM layout. We also have a separate RTDID
> register as well to program bus, device, function. While accessing EP
> config space, we have to set the bit 17:16 as 2b'01. The same config
> space address is utilized for enabling a customized nonstandard PCIe
> DMA feature. The bits are defined to differentiate the access purpose.
> The feature is not supported in this driver yet.
>
> Secondly I don't think it will matter if each port starts with bus 0.
> As long as we set the correct BDF in RTDID and set correct bits in
> config address, the config reads and writes would work. Right?
>
>>
>>> +static void xgene_pcie_setup_lanes(struct xgene_pcie_port *port)
>>> +{
>>> + void *csr_base = port->csr_base;
>>> + u32 val;
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_8);
>>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_8);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_9);
>>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_9);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_10);
>>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_10);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_11);
>>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_11);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_4);
>>> + val = (val & ~0x30) | (1 << 4);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_4);
>>> +}
>>
>> Please document what you are actually setting here. If the configuration
>> of the lanes is always the same, why do you have to set it here. If not,
>> why do you set constant values?
>
> Good point. Let me check if these values should be constant or tune-able.
>
>>
>>> +static void xgene_pcie_setup_link(struct xgene_pcie_port *port)
>>> +{
>>> + void *csr_base = port->csr_base;
>>> + u32 val;
>>> +
>>> + val = readl(csr_base + BRIDGE_CFG_14);
>>> + val |= DIRECT_TO_8GTS_MASK;
>>> + val |= SUPPORT_5GTS_MASK;
>>> + val |= SUPPORT_8GTS_MASK;
>>> + val |= DIRECT_TO_5GTS_MASK;
>>> + writel(val, csr_base + BRIDGE_CFG_14);
>>> +
>>> + val = readl(csr_base + BRIDGE_CFG_14);
>>> + val &= ~ADVT_INFINITE_CREDITS;
>>> + writel(val, csr_base + BRIDGE_CFG_14);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_0);
>>> + val |= (val & ~0xf) | 7;
>>> + val |= (val & ~0xf00) | ((7 << 8) & 0xf00);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_0);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_0);
>>> + val |= DWNSTRM_EQ_SKP_PHS_2_3;
>>> + writel(val, csr_base + BRIDGE_8G_CFG_0);
>>> +}
>>
>> Same here.
>>
>>> +static void xgene_pcie_program_core(void *csr_base)
>>> +{
>>> + u32 val;
>>> +
>>> + val = readl(csr_base + BRIDGE_CFG_0);
>>> + val |= AER_OPTIONAL_ERROR_EN;
>>> + writel(val, csr_base +
Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar tinam...@apm.com wrote:
On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann a...@arndb.de wrote:
On Wednesday 15 January 2014, Tanmay Inamdar wrote:
This patch adds the AppliedMicro X-Gene SOC PCIe controller driver.
X-Gene PCIe controller supports maxmum upto 8 lanes and GEN3 speed.
X-Gene has maximum 5 PCIe ports supported.
Signed-off-by: Tanmay Inamdar tinam...@apm.com
This already looks much better than the first version, but I have a more
comments. Most importantly, it would help to know how the root ports
are structured. Is this a standard root complex and multiple ports,
multiple root complexes with one port each, or a nonstandard organization
that is a mix of those two models?
This is multiple root complexes with one port each.
+
+/* When the address bit [17:16] is 2'b01, the Configuration access will be
+ * treated as Type 1 and it will be forwarded to external PCIe device.
+ */
+static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
+{
+ struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
+ u64 addr = (u64)port-cfg_base;
+
+ if (bus-number = (port-first_busno + 1))
+ addr |= AXI_EP_CFG_ACCESS;
+
+ return (void *)addr;
+}
Wrong type, it should be 'void __iomem *'. Also you can't assume that
bit operations work on virtual __iomem addresses, so it should be better
to just add a constant integer to the pointer, which is a valid
operation.
ok.
I also wonder why you need to do this at all. If there isn't a global
config space for all ports, but rather a separate type0/type1 config
cycle based on the bus number, I see that as an indication that the
ports are in fact separate domains and should each start with bus 0.
It is not a standard ECAM layout. We also have a separate RTDID
register as well to program bus, device, function. While accessing EP
config space, we have to set the bit 17:16 as 2b'01. The same config
space address is utilized for enabling a customized nonstandard PCIe
DMA feature. The bits are defined to differentiate the access purpose.
The feature is not supported in this driver yet.
Secondly I don't think it will matter if each port starts with bus 0.
As long as we set the correct BDF in RTDID and set correct bits in
config address, the config reads and writes would work. Right?
+static void xgene_pcie_setup_lanes(struct xgene_pcie_port *port)
+{
+ void *csr_base = port-csr_base;
+ u32 val;
+
+ val = readl(csr_base + BRIDGE_8G_CFG_8);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_8);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_9);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_9);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_10);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_10);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_11);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_11);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_4);
+ val = (val ~0x30) | (1 4);
+ writel(val, csr_base + BRIDGE_8G_CFG_4);
+}
Please document what you are actually setting here. If the configuration
of the lanes is always the same, why do you have to set it here. If not,
why do you set constant values?
Good point. Let me check if these values should be constant or tune-able.
+static void xgene_pcie_setup_link(struct xgene_pcie_port *port)
+{
+ void *csr_base = port-csr_base;
+ u32 val;
+
+ val = readl(csr_base + BRIDGE_CFG_14);
+ val |= DIRECT_TO_8GTS_MASK;
+ val |= SUPPORT_5GTS_MASK;
+ val |= SUPPORT_8GTS_MASK;
+ val |= DIRECT_TO_5GTS_MASK;
+ writel(val, csr_base + BRIDGE_CFG_14);
+
+ val = readl(csr_base + BRIDGE_CFG_14);
+ val = ~ADVT_INFINITE_CREDITS;
+ writel(val, csr_base + BRIDGE_CFG_14);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_0);
+ val |= (val ~0xf) | 7;
+ val |= (val ~0xf00) | ((7 8) 0xf00);
+ writel(val, csr_base + BRIDGE_8G_CFG_0);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_0);
+ val |= DWNSTRM_EQ_SKP_PHS_2_3;
+ writel(val, csr_base + BRIDGE_8G_CFG_0);
+}
Same here.
+static void xgene_pcie_program_core(void *csr_base)
+{
+ u32 val;
+
+ val = readl(csr_base + BRIDGE_CFG_0);
+ val |= AER_OPTIONAL_ERROR_EN;
+ writel(val, csr_base + BRIDGE_CFG_0);
+ writel(0x0, csr_base + INTXSTATUSMASK);
+ val = readl(csr_base + BRIDGE_CTRL_1);
+ val = (val ~0x) | XGENE_PCIE_DEV_CTRL;
+ writel(val, csr_base + BRIDGE_CTRL_1);
+}
'program_core'?
Some of the PCIe core related misc configurations.
+static
Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Friday 17 January 2014, Tanmay Inamdar wrote:
> On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann wrote:
> > On Wednesday 15 January 2014, Tanmay Inamdar wrote:
> >> This patch adds the AppliedMicro X-Gene SOC PCIe controller driver.
> >> X-Gene PCIe controller supports maxmum upto 8 lanes and GEN3 speed.
> >> X-Gene has maximum 5 PCIe ports supported.
> >>
> >> Signed-off-by: Tanmay Inamdar
> >
> > This already looks much better than the first version, but I have a more
> > comments. Most importantly, it would help to know how the root ports
> > are structured. Is this a standard root complex and multiple ports,
> > multiple root complexes with one port each, or a nonstandard organization
> > that is a mix of those two models?
>
> This is multiple root complexes with one port each.
Ok.
> > I also wonder why you need to do this at all. If there isn't a global
> > config space for all ports, but rather a separate type0/type1 config
> > cycle based on the bus number, I see that as an indication that the
> > ports are in fact separate domains and should each start with bus 0.
>
> It is not a standard ECAM layout. We also have a separate RTDID
> register as well to program bus, device, function. While accessing EP
> config space, we have to set the bit 17:16 as 2b'01. The same config
> space address is utilized for enabling a customized nonstandard PCIe
> DMA feature. The bits are defined to differentiate the access purpose.
> The feature is not supported in this driver yet.
>
> Secondly I don't think it will matter if each port starts with bus 0.
> As long as we set the correct BDF in RTDID and set correct bits in
> config address, the config reads and writes would work. Right?
Yes, I think the current code will work (aside from my other comment),
but it seems unnecessary to do this if you use pci domains correctly:
If you have one pci domain and one root port per root complex, you
would not get any config space cycles for the root port and can
do away with the special case here, as well as with the
xgene_pcie_fixup_bridge() that seems to be a special case to
avoid touching the root ports as well.
> >
> >> + switch (restype) {
> >> + case IORESOURCE_MEM:
> >> + res = >mem.res;
> >> + pci_addr = port->mem.pci_addr;
> >> + min_size = SZ_128M;
> >> + break;
> >> + case IORESOURCE_IO:
> >> + res = >io.res;
> >> + pci_addr = port->io.pci_addr;
> >> + min_size = 128;
> >> + flag |= OB_LO_IO;
> >> + break;
> >> + }
> >
> > I assume this works ok, but seems wrong in one detail: If the resource
> > is marked IORESOURCE_IO, res->start is supposed to be in I/O space, not
> > in memory space, which would make it the wrong number to program
> > into the hardware registers.
Sorry, I was actually wrong with my statement above and I thought I had
deleted my comment before sending the mail.
> Yes for using ioport resource. However we have decided to defer using
> it since 'pci_ioremap_io' is not yet supported from arm64 side.
>
> From HW point of view, for memory mapped IO space, it is nothing but a
> piece taken out of the ranges in address map for outbound accesses. So
> while configuring registers from SOC side, it should take the CPU
> address which is address from SOC address map. Right?
>
> Later on we can have a separate io resource like 'realio' similar to
> what pci-mvebu.c does.
I think your code is actually correct here, but please also add the
pci_ioremap_io implementation for arm64 in a separate patch in this
series. It shouldn't be hard and we need it for every pci host driver
anyway.
> >> +static int xgene_pcie_setup(int nr, struct pci_sys_data *sys)
> >> +{
> >> + struct xgene_pcie_port *pp = xgene_pcie_sys_to_port(sys);
> >> +
> >> + if (pp == NULL)
> >> + return 0;
> >> +
> >> + sys->mem_offset = pp->mem.res.start - pp->mem.pci_addr;
> >> + pci_add_resource_offset(>resources, >mem.res,
> >> + sys->mem_offset);
> >> + return 1;
> >> +}
> > Also, what would be a reason for the port to be zero here? If
> > it's something that can't happen in practice, don't try to handle
> > it gracefully. You can use BUG_ON() for fatal conditions that
> > are supposed to be impossible to reach.
>
> This function is a hook to upper layer. We register nr_controllers in
> hw_pci structure as MAX_PORTS we support. It can happen that number of
> ports actually enabled from device tree are less than the number in
> nr_controllers.
I see. I'll comment more on this below.
> >> + if (!port->link_up)
> >> + dev_info(port->dev, "(rc) link down\n");
> >> + else
> >> + dev_info(port->dev, "(rc) x%d gen-%d link up\n",
> >> + lanes, port->link_speed + 1);
> >> +#ifdef CONFIG_PCI_DOMAINS
> >> + xgene_pcie_hw.domain++;
> >> +#endif
> >> + xgene_pcie_hw.private_data[index++] =
Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Friday 17 January 2014, Tanmay Inamdar wrote:
On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann a...@arndb.de wrote:
On Wednesday 15 January 2014, Tanmay Inamdar wrote:
This patch adds the AppliedMicro X-Gene SOC PCIe controller driver.
X-Gene PCIe controller supports maxmum upto 8 lanes and GEN3 speed.
X-Gene has maximum 5 PCIe ports supported.
Signed-off-by: Tanmay Inamdar tinam...@apm.com
This already looks much better than the first version, but I have a more
comments. Most importantly, it would help to know how the root ports
are structured. Is this a standard root complex and multiple ports,
multiple root complexes with one port each, or a nonstandard organization
that is a mix of those two models?
This is multiple root complexes with one port each.
Ok.
I also wonder why you need to do this at all. If there isn't a global
config space for all ports, but rather a separate type0/type1 config
cycle based on the bus number, I see that as an indication that the
ports are in fact separate domains and should each start with bus 0.
It is not a standard ECAM layout. We also have a separate RTDID
register as well to program bus, device, function. While accessing EP
config space, we have to set the bit 17:16 as 2b'01. The same config
space address is utilized for enabling a customized nonstandard PCIe
DMA feature. The bits are defined to differentiate the access purpose.
The feature is not supported in this driver yet.
Secondly I don't think it will matter if each port starts with bus 0.
As long as we set the correct BDF in RTDID and set correct bits in
config address, the config reads and writes would work. Right?
Yes, I think the current code will work (aside from my other comment),
but it seems unnecessary to do this if you use pci domains correctly:
If you have one pci domain and one root port per root complex, you
would not get any config space cycles for the root port and can
do away with the special case here, as well as with the
xgene_pcie_fixup_bridge() that seems to be a special case to
avoid touching the root ports as well.
+ switch (restype) {
+ case IORESOURCE_MEM:
+ res = port-mem.res;
+ pci_addr = port-mem.pci_addr;
+ min_size = SZ_128M;
+ break;
+ case IORESOURCE_IO:
+ res = port-io.res;
+ pci_addr = port-io.pci_addr;
+ min_size = 128;
+ flag |= OB_LO_IO;
+ break;
+ }
I assume this works ok, but seems wrong in one detail: If the resource
is marked IORESOURCE_IO, res-start is supposed to be in I/O space, not
in memory space, which would make it the wrong number to program
into the hardware registers.
Sorry, I was actually wrong with my statement above and I thought I had
deleted my comment before sending the mail.
Yes for using ioport resource. However we have decided to defer using
it since 'pci_ioremap_io' is not yet supported from arm64 side.
From HW point of view, for memory mapped IO space, it is nothing but a
piece taken out of the ranges in address map for outbound accesses. So
while configuring registers from SOC side, it should take the CPU
address which is address from SOC address map. Right?
Later on we can have a separate io resource like 'realio' similar to
what pci-mvebu.c does.
I think your code is actually correct here, but please also add the
pci_ioremap_io implementation for arm64 in a separate patch in this
series. It shouldn't be hard and we need it for every pci host driver
anyway.
+static int xgene_pcie_setup(int nr, struct pci_sys_data *sys)
+{
+ struct xgene_pcie_port *pp = xgene_pcie_sys_to_port(sys);
+
+ if (pp == NULL)
+ return 0;
+
+ sys-mem_offset = pp-mem.res.start - pp-mem.pci_addr;
+ pci_add_resource_offset(sys-resources, pp-mem.res,
+ sys-mem_offset);
+ return 1;
+}
Also, what would be a reason for the port to be zero here? If
it's something that can't happen in practice, don't try to handle
it gracefully. You can use BUG_ON() for fatal conditions that
are supposed to be impossible to reach.
This function is a hook to upper layer. We register nr_controllers in
hw_pci structure as MAX_PORTS we support. It can happen that number of
ports actually enabled from device tree are less than the number in
nr_controllers.
I see. I'll comment more on this below.
+ if (!port-link_up)
+ dev_info(port-dev, (rc) link down\n);
+ else
+ dev_info(port-dev, (rc) x%d gen-%d link up\n,
+ lanes, port-link_speed + 1);
+#ifdef CONFIG_PCI_DOMAINS
+ xgene_pcie_hw.domain++;
+#endif
+ xgene_pcie_hw.private_data[index++] = port;
+ platform_set_drvdata(pdev, port);
+ return 0;
+}
Do you have multiple domains or not? I don't see how it can work if you
make the
Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann wrote:
> On Wednesday 15 January 2014, Tanmay Inamdar wrote:
>> This patch adds the AppliedMicro X-Gene SOC PCIe controller driver.
>> X-Gene PCIe controller supports maxmum upto 8 lanes and GEN3 speed.
>> X-Gene has maximum 5 PCIe ports supported.
>>
>> Signed-off-by: Tanmay Inamdar
>
> This already looks much better than the first version, but I have a more
> comments. Most importantly, it would help to know how the root ports
> are structured. Is this a standard root complex and multiple ports,
> multiple root complexes with one port each, or a nonstandard organization
> that is a mix of those two models?
This is multiple root complexes with one port each.
>
>> +
>> +/* When the address bit [17:16] is 2'b01, the Configuration access will be
>> + * treated as Type 1 and it will be forwarded to external PCIe device.
>> + */
>> +static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
>> +{
>> + struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
>> + u64 addr = (u64)port->cfg_base;
>> +
>> + if (bus->number >= (port->first_busno + 1))
>> + addr |= AXI_EP_CFG_ACCESS;
>> +
>> + return (void *)addr;
>> +}
>
> Wrong type, it should be 'void __iomem *'. Also you can't assume that
> bit operations work on virtual __iomem addresses, so it should be better
> to just add a constant integer to the pointer, which is a valid
> operation.
ok.
>
> I also wonder why you need to do this at all. If there isn't a global
> config space for all ports, but rather a separate type0/type1 config
> cycle based on the bus number, I see that as an indication that the
> ports are in fact separate domains and should each start with bus 0.
It is not a standard ECAM layout. We also have a separate RTDID
register as well to program bus, device, function. While accessing EP
config space, we have to set the bit 17:16 as 2b'01. The same config
space address is utilized for enabling a customized nonstandard PCIe
DMA feature. The bits are defined to differentiate the access purpose.
The feature is not supported in this driver yet.
Secondly I don't think it will matter if each port starts with bus 0.
As long as we set the correct BDF in RTDID and set correct bits in
config address, the config reads and writes would work. Right?
>
>> +static void xgene_pcie_setup_lanes(struct xgene_pcie_port *port)
>> +{
>> + void *csr_base = port->csr_base;
>> + u32 val;
>> +
>> + val = readl(csr_base + BRIDGE_8G_CFG_8);
>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>> + writel(val, csr_base + BRIDGE_8G_CFG_8);
>> +
>> + val = readl(csr_base + BRIDGE_8G_CFG_9);
>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>> + writel(val, csr_base + BRIDGE_8G_CFG_9);
>> +
>> + val = readl(csr_base + BRIDGE_8G_CFG_10);
>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>> + writel(val, csr_base + BRIDGE_8G_CFG_10);
>> +
>> + val = readl(csr_base + BRIDGE_8G_CFG_11);
>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>> + writel(val, csr_base + BRIDGE_8G_CFG_11);
>> +
>> + val = readl(csr_base + BRIDGE_8G_CFG_4);
>> + val = (val & ~0x30) | (1 << 4);
>> + writel(val, csr_base + BRIDGE_8G_CFG_4);
>> +}
>
> Please document what you are actually setting here. If the configuration
> of the lanes is always the same, why do you have to set it here. If not,
> why do you set constant values?
Good point. Let me check if these values should be constant or tune-able.
>
>> +static void xgene_pcie_setup_link(struct xgene_pcie_port *port)
>> +{
>> + void *csr_base = port->csr_base;
>> + u32 val;
>> +
>> + val = readl(csr_base + BRIDGE_CFG_14);
>> + val |= DIRECT_TO_8GTS_MASK;
>> + val |= SUPPORT_5GTS_MASK;
>> + val |= SUPPORT_8GTS_MASK;
>> + val |= DIRECT_TO_5GTS_MASK;
>> + writel(val, csr_base + BRIDGE_CFG_14);
>> +
>> + val = readl(csr_base + BRIDGE_CFG_14);
>> + val &= ~ADVT_INFINITE_CREDITS;
>> + writel(val, csr_base + BRIDGE_CFG_14);
>> +
>> + val = readl(csr_base + BRIDGE_8G_CFG_0);
>> + val |= (val & ~0xf) | 7;
>> + val |= (val & ~0xf00) | ((7 << 8) & 0xf00);
>> + writel(val, csr_base + BRIDGE_8G_CFG_0);
>> +
>> + val = readl(csr_base + BRIDGE_8G_CFG_0);
>> + val |= DWNSTRM_EQ_SKP_PHS_2_3;
>> + writel(val, csr_base + BRIDGE_8G_CFG_0);
>> +}
>
> Same here.
>
>> +static void xgene_pcie_program_core(void *csr_base)
>> +{
>> + u32 val;
>> +
>> + val = readl(csr_base + BRIDGE_CFG_0);
>> + val |= AER_OPTIONAL_ERROR_EN;
>> + writel(val, csr_base + BRIDGE_CFG_0);
>> + writel(0x0, csr_base + INTXSTATUSMASK);
>> + val = readl(csr_base + BRIDGE_CTRL_1);
>> + val = (val & ~0x) | XGENE_PCIE_DEV_CTRL;
>> + writel(val, csr_base +
Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann a...@arndb.de wrote:
On Wednesday 15 January 2014, Tanmay Inamdar wrote:
This patch adds the AppliedMicro X-Gene SOC PCIe controller driver.
X-Gene PCIe controller supports maxmum upto 8 lanes and GEN3 speed.
X-Gene has maximum 5 PCIe ports supported.
Signed-off-by: Tanmay Inamdar tinam...@apm.com
This already looks much better than the first version, but I have a more
comments. Most importantly, it would help to know how the root ports
are structured. Is this a standard root complex and multiple ports,
multiple root complexes with one port each, or a nonstandard organization
that is a mix of those two models?
This is multiple root complexes with one port each.
+
+/* When the address bit [17:16] is 2'b01, the Configuration access will be
+ * treated as Type 1 and it will be forwarded to external PCIe device.
+ */
+static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
+{
+ struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
+ u64 addr = (u64)port-cfg_base;
+
+ if (bus-number = (port-first_busno + 1))
+ addr |= AXI_EP_CFG_ACCESS;
+
+ return (void *)addr;
+}
Wrong type, it should be 'void __iomem *'. Also you can't assume that
bit operations work on virtual __iomem addresses, so it should be better
to just add a constant integer to the pointer, which is a valid
operation.
ok.
I also wonder why you need to do this at all. If there isn't a global
config space for all ports, but rather a separate type0/type1 config
cycle based on the bus number, I see that as an indication that the
ports are in fact separate domains and should each start with bus 0.
It is not a standard ECAM layout. We also have a separate RTDID
register as well to program bus, device, function. While accessing EP
config space, we have to set the bit 17:16 as 2b'01. The same config
space address is utilized for enabling a customized nonstandard PCIe
DMA feature. The bits are defined to differentiate the access purpose.
The feature is not supported in this driver yet.
Secondly I don't think it will matter if each port starts with bus 0.
As long as we set the correct BDF in RTDID and set correct bits in
config address, the config reads and writes would work. Right?
+static void xgene_pcie_setup_lanes(struct xgene_pcie_port *port)
+{
+ void *csr_base = port-csr_base;
+ u32 val;
+
+ val = readl(csr_base + BRIDGE_8G_CFG_8);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_8);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_9);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_9);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_10);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_10);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_11);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_11);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_4);
+ val = (val ~0x30) | (1 4);
+ writel(val, csr_base + BRIDGE_8G_CFG_4);
+}
Please document what you are actually setting here. If the configuration
of the lanes is always the same, why do you have to set it here. If not,
why do you set constant values?
Good point. Let me check if these values should be constant or tune-able.
+static void xgene_pcie_setup_link(struct xgene_pcie_port *port)
+{
+ void *csr_base = port-csr_base;
+ u32 val;
+
+ val = readl(csr_base + BRIDGE_CFG_14);
+ val |= DIRECT_TO_8GTS_MASK;
+ val |= SUPPORT_5GTS_MASK;
+ val |= SUPPORT_8GTS_MASK;
+ val |= DIRECT_TO_5GTS_MASK;
+ writel(val, csr_base + BRIDGE_CFG_14);
+
+ val = readl(csr_base + BRIDGE_CFG_14);
+ val = ~ADVT_INFINITE_CREDITS;
+ writel(val, csr_base + BRIDGE_CFG_14);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_0);
+ val |= (val ~0xf) | 7;
+ val |= (val ~0xf00) | ((7 8) 0xf00);
+ writel(val, csr_base + BRIDGE_8G_CFG_0);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_0);
+ val |= DWNSTRM_EQ_SKP_PHS_2_3;
+ writel(val, csr_base + BRIDGE_8G_CFG_0);
+}
Same here.
+static void xgene_pcie_program_core(void *csr_base)
+{
+ u32 val;
+
+ val = readl(csr_base + BRIDGE_CFG_0);
+ val |= AER_OPTIONAL_ERROR_EN;
+ writel(val, csr_base + BRIDGE_CFG_0);
+ writel(0x0, csr_base + INTXSTATUSMASK);
+ val = readl(csr_base + BRIDGE_CTRL_1);
+ val = (val ~0x) | XGENE_PCIE_DEV_CTRL;
+ writel(val, csr_base + BRIDGE_CTRL_1);
+}
'program_core'?
Some of the PCIe core related misc configurations.
+static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32 *lanes)
+{
+ void
Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Wednesday 15 January 2014, Tanmay Inamdar wrote:
> This patch adds the AppliedMicro X-Gene SOC PCIe controller driver.
> X-Gene PCIe controller supports maxmum upto 8 lanes and GEN3 speed.
> X-Gene has maximum 5 PCIe ports supported.
>
> Signed-off-by: Tanmay Inamdar
This already looks much better than the first version, but I have a more
comments. Most importantly, it would help to know how the root ports
are structured. Is this a standard root complex and multiple ports,
multiple root complexes with one port each, or a nonstandard organization
that is a mix of those two models?
> +
> +/* When the address bit [17:16] is 2'b01, the Configuration access will be
> + * treated as Type 1 and it will be forwarded to external PCIe device.
> + */
> +static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
> +{
> + struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
> + u64 addr = (u64)port->cfg_base;
> +
> + if (bus->number >= (port->first_busno + 1))
> + addr |= AXI_EP_CFG_ACCESS;
> +
> + return (void *)addr;
> +}
Wrong type, it should be 'void __iomem *'. Also you can't assume that
bit operations work on virtual __iomem addresses, so it should be better
to just add a constant integer to the pointer, which is a valid
operation.
I also wonder why you need to do this at all. If there isn't a global
config space for all ports, but rather a separate type0/type1 config
cycle based on the bus number, I see that as an indication that the
ports are in fact separate domains and should each start with bus 0.
> +static void xgene_pcie_setup_lanes(struct xgene_pcie_port *port)
> +{
> + void *csr_base = port->csr_base;
> + u32 val;
> +
> + val = readl(csr_base + BRIDGE_8G_CFG_8);
> + val = eq_pre_cursor_lane0_set(val, 0x7);
> + val = eq_pre_cursor_lane1_set(val, 0x7);
> + writel(val, csr_base + BRIDGE_8G_CFG_8);
> +
> + val = readl(csr_base + BRIDGE_8G_CFG_9);
> + val = eq_pre_cursor_lane0_set(val, 0x7);
> + val = eq_pre_cursor_lane1_set(val, 0x7);
> + writel(val, csr_base + BRIDGE_8G_CFG_9);
> +
> + val = readl(csr_base + BRIDGE_8G_CFG_10);
> + val = eq_pre_cursor_lane0_set(val, 0x7);
> + val = eq_pre_cursor_lane1_set(val, 0x7);
> + writel(val, csr_base + BRIDGE_8G_CFG_10);
> +
> + val = readl(csr_base + BRIDGE_8G_CFG_11);
> + val = eq_pre_cursor_lane0_set(val, 0x7);
> + val = eq_pre_cursor_lane1_set(val, 0x7);
> + writel(val, csr_base + BRIDGE_8G_CFG_11);
> +
> + val = readl(csr_base + BRIDGE_8G_CFG_4);
> + val = (val & ~0x30) | (1 << 4);
> + writel(val, csr_base + BRIDGE_8G_CFG_4);
> +}
Please document what you are actually setting here. If the configuration
of the lanes is always the same, why do you have to set it here. If not,
why do you set constant values?
> +static void xgene_pcie_setup_link(struct xgene_pcie_port *port)
> +{
> + void *csr_base = port->csr_base;
> + u32 val;
> +
> + val = readl(csr_base + BRIDGE_CFG_14);
> + val |= DIRECT_TO_8GTS_MASK;
> + val |= SUPPORT_5GTS_MASK;
> + val |= SUPPORT_8GTS_MASK;
> + val |= DIRECT_TO_5GTS_MASK;
> + writel(val, csr_base + BRIDGE_CFG_14);
> +
> + val = readl(csr_base + BRIDGE_CFG_14);
> + val &= ~ADVT_INFINITE_CREDITS;
> + writel(val, csr_base + BRIDGE_CFG_14);
> +
> + val = readl(csr_base + BRIDGE_8G_CFG_0);
> + val |= (val & ~0xf) | 7;
> + val |= (val & ~0xf00) | ((7 << 8) & 0xf00);
> + writel(val, csr_base + BRIDGE_8G_CFG_0);
> +
> + val = readl(csr_base + BRIDGE_8G_CFG_0);
> + val |= DWNSTRM_EQ_SKP_PHS_2_3;
> + writel(val, csr_base + BRIDGE_8G_CFG_0);
> +}
Same here.
> +static void xgene_pcie_program_core(void *csr_base)
> +{
> + u32 val;
> +
> + val = readl(csr_base + BRIDGE_CFG_0);
> + val |= AER_OPTIONAL_ERROR_EN;
> + writel(val, csr_base + BRIDGE_CFG_0);
> + writel(0x0, csr_base + INTXSTATUSMASK);
> + val = readl(csr_base + BRIDGE_CTRL_1);
> + val = (val & ~0x) | XGENE_PCIE_DEV_CTRL;
> + writel(val, csr_base + BRIDGE_CTRL_1);
> +}
'program_core'?
> +static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32 *lanes)
> +{
> + void *csr_base = port->csr_base;
> + u32 val32;
> + u64 start_time, time;
> +
> + /*
> + * A component enters the LTSSM Detect state within
> + * 20ms of the end of fundamental core reset.
> + */
> + msleep(XGENE_LTSSM_DETECT_WAIT);
> + port->link_up = 0;
> + start_time = jiffies;
> + do {
> + val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
> + if (val32 & LINK_UP_MASK) {
> + port->link_up = 1;
> + port->link_speed = PIPE_PHY_RATE_RD(val32);
> + val32 = readl(csr_base + BRIDGE_STATUS_0);
> + *lanes = val32 >> 26;
> + }
> + time = jiffies_to_msecs(jiffies - start_time);
> + } while ((!port->link_up) || (time
Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
On Wednesday 15 January 2014, Tanmay Inamdar wrote:
This patch adds the AppliedMicro X-Gene SOC PCIe controller driver.
X-Gene PCIe controller supports maxmum upto 8 lanes and GEN3 speed.
X-Gene has maximum 5 PCIe ports supported.
Signed-off-by: Tanmay Inamdar tinam...@apm.com
This already looks much better than the first version, but I have a more
comments. Most importantly, it would help to know how the root ports
are structured. Is this a standard root complex and multiple ports,
multiple root complexes with one port each, or a nonstandard organization
that is a mix of those two models?
+
+/* When the address bit [17:16] is 2'b01, the Configuration access will be
+ * treated as Type 1 and it will be forwarded to external PCIe device.
+ */
+static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
+{
+ struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
+ u64 addr = (u64)port-cfg_base;
+
+ if (bus-number = (port-first_busno + 1))
+ addr |= AXI_EP_CFG_ACCESS;
+
+ return (void *)addr;
+}
Wrong type, it should be 'void __iomem *'. Also you can't assume that
bit operations work on virtual __iomem addresses, so it should be better
to just add a constant integer to the pointer, which is a valid
operation.
I also wonder why you need to do this at all. If there isn't a global
config space for all ports, but rather a separate type0/type1 config
cycle based on the bus number, I see that as an indication that the
ports are in fact separate domains and should each start with bus 0.
+static void xgene_pcie_setup_lanes(struct xgene_pcie_port *port)
+{
+ void *csr_base = port-csr_base;
+ u32 val;
+
+ val = readl(csr_base + BRIDGE_8G_CFG_8);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_8);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_9);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_9);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_10);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_10);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_11);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_11);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_4);
+ val = (val ~0x30) | (1 4);
+ writel(val, csr_base + BRIDGE_8G_CFG_4);
+}
Please document what you are actually setting here. If the configuration
of the lanes is always the same, why do you have to set it here. If not,
why do you set constant values?
+static void xgene_pcie_setup_link(struct xgene_pcie_port *port)
+{
+ void *csr_base = port-csr_base;
+ u32 val;
+
+ val = readl(csr_base + BRIDGE_CFG_14);
+ val |= DIRECT_TO_8GTS_MASK;
+ val |= SUPPORT_5GTS_MASK;
+ val |= SUPPORT_8GTS_MASK;
+ val |= DIRECT_TO_5GTS_MASK;
+ writel(val, csr_base + BRIDGE_CFG_14);
+
+ val = readl(csr_base + BRIDGE_CFG_14);
+ val = ~ADVT_INFINITE_CREDITS;
+ writel(val, csr_base + BRIDGE_CFG_14);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_0);
+ val |= (val ~0xf) | 7;
+ val |= (val ~0xf00) | ((7 8) 0xf00);
+ writel(val, csr_base + BRIDGE_8G_CFG_0);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_0);
+ val |= DWNSTRM_EQ_SKP_PHS_2_3;
+ writel(val, csr_base + BRIDGE_8G_CFG_0);
+}
Same here.
+static void xgene_pcie_program_core(void *csr_base)
+{
+ u32 val;
+
+ val = readl(csr_base + BRIDGE_CFG_0);
+ val |= AER_OPTIONAL_ERROR_EN;
+ writel(val, csr_base + BRIDGE_CFG_0);
+ writel(0x0, csr_base + INTXSTATUSMASK);
+ val = readl(csr_base + BRIDGE_CTRL_1);
+ val = (val ~0x) | XGENE_PCIE_DEV_CTRL;
+ writel(val, csr_base + BRIDGE_CTRL_1);
+}
'program_core'?
+static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32 *lanes)
+{
+ void *csr_base = port-csr_base;
+ u32 val32;
+ u64 start_time, time;
+
+ /*
+ * A component enters the LTSSM Detect state within
+ * 20ms of the end of fundamental core reset.
+ */
+ msleep(XGENE_LTSSM_DETECT_WAIT);
+ port-link_up = 0;
+ start_time = jiffies;
+ do {
+ val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
+ if (val32 LINK_UP_MASK) {
+ port-link_up = 1;
+ port-link_speed = PIPE_PHY_RATE_RD(val32);
+ val32 = readl(csr_base + BRIDGE_STATUS_0);
+ *lanes = val32 26;
+ }
+ time = jiffies_to_msecs(jiffies - start_time);
+ } while ((!port-link_up) || (time = XGENE_LTSSM_L0_WAIT));
+}
Maybe another msleep() in the loop? It seems weird to first do an
unconditional sleep
