Holes management in syscon driver

Hi Dong, all,

We use syscon driver on our STi platforms to manage our system config registers. We declare one syscon instance per sysconf bank.

The problem we are facing is that these banks have holes, and when using regmap's debugfs interface to dump the registers, we get imprecise aborts.

My first idea would be do implement the .readable_reg and .writeable_reg callbacks offered by regmap in syscon driver, and provide ranges where there are registers via DT. But I am not sure how would look the DT bindings.

What is your view on this?
Are other syscon users having the same issue?

Regards,
Maxime

In reply to mail: Holes management in syscon driver.

Hi Dong, Maxime, Lee, all,

Regmap also offers .rd_table and .wr_table structures that can be used to specify valid ranges within agiven regmap configuration.

This patch uses these structures to declare holes in a syscon instance. It takes the description from DT, and fills in the structures accordingly.

Can I have your opinion on this implementation ?

Thanks and Regards,
Seraphin

Seraphin Bonnaffe (2):
mfd: syscon: Document new DT binding "holes"
mfd: syscon: specify rd_table and wr_table from DT

 Documentation/devicetree/bindings/mfd/syscon.txt |  5 ++++
 drivers/mfd/syscon.c                             | 35 ++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

1.9.1

This patch adds DT bindings to declare holes in syscon register maps.

---
 Documentation/devicetree/bindings/mfd/syscon.txt | 5 +++++
 1 file changed, 5 insertions(+)

index fe8150b..80ab9ca 100644
@@ -13,8 +13,13 @@ Required properties:
 - compatible: Should contain "syscon".
 - reg: the register region can be accessed from syscon
 
+Optional properties:
+- holes: Register regions that cannot be accessed within reg range.
+	 Each hole is described with its offset address, followed by its size.
+
 Examples:
 gpr: iomuxc-gpr at 020e0000 {
 	compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
 	reg = <0x020e0000 0x38>;
+	holes = <0x000c 0x8 0x020 0x4>;
 };

1.9.1

The syscon driver is based on regmap, which offers the possibility to declare registers as readable/writable or not, thanks to rd_table and wr_table.

This patch takes register map's holes description from DT, and fills in the rd_table and wr_table of the corresponding syson instance accordingly.

---
 drivers/mfd/syscon.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

index ca15878..2bfb45d 100644
@@ -125,9 +125,15 @@ static int syscon_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct syscon_platform_data *pdata = dev_get_platdata(dev);
+	struct device_node *np = dev->of_node;
+	struct regmap_access_table *syscon_rw_table;
+	struct regmap_range *holes;
 	struct syscon *syscon;
 	struct resource *res;
 	void __iomem *base;
+	const __be32 *hole_prop;
+	u32 min, max, size;
+	u32 i, hlen, ngaps;
 
 	syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
 	if (!syscon)

@@ -141,6 +147,35 @@ static int syscon_probe(struct platform_device *pdev)
 	if (!base)
 		return -ENOMEM;
 
+	hole_prop = of_get_property(np, "holes", &hlen);
+	if (hole_prop) {
+		hlen /= sizeof(*hole_prop);
+		ngaps = hlen / 2;
+
+		holes =  devm_kzalloc(dev, ngaps * sizeof(*holes), GFP_KERNEL);
+		if (!holes)
+			return -ENOMEM;
+
+		for (i = 0; i < ngaps; i++) {
+			min = (u32)of_read_number(&hole_prop[i * 2], 1);
+			size = (u32)of_read_number(&hole_prop[i * 2 + 1], 1);
+			max = min + size - 1;
+
+			holes[i].range_min = min;
+			holes[i].range_max = max;
+		}
+		syscon_rw_table = devm_kzalloc(dev, sizeof(*syscon_rw_table),
+					       GFP_KERNEL);
+		if (!syscon_rw_table)
+			return -ENOMEM;
+
+		syscon_rw_table->no_ranges = holes;
+		syscon_rw_table->n_no_ranges = ngaps;
+
+		syscon_regmap_config.rd_table = syscon_rw_table;
+		syscon_regmap_config.wr_table = syscon_rw_table;
+	}
+
 	syscon_regmap_config.max_register = res->end - res->start - 3;
 	if (pdata)
 		syscon_regmap_config.name = pdata->label;

1.9.1

Hi Seraphin,

As discussed face to face, it looks fine to me.

But we would like green light in the new "holes" property introduced.

Thanks,
Maxime