[U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK"

[U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK"

[Troy Kisky]

If GENMASK is REALLY desired, it should be GENMASK(23,0)
But since GENMASK is obviously more confusing, let's just revert.

This reverts commit bad490a24212c068c5b718b9189f47ea4075d078.

Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>
Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>

---
v2: add sign off/ reviewed by
---
 drivers/spi/fsl_qspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
index ed39114..dd7048a 100644
--- a/drivers/spi/fsl_qspi.c
+++ b/drivers/spi/fsl_qspi.c
@@ -24,7 +24,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #define TX_BUFFER_SIZE		0x40
 #endif
 
-#define OFFSET_BITS_MASK	GENMASK(24, 0)
+#define OFFSET_BITS_MASK	0x00ffffff
 
 #define FLASH_STATUS_WEL	0x02
 
-- 
2.5.0

[U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK"

[Jagan Teki]

On 11 December 2015 at 02:57, Troy Kisky <troy.kisky@boundarydevices.com> wrote:
> If GENMASK is REALLY desired, it should be GENMASK(23,0)
> But since GENMASK is obviously more confusing, let's just revert.
>
> This reverts commit bad490a24212c068c5b718b9189f47ea4075d078.

Sorry, just fix genmask why revert? because driver author has no
complaint on this, please don't just say simply "more confusing" more
over I usually prefer reverting bug fix patches.

>
> Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>
> ---
> v2: add sign off/ reviewed by
> ---
>  drivers/spi/fsl_qspi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
> index ed39114..dd7048a 100644
> --- a/drivers/spi/fsl_qspi.c
> +++ b/drivers/spi/fsl_qspi.c
> @@ -24,7 +24,7 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define TX_BUFFER_SIZE         0x40
>  #endif
>
> -#define OFFSET_BITS_MASK       GENMASK(24, 0)
> +#define OFFSET_BITS_MASK       0x00ffffff
>
>  #define FLASH_STATUS_WEL       0x02
>
> --
> 2.5.0

thanks!
-- 
Jagan.

[U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK"

[Marek Vasut]

On Friday, December 11, 2015 at 03:59:14 PM, Jagan Teki wrote:
> On 11 December 2015 at 02:57, Troy Kisky <troy.kisky@boundarydevices.com> 
wrote:
> > If GENMASK is REALLY desired, it should be GENMASK(23,0)
> > But since GENMASK is obviously more confusing, let's just revert.
> > 
> > This reverts commit bad490a24212c068c5b718b9189f47ea4075d078.
> 
> Sorry, just fix genmask why revert? because driver author has no
> complaint on this, please don't just say simply "more confusing" more
> over I usually prefer reverting bug fix patches.

I also agree this GENMASK() crap is confusing.

> > Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>
> > Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> > 
> > ---
> > v2: add sign off/ reviewed by
> > ---
> > 
> >  drivers/spi/fsl_qspi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
> > index ed39114..dd7048a 100644
> > --- a/drivers/spi/fsl_qspi.c
> > +++ b/drivers/spi/fsl_qspi.c
> > @@ -24,7 +24,7 @@ DECLARE_GLOBAL_DATA_PTR;
> > 
> >  #define TX_BUFFER_SIZE         0x40
> >  #endif
> > 
> > -#define OFFSET_BITS_MASK       GENMASK(24, 0)
> > +#define OFFSET_BITS_MASK       0x00ffffff
> > 
> >  #define FLASH_STATUS_WEL       0x02
> > 
> > --
> > 2.5.0
> 
> thanks!

Best regards,
Marek Vasut

[U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK"

[Fabio Estevam]

On Fri, Dec 11, 2015 at 1:24 PM, Marek Vasut <marex@denx.de> wrote:
> On Friday, December 11, 2015 at 03:59:14 PM, Jagan Teki wrote:
>> On 11 December 2015 at 02:57, Troy Kisky <troy.kisky@boundarydevices.com>
> wrote:
>> > If GENMASK is REALLY desired, it should be GENMASK(23,0)
>> > But since GENMASK is obviously more confusing, let's just revert.
>> >
>> > This reverts commit bad490a24212c068c5b718b9189f47ea4075d078.
>>
>> Sorry, just fix genmask why revert? because driver author has no
>> complaint on this, please don't just say simply "more confusing" more
>> over I usually prefer reverting bug fix patches.
>
> I also agree this GENMASK() crap is confusing.

Agreed. A simple proof that this is confusing is that it broke the
fsl_qspi.c driver.

[U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK"

[Jagan Teki]

On 11 December 2015 at 20:58, Fabio Estevam <festevam@gmail.com> wrote:
> On Fri, Dec 11, 2015 at 1:24 PM, Marek Vasut <marex@denx.de> wrote:
>> On Friday, December 11, 2015 at 03:59:14 PM, Jagan Teki wrote:
>>> On 11 December 2015 at 02:57, Troy Kisky <troy.kisky@boundarydevices.com>
>> wrote:
>>> > If GENMASK is REALLY desired, it should be GENMASK(23,0)
>>> > But since GENMASK is obviously more confusing, let's just revert.
>>> >
>>> > This reverts commit bad490a24212c068c5b718b9189f47ea4075d078.
>>>
>>> Sorry, just fix genmask why revert? because driver author has no
>>> complaint on this, please don't just say simply "more confusing" more
>>> over I usually prefer reverting bug fix patches.
>>
>> I also agree this GENMASK() crap is confusing.
>
> Agreed. A simple proof that this is confusing is that it broke the
> fsl_qspi.c driver.

Probably your pointed at wrong h on genmask - but this wasn't be a
proof of any global defined macro it's a mistake of using improper h
value. If something went wrong with the patch then always ask and fix
for proper thing and blaming global macro which used in other files is
not a good thing.

thanks!
-- 
Jagan.

[U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK"

[Marek Vasut]

On Friday, December 11, 2015 at 04:41:50 PM, Jagan Teki wrote:
> On 11 December 2015 at 20:58, Fabio Estevam <festevam@gmail.com> wrote:
> > On Fri, Dec 11, 2015 at 1:24 PM, Marek Vasut <marex@denx.de> wrote:
> >> On Friday, December 11, 2015 at 03:59:14 PM, Jagan Teki wrote:
> >>> On 11 December 2015 at 02:57, Troy Kisky
> >>> <troy.kisky@boundarydevices.com>
> >> 
> >> wrote:
> >>> > If GENMASK is REALLY desired, it should be GENMASK(23,0)
> >>> > But since GENMASK is obviously more confusing, let's just revert.
> >>> > 
> >>> > This reverts commit bad490a24212c068c5b718b9189f47ea4075d078.
> >>> 
> >>> Sorry, just fix genmask why revert? because driver author has no
> >>> complaint on this, please don't just say simply "more confusing" more
> >>> over I usually prefer reverting bug fix patches.
> >> 
> >> I also agree this GENMASK() crap is confusing.
> > 
> > Agreed. A simple proof that this is confusing is that it broke the
> > fsl_qspi.c driver.
> 
> Probably your pointed at wrong h on genmask - but this wasn't be a
> proof of any global defined macro it's a mistake of using improper h
> value.

Sorry, I do not understand this sentence at all.

> If something went wrong with the patch then always ask and fix
> for proper thing and blaming global macro which used in other files is
> not a good thing.

Usage of the macro itself is so confusing that even the conversion went
wrong and introduced bugs. Clearly, using the macro is NOT an improvement.
I also vote for the revert.

Best regards,
Marek Vasut

[U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK"

[Stefano Babic]

Hi Jagan, Marek, Troy,

On 11/12/2015 16:24, Marek Vasut wrote:
> On Friday, December 11, 2015 at 03:59:14 PM, Jagan Teki wrote:
>> On 11 December 2015 at 02:57, Troy Kisky <troy.kisky@boundarydevices.com> 
> wrote:
>>> If GENMASK is REALLY desired, it should be GENMASK(23,0)
>>> But since GENMASK is obviously more confusing, let's just revert.
>>>
>>> This reverts commit bad490a24212c068c5b718b9189f47ea4075d078.
>>
>> Sorry, just fix genmask why revert? because driver author has no
>> complaint on this, please don't just say simply "more confusing" more
>> over I usually prefer reverting bug fix patches.
> 
> I also agree this GENMASK() crap is confusing.
> 

IMHO it is easier for me to check directly the mask, as relying to a
macro. But this is only my personal opinion.

On the other side, the GENMASK macro is taken from the kernel and it is
used by several SOCs, in U-Boot too. Maybe we like to see directly the
mask, but several users like use it and I should accept it. Both
solutions (fixing GENMASK or revert it) are fine with me.

Today a new patch was sent :

	http://patchwork.ozlabs.org/patch/556361/

same issue, fixing the GENMASK().

Troy's patch was assigned to me, but it belongs to SPI's custodian, so I
move it to Jagan.


Best regards,
Stefano Babic

>>> Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>
>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>>
>>> ---
>>> v2: add sign off/ reviewed by
>>> ---
>>>
>>>  drivers/spi/fsl_qspi.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
>>> index ed39114..dd7048a 100644
>>> --- a/drivers/spi/fsl_qspi.c
>>> +++ b/drivers/spi/fsl_qspi.c
>>> @@ -24,7 +24,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>>
>>>  #define TX_BUFFER_SIZE         0x40
>>>  #endif
>>>
>>> -#define OFFSET_BITS_MASK       GENMASK(24, 0)
>>> +#define OFFSET_BITS_MASK       0x00ffffff
>>>
>>>  #define FLASH_STATUS_WEL       0x02
>>>
>>> --
>>> 2.5.0
>>
>> thanks!
> 
> Best regards,
> Marek Vasut
> 


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================