[net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Mugunthan V N]

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>
---
 drivers/net/ethernet/ti/cpsw.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

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];
-- 
1.8.4.rc0.11.g35f5eaa

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Richard Cochran]

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

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 989 bytes --]

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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Richard Cochran]

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

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]

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 ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Richard Cochran]

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

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2499 bytes --]

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 ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

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 ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Richard Cochran]

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

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 756 bytes --]

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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Richard Cochran]

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

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]

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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Richard Cochran]

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

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1025 bytes --]

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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[David Miller]

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.

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Richard Cochran]

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

Re: [net-next PATCH 1/1] drivers: net: cpsw: Add support for new CPSW IP version

[Richard Cochran]

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