diff mbox

[v2,3/6] pinctrl: sh-pfc: r8a7790: Add separate functions for SDHI 1.8V operation

Message ID 1433892211.12074.52.camel@codethink.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Ben Hutchings June 9, 2015, 11:23 p.m. UTC
All the SHDIs can operate with either 3.3V or 1.8V signals, depending
on negotiation with the card.

Add separate functions for the 1.8V mode, and implement the set_mux
operation on all SDHI functions to configure the voltage for each
group of pins.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/pinctrl/sh-pfc/core.c        |  2 +-
 drivers/pinctrl/sh-pfc/core.h        |  1 +
 drivers/pinctrl/sh-pfc/pfc-r8a7790.c | 70 +++++++++++++++++++++++++++++++++---
 3 files changed, 68 insertions(+), 5 deletions(-)

Comments

Laurent Pinchart June 12, 2015, 7:18 a.m. UTC | #1
Hi Ben,

(CC'ing Linus Walleij)

Thank you for the patch.

On Wednesday 10 June 2015 00:23:31 Ben Hutchings wrote:
> All the SHDIs can operate with either 3.3V or 1.8V signals, depending
> on negotiation with the card.
> 
> Add separate functions for the 1.8V mode, and implement the set_mux
> operation on all SDHI functions to configure the voltage for each
> group of pins.

I don't think duplicating all functions for 1.8V and 3.3V is the right way to 
go. Can't we use the pinconf API instead, and in particular the 
PIN_CONFIG_POWER_SOURCE configuration ?

> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> ---
>  drivers/pinctrl/sh-pfc/core.c        |  2 +-
>  drivers/pinctrl/sh-pfc/core.h        |  1 +
>  drivers/pinctrl/sh-pfc/pfc-r8a7790.c | 70 ++++++++++++++++++++++++++++++---
>  3 files changed, 68 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
> index 7b2c9495c383..7d51f96afc9a 100644
> --- a/drivers/pinctrl/sh-pfc/core.c
> +++ b/drivers/pinctrl/sh-pfc/core.c
> @@ -92,7 +92,7 @@ static int sh_pfc_map_resources(struct sh_pfc *pfc,
>  	return 0;
>  }
> 
> -static void __iomem *sh_pfc_phys_to_virt(struct sh_pfc *pfc, u32 reg)
> +void __iomem *sh_pfc_phys_to_virt(struct sh_pfc *pfc, u32 reg)
>  {
>  	struct sh_pfc_window *window;
>  	phys_addr_t address = reg;
> diff --git a/drivers/pinctrl/sh-pfc/core.h b/drivers/pinctrl/sh-pfc/core.h
> index 6dc8a6fc2746..af355629c5d2 100644
> --- a/drivers/pinctrl/sh-pfc/core.h
> +++ b/drivers/pinctrl/sh-pfc/core.h
> @@ -57,6 +57,7 @@ int sh_pfc_unregister_gpiochip(struct sh_pfc *pfc);
>  int sh_pfc_register_pinctrl(struct sh_pfc *pfc);
>  int sh_pfc_unregister_pinctrl(struct sh_pfc *pfc);
> 
> +void __iomem *sh_pfc_phys_to_virt(struct sh_pfc *pfc, u32 address);
>  u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned int reg_width);
>  void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned int reg_width,
> u32 data);
> diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c index 22a5470889f5..baba3151687f
> 100644
> --- a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> +++ b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> @@ -4472,6 +4472,8 @@ static const char * const sdhi0_groups[] = {
>  	"sdhi0_wp",
>  };
> 
> +#define sdhi0_1v8_groups sdhi0_groups
> +
>  static const char * const sdhi1_groups[] = {
>  	"sdhi1_data1",
>  	"sdhi1_data4",
> @@ -4480,6 +4482,8 @@ static const char * const sdhi1_groups[] = {
>  	"sdhi1_wp",
>  };
> 
> +#define sdhi1_1v8_groups sdhi1_groups
> +
>  static const char * const sdhi2_groups[] = {
>  	"sdhi2_data1",
>  	"sdhi2_data4",
> @@ -4488,6 +4492,8 @@ static const char * const sdhi2_groups[] = {
>  	"sdhi2_wp",
>  };
> 
> +#define sdhi2_1v8_groups sdhi2_groups
> +
>  static const char * const sdhi3_groups[] = {
>  	"sdhi3_data1",
>  	"sdhi3_data4",
> @@ -4496,6 +4502,8 @@ static const char * const sdhi3_groups[] = {
>  	"sdhi3_wp",
>  };
> 
> +#define sdhi3_1v8_groups sdhi3_groups
> +
>  static const char * const ssi_groups[] = {
>  	"ssi0_data",
>  	"ssi0129_ctrl",
> @@ -4595,6 +4603,56 @@ static const char * const vin3_groups[] = {
>  	"vin3_clk",
>  };
> 
> +static void sdhi_set_voltage(struct sh_pfc *pfc,
> +			     const struct sh_pfc_function *func,
> +			     const struct sh_pfc_pin_group *grp)
> +{
> +	void __iomem *mapped_reg;
> +	u32 data, mask;
> +	int index;
> +
> +	if (WARN_ON(strncmp(func->name, "sdhi", 4)))
> +		return;
> +	if (WARN_ON(strncmp(func->name, grp->name, 5)))
> +		return;
> +
> +	/* Calculate mask in IOCTRL6 based on the group */
> +	index = func->name[4] - '0';
> +	if (WARN_ON(index < 0 || index > 3))
> +		return;
> +	if (!strcmp(grp->name + 5, "_data1"))
> +		mask = 0x20;
> +	else if (!strcmp(grp->name + 5, "_data4"))
> +		mask = 0x3c;
> +	else if (!strcmp(grp->name + 5, "_ctrl"))
> +		mask = 0xc0;
> +	else if (!strcmp(grp->name + 5, "_cd"))
> +		mask = 0x02;
> +	else if (!strcmp(grp->name + 5, "_wp"))
> +		mask = 0x01;
> +	else {
> +		WARN_ON(1);
> +		return;
> +	}
> +	mask <<= 8 * (3 - index);
> +
> +	/* Map IOCTRL6 */
> +	mapped_reg = sh_pfc_phys_to_virt(pfc, 0xe606008c);
> +
> +	data = sh_pfc_read_raw_reg(mapped_reg, 32);
> +
> +	/* Set for 3.3V (high) or 1.8V (low) depending on the function name */
> +	if (strcmp(func->name + 5, "_1v8"))
> +		data |= mask;
> +	else
> +		data &= ~mask;
> +
> +	sh_pfc_write_raw_reg(
> +		sh_pfc_phys_to_virt(pfc, pfc->info->unlock_reg), 32,
> +		~data);
> +	sh_pfc_write_raw_reg(mapped_reg, 32, data);
> +}
> +
>  static const struct sh_pfc_function pinmux_functions[] = {
>  	SH_PFC_FUNCTION(audio_clk),
>  	SH_PFC_FUNCTION(avb),
> @@ -4631,10 +4689,14 @@ static const struct sh_pfc_function
> pinmux_functions[] = { SH_PFC_FUNCTION(scifb0),
>  	SH_PFC_FUNCTION(scifb1),
>  	SH_PFC_FUNCTION(scifb2),
> -	SH_PFC_FUNCTION(sdhi0),
> -	SH_PFC_FUNCTION(sdhi1),
> -	SH_PFC_FUNCTION(sdhi2),
> -	SH_PFC_FUNCTION(sdhi3),
> +	SH_PFC_FUNCTION_SPECIAL(sdhi0, sdhi_set_voltage),
> +	SH_PFC_FUNCTION_SPECIAL(sdhi0_1v8, sdhi_set_voltage),
> +	SH_PFC_FUNCTION_SPECIAL(sdhi1, sdhi_set_voltage),
> +	SH_PFC_FUNCTION_SPECIAL(sdhi1_1v8, sdhi_set_voltage),
> +	SH_PFC_FUNCTION_SPECIAL(sdhi2, sdhi_set_voltage),
> +	SH_PFC_FUNCTION_SPECIAL(sdhi2_1v8, sdhi_set_voltage),
> +	SH_PFC_FUNCTION_SPECIAL(sdhi3, sdhi_set_voltage),
> +	SH_PFC_FUNCTION_SPECIAL(sdhi3_1v8, sdhi_set_voltage),
>  	SH_PFC_FUNCTION(ssi),
>  	SH_PFC_FUNCTION(tpu0),
>  	SH_PFC_FUNCTION(usb0),
Ben Hutchings June 12, 2015, 1:23 p.m. UTC | #2
On Fri, 2015-06-12 at 10:18 +0300, Laurent Pinchart wrote:
> Hi Ben,
> 
> (CC'ing Linus Walleij)
> 
> Thank you for the patch.
> 
> On Wednesday 10 June 2015 00:23:31 Ben Hutchings wrote:
> > All the SHDIs can operate with either 3.3V or 1.8V signals, depending
> > on negotiation with the card.
> > 
> > Add separate functions for the 1.8V mode, and implement the set_mux
> > operation on all SDHI functions to configure the voltage for each
> > group of pins.
> 
> I don't think duplicating all functions for 1.8V and 3.3V is the right way to 
> go. Can't we use the pinconf API instead, and in particular the 
> PIN_CONFIG_POWER_SOURCE configuration ?
[...]

So far as I could see, you can't use the pinconf API with pinmux groups.

Ben.


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart June 12, 2015, 7:07 p.m. UTC | #3
Hi Ben,

On Friday 12 June 2015 14:23:55 Ben Hutchings wrote:
> On Fri, 2015-06-12 at 10:18 +0300, Laurent Pinchart wrote:
> > Hi Ben,
> > 
> > (CC'ing Linus Walleij)
> > 
> > Thank you for the patch.
> > 
> > On Wednesday 10 June 2015 00:23:31 Ben Hutchings wrote:
> > > All the SHDIs can operate with either 3.3V or 1.8V signals, depending
> > > on negotiation with the card.
> > > 
> > > Add separate functions for the 1.8V mode, and implement the set_mux
> > > operation on all SDHI functions to configure the voltage for each
> > > group of pins.
> > 
> > I don't think duplicating all functions for 1.8V and 3.3V is the right way
> > to go. Can't we use the pinconf API instead, and in particular the
> > PIN_CONFIG_POWER_SOURCE configuration ?
> 
> [...]
> 
> So far as I could see, you can't use the pinconf API with pinmux groups.

Can't you ? Both Documentation/devicetree/bindings/pinctrl/pinctrl-
bindings.txt and Documentation/devicetree/bindings/pinctrl/renesas,pfc-
pinctrl.txt document the usage of pinconf with groups (see for instance 
example 3 in the latter).
Ben Hutchings June 15, 2015, 12:40 a.m. UTC | #4
On Fri, 2015-06-12 at 22:07 +0300, Laurent Pinchart wrote:
> Hi Ben,
> 
> On Friday 12 June 2015 14:23:55 Ben Hutchings wrote:
> > On Fri, 2015-06-12 at 10:18 +0300, Laurent Pinchart wrote:
> > > Hi Ben,
> > > 
> > > (CC'ing Linus Walleij)
> > > 
> > > Thank you for the patch.
> > > 
> > > On Wednesday 10 June 2015 00:23:31 Ben Hutchings wrote:
> > > > All the SHDIs can operate with either 3.3V or 1.8V signals, depending
> > > > on negotiation with the card.
> > > > 
> > > > Add separate functions for the 1.8V mode, and implement the set_mux
> > > > operation on all SDHI functions to configure the voltage for each
> > > > group of pins.
> > > 
> > > I don't think duplicating all functions for 1.8V and 3.3V is the right way
> > > to go. Can't we use the pinconf API instead, and in particular the
> > > PIN_CONFIG_POWER_SOURCE configuration ?
> > 
> > [...]
> > 
> > So far as I could see, you can't use the pinconf API with pinmux groups.
> 
> Can't you ? Both Documentation/devicetree/bindings/pinctrl/pinctrl-
> bindings.txt and Documentation/devicetree/bindings/pinctrl/renesas,pfc-
> pinctrl.txt document the usage of pinconf with groups (see for instance 
> example 3 in the latter).

Sorry, I should say you can't use them with pinmux *functions*.  You can
either bind a group to a function *or* set the various pinconf
properties for the group, but not both.  Unless I'm very much mistaken.

Ben.


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart June 15, 2015, 2:02 a.m. UTC | #5
Hi Ben,

On Monday 15 June 2015 01:40:40 Ben Hutchings wrote:
> On Fri, 2015-06-12 at 22:07 +0300, Laurent Pinchart wrote:
> > On Friday 12 June 2015 14:23:55 Ben Hutchings wrote:
> >> On Fri, 2015-06-12 at 10:18 +0300, Laurent Pinchart wrote:
> >>> On Wednesday 10 June 2015 00:23:31 Ben Hutchings wrote:
> >>>> All the SHDIs can operate with either 3.3V or 1.8V signals,
> >>>> depending on negotiation with the card.
> >>>> 
> >>>> Add separate functions for the 1.8V mode, and implement the set_mux
> >>>> operation on all SDHI functions to configure the voltage for each
> >>>> group of pins.
> >>> 
> >>> I don't think duplicating all functions for 1.8V and 3.3V is the right
> >>> way to go. Can't we use the pinconf API instead, and in particular the
> >>> PIN_CONFIG_POWER_SOURCE configuration ?
> >> 
> >> [...]
> >> 
> >> So far as I could see, you can't use the pinconf API with pinmux groups.
> > 
> > Can't you ? Both Documentation/devicetree/bindings/pinctrl/pinctrl-
> > bindings.txt and Documentation/devicetree/bindings/pinctrl/renesas,pfc-
> > pinctrl.txt document the usage of pinconf with groups (see for instance
> > example 3 in the latter).
> 
> Sorry, I should say you can't use them with pinmux *functions*.  You can
> either bind a group to a function *or* set the various pinconf
> properties for the group, but not both.  Unless I'm very much mistaken.

One of us probably is :-)

I'd have to recheck whether this is really a standard behaviour, but the PFC 
driver at least supports setting both functions and configuration as far as I 
can tell. The relevant example from the PFC bindings is

        mmcif_pins: mmcif {
                mux {
                        renesas,groups = "mmc0_data8_0", "mmc0_ctrl_0";
                        renesas,function = "mmc0";
                };
                cfg {
                        renesas,groups = "mmc0_data8_0";
                        renesas,pins = "PORT279";
                        bias-pull-up;
                };
        };

It's a bit more verbose than I'd like, but I think the following should work 
too.

        mmcif_pins: mmcif {
                renesas,groups = "mmc0_data8_0";
                renesas,function = "mmc0";
                bias-pull-up;
        };

(it requires all pins from the groups to use the same configuration though)

I believe the former is the intended way to set both function and config, 
while the latter is a PFC "extension".

On a side note, I have a patch to support the standard "groups", "functions" 
and "pins" properties instead of the Renesas-specific versions, I'll try to 
revive it.
Linus Walleij June 30, 2015, 6:05 a.m. UTC | #6
On Mon, Jun 15, 2015 at 4:02 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> On a side note, I have a patch to support the standard "groups", "functions"
> and "pins" properties instead of the Renesas-specific versions, I'll try to
> revive it.

Yes please :)

Makes things a little less confusing in this confusing boundary between
electronics and code...

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart June 30, 2015, 8:30 a.m. UTC | #7
On Tuesday 30 June 2015 08:05:34 Linus Walleij wrote:
> On Mon, Jun 15, 2015 at 4:02 AM, Laurent Pinchart
> 
> <laurent.pinchart@ideasonboard.com> wrote:
> > On a side note, I have a patch to support the standard "groups",
> > "functions" and "pins" properties instead of the Renesas-specific
> > versions, I'll try to revive it.
> 
> Yes please :)

Done :-)

> Makes things a little less confusing in this confusing boundary between
> electronics and code...
diff mbox

Patch

diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
index 7b2c9495c383..7d51f96afc9a 100644
--- a/drivers/pinctrl/sh-pfc/core.c
+++ b/drivers/pinctrl/sh-pfc/core.c
@@ -92,7 +92,7 @@  static int sh_pfc_map_resources(struct sh_pfc *pfc,
 	return 0;
 }
 
-static void __iomem *sh_pfc_phys_to_virt(struct sh_pfc *pfc, u32 reg)
+void __iomem *sh_pfc_phys_to_virt(struct sh_pfc *pfc, u32 reg)
 {
 	struct sh_pfc_window *window;
 	phys_addr_t address = reg;
diff --git a/drivers/pinctrl/sh-pfc/core.h b/drivers/pinctrl/sh-pfc/core.h
index 6dc8a6fc2746..af355629c5d2 100644
--- a/drivers/pinctrl/sh-pfc/core.h
+++ b/drivers/pinctrl/sh-pfc/core.h
@@ -57,6 +57,7 @@  int sh_pfc_unregister_gpiochip(struct sh_pfc *pfc);
 int sh_pfc_register_pinctrl(struct sh_pfc *pfc);
 int sh_pfc_unregister_pinctrl(struct sh_pfc *pfc);
 
+void __iomem *sh_pfc_phys_to_virt(struct sh_pfc *pfc, u32 address);
 u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned int reg_width);
 void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned int reg_width,
 			  u32 data);
diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
index 22a5470889f5..baba3151687f 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
@@ -4472,6 +4472,8 @@  static const char * const sdhi0_groups[] = {
 	"sdhi0_wp",
 };
 
+#define sdhi0_1v8_groups sdhi0_groups
+
 static const char * const sdhi1_groups[] = {
 	"sdhi1_data1",
 	"sdhi1_data4",
@@ -4480,6 +4482,8 @@  static const char * const sdhi1_groups[] = {
 	"sdhi1_wp",
 };
 
+#define sdhi1_1v8_groups sdhi1_groups
+
 static const char * const sdhi2_groups[] = {
 	"sdhi2_data1",
 	"sdhi2_data4",
@@ -4488,6 +4492,8 @@  static const char * const sdhi2_groups[] = {
 	"sdhi2_wp",
 };
 
+#define sdhi2_1v8_groups sdhi2_groups
+
 static const char * const sdhi3_groups[] = {
 	"sdhi3_data1",
 	"sdhi3_data4",
@@ -4496,6 +4502,8 @@  static const char * const sdhi3_groups[] = {
 	"sdhi3_wp",
 };
 
+#define sdhi3_1v8_groups sdhi3_groups
+
 static const char * const ssi_groups[] = {
 	"ssi0_data",
 	"ssi0129_ctrl",
@@ -4595,6 +4603,56 @@  static const char * const vin3_groups[] = {
 	"vin3_clk",
 };
 
+static void sdhi_set_voltage(struct sh_pfc *pfc,
+			     const struct sh_pfc_function *func,
+			     const struct sh_pfc_pin_group *grp)
+{
+	void __iomem *mapped_reg;
+	u32 data, mask;
+	int index;
+
+	if (WARN_ON(strncmp(func->name, "sdhi", 4)))
+		return;
+	if (WARN_ON(strncmp(func->name, grp->name, 5)))
+		return;
+
+	/* Calculate mask in IOCTRL6 based on the group */
+	index = func->name[4] - '0';
+	if (WARN_ON(index < 0 || index > 3))
+		return;
+	if (!strcmp(grp->name + 5, "_data1"))
+		mask = 0x20;
+	else if (!strcmp(grp->name + 5, "_data4"))
+		mask = 0x3c;
+	else if (!strcmp(grp->name + 5, "_ctrl"))
+		mask = 0xc0;
+	else if (!strcmp(grp->name + 5, "_cd"))
+		mask = 0x02;
+	else if (!strcmp(grp->name + 5, "_wp"))
+		mask = 0x01;
+	else {
+		WARN_ON(1);
+		return;
+	}
+	mask <<= 8 * (3 - index);
+
+	/* Map IOCTRL6 */
+	mapped_reg = sh_pfc_phys_to_virt(pfc, 0xe606008c);
+
+	data = sh_pfc_read_raw_reg(mapped_reg, 32);
+
+	/* Set for 3.3V (high) or 1.8V (low) depending on the function name */
+	if (strcmp(func->name + 5, "_1v8"))
+		data |= mask;
+	else
+		data &= ~mask;
+
+	sh_pfc_write_raw_reg(
+		sh_pfc_phys_to_virt(pfc, pfc->info->unlock_reg), 32,
+		~data);
+	sh_pfc_write_raw_reg(mapped_reg, 32, data);
+}
+
 static const struct sh_pfc_function pinmux_functions[] = {
 	SH_PFC_FUNCTION(audio_clk),
 	SH_PFC_FUNCTION(avb),
@@ -4631,10 +4689,14 @@  static const struct sh_pfc_function pinmux_functions[] = {
 	SH_PFC_FUNCTION(scifb0),
 	SH_PFC_FUNCTION(scifb1),
 	SH_PFC_FUNCTION(scifb2),
-	SH_PFC_FUNCTION(sdhi0),
-	SH_PFC_FUNCTION(sdhi1),
-	SH_PFC_FUNCTION(sdhi2),
-	SH_PFC_FUNCTION(sdhi3),
+	SH_PFC_FUNCTION_SPECIAL(sdhi0, sdhi_set_voltage),
+	SH_PFC_FUNCTION_SPECIAL(sdhi0_1v8, sdhi_set_voltage),
+	SH_PFC_FUNCTION_SPECIAL(sdhi1, sdhi_set_voltage),
+	SH_PFC_FUNCTION_SPECIAL(sdhi1_1v8, sdhi_set_voltage),
+	SH_PFC_FUNCTION_SPECIAL(sdhi2, sdhi_set_voltage),
+	SH_PFC_FUNCTION_SPECIAL(sdhi2_1v8, sdhi_set_voltage),
+	SH_PFC_FUNCTION_SPECIAL(sdhi3, sdhi_set_voltage),
+	SH_PFC_FUNCTION_SPECIAL(sdhi3_1v8, sdhi_set_voltage),
 	SH_PFC_FUNCTION(ssi),
 	SH_PFC_FUNCTION(tpu0),
 	SH_PFC_FUNCTION(usb0),