Re: [PATCH 3/8] staging: et131x: Use for loop to initialise contiguous registers to zero
On Sun, Aug 31, 2014 at 03:25:03PM +0100, Mark Einon wrote: > On Sat, Aug 30, 2014 at 01:32:16PM -0700, Greg KH wrote: > > On Wed, Aug 20, 2014 at 11:17:53PM +0100, Mark Einon wrote: > > > Replace a long list of contiguous writel() calls with a for loop iterating > > > over the same values. > > > > > > Signed-off-by: Mark Einon > > > --- > > > drivers/staging/et131x/et131x.c | 27 +++ > > > 1 file changed, 3 insertions(+), 24 deletions(-) > > > > > > diff --git a/drivers/staging/et131x/et131x.c > > > b/drivers/staging/et131x/et131x.c > > > index fffe763..44cc684 100644 > > > --- a/drivers/staging/et131x/et131x.c > > > +++ b/drivers/staging/et131x/et131x.c > > > @@ -1138,6 +1138,7 @@ static void et1310_config_rxmac_regs(struct > > > et131x_adapter *adapter) > > > u32 sa_lo; > > > u32 sa_hi = 0; > > > u32 pf_ctrl = 0; > > > + u32 *wolw; > > > > > > /* Disable the MAC while it is being configured (also disable WOL) */ > > > writel(0x8, >ctrl); > > > @@ -1151,30 +1152,8 @@ static void et1310_config_rxmac_regs(struct > > > et131x_adapter *adapter) > > >* its default Values of 0x because there are not WOL masks > > >* as of this time. > > >*/ > > > - writel(0, >mask0_word0); > > > - writel(0, >mask0_word1); > > > - writel(0, >mask0_word2); > > > - writel(0, >mask0_word3); > > > - > > > - writel(0, >mask1_word0); > > > - writel(0, >mask1_word1); > > > - writel(0, >mask1_word2); > > > - writel(0, >mask1_word3); > > > - > > > - writel(0, >mask2_word0); > > > - writel(0, >mask2_word1); > > > - writel(0, >mask2_word2); > > > - writel(0, >mask2_word3); > > > - > > > - writel(0, >mask3_word0); > > > - writel(0, >mask3_word1); > > > - writel(0, >mask3_word2); > > > - writel(0, >mask3_word3); > > > - > > > - writel(0, >mask4_word0); > > > - writel(0, >mask4_word1); > > > - writel(0, >mask4_word2); > > > - writel(0, >mask4_word3); > > > + for (wolw = >mask0_word0; wolw <= >mask4_word3; wolw++) > > > + writel(0, wolw); > > > > You are now only writing to all locations 1 time, instead of 4 times, > > like before, are you sure that is ok? Hardware is flaky, sometimes it > > wants to be written to multiple times... > > Hi Greg, > > Thanks for the review. > > As far as my understanding goes, the new code is equivalent to the old > code - it's a little confusing that the name refers to a word, but the > masks are all 32 bit values, and the loop iterates over the contiguous > list of masks found in txmac_regs (et131x.h:891 - the masks are also > unused in the driver after being set). > > Or am I missing something here? No, I was wrong, sorry, your explanation makes sense, sorry for the noise. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/8] staging: et131x: Use for loop to initialise contiguous registers to zero
On Sat, Aug 30, 2014 at 01:32:16PM -0700, Greg KH wrote: > On Wed, Aug 20, 2014 at 11:17:53PM +0100, Mark Einon wrote: > > Replace a long list of contiguous writel() calls with a for loop iterating > > over the same values. > > > > Signed-off-by: Mark Einon > > --- > > drivers/staging/et131x/et131x.c | 27 +++ > > 1 file changed, 3 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/staging/et131x/et131x.c > > b/drivers/staging/et131x/et131x.c > > index fffe763..44cc684 100644 > > --- a/drivers/staging/et131x/et131x.c > > +++ b/drivers/staging/et131x/et131x.c > > @@ -1138,6 +1138,7 @@ static void et1310_config_rxmac_regs(struct > > et131x_adapter *adapter) > > u32 sa_lo; > > u32 sa_hi = 0; > > u32 pf_ctrl = 0; > > + u32 *wolw; > > > > /* Disable the MAC while it is being configured (also disable WOL) */ > > writel(0x8, >ctrl); > > @@ -1151,30 +1152,8 @@ static void et1310_config_rxmac_regs(struct > > et131x_adapter *adapter) > > * its default Values of 0x because there are not WOL masks > > * as of this time. > > */ > > - writel(0, >mask0_word0); > > - writel(0, >mask0_word1); > > - writel(0, >mask0_word2); > > - writel(0, >mask0_word3); > > - > > - writel(0, >mask1_word0); > > - writel(0, >mask1_word1); > > - writel(0, >mask1_word2); > > - writel(0, >mask1_word3); > > - > > - writel(0, >mask2_word0); > > - writel(0, >mask2_word1); > > - writel(0, >mask2_word2); > > - writel(0, >mask2_word3); > > - > > - writel(0, >mask3_word0); > > - writel(0, >mask3_word1); > > - writel(0, >mask3_word2); > > - writel(0, >mask3_word3); > > - > > - writel(0, >mask4_word0); > > - writel(0, >mask4_word1); > > - writel(0, >mask4_word2); > > - writel(0, >mask4_word3); > > + for (wolw = >mask0_word0; wolw <= >mask4_word3; wolw++) > > + writel(0, wolw); > > You are now only writing to all locations 1 time, instead of 4 times, > like before, are you sure that is ok? Hardware is flaky, sometimes it > wants to be written to multiple times... Hi Greg, Thanks for the review. As far as my understanding goes, the new code is equivalent to the old code - it's a little confusing that the name refers to a word, but the masks are all 32 bit values, and the loop iterates over the contiguous list of masks found in txmac_regs (et131x.h:891 - the masks are also unused in the driver after being set). Or am I missing something here? Cheers, Mark -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/8] staging: et131x: Use for loop to initialise contiguous registers to zero
On Sat, Aug 30, 2014 at 01:32:16PM -0700, Greg KH wrote: On Wed, Aug 20, 2014 at 11:17:53PM +0100, Mark Einon wrote: Replace a long list of contiguous writel() calls with a for loop iterating over the same values. Signed-off-by: Mark Einon mark.ei...@gmail.com --- drivers/staging/et131x/et131x.c | 27 +++ 1 file changed, 3 insertions(+), 24 deletions(-) diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c index fffe763..44cc684 100644 --- a/drivers/staging/et131x/et131x.c +++ b/drivers/staging/et131x/et131x.c @@ -1138,6 +1138,7 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter) u32 sa_lo; u32 sa_hi = 0; u32 pf_ctrl = 0; + u32 *wolw; /* Disable the MAC while it is being configured (also disable WOL) */ writel(0x8, rxmac-ctrl); @@ -1151,30 +1152,8 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter) * its default Values of 0x because there are not WOL masks * as of this time. */ - writel(0, rxmac-mask0_word0); - writel(0, rxmac-mask0_word1); - writel(0, rxmac-mask0_word2); - writel(0, rxmac-mask0_word3); - - writel(0, rxmac-mask1_word0); - writel(0, rxmac-mask1_word1); - writel(0, rxmac-mask1_word2); - writel(0, rxmac-mask1_word3); - - writel(0, rxmac-mask2_word0); - writel(0, rxmac-mask2_word1); - writel(0, rxmac-mask2_word2); - writel(0, rxmac-mask2_word3); - - writel(0, rxmac-mask3_word0); - writel(0, rxmac-mask3_word1); - writel(0, rxmac-mask3_word2); - writel(0, rxmac-mask3_word3); - - writel(0, rxmac-mask4_word0); - writel(0, rxmac-mask4_word1); - writel(0, rxmac-mask4_word2); - writel(0, rxmac-mask4_word3); + for (wolw = rxmac-mask0_word0; wolw = rxmac-mask4_word3; wolw++) + writel(0, wolw); You are now only writing to all locations 1 time, instead of 4 times, like before, are you sure that is ok? Hardware is flaky, sometimes it wants to be written to multiple times... Hi Greg, Thanks for the review. As far as my understanding goes, the new code is equivalent to the old code - it's a little confusing that the name refers to a word, but the masks are all 32 bit values, and the loop iterates over the contiguous list of masks found in txmac_regs (et131x.h:891 - the masks are also unused in the driver after being set). Or am I missing something here? Cheers, Mark -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/8] staging: et131x: Use for loop to initialise contiguous registers to zero
On Sun, Aug 31, 2014 at 03:25:03PM +0100, Mark Einon wrote: On Sat, Aug 30, 2014 at 01:32:16PM -0700, Greg KH wrote: On Wed, Aug 20, 2014 at 11:17:53PM +0100, Mark Einon wrote: Replace a long list of contiguous writel() calls with a for loop iterating over the same values. Signed-off-by: Mark Einon mark.ei...@gmail.com --- drivers/staging/et131x/et131x.c | 27 +++ 1 file changed, 3 insertions(+), 24 deletions(-) diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c index fffe763..44cc684 100644 --- a/drivers/staging/et131x/et131x.c +++ b/drivers/staging/et131x/et131x.c @@ -1138,6 +1138,7 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter) u32 sa_lo; u32 sa_hi = 0; u32 pf_ctrl = 0; + u32 *wolw; /* Disable the MAC while it is being configured (also disable WOL) */ writel(0x8, rxmac-ctrl); @@ -1151,30 +1152,8 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter) * its default Values of 0x because there are not WOL masks * as of this time. */ - writel(0, rxmac-mask0_word0); - writel(0, rxmac-mask0_word1); - writel(0, rxmac-mask0_word2); - writel(0, rxmac-mask0_word3); - - writel(0, rxmac-mask1_word0); - writel(0, rxmac-mask1_word1); - writel(0, rxmac-mask1_word2); - writel(0, rxmac-mask1_word3); - - writel(0, rxmac-mask2_word0); - writel(0, rxmac-mask2_word1); - writel(0, rxmac-mask2_word2); - writel(0, rxmac-mask2_word3); - - writel(0, rxmac-mask3_word0); - writel(0, rxmac-mask3_word1); - writel(0, rxmac-mask3_word2); - writel(0, rxmac-mask3_word3); - - writel(0, rxmac-mask4_word0); - writel(0, rxmac-mask4_word1); - writel(0, rxmac-mask4_word2); - writel(0, rxmac-mask4_word3); + for (wolw = rxmac-mask0_word0; wolw = rxmac-mask4_word3; wolw++) + writel(0, wolw); You are now only writing to all locations 1 time, instead of 4 times, like before, are you sure that is ok? Hardware is flaky, sometimes it wants to be written to multiple times... Hi Greg, Thanks for the review. As far as my understanding goes, the new code is equivalent to the old code - it's a little confusing that the name refers to a word, but the masks are all 32 bit values, and the loop iterates over the contiguous list of masks found in txmac_regs (et131x.h:891 - the masks are also unused in the driver after being set). Or am I missing something here? No, I was wrong, sorry, your explanation makes sense, sorry for the noise. greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/8] staging: et131x: Use for loop to initialise contiguous registers to zero
On Wed, Aug 20, 2014 at 11:17:53PM +0100, Mark Einon wrote: > Replace a long list of contiguous writel() calls with a for loop iterating > over the same values. > > Signed-off-by: Mark Einon > --- > drivers/staging/et131x/et131x.c | 27 +++ > 1 file changed, 3 insertions(+), 24 deletions(-) > > diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c > index fffe763..44cc684 100644 > --- a/drivers/staging/et131x/et131x.c > +++ b/drivers/staging/et131x/et131x.c > @@ -1138,6 +1138,7 @@ static void et1310_config_rxmac_regs(struct > et131x_adapter *adapter) > u32 sa_lo; > u32 sa_hi = 0; > u32 pf_ctrl = 0; > + u32 *wolw; > > /* Disable the MAC while it is being configured (also disable WOL) */ > writel(0x8, >ctrl); > @@ -1151,30 +1152,8 @@ static void et1310_config_rxmac_regs(struct > et131x_adapter *adapter) >* its default Values of 0x because there are not WOL masks >* as of this time. >*/ > - writel(0, >mask0_word0); > - writel(0, >mask0_word1); > - writel(0, >mask0_word2); > - writel(0, >mask0_word3); > - > - writel(0, >mask1_word0); > - writel(0, >mask1_word1); > - writel(0, >mask1_word2); > - writel(0, >mask1_word3); > - > - writel(0, >mask2_word0); > - writel(0, >mask2_word1); > - writel(0, >mask2_word2); > - writel(0, >mask2_word3); > - > - writel(0, >mask3_word0); > - writel(0, >mask3_word1); > - writel(0, >mask3_word2); > - writel(0, >mask3_word3); > - > - writel(0, >mask4_word0); > - writel(0, >mask4_word1); > - writel(0, >mask4_word2); > - writel(0, >mask4_word3); > + for (wolw = >mask0_word0; wolw <= >mask4_word3; wolw++) > + writel(0, wolw); You are now only writing to all locations 1 time, instead of 4 times, like before, are you sure that is ok? Hardware is flaky, sometimes it wants to be written to multiple times... greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/8] staging: et131x: Use for loop to initialise contiguous registers to zero
On Wed, Aug 20, 2014 at 11:17:53PM +0100, Mark Einon wrote: Replace a long list of contiguous writel() calls with a for loop iterating over the same values. Signed-off-by: Mark Einon mark.ei...@gmail.com --- drivers/staging/et131x/et131x.c | 27 +++ 1 file changed, 3 insertions(+), 24 deletions(-) diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c index fffe763..44cc684 100644 --- a/drivers/staging/et131x/et131x.c +++ b/drivers/staging/et131x/et131x.c @@ -1138,6 +1138,7 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter) u32 sa_lo; u32 sa_hi = 0; u32 pf_ctrl = 0; + u32 *wolw; /* Disable the MAC while it is being configured (also disable WOL) */ writel(0x8, rxmac-ctrl); @@ -1151,30 +1152,8 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter) * its default Values of 0x because there are not WOL masks * as of this time. */ - writel(0, rxmac-mask0_word0); - writel(0, rxmac-mask0_word1); - writel(0, rxmac-mask0_word2); - writel(0, rxmac-mask0_word3); - - writel(0, rxmac-mask1_word0); - writel(0, rxmac-mask1_word1); - writel(0, rxmac-mask1_word2); - writel(0, rxmac-mask1_word3); - - writel(0, rxmac-mask2_word0); - writel(0, rxmac-mask2_word1); - writel(0, rxmac-mask2_word2); - writel(0, rxmac-mask2_word3); - - writel(0, rxmac-mask3_word0); - writel(0, rxmac-mask3_word1); - writel(0, rxmac-mask3_word2); - writel(0, rxmac-mask3_word3); - - writel(0, rxmac-mask4_word0); - writel(0, rxmac-mask4_word1); - writel(0, rxmac-mask4_word2); - writel(0, rxmac-mask4_word3); + for (wolw = rxmac-mask0_word0; wolw = rxmac-mask4_word3; wolw++) + writel(0, wolw); You are now only writing to all locations 1 time, instead of 4 times, like before, are you sure that is ok? Hardware is flaky, sometimes it wants to be written to multiple times... greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/8] staging: et131x: Use for loop to initialise contiguous registers to zero
Replace a long list of contiguous writel() calls with a for loop iterating over the same values. Signed-off-by: Mark Einon --- drivers/staging/et131x/et131x.c | 27 +++ 1 file changed, 3 insertions(+), 24 deletions(-) diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c index fffe763..44cc684 100644 --- a/drivers/staging/et131x/et131x.c +++ b/drivers/staging/et131x/et131x.c @@ -1138,6 +1138,7 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter) u32 sa_lo; u32 sa_hi = 0; u32 pf_ctrl = 0; + u32 *wolw; /* Disable the MAC while it is being configured (also disable WOL) */ writel(0x8, >ctrl); @@ -1151,30 +1152,8 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter) * its default Values of 0x because there are not WOL masks * as of this time. */ - writel(0, >mask0_word0); - writel(0, >mask0_word1); - writel(0, >mask0_word2); - writel(0, >mask0_word3); - - writel(0, >mask1_word0); - writel(0, >mask1_word1); - writel(0, >mask1_word2); - writel(0, >mask1_word3); - - writel(0, >mask2_word0); - writel(0, >mask2_word1); - writel(0, >mask2_word2); - writel(0, >mask2_word3); - - writel(0, >mask3_word0); - writel(0, >mask3_word1); - writel(0, >mask3_word2); - writel(0, >mask3_word3); - - writel(0, >mask4_word0); - writel(0, >mask4_word1); - writel(0, >mask4_word2); - writel(0, >mask4_word3); + for (wolw = >mask0_word0; wolw <= >mask4_word3; wolw++) + writel(0, wolw); /* Lets setup the WOL Source Address */ sa_lo = (adapter->addr[2] << ET_RX_WOL_LO_SA3_SHIFT) | -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/8] staging: et131x: Use for loop to initialise contiguous registers to zero
Replace a long list of contiguous writel() calls with a for loop iterating over the same values. Signed-off-by: Mark Einon mark.ei...@gmail.com --- drivers/staging/et131x/et131x.c | 27 +++ 1 file changed, 3 insertions(+), 24 deletions(-) diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c index fffe763..44cc684 100644 --- a/drivers/staging/et131x/et131x.c +++ b/drivers/staging/et131x/et131x.c @@ -1138,6 +1138,7 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter) u32 sa_lo; u32 sa_hi = 0; u32 pf_ctrl = 0; + u32 *wolw; /* Disable the MAC while it is being configured (also disable WOL) */ writel(0x8, rxmac-ctrl); @@ -1151,30 +1152,8 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter) * its default Values of 0x because there are not WOL masks * as of this time. */ - writel(0, rxmac-mask0_word0); - writel(0, rxmac-mask0_word1); - writel(0, rxmac-mask0_word2); - writel(0, rxmac-mask0_word3); - - writel(0, rxmac-mask1_word0); - writel(0, rxmac-mask1_word1); - writel(0, rxmac-mask1_word2); - writel(0, rxmac-mask1_word3); - - writel(0, rxmac-mask2_word0); - writel(0, rxmac-mask2_word1); - writel(0, rxmac-mask2_word2); - writel(0, rxmac-mask2_word3); - - writel(0, rxmac-mask3_word0); - writel(0, rxmac-mask3_word1); - writel(0, rxmac-mask3_word2); - writel(0, rxmac-mask3_word3); - - writel(0, rxmac-mask4_word0); - writel(0, rxmac-mask4_word1); - writel(0, rxmac-mask4_word2); - writel(0, rxmac-mask4_word3); + for (wolw = rxmac-mask0_word0; wolw = rxmac-mask4_word3; wolw++) + writel(0, wolw); /* Lets setup the WOL Source Address */ sa_lo = (adapter-addr[2] ET_RX_WOL_LO_SA3_SHIFT) | -- 2.1.0 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
