| Message ID | 1387058295-20641-3-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Hans, On Sat, Dec 14, 2013 at 10:58:12PM +0100, Hans de Goede wrote: > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Commit-log :) > --- > arch/arm/boot/dts/sun4i-a10-cubieboard.dts | 16 ++++++++++++++++ > arch/arm/boot/dts/sun4i-a10.dtsi | 16 ++++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts > index 425a7db..d193937 100644 > --- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts > +++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts > @@ -42,7 +42,23 @@ > }; > }; > > + sdc0: sdc@01c0f000 { > + pinctrl-names = "default"; > + pinctrl-0 = <&sdc0_pins_a>; > + pinctrl-1 = <&mmc0_cd_pin_cubieboard>; > + cd-gpios = <&pio 7 1 0>; /* PH1 */ > + cd-mode = <1>; > + status = "okay"; > + }; > + > pinctrl@01c20800 { > + mmc0_cd_pin_cubieboard: mmc0_cd_pin@0 { > + allwinner,pins = "PH1"; > + allwinner,function = "gpio_in"; > + allwinner,drive = <0>; > + allwinner,pull = <0>; > + }; > + You seem to be using mmc and sdc without any real distinction. Please try to be consistent and use only one of the two (and I have a slight preference for mmc :)) > led_pins_cubieboard: led_pins@0 { > allwinner,pins = "PH20", "PH21"; > allwinner,function = "gpio_out"; > diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi > index 4dccdb0..13bccd5 100644 > --- a/arch/arm/boot/dts/sun4i-a10.dtsi > +++ b/arch/arm/boot/dts/sun4i-a10.dtsi > @@ -306,6 +306,15 @@ > #size-cells = <0>; > }; > > + sdc0: sdc@01c0f000 { > + compatible = "allwinner,sun4i-mmc"; > + reg = <0x01c0f000 0x1000>; > + clocks = <&ahb_gates 8>, <&mmc0>; > + interrupts = <32>; > + bus-width = <4>; > + status = "disabled"; > + }; > + The A10 has much more mmc controller doesn't it? Could you add all of them in the DT ? > intc: interrupt-controller@01c20400 { > compatible = "allwinner,sun4i-ic"; > reg = <0x01c20400 0x400>; > @@ -376,6 +385,13 @@ > allwinner,drive = <0>; > allwinner,pull = <0>; > }; > + > + sdc0_pins_a: sdc0@0 { > + allwinner,pins = "PF0","PF1","PF2","PF3","PF4","PF5"; > + allwinner,function = "mmc0"; > + allwinner,drive = <3>; > + allwinner,pull = <1>; Wow, you need both the pullups and a 40mA output? Ideally, I'd like this patch to be splitted into three: - One that adds the MMC controller nodes to the DTSI - One that adds the muxing options you need to the pinctrl node - One that enables the controller on the boards Thanks, Maxime
Hi, On 12/15/2013 02:58 PM, Maxime Ripard wrote: > Hi Hans, > > On Sat, Dec 14, 2013 at 10:58:12PM +0100, Hans de Goede wrote: >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Commit-log :) Heh. Note as said before I expect David to take things from here. > >> --- >> arch/arm/boot/dts/sun4i-a10-cubieboard.dts | 16 ++++++++++++++++ >> arch/arm/boot/dts/sun4i-a10.dtsi | 16 ++++++++++++++++ >> 2 files changed, 32 insertions(+) >> >> diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts >> index 425a7db..d193937 100644 >> --- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts >> +++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts >> @@ -42,7 +42,23 @@ >> }; >> }; >> >> + sdc0: sdc@01c0f000 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&sdc0_pins_a>; >> + pinctrl-1 = <&mmc0_cd_pin_cubieboard>; >> + cd-gpios = <&pio 7 1 0>; /* PH1 */ >> + cd-mode = <1>; >> + status = "okay"; >> + }; >> + >> pinctrl@01c20800 { >> + mmc0_cd_pin_cubieboard: mmc0_cd_pin@0 { >> + allwinner,pins = "PH1"; >> + allwinner,function = "gpio_in"; >> + allwinner,drive = <0>; >> + allwinner,pull = <0>; >> + }; >> + > > You seem to be using mmc and sdc without any real distinction. Please > try to be consistent and use only one of the two (and I have a slight > preference for mmc :)) I just took what David was using. I agree picking one and staying with it would be better. > >> led_pins_cubieboard: led_pins@0 { >> allwinner,pins = "PH20", "PH21"; >> allwinner,function = "gpio_out"; >> diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi >> index 4dccdb0..13bccd5 100644 >> --- a/arch/arm/boot/dts/sun4i-a10.dtsi >> +++ b/arch/arm/boot/dts/sun4i-a10.dtsi >> @@ -306,6 +306,15 @@ >> #size-cells = <0>; >> }; >> >> + sdc0: sdc@01c0f000 { >> + compatible = "allwinner,sun4i-mmc"; >> + reg = <0x01c0f000 0x1000>; >> + clocks = <&ahb_gates 8>, <&mmc0>; >> + interrupts = <32>; >> + bus-width = <4>; >> + status = "disabled"; >> + }; >> + > > The A10 has much more mmc controller doesn't it? Yes 4 of them. > Could you add all of them in the DT ? > >> intc: interrupt-controller@01c20400 { >> compatible = "allwinner,sun4i-ic"; >> reg = <0x01c20400 0x400>; >> @@ -376,6 +385,13 @@ >> allwinner,drive = <0>; >> allwinner,pull = <0>; >> }; >> + >> + sdc0_pins_a: sdc0@0 { >> + allwinner,pins = "PF0","PF1","PF2","PF3","PF4","PF5"; >> + allwinner,function = "mmc0"; >> + allwinner,drive = <3>; >> + allwinner,pull = <1>; > > Wow, you need both the pullups and a 40mA output? Yes and no. The pullups are enabled by uboot and the allwinner sources, most of my testing has been done without them, which seems to work fine. I've enabled them to be consisten with the allwinner sources and u-boot. As for drive = <3>, we need drive = <2> for normal modes, and drive = <3> for ddr mode. I simply picked <3> to keep things KISS. It should not matter much in power usage, as it will only make the flanks of the signal more steep. Once the desired output level is reached the current will drop off. It will use more current when changing the level, but for half the time, so the effective power usage (current * time) is the same. > Ideally, I'd like this patch to be splitted into three: > - One that adds the MMC controller nodes to the DTSI > - One that adds the muxing options you need to the pinctrl node > - One that enables the controller on the boards Sounds like a good job for David. Note I've a lot more boards for which I would like to add mmc support or dts files in general (will do so as time permits). For those boards which already have a dts I'll send mmc adding dts patches to David for now so he can add the changes to the patch-set. How do you want to deal with new boards ? Send the addition of the base board to you (and CC David as he will need them in his tree too), and then send a patch to add mmc to the dts to David ? Or should I simply make it one big patch including mmc in the initial commit ? Regards, Hans
sön 2013-12-15 klockan 15:31 +0100 skrev Hans de Goede: > Yes and no. The pullups are enabled by uboot and the allwinner > sources, most of my testing has been done without them, which > seems to work fine. I've enabled them to be consisten with the > allwinner sources and u-boot. Many boards have external pullups where needed for the MMC bus. On such boards it's likely not a good idea to enable internal pullups. Regards Henrik
Hi > Note as said before I expect David to take things from here. I will start working on it this evening. > I just took what David was using. I agree picking one and staying with > it would be better. There is a reason why my working branch was called "plsdsif" (short for "Please don't shout at me in Finnish") > Yes and no. The pullups are enabled by uboot and the allwinner > sources, most of my testing has been done without them, which > seems to work fine. I've enabled them to be consisten with the > allwinner sources and u-boot. Ok > As for drive = <3>, we need drive = <2> for normal modes, > and drive = <3> for ddr mode. I simply picked <3> to keep > things KISS. It should not matter much in power usage, as > it will only make the flanks of the signal more steep. Once the > desired output level is reached the current will drop off. It will > use more current when changing the level, but for half the time, > so the effective power usage (current * time) is the same. In this case lets implement a switch which enables the pull ups for DDR mode and lets disable it by default in turn. This way we can make sure that we don't have doubled pull up, since this could actually mess up some circuitry. > > Ideally, I'd like this patch to be splitted into three: > > - One that adds the MMC controller nodes to the DTSI > > - One that adds the muxing options you need to the pinctrl node > > - One that enables the controller on the boards > Sounds like a good job for David. Note I've a lot more boards for > which I would like to add mmc support or dts files in general > (will do so as time permits). Ok. I will work on it this evening. > For those boards which already have a dts I'll send mmc adding dts > patches to David for now so he can add the changes to the patch-set. Ok. I will create another branch and will incorporate your DTS files. > How do you want to deal with new boards ? Send the addition of the > base board to you (and CC David as he will need them in his tree too), > and then send a patch to add mmc to the dts to David ? Sounds good to me... > Or should I simply make it one big patch including mmc in the initial > commit ? I imagine that a messy approach. Especially if I'm supposed to refactor the patchset into a new one... cheers
Hi Hans, Damn google-groups removed me from Cc :) On Sun, Dec 15, 2013 at 03:31:29PM +0100, Hans de Goede wrote: > >>@@ -376,6 +385,13 @@ > >> allwinner,drive = <0>; > >> allwinner,pull = <0>; > >> }; > >>+ > >>+ sdc0_pins_a: sdc0@0 { > >>+ allwinner,pins = "PF0","PF1","PF2","PF3","PF4","PF5"; > >>+ allwinner,function = "mmc0"; > >>+ allwinner,drive = <3>; > >>+ allwinner,pull = <1>; > > > >Wow, you need both the pullups and a 40mA output? > > Yes and no. The pullups are enabled by uboot and the allwinner > sources, most of my testing has been done without them, which > seems to work fine. I've enabled them to be consisten with the > allwinner sources and u-boot. Henrik was suggesting that in most boards it's not required to set up the internal pullups. Maybe we can just disable them by default, and we will always be able to enable them at the board level if needed. > As for drive = <3>, we need drive = <2> for normal modes, > and drive = <3> for ddr mode. I simply picked <3> to keep > things KISS. It should not matter much in power usage, as > it will only make the flanks of the signal more steep. Once the > desired output level is reached the current will drop off. It will > use more current when changing the level, but for half the time, > so the effective power usage (current * time) is the same. Ok, let's keep it to 40mA then. It will indeed be simpler :) > >Ideally, I'd like this patch to be splitted into three: > > - One that adds the MMC controller nodes to the DTSI > > - One that adds the muxing options you need to the pinctrl node > > - One that enables the controller on the boards > > Sounds like a good job for David. Note I've a lot more boards for > which I would like to add mmc support or dts files in general > (will do so as time permits). > > For those boards which already have a dts I'll send mmc adding dts > patches to David for now so he can add the changes to the patch-set. > > How do you want to deal with new boards ? Send the addition of the > base board to you (and CC David as he will need them in his tree too), > and then send a patch to add mmc to the dts to David ? I don't really like having a variable-sweep patch serie. Just send your DT additions to me, saying that you depend on the MMC serie from David. I'm pretty happy with the DT bindings so far, so we can even merge these DT patches before the MMC driver has been merged > Or should I simply make it one big patch including mmc in the initial > commit ? I really prefer to have patches as small as possible (while being reasonable). It's much easier to merge and allows to have a cleaner history. Maxime
Hi, On 12/16/2013 10:04 AM, David Lanzendörfer wrote: <snip> >> Yes and no. The pullups are enabled by uboot and the allwinner >> sources, most of my testing has been done without them, which >> seems to work fine. I've enabled them to be consisten with the >> allwinner sources and u-boot. > Ok > >> As for drive = <3>, we need drive = <2> for normal modes, >> and drive = <3> for ddr mode. I simply picked <3> to keep >> things KISS. It should not matter much in power usage, as >> it will only make the flanks of the signal more steep. Once the >> desired output level is reached the current will drop off. It will >> use more current when changing the level, but for half the time, >> so the effective power usage (current * time) is the same. > In this case lets implement a switch which enables the pull ups for DDR > mode and lets disable it by default in turn. I assume you mean enable drive = <3> mode and use drive = <2> mode by default? Changing drive level at runtime, rather the at dts configure time seems to be non trivial. Also see Maxime's mail further down in this thread where he says he is fine with keeping drive level at <3> but suggest disabling the pull-ups by default. I'm fine with disabling the pull-ups by default, so lets disable the pull-ups and keep the drive level at 3. > This way we can make sure that we don't have doubled pull up, > since this could actually mess up some circuitry. > >>> Ideally, I'd like this patch to be splitted into three: >>> - One that adds the MMC controller nodes to the DTSI >>> - One that adds the muxing options you need to the pinctrl node >>> - One that enables the controller on the boards >> Sounds like a good job for David. Note I've a lot more boards for >> which I would like to add mmc support or dts files in general >> (will do so as time permits). > Ok. I will work on it this evening. > >> For those boards which already have a dts I'll send mmc adding dts >> patches to David for now so he can add the changes to the patch-set. > Ok. I will create another branch and will incorporate your DTS files. > >> How do you want to deal with new boards ? Send the addition of the >> base board to you (and CC David as he will need them in his tree too), >> and then send a patch to add mmc to the dts to David ? > Sounds good to me... In Maxime's reply I was talking about above he suggests to simply send complete dts files with the new mmc node present to him, so I'll do that instead if / when I add dts files for new boards. Regards, Hans
Hi, On 12/16/2013 11:02 AM, Maxime Ripard wrote: > Hi Hans, > > Damn google-groups removed me from Cc :) > > On Sun, Dec 15, 2013 at 03:31:29PM +0100, Hans de Goede wrote: >>>> @@ -376,6 +385,13 @@ >>>> allwinner,drive = <0>; >>>> allwinner,pull = <0>; >>>> }; >>>> + >>>> + sdc0_pins_a: sdc0@0 { >>>> + allwinner,pins = "PF0","PF1","PF2","PF3","PF4","PF5"; >>>> + allwinner,function = "mmc0"; >>>> + allwinner,drive = <3>; >>>> + allwinner,pull = <1>; >>> >>> Wow, you need both the pullups and a 40mA output? >> >> Yes and no. The pullups are enabled by uboot and the allwinner >> sources, most of my testing has been done without them, which >> seems to work fine. I've enabled them to be consisten with the >> allwinner sources and u-boot. > > Henrik was suggesting that in most boards it's not required to set up > the internal pullups. Maybe we can just disable them by default, and > we will always be able to enable them at the board level if needed. Ok, I'm fine with disabling the pull-ups by default. >> As for drive = <3>, we need drive = <2> for normal modes, >> and drive = <3> for ddr mode. I simply picked <3> to keep >> things KISS. It should not matter much in power usage, as >> it will only make the flanks of the signal more steep. Once the >> desired output level is reached the current will drop off. It will >> use more current when changing the level, but for half the time, >> so the effective power usage (current * time) is the same. > > Ok, let's keep it to 40mA then. It will indeed be simpler :) Ack. > >>> Ideally, I'd like this patch to be splitted into three: >>> - One that adds the MMC controller nodes to the DTSI >>> - One that adds the muxing options you need to the pinctrl node >>> - One that enables the controller on the boards >> >> Sounds like a good job for David. Note I've a lot more boards for >> which I would like to add mmc support or dts files in general >> (will do so as time permits). >> >> For those boards which already have a dts I'll send mmc adding dts >> patches to David for now so he can add the changes to the patch-set. >> >> How do you want to deal with new boards ? Send the addition of the >> base board to you (and CC David as he will need them in his tree too), >> and then send a patch to add mmc to the dts to David ? > > I don't really like having a variable-sweep patch serie. Just send > your DT additions to me, saying that you depend on the MMC serie from > David. Ok will do if / when I've dts files for new boards. Regards, Hans
diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts index 425a7db..d193937 100644 --- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts +++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts @@ -42,7 +42,23 @@ }; }; + sdc0: sdc@01c0f000 { + pinctrl-names = "default"; + pinctrl-0 = <&sdc0_pins_a>; + pinctrl-1 = <&mmc0_cd_pin_cubieboard>; + cd-gpios = <&pio 7 1 0>; /* PH1 */ + cd-mode = <1>; + status = "okay"; + }; + pinctrl@01c20800 { + mmc0_cd_pin_cubieboard: mmc0_cd_pin@0 { + allwinner,pins = "PH1"; + allwinner,function = "gpio_in"; + allwinner,drive = <0>; + allwinner,pull = <0>; + }; + led_pins_cubieboard: led_pins@0 { allwinner,pins = "PH20", "PH21"; allwinner,function = "gpio_out"; diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi index 4dccdb0..13bccd5 100644 --- a/arch/arm/boot/dts/sun4i-a10.dtsi +++ b/arch/arm/boot/dts/sun4i-a10.dtsi @@ -306,6 +306,15 @@ #size-cells = <0>; }; + sdc0: sdc@01c0f000 { + compatible = "allwinner,sun4i-mmc"; + reg = <0x01c0f000 0x1000>; + clocks = <&ahb_gates 8>, <&mmc0>; + interrupts = <32>; + bus-width = <4>; + status = "disabled"; + }; + intc: interrupt-controller@01c20400 { compatible = "allwinner,sun4i-ic"; reg = <0x01c20400 0x400>; @@ -376,6 +385,13 @@ allwinner,drive = <0>; allwinner,pull = <0>; }; + + sdc0_pins_a: sdc0@0 { + allwinner,pins = "PF0","PF1","PF2","PF3","PF4","PF5"; + allwinner,function = "mmc0"; + allwinner,drive = <3>; + allwinner,pull = <1>; + }; }; timer@01c20c00 {
Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- arch/arm/boot/dts/sun4i-a10-cubieboard.dts | 16 ++++++++++++++++ arch/arm/boot/dts/sun4i-a10.dtsi | 16 ++++++++++++++++ 2 files changed, 32 insertions(+)