Re: [U-Boot] [PATCH v2] Add 16-bit single register pin controller support

[James Balean]

Hi Felix,

On 7 April 2017 at 23:22, Felix Brack  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

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2] Add 16-bit single register pin controller support

[Felix Brack]

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
> 

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2] Add 16-bit single register pin controller support

[Felix Brack]

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 
> Cc: Felix Brack 
> Cc: Simon Glass 
> ---
> 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;
>   }
>   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;

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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2] Add 16-bit single register pin controller support

[James Balean]

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

[U-Boot] [PATCH v2] Add 16-bit single register pin controller support

[James Balean]

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 
Cc: Felix Brack 
Cc: Simon Glass 
---
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;
 }
 
-- 
2.7.4
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot