[PATCH 00/11] Add support for pinmux status command on beaglebone
The series was born from the need to check the pinmux setting of a peripheral on a beaglebone board. I then ran the 'pinmux status -a' command but it failed because some operations (get_pin_muxing, get_pin_name and get_pins_count) were missing in the 'pinctrl-single' driver.
The patch series can be cleanly applied to the HEAD of the master which at the time of release points to e716c9022970dac9be15856a6651a07132463578 commit (Revert "doc: update Kernel documentation build system").
Dario Binacchi (11): pinctrl: single: fix format of structure documentation pinctrl: single: fix the loop counter variable type pinctrl: single: fix debug messages formatting pinctrl: single: get register area size by device API pinctrl: single: check "register-width" DT property pinctrl: single: change function mask default value pinctrl: single: use function pointer for register access pinctrl: single: add get_pins_count operation pinctrl: single: add get_pin_name operation pinctrl: single: add get_pin_muxing operation test: pinmux: add test for 'pinctrl-single' driver
arch/sandbox/dts/test.dts | 65 ++++ configs/sandbox_defconfig | 1 + drivers/pinctrl/pinctrl-single.c | 488 +++++++++++++++++++++++++++---- test/dm/pinmux.c | 81 ++++- 4 files changed, 570 insertions(+), 65 deletions(-)
U-Boot adopted the kernel-doc annotation style.
Signed-off-by: Dario Binacchi dariobin@libero.it ---
drivers/pinctrl/pinctrl-single.c | 45 +++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 9 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 20c3c82aa9..c9a6c272bf 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -10,23 +10,50 @@ #include <linux/libfdt.h> #include <asm/io.h>
+/** + * struct single_pdata - platform data + * @base: first configuration register + * @offset: index of last configuration register + * @mask: configuration-value mask bits + * @width: configuration register bit width + * @bits_per_mux: true if one register controls more than one pin + */ struct single_pdata { - fdt_addr_t base; /* first configuration register */ - int offset; /* index of last configuration register */ - u32 mask; /* configuration-value mask bits */ - int width; /* configuration register bit width */ + fdt_addr_t base; + int offset; + u32 mask; + int width; bool bits_per_mux; };
+/** + * struct single_fdt_pin_cfg - pin configuration + * + * This structure is used for the pin configuration parameters in case + * the register controls only one pin. + * + * @reg: configuration register offset + * @val: configuration register value + */ struct single_fdt_pin_cfg { - fdt32_t reg; /* configuration register offset */ - fdt32_t val; /* configuration register value */ + fdt32_t reg; + fdt32_t val; };
+/** + * struct single_fdt_bits_cfg - pin configuration + * + * This structure is used for the pin configuration parameters in case + * the register controls more than one pin. + * + * @reg: configuration register offset + * @val: configuration register value + * @mask: configuration register mask + */ struct single_fdt_bits_cfg { - fdt32_t reg; /* configuration register offset */ - fdt32_t val; /* configuration register value */ - fdt32_t mask; /* configuration register mask */ + fdt32_t reg; + fdt32_t val; + fdt32_t mask; };
/**
On Sat, 23 Jan 2021 at 11:27, Dario Binacchi dariobin@libero.it wrote:
U-Boot adopted the kernel-doc annotation style.
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 45 +++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 9 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
The patch changes the variable 'n' type from phys_addr_t to int.
Signed-off-by: Dario Binacchi dariobin@libero.it ---
drivers/pinctrl/pinctrl-single.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index c9a6c272bf..49ed15211d 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -75,8 +75,8 @@ static int single_configure_pins(struct udevice *dev, int size) { struct single_pdata *pdata = dev_get_plat(dev); - int count = size / sizeof(struct single_fdt_pin_cfg); - phys_addr_t n, reg; + int n, count = size / sizeof(struct single_fdt_pin_cfg); + phys_addr_t reg; u32 val;
for (n = 0; n < count; n++, pins++) { @@ -109,8 +109,8 @@ static int single_configure_bits(struct udevice *dev, int size) { struct single_pdata *pdata = dev_get_plat(dev); - int count = size / sizeof(struct single_fdt_bits_cfg); - phys_addr_t n, reg; + int n, count = size / sizeof(struct single_fdt_bits_cfg); + phys_addr_t reg; u32 val, mask;
for (n = 0; n < count; n++, pins++) {
On Sat, 23 Jan 2021 at 11:27, Dario Binacchi dariobin@libero.it wrote:
The patch changes the variable 'n' type from phys_addr_t to int.
s/The patch changes/Change/
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-sing
Reviewed-by: Simon Glass sjg@chromium.org
Hi Dario,
On 23/01/21 07:27PM, Dario Binacchi wrote:
The patch changes the variable 'n' type from phys_addr_t to int.
This information can very easily be obtained by reading the diff. The commit message should also explain _why_ the change is made. For example here it would be a good idea to add something like:
n is used as a loop counter, not as a physical address, and is used in a comparison with an int. So it makes sense to set its type to int.
Other than that,
Reviewed-by: Pratyush Yadav p.yadav@ti.com
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index c9a6c272bf..49ed15211d 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -75,8 +75,8 @@ static int single_configure_pins(struct udevice *dev, int size) { struct single_pdata *pdata = dev_get_plat(dev);
- int count = size / sizeof(struct single_fdt_pin_cfg);
- phys_addr_t n, reg;
int n, count = size / sizeof(struct single_fdt_pin_cfg);
phys_addr_t reg; u32 val;
for (n = 0; n < count; n++, pins++) {
@@ -109,8 +109,8 @@ static int single_configure_bits(struct udevice *dev, int size) { struct single_pdata *pdata = dev_get_plat(dev);
- int count = size / sizeof(struct single_fdt_bits_cfg);
- phys_addr_t n, reg;
int n, count = size / sizeof(struct single_fdt_bits_cfg);
phys_addr_t reg; u32 val, mask;
for (n = 0; n < count; n++, pins++) {
-- 2.17.1
The printf '%pa' format specifier appends the '0x' prefix to the displayed address. Furthermore, the offset variable is displayed with the '%x' format specifier instead of '%pa'.
Signed-off-by: Dario Binacchi dariobin@libero.it ---
drivers/pinctrl/pinctrl-single.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 49ed15211d..cec00e289c 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -77,15 +77,17 @@ static int single_configure_pins(struct udevice *dev, struct single_pdata *pdata = dev_get_plat(dev); int n, count = size / sizeof(struct single_fdt_pin_cfg); phys_addr_t reg; - u32 val; + u32 offset, val;
for (n = 0; n < count; n++, pins++) { - reg = fdt32_to_cpu(pins->reg); - if ((reg < 0) || (reg > pdata->offset)) { - dev_dbg(dev, " invalid register offset 0x%pa\n", ®); + offset = fdt32_to_cpu(pins->reg); + if (offset < 0 || offset > pdata->offset) { + dev_dbg(dev, " invalid register offset 0x%x\n", + offset); continue; } - reg += pdata->base; + + reg = pdata->base + offset; val = fdt32_to_cpu(pins->val) & pdata->mask; switch (pdata->width) { case 16: @@ -99,7 +101,7 @@ static int single_configure_pins(struct udevice *dev, pdata->width); continue; } - dev_dbg(dev, " reg/val 0x%pa/0x%08x\n", ®, val); + dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val); } return 0; } @@ -111,15 +113,17 @@ static int single_configure_bits(struct udevice *dev, struct single_pdata *pdata = dev_get_plat(dev); int n, count = size / sizeof(struct single_fdt_bits_cfg); phys_addr_t reg; - u32 val, mask; + u32 offset, val, mask;
for (n = 0; n < count; n++, pins++) { - reg = fdt32_to_cpu(pins->reg); - if ((reg < 0) || (reg > pdata->offset)) { - dev_dbg(dev, " invalid register offset 0x%pa\n", ®); + offset = fdt32_to_cpu(pins->reg); + if (offset < 0 || offset > pdata->offset) { + dev_dbg(dev, " invalid register offset 0x%x\n", + offset); continue; } - reg += pdata->base; + + reg = pdata->base + offset;
mask = fdt32_to_cpu(pins->mask); val = fdt32_to_cpu(pins->val) & mask; @@ -136,7 +140,7 @@ static int single_configure_bits(struct udevice *dev, pdata->width); continue; } - dev_dbg(dev, " reg/val 0x%pa/0x%08x\n", ®, val); + dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val); } return 0; }
Hi Dario,
On Sat, 23 Jan 2021 at 11:27, Dario Binacchi dariobin@libero.it wrote:
The printf '%pa' format specifier appends the '0x' prefix to the displayed address. Furthermore, the offset variable is displayed with the '%x' format specifier instead of '%pa'.
Please start your commit message with the problem and then describe what your patch does. It is unclear from this commit message why we need this patch :-)
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)
Regards, Simon
Hi Dario,
On 23/01/21 07:27PM, Dario Binacchi wrote:
The printf '%pa' format specifier appends the '0x' prefix to the displayed address. Furthermore, the offset variable is displayed with the '%x' format specifier instead of '%pa'.
I agree with Simon that the commit message does not explain why this change is needed.
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 49ed15211d..cec00e289c 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -77,15 +77,17 @@ static int single_configure_pins(struct udevice *dev, struct single_pdata *pdata = dev_get_plat(dev); int n, count = size / sizeof(struct single_fdt_pin_cfg); phys_addr_t reg;
- u32 val;
u32 offset, val;
for (n = 0; n < count; n++, pins++) {
reg = fdt32_to_cpu(pins->reg);if ((reg < 0) || (reg > pdata->offset)) {dev_dbg(dev, " invalid register offset 0x%pa\n", ®);
offset = fdt32_to_cpu(pins->reg);if (offset < 0 || offset > pdata->offset) {dev_dbg(dev, " invalid register offset 0x%x\n",offset);
You are not just fixing "debug messages formatting" here. You have made other changes to the structure of the code here. While these changes might all be correct, they don't belong in a commit that claims to fix message formatting.
For example, this dev_dbg() statement in the pre-image prints "®" as the offset. This would be the address of the variable reg on the stack. This looks like it is a bug. The offset is stored in "reg", not in "®". The post-image fixes this bug. A person reading this diff might not look so closely at this because you only claim to change formatting. And some important discussion/context on this bug might be skipped over.
Please re-word the commit subject and message to clearly explain why you are making these structural changes and separate them from the formatting changes (which also warrant an explanation on their own).
continue; }
reg += pdata->base;
reg = pdata->base + offset;@@ -99,7 +101,7 @@ static int single_configure_pins(struct udevice *dev, pdata->width); continue; }
dev_dbg(dev, " reg/val 0x%pa/0x%08x\n", ®, val);
dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val);
In a similar fashion as above, shouldn't "®" be replaced with "reg"? I am not too familiar with the code to say for sure. Same for the changes below.
} return 0; } @@ -111,15 +113,17 @@ static int single_configure_bits(struct udevice *dev, struct single_pdata *pdata = dev_get_plat(dev); int n, count = size / sizeof(struct single_fdt_bits_cfg); phys_addr_t reg;
- u32 val, mask;
u32 offset, val, mask;
for (n = 0; n < count; n++, pins++) {
reg = fdt32_to_cpu(pins->reg);if ((reg < 0) || (reg > pdata->offset)) {dev_dbg(dev, " invalid register offset 0x%pa\n", ®);
offset = fdt32_to_cpu(pins->reg);if (offset < 0 || offset > pdata->offset) {dev_dbg(dev, " invalid register offset 0x%x\n",offset); continue;
reg += pdata->base;
reg = pdata->base + offset;mask = fdt32_to_cpu(pins->mask); val = fdt32_to_cpu(pins->val) & mask;
@@ -136,7 +140,7 @@ static int single_configure_bits(struct udevice *dev, pdata->width); continue; }
dev_dbg(dev, " reg/val 0x%pa/0x%08x\n", ®, val);
dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val);}
2.17.1
Hi Pratyush,
Il 25/01/2021 18:09 Pratyush Yadav p.yadav@ti.com ha scritto:
Hi Dario,
On 23/01/21 07:27PM, Dario Binacchi wrote:
The printf '%pa' format specifier appends the '0x' prefix to the displayed address. Furthermore, the offset variable is displayed with the '%x' format specifier instead of '%pa'.
I agree with Simon that the commit message does not explain why this change is needed.
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 49ed15211d..cec00e289c 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -77,15 +77,17 @@ static int single_configure_pins(struct udevice *dev, struct single_pdata *pdata = dev_get_plat(dev); int n, count = size / sizeof(struct single_fdt_pin_cfg); phys_addr_t reg;
- u32 val;
u32 offset, val;
for (n = 0; n < count; n++, pins++) {
reg = fdt32_to_cpu(pins->reg);if ((reg < 0) || (reg > pdata->offset)) {dev_dbg(dev, " invalid register offset 0x%pa\n", ®);
offset = fdt32_to_cpu(pins->reg);if (offset < 0 || offset > pdata->offset) {dev_dbg(dev, " invalid register offset 0x%x\n",offset);You are not just fixing "debug messages formatting" here. You have made other changes to the structure of the code here. While these changes might all be correct, they don't belong in a commit that claims to fix message formatting.
For example, this dev_dbg() statement in the pre-image prints "®" as the offset. This would be the address of the variable reg on the stack. This looks like it is a bug. The offset is stored in "reg", not in "®". The post-image fixes this bug. A person reading this diff might not look so closely at this because you only claim to change formatting. And some important discussion/context on this bug might be skipped over.
Please re-word the commit subject and message to clearly explain why you are making these structural changes and separate them from the formatting changes (which also warrant an explanation on their own).
continue; }
reg += pdata->base;
reg = pdata->base + offset;@@ -99,7 +101,7 @@ static int single_configure_pins(struct udevice *dev, pdata->width); continue; }
dev_dbg(dev, " reg/val 0x%pa/0x%08x\n", ®, val);
dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val);In a similar fashion as above, shouldn't "®" be replaced with "reg"? I am not too familiar with the code to say for sure. Same for the changes below.
Also I would have expected reg instead of ® but at the link https://www.kernel.org/doc/html/latest/core-api/printk-formats.html is explained: ... "Phys_addr_t Physical Address Types"
%pa [p] 0x01234567 or 0x0123456789abcdef For printing a phys_addr_t type (and its derivatives, such as resource_size_t) which can vary based on build options, regardless of the width of the CPU data path.
Passed by reference. ...
I also did some tests on the board: dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val); --> reg/val 0x44e10940/0x00000030 dev_dbg(dev, " reg/val %p/0x%08x\n", reg, val); --> reg/val 44e10940/0x00000030
I'll continue to use the first one, which is documented.
} return 0; } @@ -111,15 +113,17 @@ static int single_configure_bits(struct udevice *dev, struct single_pdata *pdata = dev_get_plat(dev); int n, count = size / sizeof(struct single_fdt_bits_cfg); phys_addr_t reg;
- u32 val, mask;
u32 offset, val, mask;
for (n = 0; n < count; n++, pins++) {
reg = fdt32_to_cpu(pins->reg);if ((reg < 0) || (reg > pdata->offset)) {dev_dbg(dev, " invalid register offset 0x%pa\n", ®);
offset = fdt32_to_cpu(pins->reg);if (offset < 0 || offset > pdata->offset) {dev_dbg(dev, " invalid register offset 0x%x\n",offset); continue;
reg += pdata->base;
reg = pdata->base + offset;mask = fdt32_to_cpu(pins->mask); val = fdt32_to_cpu(pins->val) & mask;
@@ -136,7 +140,7 @@ static int single_configure_bits(struct udevice *dev, pdata->width); continue; }
dev_dbg(dev, " reg/val 0x%pa/0x%08x\n", ®, val);
dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val);}
2.17.1
-- Regards, Pratyush Yadav Texas Instruments India
Regards, Dario
On 26/01/21 12:20PM, Dario Binacchi wrote:
Hi Pratyush,
Il 25/01/2021 18:09 Pratyush Yadav p.yadav@ti.com ha scritto:
Hi Dario,
On 23/01/21 07:27PM, Dario Binacchi wrote:
The printf '%pa' format specifier appends the '0x' prefix to the displayed address. Furthermore, the offset variable is displayed with the '%x' format specifier instead of '%pa'.
I agree with Simon that the commit message does not explain why this change is needed.
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 49ed15211d..cec00e289c 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -77,15 +77,17 @@ static int single_configure_pins(struct udevice *dev, struct single_pdata *pdata = dev_get_plat(dev); int n, count = size / sizeof(struct single_fdt_pin_cfg); phys_addr_t reg;
- u32 val;
u32 offset, val;
for (n = 0; n < count; n++, pins++) {
reg = fdt32_to_cpu(pins->reg);if ((reg < 0) || (reg > pdata->offset)) {dev_dbg(dev, " invalid register offset 0x%pa\n", ®);
offset = fdt32_to_cpu(pins->reg);if (offset < 0 || offset > pdata->offset) {dev_dbg(dev, " invalid register offset 0x%x\n",offset);You are not just fixing "debug messages formatting" here. You have made other changes to the structure of the code here. While these changes might all be correct, they don't belong in a commit that claims to fix message formatting.
For example, this dev_dbg() statement in the pre-image prints "®" as the offset. This would be the address of the variable reg on the stack. This looks like it is a bug. The offset is stored in "reg", not in "®". The post-image fixes this bug. A person reading this diff might not look so closely at this because you only claim to change formatting. And some important discussion/context on this bug might be skipped over.
Please re-word the commit subject and message to clearly explain why you are making these structural changes and separate them from the formatting changes (which also warrant an explanation on their own).
continue; }
reg += pdata->base;
reg = pdata->base + offset;@@ -99,7 +101,7 @@ static int single_configure_pins(struct udevice *dev, pdata->width); continue; }
dev_dbg(dev, " reg/val 0x%pa/0x%08x\n", ®, val);
dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val);In a similar fashion as above, shouldn't "®" be replaced with "reg"? I am not too familiar with the code to say for sure. Same for the changes below.
Also I would have expected reg instead of ® but at the link https://www.kernel.org/doc/html/latest/core-api/printk-formats.html is explained: ... "Phys_addr_t Physical Address Types"
%pa [p] 0x01234567 or 0x0123456789abcdef For printing a phys_addr_t type (and its derivatives, such as resource_size_t) which can vary based on build options, regardless of the width of the CPU data path.
Passed by reference. ...
I also did some tests on the board: dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val); --> reg/val 0x44e10940/0x00000030 dev_dbg(dev, " reg/val %p/0x%08x\n", reg, val); --> reg/val 44e10940/0x00000030
I'll continue to use the first one, which is documented.
Makes sense. Thanks for the explanation.
My comments about commit message and splitting out changes still hold. If you think these changes should be in the same commit then please explain why in the commit message.
} return 0; } @@ -111,15 +113,17 @@ static int single_configure_bits(struct udevice *dev, struct single_pdata *pdata = dev_get_plat(dev); int n, count = size / sizeof(struct single_fdt_bits_cfg); phys_addr_t reg;
- u32 val, mask;
u32 offset, val, mask;
for (n = 0; n < count; n++, pins++) {
reg = fdt32_to_cpu(pins->reg);if ((reg < 0) || (reg > pdata->offset)) {dev_dbg(dev, " invalid register offset 0x%pa\n", ®);
offset = fdt32_to_cpu(pins->reg);if (offset < 0 || offset > pdata->offset) {dev_dbg(dev, " invalid register offset 0x%x\n",offset); continue;
reg += pdata->base;
reg = pdata->base + offset;mask = fdt32_to_cpu(pins->mask); val = fdt32_to_cpu(pins->val) & mask;
@@ -136,7 +140,7 @@ static int single_configure_bits(struct udevice *dev, pdata->width); continue; }
dev_dbg(dev, " reg/val 0x%pa/0x%08x\n", ®, val);
dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val);}
2.17.1
-- Regards, Pratyush Yadav Texas Instruments India
Regards, Dario
Use dev_read_addr_size to get size of the controller's register area.
Signed-off-by: Dario Binacchi dariobin@libero.it ---
drivers/pinctrl/pinctrl-single.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index cec00e289c..c80a42a193 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -182,17 +182,14 @@ static int single_set_state(struct udevice *dev, static int single_of_to_plat(struct udevice *dev) { fdt_addr_t addr; - u32 of_reg[2]; - int res; + fdt_size_t size; struct single_pdata *pdata = dev_get_plat(dev);
pdata->width = dev_read_u32_default(dev, "pinctrl-single,register-width", 0);
- res = dev_read_u32_array(dev, "reg", of_reg, 2); - if (res) - return res; - pdata->offset = of_reg[1] - pdata->width / 8; + dev_read_addr_size(dev, "reg", &size); + pdata->offset = size - pdata->width / BITS_PER_BYTE;
addr = dev_read_addr(dev); if (addr == FDT_ADDR_T_NONE) {
Hi Dario,
On Sat, 23 Jan 2021 at 11:27, Dario Binacchi dariobin@libero.it wrote:
Use dev_read_addr_size to get size of the controller's register area.
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index cec00e289c..c80a42a193 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -182,17 +182,14 @@ static int single_set_state(struct udevice *dev, static int single_of_to_plat(struct udevice *dev) { fdt_addr_t addr;
u32 of_reg[2];int res;
fdt_size_t size; struct single_pdata *pdata = dev_get_plat(dev); pdata->width = dev_read_u32_default(dev, "pinctrl-single,register-width", 0);
res = dev_read_u32_array(dev, "reg", of_reg, 2);if (res)return res;pdata->offset = of_reg[1] - pdata->width / 8;
dev_read_addr_size(dev, "reg", &size);
Please check return value.
pdata->offset = size - pdata->width / BITS_PER_BYTE; addr = dev_read_addr(dev); if (addr == FDT_ADDR_T_NONE) {-- 2.17.1
Regards, Simon
In more recent versions of the Linux kernel the driver's probe function returns an error if the "pinctrl-single,register-width" DT property is missing. The lack of this information, in fact, does not allow to know whether to access the registers of the controller at 8, 16 or 32 bits.
Signed-off-by: Dario Binacchi dariobin@libero.it ---
drivers/pinctrl/pinctrl-single.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index c80a42a193..8fd3bf66de 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -22,7 +22,7 @@ struct single_pdata { fdt_addr_t base; int offset; u32 mask; - int width; + u32 width; bool bits_per_mux; };
@@ -184,9 +184,13 @@ static int single_of_to_plat(struct udevice *dev) fdt_addr_t addr; fdt_size_t size; struct single_pdata *pdata = dev_get_plat(dev); + int ret;
- pdata->width = - dev_read_u32_default(dev, "pinctrl-single,register-width", 0); + ret = dev_read_u32(dev, "pinctrl-single,register-width", &pdata->width); + if (ret) { + dev_err(dev, "missing register width\n"); + return ret; + }
dev_read_addr_size(dev, "reg", &size); pdata->offset = size - pdata->width / BITS_PER_BYTE;
Hi Dario,
On Sat, 23 Jan 2021 at 11:27, Dario Binacchi dariobin@libero.it wrote:
In more recent versions of the Linux kernel the driver's probe function returns an error if the "pinctrl-single,register-width" DT property is missing. The lack of this information, in fact, does not allow to know whether to access the registers of the controller at 8, 16 or 32 bits.
"Update it to ..."
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Consider updating the pinctrl tests for this.
The patch is inspired by more recent versions of the Linux driver. Replacing the default value 0xffffffff of the function mask with 0 is certainly more conservative in case the "pinctrl-single,function-mask" DT property is missing.
Signed-off-by: Dario Binacchi dariobin@libero.it ---
drivers/pinctrl/pinctrl-single.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 8fd3bf66de..09bb883041 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -79,6 +79,10 @@ static int single_configure_pins(struct udevice *dev, phys_addr_t reg; u32 offset, val;
+ /* If function mask is null, needn't enable it. */ + if (!pdata->mask) + return 0; + for (n = 0; n < count; n++, pins++) { offset = fdt32_to_cpu(pins->reg); if (offset < 0 || offset > pdata->offset) { @@ -202,8 +206,12 @@ static int single_of_to_plat(struct udevice *dev) } pdata->base = addr;
- pdata->mask = dev_read_u32_default(dev, "pinctrl-single,function-mask", - 0xffffffff); + ret = dev_read_u32(dev, "pinctrl-single,function-mask", &pdata->mask); + if (ret) { + pdata->mask = 0; + dev_warn(dev, "missing function register mask\n"); + } + pdata->bits_per_mux = dev_read_bool(dev, "pinctrl-single,bit-per-mux");
return 0;
The patch allows you to call the read/write functions set during probing without having to check the type of access at runtime. It also adds functions for 8-bit registers access.
Signed-off-by: Dario Binacchi dariobin@libero.it ---
drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++-------- 1 file changed, 73 insertions(+), 25 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 09bb883041..eb69e53096 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -26,6 +26,16 @@ struct single_pdata { bool bits_per_mux; };
+/** + * struct single_priv - private data + * @read: function to read from a configuration register + * @write: function to write to a configuration register + */ +struct single_priv { + unsigned int (*read)(fdt_addr_t reg); + void (*write)(unsigned int val, fdt_addr_t reg); +}; + /** * struct single_fdt_pin_cfg - pin configuration * @@ -56,6 +66,36 @@ struct single_fdt_bits_cfg { fdt32_t mask; };
+static unsigned int single_readb(fdt_addr_t reg) +{ + return readb(reg); +} + +static unsigned int single_readw(fdt_addr_t reg) +{ + return readw(reg); +} + +static unsigned int single_readl(fdt_addr_t reg) +{ + return readl(reg); +} + +static void single_writeb(unsigned int val, fdt_addr_t reg) +{ + writeb(val, reg); +} + +static void single_writew(unsigned int val, fdt_addr_t reg) +{ + writew(val, reg); +} + +static void single_writel(unsigned int val, fdt_addr_t reg) +{ + writel(val, reg); +} + /** * single_configure_pins() - Configure pins based on FDT data * @@ -75,6 +115,7 @@ static int single_configure_pins(struct udevice *dev, int size) { struct single_pdata *pdata = dev_get_plat(dev); + struct single_priv *priv = dev_get_priv(dev); int n, count = size / sizeof(struct single_fdt_pin_cfg); phys_addr_t reg; u32 offset, val; @@ -93,19 +134,9 @@ static int single_configure_pins(struct udevice *dev,
reg = pdata->base + offset; val = fdt32_to_cpu(pins->val) & pdata->mask; - switch (pdata->width) { - case 16: - writew((readw(reg) & ~pdata->mask) | val, reg); - break; - case 32: - writel((readl(reg) & ~pdata->mask) | val, reg); - break; - default: - dev_warn(dev, "unsupported register width %i\n", - pdata->width); - continue; - } + priv->write((priv->read(reg) & ~pdata->mask) | val, reg); dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val); + } return 0; } @@ -115,6 +146,7 @@ static int single_configure_bits(struct udevice *dev, int size) { struct single_pdata *pdata = dev_get_plat(dev); + struct single_priv *priv = dev_get_priv(dev); int n, count = size / sizeof(struct single_fdt_bits_cfg); phys_addr_t reg; u32 offset, val, mask; @@ -131,19 +163,7 @@ static int single_configure_bits(struct udevice *dev,
mask = fdt32_to_cpu(pins->mask); val = fdt32_to_cpu(pins->val) & mask; - - switch (pdata->width) { - case 16: - writew((readw(reg) & ~mask) | val, reg); - break; - case 32: - writel((readl(reg) & ~mask) | val, reg); - break; - default: - dev_warn(dev, "unsupported register width %i\n", - pdata->width); - continue; - } + priv->write((priv->read(reg) & ~mask) | val, reg); dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val); } return 0; @@ -183,6 +203,32 @@ static int single_set_state(struct udevice *dev, return len; }
+static int single_probe(struct udevice *dev) +{ + struct single_pdata *pdata = dev_get_plat(dev); + struct single_priv *priv = dev_get_priv(dev); + + switch (pdata->width) { + case 8: + priv->read = single_readb; + priv->write = single_writeb; + break; + case 16: + priv->read = single_readw; + priv->write = single_writew; + break; + case 32: + priv->read = single_readl; + priv->write = single_writel; + break; + default: + dev_err(dev, "wrong register width\n"); + return -EINVAL; + } + + return 0; +} + static int single_of_to_plat(struct udevice *dev) { fdt_addr_t addr; @@ -232,5 +278,7 @@ U_BOOT_DRIVER(single_pinctrl) = { .of_match = single_pinctrl_match, .ops = &single_pinctrl_ops, .plat_auto = sizeof(struct single_pdata), + .priv_auto = sizeof(struct single_priv), .of_to_plat = single_of_to_plat, + .probe = single_probe, };
Hi Dario,
On Sat, 23 Jan 2021 at 11:27, Dario Binacchi dariobin@libero.it wrote:
The patch allows you to call the read/write functions set during probing without having to check the type of access at runtime. It also adds functions for 8-bit registers access.
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++-------- 1 file changed, 73 insertions(+), 25 deletions(-)
I'm not really keen on this. A switch() statement is not expensive and function pointers add indirection/confusion and use more space.
Regards, Simon
Hi Simon,
Il 24/01/2021 03:03 Simon Glass sjg@chromium.org ha scritto:
Hi Dario,
On Sat, 23 Jan 2021 at 11:27, Dario Binacchi dariobin@libero.it wrote:
The patch allows you to call the read/write functions set during probing without having to check the type of access at runtime. It also adds functions for 8-bit registers access.
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++-------- 1 file changed, 73 insertions(+), 25 deletions(-)
I'm not really keen on this. A switch() statement is not expensive and function pointers add indirection/confusion and use more space.
I think however it is better to create two static functions for reading/writing operations (with a switch() statement inside). So as not to replicate the code. Do you agree?
Regards, Dario
Regards, Simon
Hi Dario,
On Sun, 24 Jan 2021 at 09:50, Dario Binacchi dariobin@libero.it wrote:
Hi Simon,
Il 24/01/2021 03:03 Simon Glass sjg@chromium.org ha scritto:
Hi Dario,
On Sat, 23 Jan 2021 at 11:27, Dario Binacchi dariobin@libero.it wrote:
The patch allows you to call the read/write functions set during probing without having to check the type of access at runtime. It also adds functions for 8-bit registers access.
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++-------- 1 file changed, 73 insertions(+), 25 deletions(-)
I'm not really keen on this. A switch() statement is not expensive and function pointers add indirection/confusion and use more space.
I think however it is better to create two static functions for reading/writing operations (with a switch() statement inside). So as not to replicate the code. Do you agree?
Yes that sounds good to me.
Regards, Simon
It returns the number of selectable pins.
Signed-off-by: Dario Binacchi dariobin@libero.it ---
drivers/pinctrl/pinctrl-single.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index eb69e53096..21a3bbaaa7 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -28,10 +28,14 @@ struct single_pdata {
/** * struct single_priv - private data + * @bits_per_pin: number of bits per pin + * @npins: number of selectable pins * @read: function to read from a configuration register * @write: function to write to a configuration register */ struct single_priv { + unsigned int bits_per_pin; + unsigned int npins; unsigned int (*read)(fdt_addr_t reg); void (*write)(unsigned int val, fdt_addr_t reg); }; @@ -203,10 +207,27 @@ static int single_set_state(struct udevice *dev, return len; }
+static int single_get_pins_count(struct udevice *dev) +{ + struct single_priv *priv = dev_get_priv(dev); + + return priv->npins; +} + static int single_probe(struct udevice *dev) { struct single_pdata *pdata = dev_get_plat(dev); struct single_priv *priv = dev_get_priv(dev); + u32 size; + + size = pdata->offset + pdata->width / BITS_PER_BYTE; + priv->npins = size / (pdata->width / BITS_PER_BYTE); + if (pdata->bits_per_mux) { + priv->bits_per_pin = fls(pdata->mask); + priv->npins *= (pdata->width / priv->bits_per_pin); + } + + dev_dbg(dev, "%d pins\n", priv->npins);
switch (pdata->width) { case 8: @@ -264,6 +285,7 @@ static int single_of_to_plat(struct udevice *dev) }
const struct pinctrl_ops single_pinctrl_ops = { + .get_pins_count = single_get_pins_count, .set_state = single_set_state, };
On Sat, 23 Jan 2021 at 11:27, Dario Binacchi dariobin@libero.it wrote:
It returns the number of selectable pins.
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
It returns the name of the requested pin.
Signed-off-by: Dario Binacchi dariobin@libero.it ---
drivers/pinctrl/pinctrl-single.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 21a3bbaaa7..04e2b00f7e 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -30,12 +30,14 @@ struct single_pdata { * struct single_priv - private data * @bits_per_pin: number of bits per pin * @npins: number of selectable pins + * @pin_name: temporary buffer to store the pin name * @read: function to read from a configuration register * @write: function to write to a configuration register */ struct single_priv { unsigned int bits_per_pin; unsigned int npins; + char pin_name[PINNAME_SIZE]; unsigned int (*read)(fdt_addr_t reg); void (*write)(unsigned int val, fdt_addr_t reg); }; @@ -207,6 +209,19 @@ static int single_set_state(struct udevice *dev, return len; }
+static const char *single_get_pin_name(struct udevice *dev, + unsigned int selector) +{ + struct single_priv *priv = dev_get_priv(dev); + + if (selector >= priv->npins) + snprintf(priv->pin_name, PINNAME_SIZE, "Error"); + else + snprintf(priv->pin_name, PINNAME_SIZE, "PIN%u", selector); + + return priv->pin_name; +} + static int single_get_pins_count(struct udevice *dev) { struct single_priv *priv = dev_get_priv(dev); @@ -286,6 +301,7 @@ static int single_of_to_plat(struct udevice *dev)
const struct pinctrl_ops single_pinctrl_ops = { .get_pins_count = single_get_pins_count, + .get_pin_name = single_get_pin_name, .set_state = single_set_state, };
On Sat, 23 Jan 2021 at 11:27, Dario Binacchi dariobin@libero.it wrote:
It returns the name of the requested pin.
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
It allows to display the muxing of a given pin. Inspired by more recent versions of the Linux driver, in addition to the address and the value of the configuration register I added the pin function retrieved from the DT. In doing so, the information displayed does not depend on the platform, being a generic type driver, and it can be useful for debug purposes.
Signed-off-by: Dario Binacchi dariobin@libero.it ---
drivers/pinctrl/pinctrl-single.c | 220 +++++++++++++++++++++++++++++-- 1 file changed, 211 insertions(+), 9 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 04e2b00f7e..8db0d9e3d1 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -1,14 +1,18 @@ // SPDX-License-Identifier: GPL-2.0+ /* * Copyright (C) EETS GmbH, 2017, Felix Brack f.brack@eets.ch + * Copyright (C) 2021 Dario Binacchi dariobin@libero.it */
#include <common.h> #include <dm.h> #include <dm/device_compat.h> +#include <dm/devres.h> #include <dm/pinctrl.h> #include <linux/libfdt.h> +#include <linux/list.h> #include <asm/io.h> +#include <sort.h>
/** * struct single_pdata - platform data @@ -26,6 +30,20 @@ struct single_pdata { bool bits_per_mux; };
+/** + * struct single_func - pinctrl function + * @node: list node + * @name: pinctrl function name + * @npins: number of entries in pins array + * @pins: pins array + */ +struct single_func { + struct list_head node; + const char *name; + unsigned int npins; + unsigned int *pins; +}; + /** * struct single_priv - private data * @bits_per_pin: number of bits per pin @@ -38,6 +56,7 @@ struct single_priv { unsigned int bits_per_pin; unsigned int npins; char pin_name[PINNAME_SIZE]; + struct list_head functions; unsigned int (*read)(fdt_addr_t reg); void (*write)(unsigned int val, fdt_addr_t reg); }; @@ -102,6 +121,121 @@ static void single_writel(unsigned int val, fdt_addr_t reg) writel(val, reg); }
+/** + * single_get_pin_by_offset() - get a pin based on the register offset + * @dev: single driver instance + * @offset: register offset from the base + */ +static int single_get_pin_by_offset(struct udevice *dev, unsigned int offset) +{ + struct single_pdata *pdata = dev_get_plat(dev); + struct single_priv *priv = dev_get_priv(dev); + + if (offset > pdata->offset) { + dev_err(dev, "mux offset out of range: 0x%x (0x%x)\n", + offset, pdata->offset); + return -EINVAL; + } + + if (pdata->bits_per_mux) + return (offset * BITS_PER_BYTE) / priv->bits_per_pin; + + return offset / (pdata->width / BITS_PER_BYTE); +} + +static int single_get_offset_by_pin(struct udevice *dev, unsigned int pin) +{ + struct single_pdata *pdata = dev_get_plat(dev); + struct single_priv *priv = dev_get_priv(dev); + unsigned int mux_bytes; + + if (pin >= priv->npins) + return -EINVAL; + + mux_bytes = pdata->width / BITS_PER_BYTE; + if (pdata->bits_per_mux) { + int byte_num; + + byte_num = (priv->bits_per_pin * pin) / BITS_PER_BYTE; + return (byte_num / mux_bytes) * mux_bytes; + } + + return pin * mux_bytes; +} + +static const char *single_get_pin_function(struct udevice *dev, + unsigned int pin) +{ + struct single_priv *priv = dev_get_priv(dev); + struct single_func *func; + int i; + + list_for_each_entry(func, &priv->functions, node) { + for (i = 0; i < func->npins; i++) { + if (pin == func->pins[i]) + return func->name; + + if (pin < func->pins[i]) + break; + } + } + + return NULL; +} + +static int single_get_pin_muxing(struct udevice *dev, unsigned int pin, + char *buf, int size) +{ + struct single_pdata *pdata = dev_get_plat(dev); + struct single_priv *priv = dev_get_priv(dev); + fdt_addr_t reg; + const char *fname; + unsigned int val; + int offset, pin_shift = 0; + + offset = single_get_offset_by_pin(dev, pin); + if (offset < 0) + return offset; + + reg = pdata->base + offset; + val = priv->read(reg); + + if (pdata->bits_per_mux) + pin_shift = pin % (pdata->width / priv->bits_per_pin) * + priv->bits_per_pin; + + val &= (pdata->mask << pin_shift); + fname = single_get_pin_function(dev, pin); + snprintf(buf, size, "%pa 0x%08x %s", ®, val, + fname ? fname : "UNCLAIMED"); + return 0; +} + +static struct single_func *single_allocate_function(struct udevice *dev, + unsigned int group_pins) +{ + struct single_func *func; + + func = devm_kmalloc(dev, sizeof(*func), GFP_KERNEL); + if (!func) + return ERR_PTR(-ENOMEM); + + func->pins = devm_kmalloc(dev, sizeof(unsigned int) * group_pins, + GFP_KERNEL); + if (!func->pins) + return ERR_PTR(-ENOMEM); + + return func; +} + +static int single_pin_compare(const void *s1, const void *s2) +{ + int pin1 = *(const unsigned int *)s1; + int pin2 = *(const unsigned int *)s2; + + return pin1 - pin2; +} + /** * single_configure_pins() - Configure pins based on FDT data * @@ -115,14 +249,16 @@ static void single_writel(unsigned int val, fdt_addr_t reg) * @size: Size of the 'pins' array in bytes. * The number of register/value pairs in the 'pins' array therefore * equals to 'size / sizeof(struct single_fdt_pin_cfg)'. + * @fname: Function name. */ static int single_configure_pins(struct udevice *dev, const struct single_fdt_pin_cfg *pins, - int size) + int size, const char *fname) { struct single_pdata *pdata = dev_get_plat(dev); struct single_priv *priv = dev_get_priv(dev); - int n, count = size / sizeof(struct single_fdt_pin_cfg); + int n, pin, count = size / sizeof(struct single_fdt_pin_cfg); + struct single_func *func; phys_addr_t reg; u32 offset, val;
@@ -130,33 +266,60 @@ static int single_configure_pins(struct udevice *dev, if (!pdata->mask) return 0;
+ func = single_allocate_function(dev, count); + if (IS_ERR(func)) + return PTR_ERR(func); + + func->name = fname; + func->npins = 0; for (n = 0; n < count; n++, pins++) { offset = fdt32_to_cpu(pins->reg); if (offset < 0 || offset > pdata->offset) { - dev_dbg(dev, " invalid register offset 0x%x\n", + dev_err(dev, " invalid register offset 0x%x\n", offset); continue; }
reg = pdata->base + offset; val = fdt32_to_cpu(pins->val) & pdata->mask; + pin = single_get_pin_by_offset(dev, offset); + if (pin < 0) { + dev_err(dev, " failed to get pin by offset %x\n", + offset); + continue; + } + priv->write((priv->read(reg) & ~pdata->mask) | val, reg); dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val); - + func->pins[func->npins] = pin; + func->npins++; } + + qsort(func->pins, func->npins, sizeof(func->pins[0]), + single_pin_compare); + list_add(&func->node, &priv->functions); return 0; }
static int single_configure_bits(struct udevice *dev, const struct single_fdt_bits_cfg *pins, - int size) + int size, const char *fname) { struct single_pdata *pdata = dev_get_plat(dev); struct single_priv *priv = dev_get_priv(dev); - int n, count = size / sizeof(struct single_fdt_bits_cfg); + int n, pin, count = size / sizeof(struct single_fdt_bits_cfg); + int npins_in_reg, pin_num_from_lsb; + struct single_func *func; phys_addr_t reg; - u32 offset, val, mask; + u32 offset, val, mask, bit_pos, val_pos, mask_pos, submask;
+ npins_in_reg = pdata->width / priv->bits_per_pin; + func = single_allocate_function(dev, count * npins_in_reg); + if (IS_ERR(func)) + return PTR_ERR(func); + + func->name = fname; + func->npins = 0; for (n = 0; n < count; n++, pins++) { offset = fdt32_to_cpu(pins->reg); if (offset < 0 || offset > pdata->offset) { @@ -167,11 +330,47 @@ static int single_configure_bits(struct udevice *dev,
reg = pdata->base + offset;
+ pin = single_get_pin_by_offset(dev, offset); + if (pin < 0) { + dev_err(dev, " failed to get pin by offset 0x%pa\n", + ®); + continue; + } + mask = fdt32_to_cpu(pins->mask); val = fdt32_to_cpu(pins->val) & mask; priv->write((priv->read(reg) & ~mask) | val, reg); dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val); + + while (mask) { + bit_pos = __ffs(mask); + pin_num_from_lsb = bit_pos / priv->bits_per_pin; + mask_pos = pdata->mask << bit_pos; + val_pos = val & mask_pos; + submask = mask & mask_pos; + + if ((mask & mask_pos) == 0) { + dev_err(dev, "Invalid mask at 0x%x\n", offset); + break; + } + + mask &= ~mask_pos; + + if (submask != mask_pos) { + dev_warn(dev, + "Invalid submask 0x%x at 0x%x\n", + submask, offset); + continue; + } + + func->pins[func->npins] = pin + pin_num_from_lsb; + func->npins++; + } } + + qsort(func->pins, func->npins, sizeof(func->pins[0]), + single_pin_compare); + list_add(&func->node, &priv->functions); return 0; } static int single_set_state(struct udevice *dev, @@ -189,7 +388,7 @@ static int single_set_state(struct udevice *dev, dev_dbg(dev, " invalid pin configuration in fdt\n"); return -FDT_ERR_BADSTRUCTURE; } - single_configure_pins(dev, prop, len); + single_configure_pins(dev, prop, len, config->name); return 0; }
@@ -201,7 +400,7 @@ static int single_set_state(struct udevice *dev, dev_dbg(dev, " invalid bits configuration in fdt\n"); return -FDT_ERR_BADSTRUCTURE; } - single_configure_bits(dev, prop_bits, len); + single_configure_bits(dev, prop_bits, len, config->name); return 0; }
@@ -235,6 +434,8 @@ static int single_probe(struct udevice *dev) struct single_priv *priv = dev_get_priv(dev); u32 size;
+ INIT_LIST_HEAD(&priv->functions); + size = pdata->offset + pdata->width / BITS_PER_BYTE; priv->npins = size / (pdata->width / BITS_PER_BYTE); if (pdata->bits_per_mux) { @@ -303,6 +504,7 @@ const struct pinctrl_ops single_pinctrl_ops = { .get_pins_count = single_get_pins_count, .get_pin_name = single_get_pin_name, .set_state = single_set_state, + .get_pin_muxing = single_get_pin_muxing, };
static const struct udevice_id single_pinctrl_match[] = {
Hi Dario,
On Sat, 23 Jan 2021 at 11:27, Dario Binacchi dariobin@libero.it wrote:
It allows to display the muxing of a given pin. Inspired by more recent versions of the Linux driver, in addition to the address and the value of the configuration register I added the pin function retrieved from the DT. In doing so, the information displayed does not depend on the platform, being a generic type driver, and it can be useful for debug purposes.
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 220 +++++++++++++++++++++++++++++-- 1 file changed, 211 insertions(+), 9 deletions(-)
Do we need an updated DT binding file?
Regards, Simon
Hi Simon,
Il 24/01/2021 03:03 Simon Glass sjg@chromium.org ha scritto:
Hi Dario,
On Sat, 23 Jan 2021 at 11:27, Dario Binacchi dariobin@libero.it wrote:
It allows to display the muxing of a given pin. Inspired by more recent versions of the Linux driver, in addition to the address and the value of the configuration register I added the pin function retrieved from the DT. In doing so, the information displayed does not depend on the platform, being a generic type driver, and it can be useful for debug purposes.
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 220 +++++++++++++++++++++++++++++-- 1 file changed, 211 insertions(+), 9 deletions(-)
Do we need an updated DT binding file?
I would say no, the am335x DT has recently been resynced with one of the latest versions of the Linux kernel.
Regards, Simon
Regards Dario
On Tue, 26 Jan 2021 at 04:28, Dario Binacchi dariobin@libero.it wrote:
Hi Simon,
Il 24/01/2021 03:03 Simon Glass sjg@chromium.org ha scritto:
Hi Dario,
On Sat, 23 Jan 2021 at 11:27, Dario Binacchi dariobin@libero.it wrote:
It allows to display the muxing of a given pin. Inspired by more recent versions of the Linux driver, in addition to the address and the value of the configuration register I added the pin function retrieved from the DT. In doing so, the information displayed does not depend on the platform, being a generic type driver, and it can be useful for debug purposes.
Signed-off-by: Dario Binacchi dariobin@libero.it
drivers/pinctrl/pinctrl-single.c | 220 +++++++++++++++++++++++++++++-- 1 file changed, 211 insertions(+), 9 deletions(-)
Do we need an updated DT binding file?
I would say no, the am335x DT has recently been resynced with one of the latest versions of the Linux kernel.
Reviewed-by: Simon Glass sjg@chromium.org
The test adds two pinmux nodes to the device tree, one to test when a register changes only one pin's mux (pinctrl-single,pins), and the other to test when more than one pin's mux is changed (pinctrl-single,bits). This required replacing the controller's register access functions when the driver is used on sandbox.
Signed-off-by: Dario Binacchi dariobin@libero.it
---
arch/sandbox/dts/test.dts | 65 +++++++++++++++++++++++++ configs/sandbox_defconfig | 1 + drivers/pinctrl/pinctrl-single.c | 62 +++++++++++++++++++----- test/dm/pinmux.c | 81 ++++++++++++++++++++++++++++++-- 4 files changed, 193 insertions(+), 16 deletions(-)
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index f86cd0d3b2..e00a163641 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -513,6 +513,9 @@ reg = <0 1>; compatible = "sandbox,i2c"; clock-frequency = <100000>; + pinctrl-names = "default"; + pinctrl-0 = <&pinmux_i2c0_pins>; + eeprom@2c { reg = <0x2c>; compatible = "i2c-eeprom"; @@ -592,6 +595,8 @@ lcd { u-boot,dm-pre-reloc; compatible = "sandbox,lcd-sdl"; + pinctrl-names = "default"; + pinctrl-0 = <&pinmux_lcd_pins>; xres = <1366>; yres = <768>; }; @@ -842,6 +847,8 @@ pwm: pwm { compatible = "sandbox,pwm"; #pwm-cells = <2>; + pinctrl-names = "default"; + pinctrl-0 = <&pinmux_pwm_pins>; };
pwm2 { @@ -913,6 +920,9 @@ reg = <0 1>; compatible = "sandbox,spi"; cs-gpios = <0>, <0>, <&gpio_a 0>; + pinctrl-names = "default"; + pinctrl-0 = <&pinmux_spi0_pins>; + spi.bin@0 { reg = <0>; compatible = "spansion,m25p16", "jedec,spi-nor"; @@ -1002,6 +1012,8 @@ uart0: serial { compatible = "sandbox,serial"; u-boot,dm-pre-reloc; + pinctrl-names = "default"; + pinctrl-0 = <&pinmux_uart0_pins>; };
usb_0: usb@0 { @@ -1268,6 +1280,59 @@ }; };
+ pinctrl-single-pins { + compatible = "pinctrl-single"; + reg = <0x0000 0x238>; + #pinctrl-cells = <1>; + pinctrl-single,register-width = <32>; + pinctrl-single,function-mask = <0x7f>; + + pinmux_pwm_pins: pinmux_pwm_pins { + pinctrl-single,pins = < 0x48 0x06 >; + }; + + pinmux_spi0_pins: pinmux_spi0_pins { + pinctrl-single,pins = < + 0x190 0x0c + 0x194 0x0c + 0x198 0x23 + 0x19c 0x0c + >; + }; + + pinmux_uart0_pins: pinmux_uart0_pins { + pinctrl-single,pins = < + 0x70 0x30 + 0x74 0x00 + >; + }; + }; + + pinctrl-single-bits { + compatible = "pinctrl-single"; + reg = <0x0000 0x50>; + #pinctrl-cells = <2>; + pinctrl-single,bit-per-mux; + pinctrl-single,register-width = <32>; + pinctrl-single,function-mask = <0xf>; + + pinmux_i2c0_pins: pinmux_i2c0_pins { + pinctrl-single,bits = < + 0x10 0x00002200 0x0000ff00 + >; + }; + + pinmux_lcd_pins: pinmux_lcd_pins { + pinctrl-single,bits = < + 0x40 0x22222200 0xffffff00 + 0x44 0x22222222 0xffffffff + 0x48 0x00000022 0x000000ff + 0x48 0x02000000 0x0f000000 + 0x4c 0x02000022 0x0f0000ff + >; + }; + }; + hwspinlock@0 { compatible = "sandbox,hwspinlock"; }; diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 86dc603667..1c7d49f073 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -193,6 +193,7 @@ CONFIG_PHY_SANDBOX=y CONFIG_PINCTRL=y CONFIG_PINCONF=y CONFIG_PINCTRL_SANDBOX=y +CONFIG_PINCTRL_SINGLE=y CONFIG_POWER_DOMAIN=y CONFIG_SANDBOX_POWER_DOMAIN=y CONFIG_DM_PMIC=y diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 8db0d9e3d1..0efffd48e7 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -53,12 +53,15 @@ struct single_func { * @write: function to write to a configuration register */ struct single_priv { +#if (IS_ENABLED(CONFIG_SANDBOX)) + u32 *sandbox_regs; +#endif unsigned int bits_per_pin; unsigned int npins; char pin_name[PINNAME_SIZE]; struct list_head functions; - unsigned int (*read)(fdt_addr_t reg); - void (*write)(unsigned int val, fdt_addr_t reg); + unsigned int (*read)(struct udevice *dev, fdt_addr_t reg); + void (*write)(struct udevice *dev, unsigned int val, fdt_addr_t reg); };
/** @@ -91,36 +94,64 @@ struct single_fdt_bits_cfg { fdt32_t mask; };
-static unsigned int single_readb(fdt_addr_t reg) +#if (!IS_ENABLED(CONFIG_SANDBOX)) + +static unsigned int single_readb(struct udevice *dev, fdt_addr_t reg) { return readb(reg); }
-static unsigned int single_readw(fdt_addr_t reg) +static unsigned int single_readw(struct udevice *dev, fdt_addr_t reg) { return readw(reg); }
-static unsigned int single_readl(fdt_addr_t reg) +static unsigned int single_readl(struct udevice *dev, fdt_addr_t reg) { return readl(reg); }
-static void single_writeb(unsigned int val, fdt_addr_t reg) +static void single_writeb(struct udevice *dev, unsigned int val, fdt_addr_t reg) { writeb(val, reg); }
-static void single_writew(unsigned int val, fdt_addr_t reg) +static void single_writew(struct udevice *dev, unsigned int val, fdt_addr_t reg) { writew(val, reg); }
-static void single_writel(unsigned int val, fdt_addr_t reg) +static void single_writel(struct udevice *dev, unsigned int val, fdt_addr_t reg) { writel(val, reg); }
+#else /* CONFIG_SANDBOX */ + +#define single_readb single_sandbox_read +#define single_readw single_sandbox_read +#define single_readl single_sandbox_read +#define single_writeb single_sandbox_write +#define single_writew single_sandbox_write +#define single_writel single_sandbox_write + +static unsigned int single_sandbox_read(struct udevice *dev, fdt_addr_t reg) +{ + struct single_priv *priv = dev_get_priv(dev); + + return priv->sandbox_regs[reg]; +} + +static void single_sandbox_write(struct udevice *dev, unsigned int val, + fdt_addr_t reg) +{ + struct single_priv *priv = dev_get_priv(dev); + + priv->sandbox_regs[reg] = val; +} + +#endif /* CONFIG_SANDBOX */ + /** * single_get_pin_by_offset() - get a pin based on the register offset * @dev: single driver instance @@ -198,7 +229,7 @@ static int single_get_pin_muxing(struct udevice *dev, unsigned int pin, return offset;
reg = pdata->base + offset; - val = priv->read(reg); + val = priv->read(dev, reg);
if (pdata->bits_per_mux) pin_shift = pin % (pdata->width / priv->bits_per_pin) * @@ -289,7 +320,8 @@ static int single_configure_pins(struct udevice *dev, continue; }
- priv->write((priv->read(reg) & ~pdata->mask) | val, reg); + priv->write(dev, (priv->read(dev, reg) & ~pdata->mask) | val, + reg); dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val); func->pins[func->npins] = pin; func->npins++; @@ -339,7 +371,7 @@ static int single_configure_bits(struct udevice *dev,
mask = fdt32_to_cpu(pins->mask); val = fdt32_to_cpu(pins->val) & mask; - priv->write((priv->read(reg) & ~mask) | val, reg); + priv->write(dev, (priv->read(dev, reg) & ~mask) | val, reg); dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val);
while (mask) { @@ -437,6 +469,14 @@ static int single_probe(struct udevice *dev) INIT_LIST_HEAD(&priv->functions);
size = pdata->offset + pdata->width / BITS_PER_BYTE; + #if (CONFIG_IS_ENABLED(SANDBOX)) + priv->sandbox_regs = + devm_kzalloc(dev, size * sizeof(*priv->sandbox_regs), + GFP_KERNEL); + if (!priv->sandbox_regs) + return -ENOMEM; + #endif + priv->npins = size / (pdata->width / BITS_PER_BYTE); if (pdata->bits_per_mux) { priv->bits_per_pin = fls(pdata->mask); diff --git a/test/dm/pinmux.c b/test/dm/pinmux.c index 047184d4bc..db3f14bf7d 100644 --- a/test/dm/pinmux.c +++ b/test/dm/pinmux.c @@ -9,16 +9,21 @@ #include <dm/test.h> #include <test/ut.h>
-static int dm_test_pinmux(struct unit_test_state *uts) -{ - char buf[64]; - struct udevice *dev; - +static char buf[64]; #define test_muxing(selector, expected) do { \ ut_assertok(pinctrl_get_pin_muxing(dev, selector, buf, sizeof(buf))); \ ut_asserteq_str(expected, (char *)&buf); \ } while (0)
+#define test_name(selector, expected) do { \ + ut_assertok(pinctrl_get_pin_name(dev, selector, buf, sizeof(buf))); \ + ut_asserteq_str(expected, (char *)&buf); \ +} while (0) + +static int dm_test_pinmux(struct unit_test_state *uts) +{ + struct udevice *dev; + ut_assertok(uclass_get_device_by_name(UCLASS_PINCTRL, "pinctrl", &dev)); test_muxing(0, "UART TX."); test_muxing(1, "UART RX."); @@ -55,3 +60,69 @@ static int dm_test_pinmux(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_pinmux, UT_TESTF_SCAN_FDT); + +static int dm_test_pinctrl_single(struct unit_test_state *uts) +{ + struct udevice *dev; + + ut_assertok(uclass_get_device_by_name(UCLASS_PWM, "pwm", &dev)); + ut_assertok(uclass_get_device_by_name(UCLASS_SERIAL, "serial", &dev)); + ut_assertok(uclass_get_device_by_name(UCLASS_SPI, "spi@0", &dev)); + ut_assertok(uclass_get_device_by_name(UCLASS_PINCTRL, "pinctrl-single-pins", &dev)); + ut_asserteq(142, pinctrl_get_pins_count(dev)); + test_name(0, "PIN0"); + test_name(141, "PIN141"); + test_name(142, "Error"); + test_muxing(0, "0x00000000 0x00000000 UNCLAIMED"); + test_muxing(18, "0x00000048 0x00000006 pinmux_pwm_pins"); + test_muxing(28, "0x00000070 0x00000030 pinmux_uart0_pins"); + test_muxing(29, "0x00000074 0x00000000 pinmux_uart0_pins"); + test_muxing(100, "0x00000190 0x0000000c pinmux_spi0_pins"); + test_muxing(101, "0x00000194 0x0000000c pinmux_spi0_pins"); + test_muxing(102, "0x00000198 0x00000023 pinmux_spi0_pins"); + test_muxing(103, "0x0000019c 0x0000000c pinmux_spi0_pins"); + ut_asserteq(-EINVAL, pinctrl_get_pin_muxing(dev, 142, buf, sizeof(buf))); + ut_assertok(uclass_get_device_by_name(UCLASS_I2C, "i2c@0", &dev)); + ut_assertok(uclass_get_device_by_name(UCLASS_VIDEO, "lcd", &dev)); + ut_assertok(uclass_get_device_by_name(UCLASS_PINCTRL, "pinctrl-single-bits", &dev)); + ut_asserteq(160, pinctrl_get_pins_count(dev)); + test_name(0, "PIN0"); + test_name(159, "PIN159"); + test_name(160, "Error"); + test_muxing(0, "0x00000000 0x00000000 UNCLAIMED"); + test_muxing(34, "0x00000010 0x00000200 pinmux_i2c0_pins"); + test_muxing(35, "0x00000010 0x00002000 pinmux_i2c0_pins"); + test_muxing(130, "0x00000040 0x00000200 pinmux_lcd_pins"); + test_muxing(131, "0x00000040 0x00002000 pinmux_lcd_pins"); + test_muxing(132, "0x00000040 0x00020000 pinmux_lcd_pins"); + test_muxing(133, "0x00000040 0x00200000 pinmux_lcd_pins"); + test_muxing(134, "0x00000040 0x02000000 pinmux_lcd_pins"); + test_muxing(135, "0x00000040 0x20000000 pinmux_lcd_pins"); + test_muxing(136, "0x00000044 0x00000002 pinmux_lcd_pins"); + test_muxing(137, "0x00000044 0x00000020 pinmux_lcd_pins"); + test_muxing(138, "0x00000044 0x00000200 pinmux_lcd_pins"); + test_muxing(139, "0x00000044 0x00002000 pinmux_lcd_pins"); + test_muxing(140, "0x00000044 0x00020000 pinmux_lcd_pins"); + test_muxing(141, "0x00000044 0x00200000 pinmux_lcd_pins"); + test_muxing(142, "0x00000044 0x02000000 pinmux_lcd_pins"); + test_muxing(143, "0x00000044 0x20000000 pinmux_lcd_pins"); + test_muxing(144, "0x00000048 0x00000002 pinmux_lcd_pins"); + test_muxing(145, "0x00000048 0x00000020 pinmux_lcd_pins"); + test_muxing(146, "0x00000048 0x00000000 UNCLAIMED"); + test_muxing(147, "0x00000048 0x00000000 UNCLAIMED"); + test_muxing(148, "0x00000048 0x00000000 UNCLAIMED"); + test_muxing(149, "0x00000048 0x00000000 UNCLAIMED"); + test_muxing(150, "0x00000048 0x02000000 pinmux_lcd_pins"); + test_muxing(151, "0x00000048 0x00000000 UNCLAIMED"); + test_muxing(152, "0x0000004c 0x00000002 pinmux_lcd_pins"); + test_muxing(153, "0x0000004c 0x00000020 pinmux_lcd_pins"); + test_muxing(154, "0x0000004c 0x00000000 UNCLAIMED"); + test_muxing(155, "0x0000004c 0x00000000 UNCLAIMED"); + test_muxing(156, "0x0000004c 0x00000000 UNCLAIMED"); + test_muxing(157, "0x0000004c 0x00000000 UNCLAIMED"); + test_muxing(158, "0x0000004c 0x02000000 pinmux_lcd_pins"); + test_muxing(159, "0x0000004c 0x00000000 UNCLAIMED"); + ut_asserteq(-EINVAL, pinctrl_get_pin_muxing(dev, 160, buf, sizeof(buf))); + return 0; +} +DM_TEST(dm_test_pinctrl_single, UT_TESTF_SCAN_FDT);
On Sat, 23 Jan 2021 at 11:27, Dario Binacchi dariobin@libero.it wrote:
The test adds two pinmux nodes to the device tree, one to test when a register changes only one pin's mux (pinctrl-single,pins), and the other to test when more than one pin's mux is changed (pinctrl-single,bits). This required replacing the controller's register access functions when the driver is used on sandbox.
Signed-off-by: Dario Binacchi dariobin@libero.it
arch/sandbox/dts/test.dts | 65 +++++++++++++++++++++++++ configs/sandbox_defconfig | 1 + drivers/pinctrl/pinctrl-single.c | 62 +++++++++++++++++++----- test/dm/pinmux.c | 81 ++++++++++++++++++++++++++++++-- 4 files changed, 193 insertions(+), 16 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
participants (3)
-
Dario Binacchi -
Pratyush Yadav -
Simon Glass