Re: [PATCH v2 0/5] rockchip: rk3229: add sdram and sd support

From: Kever Yang <kever.yang@rock-chips.com>
To: "Dr. Philipp Tomsich" <philipp.tomsich@theobroma-systems.com>
Cc: u-boot@lists.denx.de, linux-rockchip@lists.infradead.org,
	Albert Aribaud <albert.u.boot@aribaud.net>
Subject: Re: [PATCH v2 0/5] rockchip: rk3229: add sdram and sd support
Date: Fri, 18 Aug 2017 19:24:10 +0800	[thread overview]
Message-ID: <5f4ba87f-82fd-4048-3df7-23c6a83f2526@rock-chips.com> (raw)
In-Reply-To: <549AFCBD-88EC-4ED5-B719-5558D4FCC614@theobroma-systems.com>

On 08/18/2017 03:36 PM, Dr. Philipp Tomsich wrote:
>> On 18 Aug 2017, at 08:26, Kever Yang <kever.yang@rock-chips.com> wrote:
>>
>> Philipp,
>>
>>
>> On 08/17/2017 04:34 PM, Dr. Philipp Tomsich wrote:
>>>> On 17 Aug 2017, at 09:17, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>>
>>>>
>>>> Add sdram driver for rk3229 and other fix like pinctrl and sd node.
>>>>
>>>>
>>>> Changes in v2:
>>>> - split this patch in two patches
>>>>
>>>> Kever Yang (5):
>>>>   rockchip: rk322x: update dram bank size
>>>>   rockchip: rk322x: add sdram driver
>>>>   rockchip: rk322x: pinctrl: using compatible name same with dts
>>>>   rockchip: rk322x: pinctrl: fix IO MASK error on sdcard pin
>>>>   rockchip: dts: rk3229: remove dram channel info
>>>>
>>>> arch/arm/dts/rk3229-evb.dts                       |   1 -
>>>> arch/arm/include/asm/arch-rockchip/sdram_rk322x.h | 581 +++++++++++++++
>>>> arch/arm/mach-rockchip/rk322x-board.c             |  10 +-
>>>> arch/arm/mach-rockchip/rk322x/Makefile            |   1 +
>>>> arch/arm/mach-rockchip/rk322x/sdram_rk322x.c      | 855 ++++++++++++++++++++++
>>> Device-model DRAM controller drivers should generally go to drivers/ram; there’s
>>> already a subdirectory for the Rockchip-specific drivers created there.
>> I'm sorry, I didn't see it, even with the latest mainline U-Boot,
>> and both you and Simon had review the first version driver which send out
>> about one month ago, I don't know why it's not applied, so I send it again with
>> other patches change.
> The decision to move this over to drivers/ram is only about 6 weeks old.
> However, I didn’t want to add a new driver in the old location (as we’d then
> have to move it in the near future ; note that for the RK3399, I’ll submit a
> patch to move the driver to drivers/ram for the next release cycle).
>
> The patch has not been applied, as there’s unaddressed review comments:
> I had requested that the amount of data structures are deduplicated, as the
> pctl-register seemed the same as the rk3288 and the rk3368.
> I think there was a bit more code that could be shared already.
>
> We really need to get our DRAM drivers into shape, as these are becoming
> a major source of code duplication.

First patch do not have unaddressed comments, the second patch have,
I have reply the comment in mail of last version, but not get any response.

I still hope this patch set can merge first, and then we can move all of 
drivers
to drivers/ram together, and I believe we can try to abstract more common
function out from different SoCs, just like what I have done for 
sdram_common.c.
But you have to notice that the DRAM controller and the phy operation 
are always
tight coupling, not so easy to separate then even the pctl are very 
similar(they are
not the same).
So it's better to make the driver available first, and then we can move 
forward to make
more code be shared.

Thanks,
- Kever
>
>> Thanks,
>> - Kever
>>>> drivers/pinctrl/rockchip/pinctrl_rk322x.c         |   8 +-
>>>> 6 files changed, 1447 insertions(+), 9 deletions(-)
>>>> create mode 100644 arch/arm/include/asm/arch-rockchip/sdram_rk322x.h
>>>> create mode 100644 arch/arm/mach-rockchip/rk322x/sdram_rk322x.c
>>>>
>>>> -- 
>>>> 1.9.1
>>>>
>>> _______________________________________________
>>> Linux-rockchip mailing list
>>> Linux-rockchip@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot