[U-Boot] [PATCH] Add 8 and 16-bit single register pin controller support
Enables the pinctrl-single driver to support 8 and 16-bit registers. Only 32-bit registers were supported previously.
Signed-off-by: James Balean james@balean.com.au Cc: Felix Brack fb@ltec.ch Cc: Simon Glass sjg@chromium.org --- drivers/pinctrl/pinctrl-single.c | 58 ++++++++++++++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 8 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index d2dcec0..63fec8d 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -24,6 +24,36 @@ struct single_fdt_pin_cfg { fdt32_t val; /* configuration register value */ };
+static fdt32_t pcs_readb(fdt32_t reg) +{ + return readb(reg); +} + +static fdt32_t pcs_readw(fdt32_t reg) +{ + return readw(reg); +} + +static fdt32_t pcs_readl(fdt32_t reg) +{ + return readl(reg); +} + +static void pcs_writeb(fdt32_t val, fdt32_t reg) +{ + writeb(val, reg); +} + +static void pcs_writew(fdt32_t val, fdt32_t reg) +{ + writew(val, reg); +} + +static void pcs_writel(fdt32_t val, fdt32_t reg) +{ + writel(val, reg); +} + /** * single_configure_pins() - Configure pins based on FDT data * @@ -46,28 +76,40 @@ static int single_configure_pins(struct udevice *dev, int count = size / sizeof(struct single_fdt_pin_cfg); int n, reg; u32 val; + fdt32_t (*pcs_read)(fdt32_t reg); + void (*pcs_write)(fdt32_t val, fdt32_t reg);
- for (n = 0; n < count; n++) { + 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%08x\n", reg); - pins++; continue; } reg += pdata->base; switch (pdata->width) { + case 8: + pcs_read = pcs_readb; + pcs_write = pcs_writeb; + break; + case 16: + pcs_read = pcs_readw; + pcs_write = pcs_writew; + break; case 32: - val = readl(reg) & ~pdata->mask; - val |= fdt32_to_cpu(pins->val) & pdata->mask; - writel(val, reg); - dev_dbg(dev, " reg/val 0x%08x/0x%08x\n", - reg, val); + pcs_read = pcs_readl; + pcs_write = pcs_writel; break; default: dev_warn(dev, "unsupported register width %i\n", pdata->width); + continue; } - pins++; + + val = pcs_read(reg) & ~pdata->mask; + val |= fdt32_to_cpu(pins->val) & pdata->mask; + pcs_write(val, reg); + dev_dbg(dev, " reg/val 0x%08x/0x%08x\n", + reg, val); } return 0; }
Hi James,
On 26 March 2017 at 23:55, James Balean james@balean.com.au wrote:
Enables the pinctrl-single driver to support 8 and 16-bit registers. Only 32-bit registers were supported previously.
Can you explain in your commit message why we want this?
Signed-off-by: James Balean james@balean.com.au Cc: Felix Brack fb@ltec.ch Cc: Simon Glass sjg@chromium.org
drivers/pinctrl/pinctrl-single.c | 58 ++++++++++++++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 8 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index d2dcec0..63fec8d 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -24,6 +24,36 @@ struct single_fdt_pin_cfg { fdt32_t val; /* configuration register value */ };
+static fdt32_t pcs_readb(fdt32_t reg)
I think ulong is better than fdt32_t, which is associated with devices tree.
+{
return readb(reg);+}
+static fdt32_t pcs_readw(fdt32_t reg) +{
return readw(reg);+}
+static fdt32_t pcs_readl(fdt32_t reg) +{
return readl(reg);+}
+static void pcs_writeb(fdt32_t val, fdt32_t reg) +{
writeb(val, reg);+}
+static void pcs_writew(fdt32_t val, fdt32_t reg) +{
writew(val, reg);+}
+static void pcs_writel(fdt32_t val, fdt32_t reg) +{
writel(val, reg);+}
Instead of lots of little functions, could you have:
pcs_read(ulong reg, int size) { switch (size) { case 8: return readb(reg); ...
?
/**
- single_configure_pins() - Configure pins based on FDT data
@@ -46,28 +76,40 @@ static int single_configure_pins(struct udevice *dev, int count = size / sizeof(struct single_fdt_pin_cfg); int n, reg; u32 val;
fdt32_t (*pcs_read)(fdt32_t reg);void (*pcs_write)(fdt32_t val, fdt32_t reg);
for (n = 0; n < count; n++) {
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%08x\n", reg);
pins++; continue; } reg += pdata->base; switch (pdata->width) {
case 8:pcs_read = pcs_readb;pcs_write = pcs_writeb;break;case 16:pcs_read = pcs_readw;pcs_write = pcs_writew;break; case 32:
val = readl(reg) & ~pdata->mask;val |= fdt32_to_cpu(pins->val) & pdata->mask;writel(val, reg);dev_dbg(dev, " reg/val 0x%08x/0x%08x\n",reg, val);
pcs_read = pcs_readl;pcs_write = pcs_writel; break; default: dev_warn(dev, "unsupported register width %i\n", pdata->width);continue; }
pins++;
val = pcs_read(reg) & ~pdata->mask;val |= fdt32_to_cpu(pins->val) & pdata->mask;pcs_write(val, reg);dev_dbg(dev, " reg/val 0x%08x/0x%08x\n",reg, val); } return 0;}
2.7.4
Regards, Simon
Hello James, hello Simon,
On 01.04.2017 06:22, Simon Glass wrote:
Hi James,
On 26 March 2017 at 23:55, James Balean james@balean.com.au wrote:
Enables the pinctrl-single driver to support 8 and 16-bit registers. Only 32-bit registers were supported previously.
Can you explain in your commit message why we want this?
Signed-off-by: James Balean james@balean.com.au Cc: Felix Brack fb@ltec.ch Cc: Simon Glass sjg@chromium.org
drivers/pinctrl/pinctrl-single.c | 58 ++++++++++++++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 8 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index d2dcec0..63fec8d 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -24,6 +24,36 @@ struct single_fdt_pin_cfg { fdt32_t val; /* configuration register value */ };
+static fdt32_t pcs_readb(fdt32_t reg)
I think ulong is better than fdt32_t, which is associated with devices tree.
+{
return readb(reg);+}
+static fdt32_t pcs_readw(fdt32_t reg) +{
return readw(reg);+}
+static fdt32_t pcs_readl(fdt32_t reg) +{
return readl(reg);+}
+static void pcs_writeb(fdt32_t val, fdt32_t reg) +{
writeb(val, reg);+}
+static void pcs_writew(fdt32_t val, fdt32_t reg) +{
writew(val, reg);+}
+static void pcs_writel(fdt32_t val, fdt32_t reg) +{
writel(val, reg);+}
Instead of lots of little functions, could you have:
pcs_read(ulong reg, int size) { switch (size) { case 8: return readb(reg); ...
?
I also prefer this. The corresponding switch is already there in 'single_configure_pins(..)', i.e. no need for an additional function. Using the existing function also eliminates the 'pcs_' prefix which I would have preferred to be 'single_' due to naming consistency (nitpicking, I admit).
/**
- single_configure_pins() - Configure pins based on FDT data
@@ -46,28 +76,40 @@ static int single_configure_pins(struct udevice *dev, int count = size / sizeof(struct single_fdt_pin_cfg); int n, reg; u32 val;
fdt32_t (*pcs_read)(fdt32_t reg);void (*pcs_write)(fdt32_t val, fdt32_t reg);
for (n = 0; n < count; n++) {
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%08x\n", reg);
pins++; continue; } reg += pdata->base; switch (pdata->width) {
case 8:pcs_read = pcs_readb;pcs_write = pcs_writeb;break;case 16:pcs_read = pcs_readw;pcs_write = pcs_writew;break; case 32:
val = readl(reg) & ~pdata->mask;val |= fdt32_to_cpu(pins->val) & pdata->mask;writel(val, reg);dev_dbg(dev, " reg/val 0x%08x/0x%08x\n",reg, val);
pcs_read = pcs_readl;pcs_write = pcs_writel; break; default: dev_warn(dev, "unsupported register width %i\n", pdata->width);continue; }
pins++;
val = pcs_read(reg) & ~pdata->mask;val |= fdt32_to_cpu(pins->val) & pdata->mask;pcs_write(val, reg);dev_dbg(dev, " reg/val 0x%08x/0x%08x\n",reg, val); } return 0;}
2.7.4
Regards, Simon
regards, Felix
Hi All,
Thank you for your responses. I will submit a new version of the patch with your suggestions following this.
On 1 April 2017 at 15:22, Simon Glass sjg@chromium.org wrote:
Can you explain in your commit message why we want this?
Will do. I will be seeking to add TI OMAP device tree support to U-Boot shortly, and 16-bit register widths are needed for pinmux configuration on these platforms. Don't have an immediate need for 8-bit though, so will remove this.
I think ulong is better than fdt32_t, which is associated with devices tree.
Concur. Thanks for this suggestion.
Instead of lots of little functions, could you have:
pcs_read(ulong reg, int size) { switch (size) { case 8: return readb(reg); ...
I tried to maintain consistency with mainline Linux with the separate read functions, however it makes sense to combine the little functions.
On 2 April 2017 at 20:53, Felix Brack fb@ltec.ch wrote:
I also prefer this. The corresponding switch is already there in 'single_configure_pins(..)', i.e. no need for an additional function. Using the existing function also eliminates the 'pcs_' prefix which I would have preferred to be 'single_' due to naming consistency (nitpicking, I admit).
Thanks. Yes, I will rename this to 'single_' for consistency with the other functions (the pcs_ prefix was for consistency with Linux's pinctrl-single driver, however it doesn't make sense to copy this convention).
Keen for your thoughts on v2 of the patch (which I'll submit shortly) as to me it seems cleaner to have the register read and write switches in two separate functions, rather than in the 'single_configure_pins(..)'.
Thanks again, James
Enables the pinctrl-single driver to support 16-bit registers. Only 32-bit registers were supported previously. Reduced width registers are required for some platforms, such as OMAP.
Signed-off-by: James Balean james@balean.com.au Cc: Felix Brack fb@ltec.ch Cc: Simon Glass sjg@chromium.org --- Changes for v2: - Added explanation of why this patch is needed. - Changed fdt32_t to ulong type. - Removed 8-bit support. - Now with a single read and write function, instead of one for each register width.
drivers/pinctrl/pinctrl-single.c | 45 ++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 15 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index d2dcec0..defb66f 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -24,6 +24,30 @@ struct single_fdt_pin_cfg { fdt32_t val; /* configuration register value */ };
+static ulong single_read(ulong reg, int width) { + switch (size) { + case 16: + return readw(reg); + case 32: + return readl(reg); + default: + dev_warn(dev, "unsupported register width %i\n", width); + } +} + +static void single_write(ulong val, ulong reg, int width) { + switch (width) { + case 16: + writew(val, reg); + break; + case 32: + writel(val, reg); + break; + default: + dev_warn(dev, "unsupported register width %i\n", width; + } +} + /** * single_configure_pins() - Configure pins based on FDT data * @@ -47,28 +71,19 @@ static int single_configure_pins(struct udevice *dev, int n, reg; u32 val;
- for (n = 0; n < count; n++) { + 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%08x\n", reg); - pins++; continue; } reg += pdata->base; - switch (pdata->width) { - case 32: - val = readl(reg) & ~pdata->mask; - val |= fdt32_to_cpu(pins->val) & pdata->mask; - writel(val, reg); - dev_dbg(dev, " reg/val 0x%08x/0x%08x\n", - reg, val); - break; - default: - dev_warn(dev, "unsupported register width %i\n", - pdata->width); - } - pins++; + val = single_read(reg, pdata->width) & ~pdata->mask; + val |= fdt32_to_cpu(pins->val) & pdata->mask; + single_write(val, reg, pdata->width); + dev_dbg(dev, " reg/val 0x%08x/0x%08x\n", reg, val); } + return 0; }
Apologies! Just noticed I submitted the untested patch update. I'll gather suggestions on this (note that 'size' in the first switch should be 'width' and there's a trailing bracket missing), then submit a (hopefully) final version.
James
Hello James,
Sorry I missed your followup. Please always send to 'everybody' and not just the list. You should though read my comment as there is some more work to be done.
regards Felix
On 07.04.2017 03:44, James Balean wrote:
Apologies! Just noticed I submitted the untested patch update. I'll gather suggestions on this (note that 'size' in the first switch should be 'width' and there's a trailing bracket missing), then submit a (hopefully) final version.
James
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Hi Felix,
On 7 April 2017 at 23:22, Felix Brack fb@ltec.ch wrote:
You should though read my comment as there is some more work to be done.
Thank you for your suggestions despite my incorrect submission. I now see and agree that it would be better to have the code inline in the `single_configure_pins()` function. Especially as it's only 16 and 32-bit register widths required at the moment.
A patch update will be submitted shortly.
-- Kind regards, James
Hello James,
This patch does not compile without errors.
On 06.04.2017 07:38, James Balean wrote:
Enables the pinctrl-single driver to support 16-bit registers. Only 32-bit registers were supported previously. Reduced width registers are required for some platforms, such as OMAP.
Signed-off-by: James Balean james@balean.com.au Cc: Felix Brack fb@ltec.ch Cc: Simon Glass sjg@chromium.org
Changes for v2:
- Added explanation of why this patch is needed.
- Changed fdt32_t to ulong type.
- Removed 8-bit support.
- Now with a single read and write function, instead of one for each register width.
drivers/pinctrl/pinctrl-single.c | 45 ++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 15 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index d2dcec0..defb66f 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -24,6 +24,30 @@ struct single_fdt_pin_cfg { fdt32_t val; /* configuration register value */ };
+static ulong single_read(ulong reg, int width) {
- switch (size) {
Use 'width' instead of 'size' here.
- case 16:
return readw(reg);- case 32:
return readl(reg);- default:
dev_warn(dev, "unsupported register width %i\n", width);
This function must return a value. What would you return here, i.e., in case of failure?
- }
+}
+static void single_write(ulong val, ulong reg, int width) {
- switch (width) {
- case 16:
writew(val, reg);break;- case 32:
writel(val, reg);break;- default:
dev_warn(dev, "unsupported register width %i\n", width;
Missing closing parentheses.
- }
+}
/**
- single_configure_pins() - Configure pins based on FDT data
@@ -47,28 +71,19 @@ static int single_configure_pins(struct udevice *dev, int n, reg; u32 val;
- for (n = 0; n < count; n++) {
- 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%08x\n", reg);
pins++; continue;switch (pdata->width) {case 32:val = readl(reg) & ~pdata->mask;val |= fdt32_to_cpu(pins->val) & pdata->mask;writel(val, reg);dev_dbg(dev, " reg/val 0x%08x/0x%08x\n",reg, val);break;default:dev_warn(dev, "unsupported register width %i\n",pdata->width);}pins++;
val = single_read(reg, pdata->width) & ~pdata->mask;
This is a no go as 'single_read' may fail (see above). You will have to check the return value. This is another reason for witch again I suggest you just keep the switch statement as it was.
val |= fdt32_to_cpu(pins->val) & pdata->mask;single_write(val, reg, pdata->width);dev_dbg(dev, " reg/val 0x%08x/0x%08x\n", reg, val);- return 0;
}
IMHO: You should make sure your patch is syntactically correct i.e. at least run a build cycle. Furthermore and specifically for this patch (as it directly deals with hardware registers) you should test it for '16 bit width' on your hardware.
regards Felix
participants (3)
-
Felix Brack -
James Balean -
Simon Glass