Re: [U-Boot] [U-Boot, 2/2] rockchip: Drop call to rockchip_dnl_mode_check() for now【请注意，邮件由u-boot-boun...@lists.denx.de代发】【请注意，邮件由s...@google.com代发】

2019-03-12 Thread Andy Yan 

Hi Simon:

On 2019/3/11 上午10:30, Simon Glass wrote:

Hi Andy,

On Wed, 6 Mar 2019 at 03:52, Andy Yan  wrote:

Hi Simon:

On 2019/2/12 下午11:31, Simon Glass wrote:

HI Andy,

On Tue, 12 Feb 2019 at 04:05, Andy Yan  wrote:

Hi Philipp:

   Sorry for the late reply, we just come back from the Chinese Spring
Festival.

On 2019/2/1 下午6:26, Philipp Tomsich wrote:

Kever,

Independent of whether we revert this for the current cycle (and also
independent of
if I ever find the other patch you had been referring to — I couldn’t
find it in my local
mailing list archive) and then deprecate it for the next release
(unless converted to
DM), we still have a number of architectural issues that need to be
addressed:

I still doubt  is this a right  work-flow for patch apply. Before we
apply  a patch  which will break many other boards , should we  make
sure there is a solution patch applied for these boars first?



1.This really should be a driver under DTS control.
2.We need to not get away from configuring SOM-specific addresses via
Kconfig

Both these issues are technical debt that we’ve accumulated over the
last 18 months
and need to address for the sake of future maintainability.
E.g. ‘setting an address to 0x0 via Kconfig to disable a
driver/feature’ really isn’t in line
with the architectural direction of U-Boot.


For technical side, I think CONFIG_ROCKCHIP_BOOT_MODE_REG is necessary
here, we will read this register from save_boot_params when we get out
from bootrom,  the dtb is not available at this point.

Yes this is happening very early, but I wonder why this is necessary?
Can it be moved to later?

The call to check_back_to_brom_dnl_flag() happens much later so there
should be no problem to convert that.


On the other hand, almost rockchip based products use a recovery key to
enter download(upgrade)mode, this is a muti-funtion key, most of them
reuse with vol+- key,  we would like the u-boot share

dtb with linux kernel. To keep the linux kernel dts as clean as possible
,we don't want to add another dts property to describe this key either.
This is why I implement function rockchip_dnl_key_pressed as __weak.


OK, but given that it is called later, it seems to me that it could
use a driver.



We can't let it called later. See the discuss here :
http://patchwork.ozlabs.org/patch/812228/

Yes I read that, and took a look myself, and certainly it does not
look very easy and I take your point about potential bugs being
introduced.

It seems like you have tried quite hard, so I'm not going to object if
you can't find a way. My main objection was that it broke other
boards.



I took the code many times,  what this code done is : call 
adc_channel_single_shot to check if a download key is pressed, so it is 
very strange it broke the display on minnie. Would you please show the 
boot log when the display broke.




Is there any way the check could be delayed enough to actually be able
to read the device tree? It could call spl_early_init() quite early
and then do that.


Actually,  the interrupt overwrite happens in start.s,  so this don't do 
help.




Regards,
Simon






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

Re: [U-Boot] [U-Boot, 2/2] rockchip: Drop call to rockchip_dnl_mode_check() for now【请注意，邮件由u-boot-boun...@lists.denx.de代发】【请注意，邮件由s...@google.com代发】

2019-03-11 Thread Philipp Tomsich 

> On 11.03.2019, at 03:30, Simon Glass  wrote:
> 
> Hi Andy,
> 
> On Wed, 6 Mar 2019 at 03:52, Andy Yan  wrote:
>> 
>> Hi Simon:
>> 
>> On 2019/2/12 下午11:31, Simon Glass wrote:
>>> HI Andy,
>>> 
>>> On Tue, 12 Feb 2019 at 04:05, Andy Yan  wrote:
 Hi Philipp:
 
  Sorry for the late reply, we just come back from the Chinese Spring
 Festival.
 
 On 2019/2/1 下午6:26, Philipp Tomsich wrote:
> Kever,
> 
> Independent of whether we revert this for the current cycle (and also
> independent of
> if I ever find the other patch you had been referring to — I couldn’t
> find it in my local
> mailing list archive) and then deprecate it for the next release
> (unless converted to
> DM), we still have a number of architectural issues that need to be
> addressed:
 I still doubt  is this a right  work-flow for patch apply. Before we
 apply  a patch  which will break many other boards , should we  make
 sure there is a solution patch applied for these boars first?
 
 
> 1.This really should be a driver under DTS control.
> 2.We need to not get away from configuring SOM-specific addresses via
> Kconfig
> 
> Both these issues are technical debt that we’ve accumulated over the
> last 18 months
> and need to address for the sake of future maintainability.
> E.g. ‘setting an address to 0x0 via Kconfig to disable a
> driver/feature’ really isn’t in line
> with the architectural direction of U-Boot.
> 
 For technical side, I think CONFIG_ROCKCHIP_BOOT_MODE_REG is necessary
 here, we will read this register from save_boot_params when we get out
 from bootrom,  the dtb is not available at this point.
>>> Yes this is happening very early, but I wonder why this is necessary?
>>> Can it be moved to later?
>>> 
>>> The call to check_back_to_brom_dnl_flag() happens much later so there
>>> should be no problem to convert that.
>>> 
 On the other hand, almost rockchip based products use a recovery key to
 enter download(upgrade)mode, this is a muti-funtion key, most of them
 reuse with vol+- key,  we would like the u-boot share
 
 dtb with linux kernel. To keep the linux kernel dts as clean as possible
 ,we don't want to add another dts property to describe this key either.
 This is why I implement function rockchip_dnl_key_pressed as __weak.
 
>>> OK, but given that it is called later, it seems to me that it could
>>> use a driver.
>> 
>> 
>> 
>> We can't let it called later. See the discuss here :
>> http://patchwork.ozlabs.org/patch/812228/
> 
> Yes I read that, and took a look myself, and certainly it does not
> look very easy and I take your point about potential bugs being
> introduced.
> 
> It seems like you have tried quite hard, so I'm not going to object if
> you can't find a way. My main objection was that it broke other
> boards.
> 
> Is there any way the check could be delayed enough to actually be able
> to read the device tree? It could call spl_early_init() quite early
> and then do that.

Note that the minimum improvement that I’d expect to get this fully
enabled again would be a clean-up of the Kconfig options, so it is
easy and (from the documentation) predictable for boards to turn
this off.

I still want this to be turned back on in the current cycle.

Thanks,
Philipp.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [U-Boot, 2/2] rockchip: Drop call to rockchip_dnl_mode_check() for now【请注意，邮件由u-boot-boun...@lists.denx.de代发】【请注意，邮件由s...@google.com代发】

2019-03-10 Thread Simon Glass 
Hi Andy,

On Wed, 6 Mar 2019 at 03:52, Andy Yan  wrote:
>
> Hi Simon:
>
> On 2019/2/12 下午11:31, Simon Glass wrote:
> > HI Andy,
> >
> > On Tue, 12 Feb 2019 at 04:05, Andy Yan  wrote:
> >> Hi Philipp:
> >>
> >>   Sorry for the late reply, we just come back from the Chinese Spring
> >> Festival.
> >>
> >> On 2019/2/1 下午6:26, Philipp Tomsich wrote:
> >>> Kever,
> >>>
> >>> Independent of whether we revert this for the current cycle (and also
> >>> independent of
> >>> if I ever find the other patch you had been referring to — I couldn’t
> >>> find it in my local
> >>> mailing list archive) and then deprecate it for the next release
> >>> (unless converted to
> >>> DM), we still have a number of architectural issues that need to be
> >>> addressed:
> >> I still doubt  is this a right  work-flow for patch apply. Before we
> >> apply  a patch  which will break many other boards , should we  make
> >> sure there is a solution patch applied for these boars first?
> >>
> >>
> >>> 1.This really should be a driver under DTS control.
> >>> 2.We need to not get away from configuring SOM-specific addresses via
> >>> Kconfig
> >>>
> >>> Both these issues are technical debt that we’ve accumulated over the
> >>> last 18 months
> >>> and need to address for the sake of future maintainability.
> >>> E.g. ‘setting an address to 0x0 via Kconfig to disable a
> >>> driver/feature’ really isn’t in line
> >>> with the architectural direction of U-Boot.
> >>>
> >> For technical side, I think CONFIG_ROCKCHIP_BOOT_MODE_REG is necessary
> >> here, we will read this register from save_boot_params when we get out
> >> from bootrom,  the dtb is not available at this point.
> > Yes this is happening very early, but I wonder why this is necessary?
> > Can it be moved to later?
> >
> > The call to check_back_to_brom_dnl_flag() happens much later so there
> > should be no problem to convert that.
> >
> >> On the other hand, almost rockchip based products use a recovery key to
> >> enter download(upgrade)mode, this is a muti-funtion key, most of them
> >> reuse with vol+- key,  we would like the u-boot share
> >>
> >> dtb with linux kernel. To keep the linux kernel dts as clean as possible
> >> ,we don't want to add another dts property to describe this key either.
> >> This is why I implement function rockchip_dnl_key_pressed as __weak.
> >>
> > OK, but given that it is called later, it seems to me that it could
> > use a driver.
>
>
>
> We can't let it called later. See the discuss here :
> http://patchwork.ozlabs.org/patch/812228/

Yes I read that, and took a look myself, and certainly it does not
look very easy and I take your point about potential bugs being
introduced.

It seems like you have tried quite hard, so I'm not going to object if
you can't find a way. My main objection was that it broke other
boards.

Is there any way the check could be delayed enough to actually be able
to read the device tree? It could call spl_early_init() quite early
and then do that.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [U-Boot, 2/2] rockchip: Drop call to rockchip_dnl_mode_check() for now【请注意，邮件由u-boot-boun...@lists.denx.de代发】【请注意，邮件由s...@google.com代发】

2019-03-06 Thread Andy Yan 

Hi Simon:

On 2019/2/12 下午11:31, Simon Glass wrote:

HI Andy,

On Tue, 12 Feb 2019 at 04:05, Andy Yan  wrote:

Hi Philipp:

  Sorry for the late reply, we just come back from the Chinese Spring
Festival.

On 2019/2/1 下午6:26, Philipp Tomsich wrote:

Kever,

Independent of whether we revert this for the current cycle (and also
independent of
if I ever find the other patch you had been referring to — I couldn’t
find it in my local
mailing list archive) and then deprecate it for the next release
(unless converted to
DM), we still have a number of architectural issues that need to be
addressed:

I still doubt  is this a right  work-flow for patch apply. Before we
apply  a patch  which will break many other boards , should we  make
sure there is a solution patch applied for these boars first?



1.This really should be a driver under DTS control.
2.We need to not get away from configuring SOM-specific addresses via
Kconfig

Both these issues are technical debt that we’ve accumulated over the
last 18 months
and need to address for the sake of future maintainability.
E.g. ‘setting an address to 0x0 via Kconfig to disable a
driver/feature’ really isn’t in line
with the architectural direction of U-Boot.


For technical side, I think CONFIG_ROCKCHIP_BOOT_MODE_REG is necessary
here, we will read this register from save_boot_params when we get out
from bootrom,  the dtb is not available at this point.

Yes this is happening very early, but I wonder why this is necessary?
Can it be moved to later?

The call to check_back_to_brom_dnl_flag() happens much later so there
should be no problem to convert that.


On the other hand, almost rockchip based products use a recovery key to
enter download(upgrade)mode, this is a muti-funtion key, most of them
reuse with vol+- key,  we would like the u-boot share

dtb with linux kernel. To keep the linux kernel dts as clean as possible
,we don't want to add another dts property to describe this key either.
This is why I implement function rockchip_dnl_key_pressed as __weak.


OK, but given that it is called later, it seems to me that it could
use a driver.




We can't let it called later. See the discuss here : 
http://patchwork.ozlabs.org/patch/812228/




Regards,
Simon



I don’t have my own house completely in order (I’ve been talking for a
year now about
finally wrapping the RGMII/GMII selection into an ioctl-call to a
driver) yet, but that doesn’t
mean that we we should delay this clean-up more than absolutely necessary.

Thanks,
Philipp.


On 01.02.2019, at 10:34, Philipp Tomsich
mailto:philipp.toms...@theobroma-systems.com>> wrote:




On 01.02.2019, at 10:32, Kever Yang mailto:kever.y...@rock-chips.com>> wrote:

Hi Philipp,

This is not right,  this patch should not merged like this!!!

I have give my review comment in previous mail, and this will break
many boards.

My another patch do not break anything, but you insist NAK it
without acceptable reason;

What other patch?
I don’t remember seeing that one...


This patch definitely break other board and I have comment it, but
you just ignore other people's review and merge it, good job!

Thanks,
- Kever
On 02/01/2019 05:12 AM, Philipp Tomsich wrote:

This function causes a 5-second delay and stops the display working on
minnie. This code should be in a driver and should only be enabled by
a device-tree property, so that it does not affect devices which
do not
have this feature.

Signed-off-by: Simon Glass mailto:s...@chromium.org>>
Reviewed-by: Philipp Tomsich
mailto:philipp.toms...@theobroma-systems.com>>
---

arch/arm/mach-rockchip/boot_mode.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)


Applied to u-boot-rockchip, thanks!
___
U-Boot mailing list
U-Boot@lists.denx.de 
https://lists.denx.de/listinfo/u-boot




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

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






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