[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1b05b4ae-00e7-0f90-9c63-7da8797bdb6a@kernel.org>
Date: Thu, 10 Aug 2023 15:05:36 +0300
From: Roger Quadros <rogerq@...nel.org>
To: Md Danish Anwar <a0501179@...com>, Andrew Davis <afd@...com>,
MD Danish Anwar <danishanwar@...com>,
Randy Dunlap <rdunlap@...radead.org>,
Simon Horman <simon.horman@...igine.com>,
Vignesh Raghavendra <vigneshr@...com>,
Andrew Lunn <andrew@...n.ch>,
Richard Cochran <richardcochran@...il.com>,
Conor Dooley <conor+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
"David S. Miller" <davem@...emloft.net>
Cc: nm@...com, srk@...com, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, netdev@...r.kernel.org,
linux-omap@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 3/5] net: ti: icss-iep: Add IEP driver
On 10/08/2023 14:50, Md Danish Anwar wrote:
> Hi Andrew,
>
> On 09/08/23 8:30 pm, Andrew Davis wrote:
>> On 8/9/23 6:49 AM, MD Danish Anwar wrote:
>>> From: Roger Quadros <rogerq@...com>
>>>
>>> Add a driver for Industrial Ethernet Peripheral (IEP) block of PRUSS to
>>> support timestamping of ethernet packets and thus support PTP and PPS
>>> for PRU ethernet ports.
>>>
>>> Signed-off-by: Roger Quadros <rogerq@...com>
>>> Signed-off-by: Lokesh Vutla <lokeshvutla@...com>
>>> Signed-off-by: Murali Karicheri <m-karicheri2@...com>
>>> Signed-off-by: Vignesh Raghavendra <vigneshr@...com>
>>> Signed-off-by: MD Danish Anwar <danishanwar@...com>
>>> ---
>>> drivers/net/ethernet/ti/Kconfig | 12 +
>>> drivers/net/ethernet/ti/Makefile | 1 +
>>> drivers/net/ethernet/ti/icssg/icss_iep.c | 935 +++++++++++++++++++++++
>>> drivers/net/ethernet/ti/icssg/icss_iep.h | 38 +
>>> 4 files changed, 986 insertions(+)
>>> create mode 100644 drivers/net/ethernet/ti/icssg/icss_iep.c
>>> create mode 100644 drivers/net/ethernet/ti/icssg/icss_iep.h
>>>
>>> diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
>>> index 63e510b6860f..88b5b1b47779 100644
>>> --- a/drivers/net/ethernet/ti/Kconfig
>>> +++ b/drivers/net/ethernet/ti/Kconfig
>>> @@ -186,6 +186,7 @@ config CPMAC
>>> config TI_ICSSG_PRUETH
>>> tristate "TI Gigabit PRU Ethernet driver"
>>> select PHYLIB
>>> + select TI_ICSS_IEP
>>
>> Why not save selecting this until you add its use in the ICSSG_PRUETH driver in
>> the next patch.
>>
>
> The next patch is only adding changes to icssg-prueth .c /.h files. This patch
> is adding changes to Kconfig and the Makefile. To keep it that way selecting
> this is added in this patch. No worries, I will move this to next patch.
>
>> [...]
>>
>>> +
>>> +static u32 icss_iep_readl(struct icss_iep *iep, int reg)
>>> +{
>>> + return readl(iep->base + iep->plat_data->reg_offs[reg]);
>>> +}
>>
>> Do these one line functions really add anything? Actually why
>> not use the regmap you have here.
>
> These one line functions are not really adding anything but they are acting as
> a wrapper around readl /writel and providing some sort of encapsulation as
> directly calling readl will result in a little complicated code.
>
> /* WIth One line function */
> ts_lo = icss_iep_readl(iep, ICSS_IEP_COUNT_REG0);
>
> /* Without one line function */
> ts_lo = readl(iep->base, iep->plat_data->reg_offs[ICSS_IEP_COUNT_REG0]);
>
> Previously regmap was used in this driver. But in older commit [1] in
> 5.10-ti-linux-kernel (Before I picked the driver for upstream) it got changed
> to readl / writel stating that regmap_read / write is too slow. IEP is time
> sensitive and needs faster read and write, probably because of this they
> changed it.
This is true. Can you please pick the exact reasoning mentioned there
and put it as a comment where you use read/writel() instead of regmap()
so we don't forget this and accidentally switch it back to regmap()
in the future.
I think this is only required for read/write to the IEP count register and
SYNC_CTRL_REG when doing gettime/settime.
--
cheers,
-roger
Powered by blists - more mailing lists