Re: [PATCH/RFC v4] ARM: shmobile: armadillo800eva-reference: add SDHI and MMCIF interfaces

Re: [PATCH/RFC v4] ARM: shmobile: armadillo800eva-reference: add SDHI and MMCIF interfaces

[Simon Horman]

[ Updated devicetree ML addres ]

On Wed, Sep 25, 2013 at 02:36:36PM +0900, Simon Horman wrote:
> On Mon, Sep 23, 2013 at 05:38:47PM +0200, Guennadi Liakhovetski wrote:
> > Add SDHI0, SDHI1 and MMCIF interfaces to armadillo800eva-reference with 
> > regulators and pin configurations.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> > 
> > v4:
> > 
> > 1. now that PFC pinctrl is usable with DT, we can use proper regulators 
> > and pin configurations on armadillo800eva
> > 
> > 2. corrected SDHI compatibility strings
> > 
> > 3. RFC because I don't know how to enable choosing between CON14 and CON8. 
> > In .c version this is done by reading GPIO 6. To do the same in DT mode 
> > we'd probably have to use some run-time DT patching, which isn't possible 
> > yet, AFAICS.
> 
> I agree with your reasoning there, though perhaps Laurent or Magnus
> have a more enlightened view of things.
> 
> To my way of thinking it would be good to merge this change as is,
> which an appropriate disclaimer about functionality in the changelog
> above the scissors. But I do not feel strongly about this and I
> am happy to wait for fuller functionality.
> 
> > 
> >  .../boot/dts/r8a7740-armadillo800eva-reference.dts |   83 ++++++++++++++++++++
> >  arch/arm/boot/dts/r8a7740.dtsi                     |   33 ++++++++
> >  2 files changed, 116 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/r8a7740-armadillo800eva-reference.dts b/arch/arm/boot/dts/r8a7740-armadillo800eva-reference.dts
> > index c638e4a..af15be0 100644
> > --- a/arch/arm/boot/dts/r8a7740-armadillo800eva-reference.dts
> > +++ b/arch/arm/boot/dts/r8a7740-armadillo800eva-reference.dts
> > @@ -34,6 +34,44 @@
> >  		regulator-boot-on;
> >  	};
> >  
> > +	vcc_sdhi0: regulator@1 {
> > +		compatible = "regulator-fixed";
> > +
> > +		regulator-name = "SDHI0 Vcc";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +
> > +		gpio = <&pfc 75 GPIO_ACTIVE_HIGH>;
> > +		enable-active-high;
> > +	};
> > +
> > +	vcc_sdhi1: regulator@2 {
> > +		compatible = "regulator-fixed";
> > +
> > +		regulator-name = "SDHI1 Vcc";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +
> > +		gpio = <&pfc 16 GPIO_ACTIVE_HIGH>;
> > +		enable-active-high;
> > +	};
> > +
> > +	vccq_sdhi0: regulator@3 {
> > +		compatible = "regulator-gpio";
> > +
> > +		regulator-name = "SDHI0 VccQ";
> > +		regulator-min-microvolt = <1800000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		vin-supply = <&vcc_sdhi0>;
> > +
> > +		enable-gpio = <&pfc 74 GPIO_ACTIVE_HIGH>;
> > +		gpios = <&pfc 17 GPIO_ACTIVE_HIGH>;
> > +		states = <3300000 0
> > +			  1800000 1>;
> > +
> > +		enable-active-high;
> > +	};
> > +
> >  	leds {
> >  		compatible = "gpio-leds";
> >  		led1 {
> > @@ -76,4 +114,49 @@
> >  		renesas,groups = "intc_irq10";
> >  		renesas,function = "intc";
> >  	};
> > +
> > +	mmc0_pins: mmc0 {
> > +		renesas,groups = "mmc0_data8_1", "mmc0_ctrl_1";
> > +		renesas,function = "mmc0";
> > +	};
> > +
> > +	sdhi0_pins: sdhi0 {
> > +		renesas,groups = "sdhi0_data4", "sdhi0_ctrl", "sdhi0_wp";
> > +		renesas,function = "sdhi0";
> > +	};
> > +
> > +	sdhi1_pins: sdhi1 {
> > +		renesas,groups = "sdhi1_data4", "sdhi1_ctrl", "sdhi1_cd", "sdhi1_wp";
> > +		renesas,function = "sdhi1";
> > +	};
> > +};
> > +
> > +&mmcif0 {
> > +	pinctrl-0 = <&mmc0_pins>;
> > +	pinctrl-names = "default";
> > +
> > +	vmmc-supply = <&reg_3p3v>;
> > +	bus-width = <8>;
> > +	non-removable;
> > +	status = "okay";
> > +};
> > +
> > +&sdhi0 {
> > +	pinctrl-0 = <&sdhi0_pins>;
> > +	pinctrl-names = "default";
> > +
> > +	vmmc-supply = <&vcc_sdhi0>;
> > +	vqmmc-supply = <&vccq_sdhi0>;
> > +	bus-width = <4>;
> > +	broken-cd;
> > +	status = "okay";
> > +};
> > +
> > +&sdhi1 {
> > +	pinctrl-0 = <&sdhi1_pins>;
> > +	pinctrl-names = "default";
> > +
> > +	vmmc-supply = <&vcc_sdhi1>;
> > +	bus-width = <4>;
> > +	status = "okay";
> >  };
> > diff --git a/arch/arm/boot/dts/r8a7740.dtsi b/arch/arm/boot/dts/r8a7740.dtsi
> > index 44d3d52..018c02d 100644
> > --- a/arch/arm/boot/dts/r8a7740.dtsi
> > +++ b/arch/arm/boot/dts/r8a7740.dtsi
> > @@ -159,4 +159,37 @@
> >  		status = "disabled";
> >  		#pwm-cells = <3>;
> >  	};
> > +
> > +	mmcif0: mmcif@e6bd0000 {
> > +		compatible = "renesas,sh-mmcif";
> > +		reg = <0xe6bd0000 0x100>;
> > +		interrupt-parent = <&gic>;
> > +		interrupts = <0 56 4
> > +				0 57 4>;
> > +		status = "disabled";
> > +	};
> > +
> > +	sdhi0: sdhi@e6850000 {
> > +		compatible = "renesas,sdhi-r8a7740";
> > +		reg = <0xe6850000 0x100>;
> > +		interrupt-parent = <&gic>;
> > +		interrupts = <0 117 4
> > +				0 118 4
> > +				0 119 4>;
> > +		cap-sd-highspeed;
> > +		cap-sdio-irq;
> > +		status = "disabled";
> > +	};
> > +
> > +	sdhi1: sdhi@e6860000 {
> > +		compatible = "renesas,sdhi-r8a7740";
> > +		reg = <0xe6860000 0x100>;
> > +		interrupt-parent = <&gic>;
> > +		interrupts = <0 121 4
> > +				0 122 4
> > +				0 123 4>;
> > +		cap-sd-highspeed;
> > +		cap-sdio-irq;
> > +		status = "disabled";
> > +	};
> >  };
> > -- 
> > 1.7.2.5
> > 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC v4] ARM: shmobile: armadillo800eva-reference: add SDHI and MMCIF interfaces

[Laurent Pinchart]

Hi Linus,

[Updated the devicetree mailing list e-mail address]

On Monday 30 September 2013 01:20:46 Linus Walleij wrote:
> On Sun, Sep 29, 2013 at 8:49 AM, Laurent Pinchart wrote:
> > On Friday 27 September 2013 16:08:31 Linus Walleij wrote:
> >> Can't you just locate the node using of_find_node_by_path()
> >> or whatever and then use this:
> >> int of_update_property(struct device_node *np, struct property *newprop)?
> > 
> > Where can I do that ? I need the PFC/GPIO devices to have been probed, so
> > board init isn't an option, but I also need the device to be enabled not
> > to have been probed yet.
> > 
> > One option would be to use a bus notifier to find out when the PFC/GPIO
> > devices are available, and extend of_update_property() with a notification
> > mechanism (either generic, or specific to the enabled propery) to detect
> > when a device gets enabled and probe it.
> 
> Argh that sounds awfully complicated...
> 
> I would consider doing this right inside the PCF/GPIO driver right at the
> end of its probe function. As it is related to pins anyway... or is that too
> ugly?

As a reminder, we need to decide whether to register an SDHI (SD controller) 
device or a WiFi module device, and thus configure pinmuxing appropriately, 
based on the state of a GPIO connected to a user-accessible switch. This 
behaviour is board-specific, not SoC-specific. That's why I believe the code 
should be in board code.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC v4] ARM: shmobile: armadillo800eva-reference: add SDHI and MMCIF interfaces

[Linus Walleij]

On Mon, Sep 30, 2013 at 1:10 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Linus,
>
> [Updated the devicetree mailing list e-mail address]
>
> On Monday 30 September 2013 01:20:46 Linus Walleij wrote:
>> On Sun, Sep 29, 2013 at 8:49 AM, Laurent Pinchart wrote:
>> > On Friday 27 September 2013 16:08:31 Linus Walleij wrote:
>> >> Can't you just locate the node using of_find_node_by_path()
>> >> or whatever and then use this:
>> >> int of_update_property(struct device_node *np, struct property *newprop)?
>> >
>> > Where can I do that ? I need the PFC/GPIO devices to have been probed, so
>> > board init isn't an option, but I also need the device to be enabled not
>> > to have been probed yet.
>> >
>> > One option would be to use a bus notifier to find out when the PFC/GPIO
>> > devices are available, and extend of_update_property() with a notification
>> > mechanism (either generic, or specific to the enabled propery) to detect
>> > when a device gets enabled and probe it.
>>
>> Argh that sounds awfully complicated...
>>
>> I would consider doing this right inside the PCF/GPIO driver right at the
>> end of its probe function. As it is related to pins anyway... or is that too
>> ugly?
>
> As a reminder, we need to decide whether to register an SDHI (SD controller)
> device or a WiFi module device, and thus configure pinmuxing appropriately,
> based on the state of a GPIO connected to a user-accessible switch. This
> behaviour is board-specific, not SoC-specific. That's why I believe the code
> should be in board code.

Hm, I might have misunderstood this earlier, so if the board code is
going to register *either* SDHI *or* WIFI, both as some kind of
struct device then as you say:

> The board has two connectors (MMC/SD 1
> and wifi module) that are not usable concurrently. The user can select which
> connector to use through a hardware switch that existing board code reads at
> init time (...)

So depending on what you read there you're going to register a
struct device named mmc0 or wifi0 or something, then you just register
the pin control tables for both cases and the apropriate default state
will be selected by the device core right before the device driver
gets probed. (grep for pinctrl_bind_pins). It is perfectly fine to have
unused pin states in the table.

Then the problem is unrelated to pin control and more about how
to register these devices, nothing to do with pin control.

Such as if they should both be marked as "disabled" in the DT and
activated by the kernel, or registered by adding a platform device
or something...

But I guess I got it wrong?

Yours,
Linus Walleij

Re: [PATCH/RFC v4] ARM: shmobile: armadillo800eva-reference: add SDHI and MMCIF interfaces

[Laurent Pinchart]

Hi Linus (and Guennadi and Grant, there are questions for your below),

On Tuesday 08 October 2013 13:19:04 Linus Walleij wrote:
> On Mon, Sep 30, 2013 at 1:10 PM, Laurent Pinchart wrote:
> > Hi Linus,
> > 
> > [Updated the devicetree mailing list e-mail address]
> > 
> > On Monday 30 September 2013 01:20:46 Linus Walleij wrote:
> >> On Sun, Sep 29, 2013 at 8:49 AM, Laurent Pinchart wrote:
> >> > On Friday 27 September 2013 16:08:31 Linus Walleij wrote:
> >> >> Can't you just locate the node using of_find_node_by_path()
> >> >> or whatever and then use this:
> >> >> int of_update_property(struct device_node *np, struct property
> >> >> *newprop)?
> >> > 
> >> > Where can I do that ? I need the PFC/GPIO devices to have been probed,
> >> > so board init isn't an option, but I also need the device to be enabled
> >> > not to have been probed yet.
> >> > 
> >> > One option would be to use a bus notifier to find out when the PFC/GPIO
> >> > devices are available, and extend of_update_property() with a
> >> > notification mechanism (either generic, or specific to the enabled
> >> > propery) to detect when a device gets enabled and probe it.
> >> 
> >> Argh that sounds awfully complicated...
> >> 
> >> I would consider doing this right inside the PCF/GPIO driver right at the
> >> end of its probe function. As it is related to pins anyway... or is that
> >> too ugly?
> > 
> > As a reminder, we need to decide whether to register an SDHI (SD
> > controller) device or a WiFi module device, and thus configure pinmuxing
> > appropriately, based on the state of a GPIO connected to a
> > user-accessible switch. This behaviour is board-specific, not
> > SoC-specific. That's why I believe the code should be in board code.
> 
> Hm, I might have misunderstood this earlier, so if the board code is
> going to register *either* SDHI *or* WIFI, both as some kind of
> struct device then as you say:
>
> > The board has two connectors (MMC/SD 1
> > and wifi module) that are not usable concurrently. The user can select
> > which connector to use through a hardware switch that existing board code
> > reads at init time (...)
> 
> So depending on what you read there you're going to register a struct device
> named mmc0 or wifi0 or something, then you just register the pin control
> tables for both cases and the apropriate default state will be selected by
> the device core right before the device driver gets probed. (grep for
> pinctrl_bind_pins). It is perfectly fine to have unused pin states in the
> table.
> 
> Then the problem is unrelated to pin control and more about how to register
> these devices, nothing to do with pin control.

Correct.

> Such as if they should both be marked as "disabled" in the DT and activated
> by the kernel, or registered by adding a platform device or something...

I believe a way to mark a disabled device as enabled from board code would be 
interesting. The function could be called from a platform bus notifier that 
reacts on the GPIO device being probed. It's a bit of a corner case, but it 
doesn't sound too hackish to me.

Grant, would you be fine with such a mechanism ? The devices would be marked 
with status = "disabled" in DT and a new function would be added to enable 
them. That function would modify the DT status to "okay" and trigger device 
registration.

> But I guess I got it wrong?

I believe you got it right. At least your understanding matches mine :-) 
Guennadi, could you please confirm ?

-- 
Regards,

Laurent Pinchart