| Message ID | 20190109140045.17449-1-marek.vasut@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 12105cec654cf90648f65f7dc90d1be982d326eb |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | arm64: dts: renesas: r8a77990: ebisu: Fix backlight regulator numbering | expand |
On Wed, Jan 9, 2019 at 3:01 PM <marek.vasut@gmail.com> wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > There are two regulator1 nodes in the Ebisu DTS right now, one 3.3V for > the eMMC and one 12V for the backlight. This causes one to be overwritten > by the other, ultimatelly resulting in inoperable eMMC, which depends on > the former. Fix this by renumbering the backlight regulator to regulator2. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> > Cc: linux-renesas-soc@vger.kernel.org > Reported-by: Simon Horman <horms+renesas@verge.net.au> > Fixes: 9d16c4a10e07 ("arm64: dts: renesas: r8a77990: ebisu: Add backlight") Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts > +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts > @@ -191,7 +191,7 @@ > clock-frequency = <24576000>; > }; > > - reg_12p0v: regulator1 { > + reg_12p0v: regulator2 { > compatible = "regulator-fixed"; > regulator-name = "D12.0V"; > regulator-min-microvolt = <12000000>; Perhaps the node name should get a more descriptive suffix (e.g. "regulator-12p0v"), like is already done for some of the other regulators? Gr{oetje,eeting}s, Geert
On Wed, Jan 09, 2019 at 04:26:25PM +0100, Geert Uytterhoeven wrote: > On Wed, Jan 9, 2019 at 3:01 PM <marek.vasut@gmail.com> wrote: > > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > > > There are two regulator1 nodes in the Ebisu DTS right now, one 3.3V for > > the eMMC and one 12V for the backlight. This causes one to be overwritten > > by the other, ultimatelly resulting in inoperable eMMC, which depends on > > the former. Fix this by renumbering the backlight regulator to regulator2. > > > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > Cc: Simon Horman <horms+renesas@verge.net.au> > > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Cc: linux-renesas-soc@vger.kernel.org > > Reported-by: Simon Horman <horms+renesas@verge.net.au> > > Fixes: 9d16c4a10e07 ("arm64: dts: renesas: r8a77990: ebisu: Add backlight") > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts > > +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts > > @@ -191,7 +191,7 @@ > > clock-frequency = <24576000>; > > }; > > > > - reg_12p0v: regulator1 { > > + reg_12p0v: regulator2 { > > compatible = "regulator-fixed"; > > regulator-name = "D12.0V"; > > regulator-min-microvolt = <12000000>; > > Perhaps the node name should get a more descriptive suffix > (e.g. "regulator-12p0v"), like is already done for some of the other > regulators? I think I would prefer that addressed in a follow-up patch.
On Wed, Jan 09, 2019 at 05:58:23PM +0100, Simon Horman wrote: > On Wed, Jan 09, 2019 at 04:26:25PM +0100, Geert Uytterhoeven wrote: > > On Wed, Jan 9, 2019 at 3:01 PM <marek.vasut@gmail.com> wrote: > > > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > > > > > There are two regulator1 nodes in the Ebisu DTS right now, one 3.3V for > > > the eMMC and one 12V for the backlight. This causes one to be overwritten > > > by the other, ultimatelly resulting in inoperable eMMC, which depends on > > > the former. Fix this by renumbering the backlight regulator to regulator2. > > > > > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > > Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > > Cc: Simon Horman <horms+renesas@verge.net.au> > > > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> > > > Cc: linux-renesas-soc@vger.kernel.org > > > Reported-by: Simon Horman <horms+renesas@verge.net.au> > > > Fixes: 9d16c4a10e07 ("arm64: dts: renesas: r8a77990: ebisu: Add backlight") > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts > > > +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts > > > @@ -191,7 +191,7 @@ > > > clock-frequency = <24576000>; > > > }; > > > > > > - reg_12p0v: regulator1 { > > > + reg_12p0v: regulator2 { > > > compatible = "regulator-fixed"; > > > regulator-name = "D12.0V"; > > > regulator-min-microvolt = <12000000>; > > > > Perhaps the node name should get a more descriptive suffix > > (e.g. "regulator-12p0v"), like is already done for some of the other > > regulators? > > I think I would prefer that addressed in a follow-up patch. Thanks for this patch. I have now tested it and it looks good to me. I can now access eMMC as a block device. Tested-by: Simon Horman <horms+renesas@verge.net.au> I plan to apply this for v5.1 as the problem appears to be introduced in a patch queued-up for v5.1. # dmesg | egrep "(ee160000.sd|mmc0|mmcblk0|backlight)" [ 0.893760] pwm-backlight backlight: Linked as a consumer to regulator.3 [ 2.901953] renesas_sdhi_internal_dmac ee160000.sd: Linked as a consumer to regulator.2 [ 2.910262] renesas_sdhi_internal_dmac ee160000.sd: Linked as a consumer to regulator.1 [ 2.967591] renesas_sdhi_internal_dmac ee160000.sd: mmc0 base at 0xee160000 max clock rate 200 MHz [ 3.049943] mmc0: new HS200 MMC card at address 0001 [ 3.055843] mmcblk0: mmc0:0001 BGSD3R 29.1 GiB [ 3.064414] mmcblk0boot0: mmc0:0001 BGSD3R partition 1 16.0 MiB [ 3.074888] mmcblk0boot1: mmc0:0001 BGSD3R partition 2 16.0 MiB [ 3.081522] mmcblk0rpmb: mmc0:0001 BGSD3R partition 3 4.00 MiB, chardev (243:0) # dd if=/dev/mmcblk0 of=/dev/null bs=1M count=512 iflag=direct 512+0 records in 512+0 records out 536870912 bytes (537 MB) copied, 14.7343 s, 36.4 MB/s # dd if=/dev/mmcblk0 of=/dev/null bs=1M count=512 iflag=direct 512+0 records in 512+0 records out 536870912 bytes (537 MB) copied, 8.46809 s, 63.4 MB/s # dd if=/dev/mmcblk0 of=/dev/null bs=1M count=512 iflag=direct 512+0 records in 512+0 records out 536870912 bytes (537 MB) copied, 14.6945 s, 36.5 MB/s # dd of=/dev/mmcblk0 if=/dev/zero bs=1M count=512 oflag=direct 512+0 records in 512+0 records out 536870912 bytes (537 MB) copied, 19.1677 s, 28.0 MB/s # dd of=/dev/mmcblk0 if=/dev/zero bs=1M count=512 oflag=direct 512+0 records in 512+0 records out 536870912 bytes (537 MB) copied, 21.3139 s, 25.2 MB/s # dd of=/dev/mmcblk0 if=/dev/zero bs=1M count=512 oflag=direct 512+0 records in 512+0 records out 536870912 bytes (537 MB) copied, 19.9205 s, 27.0 MB/s
Hi Marek, Thank you for the patch. On Wednesday, 9 January 2019 16:00:45 EET marek.vasut@gmail.com wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > There are two regulator1 nodes in the Ebisu DTS right now, one 3.3V for > the eMMC and one 12V for the backlight. This causes one to be overwritten > by the other, ultimatelly resulting in inoperable eMMC, which depends on > the former. Fix this by renumbering the backlight regulator to regulator2. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> > Cc: linux-renesas-soc@vger.kernel.org > Reported-by: Simon Horman <horms+renesas@verge.net.au> > Fixes: 9d16c4a10e07 ("arm64: dts: renesas: r8a77990: ebisu: Add backlight") Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Sorry for breaking this in the first place. > --- > arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts > b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts index > e0590c0af4c4..89383aa35d65 100644 > --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts > +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts > @@ -191,7 +191,7 @@ > clock-frequency = <24576000>; > }; > > - reg_12p0v: regulator1 { > + reg_12p0v: regulator2 { > compatible = "regulator-fixed"; > regulator-name = "D12.0V"; > regulator-min-microvolt = <12000000>;
On Wednesday, 9 January 2019 18:58:23 EET Simon Horman wrote: > On Wed, Jan 09, 2019 at 04:26:25PM +0100, Geert Uytterhoeven wrote: > > On Wed, Jan 9, 2019 at 3:01 PM <marek.vasut@gmail.com> wrote: > > > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > > > > > There are two regulator1 nodes in the Ebisu DTS right now, one 3.3V for > > > the eMMC and one 12V for the backlight. This causes one to be > > > overwritten > > > by the other, ultimatelly resulting in inoperable eMMC, which depends on > > > the former. Fix this by renumbering the backlight regulator to > > > regulator2. > > > > > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > > Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > > Cc: Simon Horman <horms+renesas@verge.net.au> > > > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> > > > Cc: linux-renesas-soc@vger.kernel.org > > > Reported-by: Simon Horman <horms+renesas@verge.net.au> > > > Fixes: 9d16c4a10e07 ("arm64: dts: renesas: r8a77990: ebisu: Add > > > backlight") > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts > > > +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts > > > @@ -191,7 +191,7 @@ > > > > > > clock-frequency = <24576000>; > > > > > > }; > > > > > > - reg_12p0v: regulator1 { > > > + reg_12p0v: regulator2 { > > > > > > compatible = "regulator-fixed"; > > > regulator-name = "D12.0V"; > > > regulator-min-microvolt = <12000000>; > > > > Perhaps the node name should get a more descriptive suffix > > (e.g. "regulator-12p0v"), like is already done for some of the other > > regulators? > > I think I would prefer that addressed in a follow-up patch. Agreed, but it would still be a very good idea. I think we need to standardize names for regulators, otherwise this is bound to happen again in the future.
On 1/10/19 11:02 AM, Simon Horman wrote: > On Wed, Jan 09, 2019 at 05:58:23PM +0100, Simon Horman wrote: >> On Wed, Jan 09, 2019 at 04:26:25PM +0100, Geert Uytterhoeven wrote: >>> On Wed, Jan 9, 2019 at 3:01 PM <marek.vasut@gmail.com> wrote: >>>> From: Marek Vasut <marek.vasut+renesas@gmail.com> >>>> >>>> There are two regulator1 nodes in the Ebisu DTS right now, one 3.3V for >>>> the eMMC and one 12V for the backlight. This causes one to be overwritten >>>> by the other, ultimatelly resulting in inoperable eMMC, which depends on >>>> the former. Fix this by renumbering the backlight regulator to regulator2. >>>> >>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >>>> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >>>> Cc: Simon Horman <horms+renesas@verge.net.au> >>>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> >>>> Cc: linux-renesas-soc@vger.kernel.org >>>> Reported-by: Simon Horman <horms+renesas@verge.net.au> >>>> Fixes: 9d16c4a10e07 ("arm64: dts: renesas: r8a77990: ebisu: Add backlight") >>> >>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> >>>> --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts >>>> +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts >>>> @@ -191,7 +191,7 @@ >>>> clock-frequency = <24576000>; >>>> }; >>>> >>>> - reg_12p0v: regulator1 { >>>> + reg_12p0v: regulator2 { >>>> compatible = "regulator-fixed"; >>>> regulator-name = "D12.0V"; >>>> regulator-min-microvolt = <12000000>; >>> >>> Perhaps the node name should get a more descriptive suffix >>> (e.g. "regulator-12p0v"), like is already done for some of the other >>> regulators? >> >> I think I would prefer that addressed in a follow-up patch. > > Thanks for this patch. > > I have now tested it and it looks good to me. > I can now access eMMC as a block device. > > Tested-by: Simon Horman <horms+renesas@verge.net.au> > > I plan to apply this for v5.1 as the problem appears to > be introduced in a patch queued-up for v5.1. Thanks > # dmesg | egrep "(ee160000.sd|mmc0|mmcblk0|backlight)" > [ 0.893760] pwm-backlight backlight: Linked as a consumer to regulator.3 > [ 2.901953] renesas_sdhi_internal_dmac ee160000.sd: Linked as a consumer to regulator.2 > [ 2.910262] renesas_sdhi_internal_dmac ee160000.sd: Linked as a consumer to regulator.1 > [ 2.967591] renesas_sdhi_internal_dmac ee160000.sd: mmc0 base at 0xee160000 max clock rate 200 MHz > [ 3.049943] mmc0: new HS200 MMC card at address 0001 > [ 3.055843] mmcblk0: mmc0:0001 BGSD3R 29.1 GiB > [ 3.064414] mmcblk0boot0: mmc0:0001 BGSD3R partition 1 16.0 MiB > [ 3.074888] mmcblk0boot1: mmc0:0001 BGSD3R partition 2 16.0 MiB > [ 3.081522] mmcblk0rpmb: mmc0:0001 BGSD3R partition 3 4.00 MiB, chardev (243:0) > > # dd if=/dev/mmcblk0 of=/dev/null bs=1M count=512 iflag=direct > 512+0 records in > 512+0 records out > 536870912 bytes (537 MB) copied, 14.7343 s, 36.4 MB/s > # dd if=/dev/mmcblk0 of=/dev/null bs=1M count=512 iflag=direct > 512+0 records in > 512+0 records out > 536870912 bytes (537 MB) copied, 8.46809 s, 63.4 MB/s > # dd if=/dev/mmcblk0 of=/dev/null bs=1M count=512 iflag=direct > 512+0 records in > 512+0 records out > 536870912 bytes (537 MB) copied, 14.6945 s, 36.5 MB/s Seems a bit slow to me for an HS200 card :-) Update your ATF, it has QoS updates for SDHI.
On 1/10/19 1:59 PM, Laurent Pinchart wrote: > On Wednesday, 9 January 2019 18:58:23 EET Simon Horman wrote: >> On Wed, Jan 09, 2019 at 04:26:25PM +0100, Geert Uytterhoeven wrote: >>> On Wed, Jan 9, 2019 at 3:01 PM <marek.vasut@gmail.com> wrote: >>>> From: Marek Vasut <marek.vasut+renesas@gmail.com> >>>> >>>> There are two regulator1 nodes in the Ebisu DTS right now, one 3.3V for >>>> the eMMC and one 12V for the backlight. This causes one to be >>>> overwritten >>>> by the other, ultimatelly resulting in inoperable eMMC, which depends on >>>> the former. Fix this by renumbering the backlight regulator to >>>> regulator2. >>>> >>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >>>> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >>>> Cc: Simon Horman <horms+renesas@verge.net.au> >>>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> >>>> Cc: linux-renesas-soc@vger.kernel.org >>>> Reported-by: Simon Horman <horms+renesas@verge.net.au> >>>> Fixes: 9d16c4a10e07 ("arm64: dts: renesas: r8a77990: ebisu: Add >>>> backlight") >>> >>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> >>>> --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts >>>> +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts >>>> @@ -191,7 +191,7 @@ >>>> >>>> clock-frequency = <24576000>; >>>> >>>> }; >>>> >>>> - reg_12p0v: regulator1 { >>>> + reg_12p0v: regulator2 { >>>> >>>> compatible = "regulator-fixed"; >>>> regulator-name = "D12.0V"; >>>> regulator-min-microvolt = <12000000>; >>> >>> Perhaps the node name should get a more descriptive suffix >>> (e.g. "regulator-12p0v"), like is already done for some of the other >>> regulators? >> >> I think I would prefer that addressed in a follow-up patch. > > Agreed, but it would still be a very good idea. I think we need to standardize > names for regulators, otherwise this is bound to happen again in the future. Isn't the YAML DT schema validator supposed to catch those problems ? I'd even expect DTC to be able to catch such duplicate nodes and warn about them.
diff --git a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts index e0590c0af4c4..89383aa35d65 100644 --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts @@ -191,7 +191,7 @@ clock-frequency = <24576000>; }; - reg_12p0v: regulator1 { + reg_12p0v: regulator2 { compatible = "regulator-fixed"; regulator-name = "D12.0V"; regulator-min-microvolt = <12000000>;