[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 <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.

OK, thanks


-- 
Best regards,
Marek Vasut