| Message ID | 1433892211.12074.52.camel@codethink.co.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Simon Horman |
| Headers | show |
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),
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-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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).
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-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 --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),
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(-)