Re: [PATCH v3 3/5] net: ti: icss-iep: Add IEP driver
From: Andrew Davis
Date: Fri Aug 11 2023 - 11:24:44 EST
On 8/10/23 6:50 AM, 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@xxxxxx>
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@xxxxxx>
Signed-off-by: Lokesh Vutla <lokeshvutla@xxxxxx>
Signed-off-by: Murali Karicheri <m-karicheri2@xxxxxx>
Signed-off-by: Vignesh Raghavendra <vigneshr@xxxxxx>
Signed-off-by: MD Danish Anwar <danishanwar@xxxxxx>
---
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.
Sounds like you only need direct register access for time sensitive
gettime/settime functions, if that is the only place writel()/readl() is
needed just drop the helper and use directly in that one spot.
[...]
+static void icss_iep_enable(struct icss_iep *iep)
+{
+ regmap_update_bits(iep->map, ICSS_IEP_GLOBAL_CFG_REG,
+ IEP_GLOBAL_CFG_CNT_ENABLE,
+ IEP_GLOBAL_CFG_CNT_ENABLE);
Have you looked into regmap_fields?
No I hadn't. But now I looked into regmap_fields, seems to be another way to
update the bits, instead of passing mask and value, regmap_filed_read / write
only takes the value. But for that we will need to create a regmap field. If
you want me to switch to regmap_fields instead of regmap_update_bits I can make
the changes. But I am fine with regmap_update_bits().
I'm suggesting regmap fields as I have used it several times and it resulted
in greatly improved readability. Yes you will need a regmap field table, but
that is the best part, it lets you put all your bit definitions in one spot
that can match 1:1 with the datasheet, much easier to check for correctness
than if the bit usages are all spread out in the driver.
I won't insist on you converting this driver to use it today, but I do recommend
you at least give it a shot for your own learning.
Andrew
[...]
+
+ if (!!(iep->latch_enable & BIT(index)) == !!on)
+ goto exit;
+
There has to be a better way to write this logic..
[...]
+
+static const struct of_device_id icss_iep_of_match[];
+
Why the forward declaration?
I will remove this, I don't see any reason for this.
+static int icss_iep_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct icss_iep *iep;
+ struct clk *iep_clk;
+
+ iep = devm_kzalloc(dev, sizeof(*iep), GFP_KERNEL);
+ if (!iep)
+ return -ENOMEM;
+
+ iep->dev = dev;
+ iep->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(iep->base))
+ return -ENODEV;
+
+ iep_clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(iep_clk))
+ return PTR_ERR(iep_clk);
+
+ iep->refclk_freq = clk_get_rate(iep_clk);
+
+ iep->def_inc = NSEC_PER_SEC / iep->refclk_freq; /* ns per clock tick */
+ if (iep->def_inc > IEP_MAX_DEF_INC) {
+ dev_err(dev, "Failed to set def_inc %d. IEP_clock is too slow to be
supported\n",
+ iep->def_inc);
+ return -EINVAL;
+ }
+
+ iep->plat_data = of_device_get_match_data(dev);
Directly using of_*() functions is often wrong, try just device_get_match_data().
Sure. I will change to device_get_match_data().
[...]
+static struct platform_driver icss_iep_driver = {
+ .driver = {
+ .name = "icss-iep",
+ .of_match_table = of_match_ptr(icss_iep_of_match),
This driver cannot work without OF, using of_match_ptr() is not needed.
Sure, I will drop of_match_ptr().
Andrew
For reading and updating registers, we can have
1. icss_iep_readl / writel and regmap_update_bits() OR
2. regmap_read / write and regmap_update_bits() OR
3. icss_iep_readl / writel and regmap_fields OR
4. regmap_read / write and regmap_fields
Currently we are using 1. Please let me know if you are fine with this and I
can continue using 1. If not, please let me know your recommendation out of this 4.
[1]
https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=linux-5.10.y&id=f4f45bf71cad5be232536d63a0557d13a7eed162