| Message ID | 1375272746-24446-1-git-send-email-mugunthanvnm@ti.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Jul 31, 2013 at 05:42:26PM +0530, Mugunthan V N wrote: > The new IP version has a minor changes and the offsets are same as the previous > version, so instead of adding CPSW version number in the driver, make the driver > to fall through to the latest versions so that the new version of CPSW which has > the same register offsets will work directly without patching the driver. This doesn't make any sense to me. Why not just add the new version number? None of the hunks in your patch are on performance sensitive paths, so I really can't see any point in removing the error checking. Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 31, 2013 at 04:49:59PM +0200, Richard Cochran wrote: > On Wed, Jul 31, 2013 at 05:42:26PM +0530, Mugunthan V N wrote: > > The new IP version has a minor changes and the offsets are same as the previous > > version, so instead of adding CPSW version number in the driver, make the driver > > to fall through to the latest versions so that the new version of CPSW which has > > the same register offsets will work directly without patching the driver. > > This doesn't make any sense to me. Why not just add the new version > number? > > None of the hunks in your patch are on performance sensitive paths, so > I really can't see any point in removing the error checking. well, if a new revision of the IP comes, the driver at least has some chance to work without having to be modified. If it turns out that there are really different features, then we patch a new version, otherwise we should just assume highest known version and try it out.
On Wed, Jul 31, 2013 at 06:28:27PM +0300, Felipe Balbi wrote: > On Wed, Jul 31, 2013 at 04:49:59PM +0200, Richard Cochran wrote: > > On Wed, Jul 31, 2013 at 05:42:26PM +0530, Mugunthan V N wrote: > > > The new IP version has a minor changes and the offsets are same as the previous > > > version, so instead of adding CPSW version number in the driver, make the driver > > > to fall through to the latest versions so that the new version of CPSW which has > > > the same register offsets will work directly without patching the driver. > > > > This doesn't make any sense to me. Why not just add the new version > > number? > > > > None of the hunks in your patch are on performance sensitive paths, so > > I really can't see any point in removing the error checking. > > well, if a new revision of the IP comes, the driver at least has some > chance to work without having to be modified. If it turns out that there > are really different features, then we patch a new version, otherwise we > should just assume highest known version and try it out. And if the driver reads junk from some random address due to bootloader/DT/multikernel madness, it will happily peek and poke around instead of rejecting the wrong version number. Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Jul 31, 2013 at 06:38:46PM +0200, Richard Cochran wrote: > On Wed, Jul 31, 2013 at 06:28:27PM +0300, Felipe Balbi wrote: > > On Wed, Jul 31, 2013 at 04:49:59PM +0200, Richard Cochran wrote: > > > On Wed, Jul 31, 2013 at 05:42:26PM +0530, Mugunthan V N wrote: > > > > The new IP version has a minor changes and the offsets are same as the previous > > > > version, so instead of adding CPSW version number in the driver, make the driver > > > > to fall through to the latest versions so that the new version of CPSW which has > > > > the same register offsets will work directly without patching the driver. > > > > > > This doesn't make any sense to me. Why not just add the new version > > > number? > > > > > > None of the hunks in your patch are on performance sensitive paths, so > > > I really can't see any point in removing the error checking. > > > > well, if a new revision of the IP comes, the driver at least has some > > chance to work without having to be modified. If it turns out that there > > are really different features, then we patch a new version, otherwise we > > should just assume highest known version and try it out. > > And if the driver reads junk from some random address due to > bootloader/DT/multikernel madness, it will happily peek and poke > around instead of rejecting the wrong version number. that'd be a bug in the DT anyway, why should the driver have to cope with broken data ?
On Wed, Jul 31, 2013 at 09:45:25PM +0300, Felipe Balbi wrote: > On Wed, Jul 31, 2013 at 06:38:46PM +0200, Richard Cochran wrote: > > On Wed, Jul 31, 2013 at 06:28:27PM +0300, Felipe Balbi wrote: > > > On Wed, Jul 31, 2013 at 04:49:59PM +0200, Richard Cochran wrote: > > > > On Wed, Jul 31, 2013 at 05:42:26PM +0530, Mugunthan V N wrote: > > > > > The new IP version has a minor changes and the offsets are same as the previous > > > > > version, so instead of adding CPSW version number in the driver, make the driver > > > > > to fall through to the latest versions so that the new version of CPSW which has > > > > > the same register offsets will work directly without patching the driver. > > > > > > > > This doesn't make any sense to me. Why not just add the new version > > > > number? > > > > > > > > None of the hunks in your patch are on performance sensitive paths, so > > > > I really can't see any point in removing the error checking. > > > > > > well, if a new revision of the IP comes, the driver at least has some > > > chance to work without having to be modified. If it turns out that there > > > are really different features, then we patch a new version, otherwise we > > > should just assume highest known version and try it out. > > > > And if the driver reads junk from some random address due to > > bootloader/DT/multikernel madness, it will happily peek and poke > > around instead of rejecting the wrong version number. > > that'd be a bug in the DT anyway, why should the driver have to cope > with broken data ? Um, it is called error checking? Besides, by not checking the version number, you are pre-programming a disaster that will occur when an older kernel is booted on the first new IP version with important changes. Can you really be sure that all users will have the new, patched kernel? Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Jul 31, 2013 at 09:22:29PM +0200, Richard Cochran wrote: > On Wed, Jul 31, 2013 at 09:45:25PM +0300, Felipe Balbi wrote: > > On Wed, Jul 31, 2013 at 06:38:46PM +0200, Richard Cochran wrote: > > > On Wed, Jul 31, 2013 at 06:28:27PM +0300, Felipe Balbi wrote: > > > > On Wed, Jul 31, 2013 at 04:49:59PM +0200, Richard Cochran wrote: > > > > > On Wed, Jul 31, 2013 at 05:42:26PM +0530, Mugunthan V N wrote: > > > > > > The new IP version has a minor changes and the offsets are same as the previous > > > > > > version, so instead of adding CPSW version number in the driver, make the driver > > > > > > to fall through to the latest versions so that the new version of CPSW which has > > > > > > the same register offsets will work directly without patching the driver. > > > > > > > > > > This doesn't make any sense to me. Why not just add the new version > > > > > number? > > > > > > > > > > None of the hunks in your patch are on performance sensitive paths, so > > > > > I really can't see any point in removing the error checking. > > > > > > > > well, if a new revision of the IP comes, the driver at least has some > > > > chance to work without having to be modified. If it turns out that there > > > > are really different features, then we patch a new version, otherwise we > > > > should just assume highest known version and try it out. > > > > > > And if the driver reads junk from some random address due to > > > bootloader/DT/multikernel madness, it will happily peek and poke > > > around instead of rejecting the wrong version number. > > > > that'd be a bug in the DT anyway, why should the driver have to cope > > with broken data ? > > Um, it is called error checking? right, now go check on the archives what Linus (and many others, for that matter) have said about version checking. If it's not the version you expect, you assume the latest. Imagine the situation where new IP version has new features, but all the others still work and we don't use that new feature. We'd have to patch the kernel just to get the driver to probe() even though the entire driver is still 'compliant' with the new IP. > Besides, by not checking the version number, you are pre-programming a > disaster that will occur when an older kernel is booted on the first > new IP version with important changes. Can you really be sure that all > users will have the new, patched kernel? why will there be a disaster ?
Hi, On Wed, Jul 31, 2013 at 10:43:32PM +0300, Felipe Balbi wrote: > > > > > > > The new IP version has a minor changes and the offsets are same as the previous > > > > > > > version, so instead of adding CPSW version number in the driver, make the driver > > > > > > > to fall through to the latest versions so that the new version of CPSW which has > > > > > > > the same register offsets will work directly without patching the driver. > > > > > > > > > > > > This doesn't make any sense to me. Why not just add the new version > > > > > > number? > > > > > > > > > > > > None of the hunks in your patch are on performance sensitive paths, so > > > > > > I really can't see any point in removing the error checking. > > > > > > > > > > well, if a new revision of the IP comes, the driver at least has some > > > > > chance to work without having to be modified. If it turns out that there > > > > > are really different features, then we patch a new version, otherwise we > > > > > should just assume highest known version and try it out. > > > > > > > > And if the driver reads junk from some random address due to > > > > bootloader/DT/multikernel madness, it will happily peek and poke > > > > around instead of rejecting the wrong version number. > > > > > > that'd be a bug in the DT anyway, why should the driver have to cope > > > with broken data ? > > > > Um, it is called error checking? one more thing, why do you consider a new revision to be an error ?
On Wed, Jul 31, 2013 at 10:45:23PM +0300, Felipe Balbi wrote: > > one more thing, why do you consider a new revision to be an error ? Okay, so why don't you go and remove the version checking code altogether? Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Jul 31, 2013 at 10:04:28PM +0200, Richard Cochran wrote: > On Wed, Jul 31, 2013 at 10:45:23PM +0300, Felipe Balbi wrote: > > > > one more thing, why do you consider a new revision to be an error ? > > Okay, so why don't you go and remove the version checking code > altogether? if you need to treak certain aspects of the IP differently, you need the revision check, don't be so childish. what I'm saying is that we can give new IP revision a chance to work if they have no programming model differences (except for, perhaps, new features and different erratas). On dwc3 (drivers/usb/dwc3) we support every single revision of the IP. We only have revision checks to enable (or not) known silicon erratas.
On Wed, Jul 31, 2013 at 11:07:56PM +0300, Felipe Balbi wrote: > > what I'm saying is that we can give new IP revision a chance to work if > they have no programming model differences (except for, perhaps, new > features and different erratas). But it also has a chance to fail when there are differences. Comparing CPSW V1 with V2, it appears that TI likes to move the registers around between versions. To me, this is reason enough to make the driver defensive. Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Jul 31, 2013 at 10:20:07PM +0200, Richard Cochran wrote: > On Wed, Jul 31, 2013 at 11:07:56PM +0300, Felipe Balbi wrote: > > > > what I'm saying is that we can give new IP revision a chance to work if > > they have no programming model differences (except for, perhaps, new > > features and different erratas). > > But it also has a chance to fail when there are differences. > Comparing CPSW V1 with V2, it appears that TI likes to move the > registers around between versions. To me, this is reason enough to > make the driver defensive. oh well, we can go on and on with this. Unfortunately we (SW team) don't have control over the HW folks. We strongly suggest that they don't break SW compatibility, and that's starting to become true. You can very well expect next version of CPSW to be SW compatible. If it isn't, then TI will send patches to add a new revision check and treat it well. We are the first ones to have access to new versions of all our IPs anyway. And, IMHO, even if HW engineers decides to move registers around in CPSW v3, that still doesn't chage the fact that defaulting to highest known revision is a good practice. Bailing out just because the revision check isn't what you expect it to be is a very poor practice and leads to periodic patches updating 'switch' statements all over the place.
On Wed, Jul 31, 2013 at 11:26:17PM +0300, Felipe Balbi wrote: > > oh well, we can go on and on with this. Unfortunately we (SW team) don't > have control over the HW folks. We strongly suggest that they don't > break SW compatibility, and that's starting to become true. > > You can very well expect next version of CPSW to be SW compatible. If it > isn't, then TI will send patches to add a new revision check and treat > it well. We are the first ones to have access to new versions of all > our IPs anyway. Okay, so starting with V3 the registers probably won't be moving around any more. But at the very least your patch should include macros for the known V3 along with the default case. Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 31, 2013 at 10:34:06PM +0200, Richard Cochran wrote: > On Wed, Jul 31, 2013 at 11:26:17PM +0300, Felipe Balbi wrote: > > > > oh well, we can go on and on with this. Unfortunately we (SW team) don't > > have control over the HW folks. We strongly suggest that they don't > > break SW compatibility, and that's starting to become true. > > > > You can very well expect next version of CPSW to be SW compatible. If it > > isn't, then TI will send patches to add a new revision check and treat > > it well. We are the first ones to have access to new versions of all > > our IPs anyway. > > Okay, so starting with V3 the registers probably won't be moving > around any more. But at the very least your patch should include > macros for the known V3 along with the default case. that's the point, there is no known V3. Once it has, surely we will add such macros, but until then, we let the driver assume the highest known revision if it finds a register with an unknown revision.
From: Mugunthan V N <mugunthanvnm@ti.com> Date: Wed, 31 Jul 2013 17:42:26 +0530 > The new IP version has a minor changes and the offsets are same as the previous > version, so instead of adding CPSW version number in the driver, make the driver > to fall through to the latest versions so that the new version of CPSW which has > the same register offsets will work directly without patching the driver. > > Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com> > Reviewed-by: Felipe Balbi <balbi@ti.com> Like others, I really think you should check the version explicitly. Please respin this patch so that it supports new IP versions in that way. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 01, 2013 at 12:11:00AM +0300, Felipe Balbi wrote: > > that's the point, there is no known V3. Once it has, surely we will add > such macros, but until then, we let the driver assume the highest known > revision if it finds a register with an unknown revision. I am confused. The patch description says The new IP version has a minor changes and the offsets are same as the previous version, but you are saying there is no new version? Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 31, 2013 at 10:43:32PM +0300, Felipe Balbi wrote: > > right, now go check on the archives what Linus (and many others, for > that matter) have said about version checking. If it's not the version > you expect, you assume the latest. If you are talking about his essay about user space checking the kernel version, then that is another kettle of fish. Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c index 05a1674..a6b9700 100644 --- a/drivers/net/ethernet/ti/cpsw.c +++ b/drivers/net/ethernet/ti/cpsw.c @@ -799,6 +799,7 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv) slave_write(slave, TX_PRIORITY_MAPPING, CPSW1_TX_PRI_MAP); break; case CPSW_VERSION_2: + default: slave_write(slave, TX_PRIORITY_MAPPING, CPSW2_TX_PRI_MAP); break; } @@ -1180,10 +1181,9 @@ static int cpsw_hwtstamp_ioctl(struct net_device *dev, struct ifreq *ifr) cpsw_hwtstamp_v1(priv); break; case CPSW_VERSION_2: + default: cpsw_hwtstamp_v2(priv); break; - default: - return -ENOTSUPP; } return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0; @@ -1790,6 +1790,7 @@ static int cpsw_probe(struct platform_device *pdev) dma_params.desc_mem_phys = 0; break; case CPSW_VERSION_2: + default: priv->host_port_regs = ss_regs + CPSW2_HOST_PORT_OFFSET; priv->cpts->reg = ss_regs + CPSW2_CPTS_OFFSET; dma_params.dmaregs = ss_regs + CPSW2_CPDMA_OFFSET; @@ -1801,10 +1802,6 @@ static int cpsw_probe(struct platform_device *pdev) dma_params.desc_mem_phys = (u32 __force) priv->cpsw_res->start + CPSW2_BD_OFFSET; break; - default: - dev_err(priv->dev, "unknown version 0x%08x\n", priv->version); - ret = -ENODEV; - goto clean_cpsw_wr_iores_ret; } for (i = 0; i < priv->data.slaves; i++) { struct cpsw_slave *slave = &priv->slaves[i];