Re: [U-Boot] [PATCH v4 15/28] arm: socfpga: combine clrbits/setbits into a single clrsetbits

[Marek Vasut]

On 02/16/2017 04:34 AM, Ley Foon Tan wrote:
> Hi Marek
> 
> On Mon, Jan 23, 2017 at 11:58 AM, Marek Vasut  wrote:
>>
>> On 01/10/2017 06:20 AM, Chee Tien Fong wrote:
>>> From: Tien Fong Chee 
>>>
>>> There is no dependency on doing a separate clrbits first in the
>>> dwmac_deassert_reset function. Combine them into a single
>>> clrsetbits call.
>>>
>>> Signed-off-by: Dinh Nguyen 
>>> Signed-off-by: Tien Fong Chee 
>>> Cc: Marek Vasut 
>>> Cc: Dinh Nguyen 
>>> Cc: Chin Liang See 
>>> Cc: Tien Fong 
>>> ---
>>>  arch/arm/mach-socfpga/misc.c | 9 +++--
>>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
>>> index 2645129..c97caea 100644
>>> --- a/arch/arm/mach-socfpga/misc.c
>>> +++ b/arch/arm/mach-socfpga/misc.c
>>> @@ -100,13 +100,10 @@ static void dwmac_deassert_reset(const unsigned int 
>>> of_reset_id,
>>>   return;
>>>   }
>>>
>>> - /* Clearing emac0 PHY interface select to 0 */
>>> - clrbits_le32(_regs->emacgrp_ctrl,
>>> -  SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << physhift);
>>> -
>>>   /* configure to PHY interface select choosed */
>>> - setbits_le32(_regs->emacgrp_ctrl,
>>> -  phymode << physhift);
>>
>> I don't think this patch is correct. The purpose of using these calls
>> separately is so that the write with cleared physel mask actually
>> reaches hardware, followed by read and another write with the physel
>> mask set. clrsetbits will do only one read-modify-write cycle.
> The cleared physel mask is OR with the phymode set and then write to
> hardware. So, I think the
> result is the same.

The original code does TWO writes into the hardware (one with zeroed
physel mask, the other with configured physel mask), the new code does
ONE write into the hardware. Why ? Is that because of some special
hardware property ? Or is one write really fine ?

>  #define clrsetbits(type, addr, clear, set) \
>  out_##type((addr), (in_##type(addr) & ~(clear)) | (set))
> 
>>
>>> + clrsetbits_le32(_regs->emacgrp_ctrl,
>>> + SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << physhift,
>>> + phymode << physhift);
>>>
>>>   /* Release the EMAC controller from reset */
>>>   socfpga_per_reset(reset, 0);
>>>
>>
>>
>> --
>> Best regards,
>> Marek Vasut
>> ___
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v4 15/28] arm: socfpga: combine clrbits/setbits into a single clrsetbits

[Marek Vasut]

On 02/17/2017 10:10 AM, Ley Foon Tan wrote:
> On Fri, Feb 17, 2017 at 3:54 PM, Marek Vasut  wrote:
>> On 02/16/2017 04:34 AM, Ley Foon Tan wrote:
>>> Hi Marek
>>>
>>> On Mon, Jan 23, 2017 at 11:58 AM, Marek Vasut  wrote:

 On 01/10/2017 06:20 AM, Chee Tien Fong wrote:
> From: Tien Fong Chee 
>
> There is no dependency on doing a separate clrbits first in the
> dwmac_deassert_reset function. Combine them into a single
> clrsetbits call.
>
> Signed-off-by: Dinh Nguyen 
> Signed-off-by: Tien Fong Chee 
> Cc: Marek Vasut 
> Cc: Dinh Nguyen 
> Cc: Chin Liang See 
> Cc: Tien Fong 
> ---
>  arch/arm/mach-socfpga/misc.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
> index 2645129..c97caea 100644
> --- a/arch/arm/mach-socfpga/misc.c
> +++ b/arch/arm/mach-socfpga/misc.c
> @@ -100,13 +100,10 @@ static void dwmac_deassert_reset(const unsigned int 
> of_reset_id,
>   return;
>   }
>
> - /* Clearing emac0 PHY interface select to 0 */
> - clrbits_le32(_regs->emacgrp_ctrl,
> -  SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << physhift);
> -
>   /* configure to PHY interface select choosed */
> - setbits_le32(_regs->emacgrp_ctrl,
> -  phymode << physhift);

 I don't think this patch is correct. The purpose of using these calls
 separately is so that the write with cleared physel mask actually
 reaches hardware, followed by read and another write with the physel
 mask set. clrsetbits will do only one read-modify-write cycle.
>>>
>>> The cleared physel mask is OR with the phymode set and then write to
>>> hardware. So, I think the
>>> result is the same.
>>
>> The result isn't the same, look at how setbits_le32() is implemented.
>> The previous code will perform two read-modify-writes, while
>> clrsetbits_le32() will perform one read-modify-write.
>>
>> I dunno how the hardware is implemented internally, but there might be a
>> reason why the original code contained two writes.
> I checked the code in linux driver, it only need one read-modify-write.
> So, the new code should be fine.

OK, thanks


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v4 15/28] arm: socfpga: combine clrbits/setbits into a single clrsetbits

[Ley Foon Tan]

On Fri, Feb 17, 2017 at 3:54 PM, Marek Vasut  wrote:
> On 02/16/2017 04:34 AM, Ley Foon Tan wrote:
>> Hi Marek
>>
>> On Mon, Jan 23, 2017 at 11:58 AM, Marek Vasut  wrote:
>>>
>>> On 01/10/2017 06:20 AM, Chee Tien Fong wrote:
 From: Tien Fong Chee 

 There is no dependency on doing a separate clrbits first in the
 dwmac_deassert_reset function. Combine them into a single
 clrsetbits call.

 Signed-off-by: Dinh Nguyen 
 Signed-off-by: Tien Fong Chee 
 Cc: Marek Vasut 
 Cc: Dinh Nguyen 
 Cc: Chin Liang See 
 Cc: Tien Fong 
 ---
  arch/arm/mach-socfpga/misc.c | 9 +++--
  1 file changed, 3 insertions(+), 6 deletions(-)

 diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
 index 2645129..c97caea 100644
 --- a/arch/arm/mach-socfpga/misc.c
 +++ b/arch/arm/mach-socfpga/misc.c
 @@ -100,13 +100,10 @@ static void dwmac_deassert_reset(const unsigned int 
 of_reset_id,
   return;
   }

 - /* Clearing emac0 PHY interface select to 0 */
 - clrbits_le32(_regs->emacgrp_ctrl,
 -  SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << physhift);
 -
   /* configure to PHY interface select choosed */
 - setbits_le32(_regs->emacgrp_ctrl,
 -  phymode << physhift);
>>>
>>> I don't think this patch is correct. The purpose of using these calls
>>> separately is so that the write with cleared physel mask actually
>>> reaches hardware, followed by read and another write with the physel
>>> mask set. clrsetbits will do only one read-modify-write cycle.
>>
>> The cleared physel mask is OR with the phymode set and then write to
>> hardware. So, I think the
>> result is the same.
>
> The result isn't the same, look at how setbits_le32() is implemented.
> The previous code will perform two read-modify-writes, while
> clrsetbits_le32() will perform one read-modify-write.
>
> I dunno how the hardware is implemented internally, but there might be a
> reason why the original code contained two writes.
I checked the code in linux driver, it only need one read-modify-write.
So, the new code should be fine.

Regards
Ley Foon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v4 15/28] arm: socfpga: combine clrbits/setbits into a single clrsetbits

[Marek Vasut]

On 02/16/2017 04:34 AM, Ley Foon Tan wrote:
> Hi Marek
> 
> On Mon, Jan 23, 2017 at 11:58 AM, Marek Vasut  wrote:
>>
>> On 01/10/2017 06:20 AM, Chee Tien Fong wrote:
>>> From: Tien Fong Chee 
>>>
>>> There is no dependency on doing a separate clrbits first in the
>>> dwmac_deassert_reset function. Combine them into a single
>>> clrsetbits call.
>>>
>>> Signed-off-by: Dinh Nguyen 
>>> Signed-off-by: Tien Fong Chee 
>>> Cc: Marek Vasut 
>>> Cc: Dinh Nguyen 
>>> Cc: Chin Liang See 
>>> Cc: Tien Fong 
>>> ---
>>>  arch/arm/mach-socfpga/misc.c | 9 +++--
>>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
>>> index 2645129..c97caea 100644
>>> --- a/arch/arm/mach-socfpga/misc.c
>>> +++ b/arch/arm/mach-socfpga/misc.c
>>> @@ -100,13 +100,10 @@ static void dwmac_deassert_reset(const unsigned int 
>>> of_reset_id,
>>>   return;
>>>   }
>>>
>>> - /* Clearing emac0 PHY interface select to 0 */
>>> - clrbits_le32(_regs->emacgrp_ctrl,
>>> -  SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << physhift);
>>> -
>>>   /* configure to PHY interface select choosed */
>>> - setbits_le32(_regs->emacgrp_ctrl,
>>> -  phymode << physhift);
>>
>> I don't think this patch is correct. The purpose of using these calls
>> separately is so that the write with cleared physel mask actually
>> reaches hardware, followed by read and another write with the physel
>> mask set. clrsetbits will do only one read-modify-write cycle.
>
> The cleared physel mask is OR with the phymode set and then write to
> hardware. So, I think the
> result is the same.

The result isn't the same, look at how setbits_le32() is implemented.
The previous code will perform two read-modify-writes, while
clrsetbits_le32() will perform one read-modify-write.

I dunno how the hardware is implemented internally, but there might be a
reason why the original code contained two writes.

>  #define clrsetbits(type, addr, clear, set) \
>  out_##type((addr), (in_##type(addr) & ~(clear)) | (set))
> 
>>
>>> + clrsetbits_le32(_regs->emacgrp_ctrl,
>>> + SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << physhift,
>>> + phymode << physhift);
>>>
>>>   /* Release the EMAC controller from reset */
>>>   socfpga_per_reset(reset, 0);
>>>
>>
>>
>> --
>> Best regards,
>> Marek Vasut
>> ___
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v4 15/28] arm: socfpga: combine clrbits/setbits into a single clrsetbits

[Ley Foon Tan]

Hi Marek

On Mon, Jan 23, 2017 at 11:58 AM, Marek Vasut  wrote:
>
> On 01/10/2017 06:20 AM, Chee Tien Fong wrote:
> > From: Tien Fong Chee 
> >
> > There is no dependency on doing a separate clrbits first in the
> > dwmac_deassert_reset function. Combine them into a single
> > clrsetbits call.
> >
> > Signed-off-by: Dinh Nguyen 
> > Signed-off-by: Tien Fong Chee 
> > Cc: Marek Vasut 
> > Cc: Dinh Nguyen 
> > Cc: Chin Liang See 
> > Cc: Tien Fong 
> > ---
> >  arch/arm/mach-socfpga/misc.c | 9 +++--
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
> > index 2645129..c97caea 100644
> > --- a/arch/arm/mach-socfpga/misc.c
> > +++ b/arch/arm/mach-socfpga/misc.c
> > @@ -100,13 +100,10 @@ static void dwmac_deassert_reset(const unsigned int 
> > of_reset_id,
> >   return;
> >   }
> >
> > - /* Clearing emac0 PHY interface select to 0 */
> > - clrbits_le32(_regs->emacgrp_ctrl,
> > -  SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << physhift);
> > -
> >   /* configure to PHY interface select choosed */
> > - setbits_le32(_regs->emacgrp_ctrl,
> > -  phymode << physhift);
>
> I don't think this patch is correct. The purpose of using these calls
> separately is so that the write with cleared physel mask actually
> reaches hardware, followed by read and another write with the physel
> mask set. clrsetbits will do only one read-modify-write cycle.
The cleared physel mask is OR with the phymode set and then write to
hardware. So, I think the
result is the same.

 #define clrsetbits(type, addr, clear, set) \
 out_##type((addr), (in_##type(addr) & ~(clear)) | (set))

>
> > + clrsetbits_le32(_regs->emacgrp_ctrl,
> > + SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << physhift,
> > + phymode << physhift);
> >
> >   /* Release the EMAC controller from reset */
> >   socfpga_per_reset(reset, 0);
> >
>
>
> --
> Best regards,
> Marek Vasut
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v4 15/28] arm: socfpga: combine clrbits/setbits into a single clrsetbits

[Marek Vasut]

On 01/10/2017 06:20 AM, Chee Tien Fong wrote:
> From: Tien Fong Chee 
> 
> There is no dependency on doing a separate clrbits first in the
> dwmac_deassert_reset function. Combine them into a single
> clrsetbits call.
> 
> Signed-off-by: Dinh Nguyen 
> Signed-off-by: Tien Fong Chee 
> Cc: Marek Vasut 
> Cc: Dinh Nguyen 
> Cc: Chin Liang See 
> Cc: Tien Fong 
> ---
>  arch/arm/mach-socfpga/misc.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
> index 2645129..c97caea 100644
> --- a/arch/arm/mach-socfpga/misc.c
> +++ b/arch/arm/mach-socfpga/misc.c
> @@ -100,13 +100,10 @@ static void dwmac_deassert_reset(const unsigned int 
> of_reset_id,
>   return;
>   }
>  
> - /* Clearing emac0 PHY interface select to 0 */
> - clrbits_le32(_regs->emacgrp_ctrl,
> -  SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << physhift);
> -
>   /* configure to PHY interface select choosed */
> - setbits_le32(_regs->emacgrp_ctrl,
> -  phymode << physhift);

I don't think this patch is correct. The purpose of using these calls
separately is so that the write with cleared physel mask actually
reaches hardware, followed by read and another write with the physel
mask set. clrsetbits will do only one read-modify-write cycle.

> + clrsetbits_le32(_regs->emacgrp_ctrl,
> + SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << physhift,
> + phymode << physhift);
>  
>   /* Release the EMAC controller from reset */
>   socfpga_per_reset(reset, 0);
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH v4 15/28] arm: socfpga: combine clrbits/setbits into a single clrsetbits

[Chee Tien Fong]

From: Tien Fong Chee 

There is no dependency on doing a separate clrbits first in the
dwmac_deassert_reset function. Combine them into a single
clrsetbits call.

Signed-off-by: Dinh Nguyen 
Signed-off-by: Tien Fong Chee 
Cc: Marek Vasut 
Cc: Dinh Nguyen 
Cc: Chin Liang See 
Cc: Tien Fong 
---
 arch/arm/mach-socfpga/misc.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
index 2645129..c97caea 100644
--- a/arch/arm/mach-socfpga/misc.c
+++ b/arch/arm/mach-socfpga/misc.c
@@ -100,13 +100,10 @@ static void dwmac_deassert_reset(const unsigned int 
of_reset_id,
return;
}
 
-   /* Clearing emac0 PHY interface select to 0 */
-   clrbits_le32(_regs->emacgrp_ctrl,
-SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << physhift);
-
/* configure to PHY interface select choosed */
-   setbits_le32(_regs->emacgrp_ctrl,
-phymode << physhift);
+   clrsetbits_le32(_regs->emacgrp_ctrl,
+   SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << physhift,
+   phymode << physhift);
 
/* Release the EMAC controller from reset */
socfpga_per_reset(reset, 0);
-- 
2.2.0

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot