[RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver

[Tanmay Inamdar]

On Sat, Jan 25, 2014 at 12:11 PM, Arnd Bergmann <arnd at 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 <tinamdar at apm.com> wrote:
>> > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <arnd at 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