[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 <marex at denx.de> 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 <marex at denx.de> wrote:
>>>
>>> On 01/10/2017 06:20 AM, Chee Tien Fong wrote:
>>>> From: Tien Fong Chee <tien.fong.chee at intel.com>
>>>>
>>>> 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 <dinguyen at opensource.altera.com>
>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com>
>>>> Cc: Marek Vasut <marex at denx.de>
>>>> Cc: Dinh Nguyen <dinguyen at kernel.org>
>>>> Cc: Chin Liang See <chin.liang.see at intel.com>
>>>> Cc: Tien Fong <skywindctf at gmail.com>
>>>> ---
>>>>  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(&sysmgr_regs->emacgrp_ctrl,
>>>> -                  SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << physhift);
>>>> -
>>>>       /* configure to PHY interface select choosed */
>>>> -     setbits_le32(&sysmgr_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