On 28.12.2023 21:07, Sergey Shtylyov wrote:
> On 12/27/23 1:10 PM, claudiu beznea wrote:
>
> [...]
>
>>>> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>>>>
>>>> CSR.OPS bits specify the current operating mode and (according to
>>>> documentation) they are updated by HW when the operating mode change
>>>> request is processed. To comply with this check CSR.OPS before proceeding.
>>>>
>>>> Commit introduces ravb_set_opmode() that does all the necessities for
>>>> setting the operating mode (set DMA.CCC and wait for CSR.OPS) and call it
>>>> where needed. This should comply with all the HW manuals requirements as
>>>> different manual variants specify that different modes need to be checked
>>>> in CSR.OPS when setting DMA.CCC.
>>>>
>>>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>>>> ---
>>>> drivers/net/ethernet/renesas/ravb_main.c | 52 ++++++++++++++----------
>>>> 1 file changed, 31 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>> index 664eda4b5a11..ae99d035a3b6 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>> @@ -66,14 +66,15 @@ int ravb_wait(struct net_device *ndev, enum ravb_reg reg, u32 mask, u32 value)
>>>> return -ETIMEDOUT;
>>>> }
>>>>
>>>> -static int ravb_config(struct net_device *ndev)
>>>> +static int ravb_set_opmode(struct net_device *ndev, u32 opmode)
>>>
>>> Since you pass the complete CCC register value below, you should
>>> rather call the function ravb_set_ccc() and call the parameter opmode
>>> ccc.
>>
>> This will be confusing. E.g., if renaming it ravb_set_ccc() one would
>> expect to set any fields of CCC though this function but this is not true
>> as ravb_modify() in this function masks only CCC_OPC. The call of:
>>
>> error = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
>>
>> bellow is just to comply with datasheet requirements, previous code and at
>> the same time re-use this function.
>
> How about the following then (ugly... but does the job):
>
> /* Set operating mode */
> if (opmode & ~CCC_OPC)
> ravb_write(ndev, opmode, CCC);
> else
> ravb_modify(ndev, CCC, CCC_OPC, opmode);
>
> Either that or just don't use ravb_set_opmode() when writing the whole
> 32-bit value below...
This looks uglier to me...
We have this discussion because of ccc_gac. For ccc_gac platforms we need
to set OPC, GAC, CSEL at the same time. This is how we can change the
operating mode to configuration mode in case we also need to configure GAC
(due to restrictions imposed by hardware).
What I want to say is that setting GAC and CSEL along with CCC is part of
changing the operating mode to configuration mode for platforms supporting
GAC because of hardware limitations.
>
> [...]
>
>>>> @@ -2560,21 +2559,23 @@ static int ravb_set_gti(struct net_device *ndev)
> [...]
>>>
>>>> /* Set CSEL value */
>>>> ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
>>>> } else if (info->ccc_gac) {
>>>> - ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG |
>>>> - CCC_GAC | CCC_CSEL_HPB);
>>>> + error = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
>
> ... like this?
>
> ravb_write(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB, CCC);
> error = ravb_wait(ndev, CSR, CSR_OPS, CSR_OPS_CONFIG);
>
> [...]
>
> MBR, Sergey
