RE: [PATCH 3/5][v2] fsl-rio: Add two ports and rapidio message units support

[Bounine, Alexandre]

On Sat, Oct 22, 2011 at 1:15 AM, Liu Gang-B34182 <B34182@xxxxxxxxxxxxx>
wrote:
> 
> From: Bounine, Alexandre [mailto:Alexandre.Bounine@xxxxxxx]
> Sent: Thursday, October 20, 2011 3:54 AM
> To: Kumar Gala; linuxppc-dev@xxxxxxxxxx
> Cc: Andrew Morton; Liu Gang-B34182; linux-kernel@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH 3/5][v2] fsl-rio: Add two ports and rapidio
message
> units support
> 
> On Thu, Oct 13, 2011 at 10:09 AM, Kumar Gala wrote:
> >
> > From: Liu Gang <Gang.Liu@xxxxxxxxxxxxx>
> >
> > Usually, freescale rapidio endpoint can support one 1X or two 4X LP-
> > Serial link interfaces, and rapidio message transactions can be
> > implemented
> by
> > two
> 
> Is the number of 1x ports described correctly?
> Can we have two 1x ports as well?
> [Liu Gang-B34182] Yes you are right. endpoint can also have two 1x
> ports. I'll correct it.
> 
> > message units. This patch adds the support of two rapidio ports and
> > initializes message unit 0 and message unit 1. And these ports and
> > message
> ... skip ...
> > +
> > +  /* Probe the master port phy type */
> > +  ccsr = in_be32(priv->regs_win + RIO_CCSR + i*0x20);
> > +  port->phy_type = (ccsr & 1) ? RIO_PHY_SERIAL :
> > RIO_PHY_PARALLEL;
> > +  dev_info(&dev->dev, "RapidIO PHY type: %s\n",
> > +    (port->phy_type == RIO_PHY_PARALLEL) ?
> > +    "parallel" :
> > +    ((port->phy_type == RIO_PHY_SERIAL) ?
> "serial"
> > :
> > +     "unknown"));
> > +  /* Checking the port training status */
> > +  if (in_be32((priv->regs_win + RIO_ESCSR + i*0x20)) & 1)
> {
> > +   dev_err(&dev->dev, "Port %d is not ready. "
> > +    "Try to restart connection...\n", i);
> > +   switch (port->phy_type) {
> > +   case RIO_PHY_SERIAL:
> > +    /* Disable ports */
> > +    out_be32(priv->regs_win
> > +     + RIO_CCSR + i*0x20, 0);
> > +    /* Set 1x lane */
> > +    setbits32(priv->regs_win
> > +     + RIO_CCSR + i*0x20,
> 0x02000000);
> > +    /* Enable ports */
> > +    setbits32(priv->regs_win
> > +     + RIO_CCSR + i*0x20,
> 0x00600000);
> > +    break;
> > +   case RIO_PHY_PARALLEL:
> > +    /* Disable ports */
> > +    out_be32(priv->regs_win
> > +     + RIO_CCSR + i*0x20,
> 0x22000000);
> > +    /* Enable ports */
> > +    out_be32(priv->regs_win
> > +     + RIO_CCSR + i*0x20,
> 0x44000000);
> > +    break;
> > +   }
> 
> Probably this may be a good moment to drop the support for parallel
> link.
> Especially after you renamed controller to "srio" in the device tree.
> [Liu Gang-B34182] I'm also considering if we should drop the parallel
> link support and doorbell outbound ATMU configuration.
> I found some older powerpc chips support parallel link, like mpc8540
> and so on. But DTS files of these chips do not support
> Rapidio nodes. For example we can't find rapidio node in
> arch/powerpc/boot/dts/mpc8540ads.dts file. So can we conclude that
> these chips with parallel rapidio link do not need the support for
> rapidio module and the code for parallel link can be removed?

We are not aware about any use of tsi500 P-RIO switches.
I would consider parallel implementation as an early stage
of RapidIO development which may be safely dropped.
I will keep P-RIO related definitions only because they are part of
the spec. I consider removing tsi500 switch driver as well.

> 
> > +   msleep(100);
... skip ...

> 
> >
> > @@ -340,35 +327,45 @@ fsl_rio_dbell_handler(int irq, void
> > *dev_instance)
> >     " sid %2.2x tid %2.2x info %4.4x\n",
> >     DBELL_SID(dmsg), DBELL_TID(dmsg),
> DBELL_INF(dmsg));
> >
> > -  list_for_each_entry(dbell, &port->dbells, node) {
> > -   if ((dbell->res->start <= DBELL_INF(dmsg)) &&
> > -    (dbell->res->end >= DBELL_INF(dmsg))) {
> > -    found = 1;
> > -    break;
> > +  for (i = 0; i < MAX_PORT_NUM; i++) {
> > +   if (fsl_dbell->mport[i]) {
> > +    list_for_each_entry(dbell,
> > +     &fsl_dbell->mport[i]->dbells,
> node) {
> > +     if ((dbell->res->start
> > +      <= DBELL_INF(dmsg))
> > +      && (dbell->res->end
> > +      >= DBELL_INF(dmsg))) {
> > +      found = 1;
> > +      break;
> > +     }
> > +    }
> > +    if (found && dbell->dinb) {
> > +     dbell->dinb(fsl_dbell->mport[i],
> > +      dbell->dev_id,
> DBELL_SID(dmsg),
> > +      DBELL_TID(dmsg),
> > +      DBELL_INF(dmsg));
> > +     break;
> > +    }
> >     }
> >    }
> 
> Do we need to check for matching DBELL_TID and mport destID here and
> scan only doorbell list attached to the right port? Otherwise this may
> call service routine associated with doorbell on a wrong port.
> [Liu Gang-B34182] Do you mean to match DBELL_TID and mport DevID?


Yes.

> Usually this is a reliable method, but for the rapidio module of
> powerpc, will encounter some problem. We set the port's DevID by
> the register "Base Device ID CSR" defined in Rapidio Specification.
The
> rapidio module of powerpc can support two ports but have only one the
> Base Device ID CSR. So these two ports will have the same
> DevID. Although there are two registers "Alternate Device ID CSR" to
> separate deviceIDs for each port, they are specific registers of the
> freescale rapidio and can't be accessed by getting the extended
feature
> space block offset. For this doobell issue, in order to call a right
> service routine, perhaps we should ensure that different ports in
> different "res->start and res->end" configurations.

This gives us an issue that has to be solved at some point.
Splitting doorbell resources may be a way in this case but should be
considered in context of RIO network with different endpoints and
therefore
it will be some device-specific quirk.

Probably the best approach in this case is to keep doorbell handler as
it is now (for purpose of this patchset) and address doorbell resource
assignment during enumeration/discovery. At least this has to be well
commented. 

> 
> > -  if (found) {
> > -   dbell->dinb(port, dbell->dev_id,
> > -     DBELL_SID(dmsg),
> > -     DBELL_TID(dmsg),
> DBELL_INF(dmsg));
> > -  } else {
> > +
> > +  if (!found) {
> >     pr_debug
> >      ("RIO: spurious doorbell,"
> >      " sid %2.2x tid %2.2x info %4.4x\n",
> >      DBELL_SID(dmsg), DBELL_TID(dmsg),
> >      DBELL_INF(dmsg));
> >    }
> > -  setbits32(&rmu->msg_regs->dmr, DOORBELL_DMR_DI);
> > -  out_be32(&rmu->msg_regs->dsr, DOORBELL_DSR_DIQI);
> > +  setbits32(&fsl_dbell->dbell_regs->dmr, DOORBELL_DMR_DI);
> > +  out_be32(&fsl_dbell->dbell_regs->dsr,
> DOORBELL_DSR_DIQI);
> >   }
> >
> >  out:
> >   return IRQ_HANDLED;
> >  }
> >
... skip ...
 
Regards,

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