Hi, Alex,
Thanks for your comments, please find my replies inlines.
-----Original Message-----
From: Bounine, Alexandre [mailto:<redacted>]=20
Sent: Thursday, October 20, 2011 3:54 AM
To: Kumar Gala; <redacted>
Cc: Andrew Morton; Liu Gang-B34182; <redacted>
Subject: RE: [PATCH 3/5][v2] fsl-rio: Add two ports and rapidio message uni=
ts support
On Thu, Oct 13, 2011 at 10:09 AM, Kumar Gala wrote:
=20
From: Liu Gang <redacted>
=20
Usually, freescale rapidio endpoint can support one 1X or two 4X LP-=20
Serial link interfaces, and rapidio message transactions can be=20
implemented
by
two
Is the number of 1x ports described correctly?
Can we have two 1x ports as well?=20
[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=20
initializes message unit 0 and message unit 1. And these ports and=20
message
... skip ...
+
+ /* Probe the master port phy type */
+ ccsr =3D in_be32(priv->regs_win + RIO_CCSR + i*0x20);
+ port->phy_type =3D (ccsr & 1) ? RIO_PHY_SERIAL :
RIO_PHY_PARALLEL;
+ dev_info(&dev->dev, "RapidIO PHY type: %s\n",
+ (port->phy_type =3D=3D RIO_PHY_PARALLEL) ?
+ "parallel" :
+ ((port->phy_type =3D=3D RIO_PHY_SERIAL) ?
"serial"
:
+ "unknown"));
+ /* Checking the port training status */
+ if (in_be32((priv->regs_win + RIO_ESCSR + i*0x20)) & 1)
{quoted hunk ↗ jump to hunk
+ 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);
quoted hunk ↗ jump to hunk
+ /* Enable ports */
+ setbits32(priv->regs_win
+ + RIO_CCSR + i*0x20,
0x00600000);
quoted hunk ↗ jump to hunk
+ break;
+ case RIO_PHY_PARALLEL:
+ /* Disable ports */
+ out_be32(priv->regs_win
+ + RIO_CCSR + i*0x20,
0x22000000);
quoted hunk ↗ jump to hunk
+ /* 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?=20
quoted hunk ↗ jump to hunk
+ msleep(100);
+ if (in_be32((priv->regs_win
+ + RIO_ESCSR + i*0x20)) & 1) {
+ dev_err(&dev->dev,
+ "Port %d restart failed.\n", i);
+ release_resource(&port->iores);
+ kfree(priv);
+ kfree(port);
+ continue;
+ }
+ dev_info(&dev->dev, "Port %d restart
success!\n", i);
quoted hunk ↗ jump to hunk
+ }
+ fsl_rio_info(&dev->dev, ccsr);
+
... skip ...
=20
struct rio_msg_regs {
- u32 omr; /* 0xD_3000 - Outbound message 0 mode register
*/
- u32 osr; /* 0xD_3004 - Outbound message 0 status register
*/
+ u32 omr;
+ u32 osr;
u32 pad1;
- u32 odqdpar; /* 0xD_300C - Outbound message 0 descriptor
queue
- dequeue pointer address register */
+ u32 odqdpar;
u32 pad2;
- u32 osar; /* 0xD_3014 - Outbound message 0 source address
- register */
- u32 odpr; /* 0xD_3018 - Outbound message 0 destination
port
- register */
- u32 odatr; /* 0xD_301C - Outbound message 0 destination
attributes
- Register*/
- u32 odcr; /* 0xD_3020 - Outbound message 0 double-word
count
- register */
+ u32 osar;
+ u32 odpr;
+ u32 odatr;
+ u32 odcr;
u32 pad3;
- u32 odqepar; /* 0xD_3028 - Outbound message 0 descriptor
queue
- enqueue pointer address register */
+ u32 odqepar;
u32 pad4[13];
- u32 imr; /* 0xD_3060 - Inbound message 0 mode register */
- u32 isr; /* 0xD_3064 - Inbound message 0 status register
*/
+ u32 imr;
+ u32 isr;
u32 pad5;
- u32 ifqdpar; /* 0xD_306C - Inbound message 0 frame queue
dequeue
- pointer address register*/
+ u32 ifqdpar;
u32 pad6;
- u32 ifqepar; /* 0xD_3074 - Inbound message 0 frame queue
enqueue
- pointer address register */
- u32 pad7[226];
- u32 odmr; /* 0xD_3400 - Outbound doorbell mode register */
- u32 odsr; /* 0xD_3404 - Outbound doorbell status register
*/
+ u32 ifqepar;
+ u32 pad7;
Do we need pad7 here?
[Liu Gang-B34182] Yeah, it's not required here. Forgot to remove it when re=
-wrote this struct.
quoted hunk ↗ jump to hunk
+};
+
+struct rio_dbell_regs {
+ u32 odmr;
+ u32 odsr;
u32 res0[4];
- u32 oddpr; /* 0xD_3418 - Outbound doorbell destination port
- register */
... skip ...
quoted hunk ↗ jump to hunk
=20
@@ -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));
=20
- list_for_each_entry(dbell, &port->dbells, node) {
- if ((dbell->res->start <=3D DBELL_INF(dmsg)) &&
- (dbell->res->end >=3D DBELL_INF(dmsg))) {
- found =3D 1;
- break;
+ for (i =3D 0; i < MAX_PORT_NUM; i++) {
+ if (fsl_dbell->mport[i]) {
+ list_for_each_entry(dbell,
+ &fsl_dbell->mport[i]->dbells,
node) {quoted hunk ↗ jump to hunk
+ if ((dbell->res->start
+ <=3D DBELL_INF(dmsg))
+ && (dbell->res->end
+ >=3D DBELL_INF(dmsg))) {
+ found =3D 1;
+ break;
+ }
+ }
+ if (found && dbell->dinb) {
+ dbell->dinb(fsl_dbell->mport[i],
+ dbell->dev_id,
DBELL_SID(dmsg),
quoted hunk ↗ jump to hunk
+ DBELL_TID(dmsg),
+ DBELL_INF(dmsg));
+ break;
+ }
}
}
Do we need to check for matching DBELL_TID and mport destID here and scan o=
nly doorbell list attached to the right port? Otherwise this may call servi=
ce routine associated with doorbell on a wrong port.
[Liu Gang-B34182] Do you mean to match DBELL_TID and mport DevID? Usually t=
his is a reliable method, but for the rapidio module of powerpc, will encou=
nter some problem. We set the port's DevID by
the register "Base Device ID CSR" defined in Rapidio Specification. The rap=
idio module of powerpc can support two ports but have only one the Base Dev=
ice ID CSR. So these two ports will have the same
DevID. Although there are two registers "Alternate Device ID CSR" to separa=
te deviceIDs for each port, they are specific registers of the freescale ra=
pidio and can't be accessed by getting the extended feature
space block offset. For this doobell issue, in order to call a right servic=
e routine, perhaps we should ensure that different ports in different "res-=
start and res->end" configurations.
- if (found) {
- dbell->dinb(port, dbell->dev_id,
- DBELL_SID(dmsg),
- DBELL_TID(dmsg),
DBELL_INF(dmsg));
quoted hunk ↗ jump to hunk
- } 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);
}
=20
out:
return IRQ_HANDLED;
}
=20
... skip ...
quoted hunk ↗ jump to hunk
@@ -1114,50 +1104,48 @@ int fsl_rio_setup_rmu(struct rio_mport *mport,=20
struct device_node *node) {
struct rio_priv *priv;
struct fsl_rmu *rmu;
- struct rio_ops *ops;
+ u64 msg_start;
+ const u32 *msg_addr;
+ int mlen;
+ int aw;
=20
- if (!mport || !mport->priv || !node)
- return -1;
+ if (!mport || !mport->priv)
+ return -EFAULT;
EINVAL may be better here?
[Liu Gang-B34182] Yes EINVAL will be better and I'll correct. Thanks!
quoted hunk ↗ jump to hunk
+
+ priv =3D mport->priv;
+
+ if (!node) {
+ dev_warn(priv->dev, "Can't get %s property 'fsl,rmu'\n",
+ priv->dev->of_node->full_name);
+ return -EFAULT;
EINVAL as well?
[Liu Gang-B34182] Ditto.
+ }
Regards,
Alex.=