| Message ID | 20220815093905.134164-1-hsinyi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: ps8640: Add double reset T4 and T5 to power-on sequence | expand |
Reviewed-by: Rock Chiu rock.chiu@paradetech.corp-partner.google.com Rock Chiu Hsin-Yi Wang <hsinyi@chromium.org> 於 2022年8月15日 週一 下午5:39寫道： > The double reset power-on sequence is a workaround for the hardware > flaw in some chip that SPI Clock output glitch and cause internal MPU > unable to read firmware correctly. The sequence is suggested in ps8640 > application note. > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > --- > drivers/gpu/drm/bridge/parade-ps8640.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c > b/drivers/gpu/drm/bridge/parade-ps8640.c > index 49107a6cdac18..d7483c13c569b 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > @@ -375,6 +375,11 @@ static int __maybe_unused ps8640_resume(struct device > *dev) > gpiod_set_value(ps_bridge->gpio_reset, 1); > usleep_range(2000, 2500); > gpiod_set_value(ps_bridge->gpio_reset, 0); > + /* Double reset for T4 and T5 */ > + msleep(50); > + gpiod_set_value(ps_bridge->gpio_reset, 1); > + msleep(50); > + gpiod_set_value(ps_bridge->gpio_reset, 0); > > /* > * Mystery 200 ms delay for the "MCU to be ready". It's unclear if > -- > 2.37.1.595.g718a3a8f04-goog > >
Hsin-Yi Wang <hsinyi@chromium.org> 於 2022年8月15日 週一 下午5:39寫道： > > The double reset power-on sequence is a workaround for the hardware > flaw in some chip that SPI Clock output glitch and cause internal MPU > unable to read firmware correctly. The sequence is suggested in ps8640 > application note. > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> Reviewed-by: Rock Chiu <rock.chiu@paradetech.corp-partner.google.com> > > --- > > drivers/gpu/drm/bridge/parade-ps8640.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > index 49107a6cdac18..d7483c13c569b 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > @@ -375,6 +375,11 @@ static int __maybe_unused ps8640_resume(struct device *dev) > gpiod_set_value(ps_bridge->gpio_reset, 1); > usleep_range(2000, 2500); > gpiod_set_value(ps_bridge->gpio_reset, 0); > + /* Double reset for T4 and T5 */ > + msleep(50); > + gpiod_set_value(ps_bridge->gpio_reset, 1); > + msleep(50); > + gpiod_set_value(ps_bridge->gpio_reset, 0); > > /* > * Mystery 200 ms delay for the "MCU to be ready". It's unclear if > -- > 2.37.1.595.g718a3a8f04-goog >
Hi, On Mon, Aug 15, 2022 at 2:39 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > The double reset power-on sequence is a workaround for the hardware > flaw in some chip that SPI Clock output glitch and cause internal MPU > unable to read firmware correctly. The sequence is suggested in ps8640 > application note. > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > --- > drivers/gpu/drm/bridge/parade-ps8640.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > index 49107a6cdac18..d7483c13c569b 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > @@ -375,6 +375,11 @@ static int __maybe_unused ps8640_resume(struct device *dev) > gpiod_set_value(ps_bridge->gpio_reset, 1); > usleep_range(2000, 2500); > gpiod_set_value(ps_bridge->gpio_reset, 0); > + /* Double reset for T4 and T5 */ > + msleep(50); > + gpiod_set_value(ps_bridge->gpio_reset, 1); > + msleep(50); > + gpiod_set_value(ps_bridge->gpio_reset, 0); We really need another 100 ms here? ps8640 is already quite slow at powering itself up and that has a real user impact. Why was it only 2.5 ms for the first reset and 50 ms for the second? -Doug
On Thu, Aug 18, 2022 at 6:54 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Mon, Aug 15, 2022 at 2:39 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > > > The double reset power-on sequence is a workaround for the hardware > > flaw in some chip that SPI Clock output glitch and cause internal MPU > > unable to read firmware correctly. The sequence is suggested in ps8640 > > application note. > > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > > --- > > drivers/gpu/drm/bridge/parade-ps8640.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > index 49107a6cdac18..d7483c13c569b 100644 > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > @@ -375,6 +375,11 @@ static int __maybe_unused ps8640_resume(struct device *dev) > > gpiod_set_value(ps_bridge->gpio_reset, 1); > > usleep_range(2000, 2500); > > gpiod_set_value(ps_bridge->gpio_reset, 0); > > + /* Double reset for T4 and T5 */ > > + msleep(50); > > + gpiod_set_value(ps_bridge->gpio_reset, 1); > > + msleep(50); > > + gpiod_set_value(ps_bridge->gpio_reset, 0); > > We really need another 100 ms here? ps8640 is already quite slow at > powering itself up and that has a real user impact. Why was it only > 2.5 ms for the first reset and 50 ms for the second? > The T4 and T5 are required by Parade. I'm wondering if they can shorten the 200ms below: /* * Mystery 200 ms delay for the "MCU to be ready". It's unclear if * this is truly necessary since the MCU will already signal that * things are "good to go" by signaling HPD on "gpio 9". See * _ps8640_wait_hpd_asserted(). For now we'll keep this mystery delay * just in case. */ msleep(200); Does this have to wait 200ms? Can it shorten to 100 due to the additional 100ms from T4 and T5? > -Doug
How does T4/T5 impact the real case? We talked previously the T4/T5 shouldn't cause user impact. Do we have testing data from ODM? Rock Chiu Hsin-Yi Wang <hsinyi@chromium.org> 於 2022年8月18日 週四 上午10:43寫道： > > On Thu, Aug 18, 2022 at 6:54 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Mon, Aug 15, 2022 at 2:39 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > > > > > The double reset power-on sequence is a workaround for the hardware > > > flaw in some chip that SPI Clock output glitch and cause internal MPU > > > unable to read firmware correctly. The sequence is suggested in ps8640 > > > application note. > > > > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > > > --- > > > drivers/gpu/drm/bridge/parade-ps8640.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > > index 49107a6cdac18..d7483c13c569b 100644 > > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > > @@ -375,6 +375,11 @@ static int __maybe_unused ps8640_resume(struct device *dev) > > > gpiod_set_value(ps_bridge->gpio_reset, 1); > > > usleep_range(2000, 2500); > > > gpiod_set_value(ps_bridge->gpio_reset, 0); > > > + /* Double reset for T4 and T5 */ > > > + msleep(50); > > > + gpiod_set_value(ps_bridge->gpio_reset, 1); > > > + msleep(50); > > > + gpiod_set_value(ps_bridge->gpio_reset, 0); > > > > We really need another 100 ms here? ps8640 is already quite slow at > > powering itself up and that has a real user impact. Why was it only > > 2.5 ms for the first reset and 50 ms for the second? > > > > The T4 and T5 are required by Parade. I'm wondering if they can > shorten the 200ms below: > > /* > * Mystery 200 ms delay for the "MCU to be ready". It's unclear if > * this is truly necessary since the MCU will already signal that > * things are "good to go" by signaling HPD on "gpio 9". See > * _ps8640_wait_hpd_asserted(). For now we'll keep this mystery delay > * just in case. > */ > msleep(200); > > Does this have to wait 200ms? Can it shorten to 100 due to the > additional 100ms from T4 and T5? > > > -Doug
On Thu, Aug 18, 2022 at 11:19 AM Rock Chiu <rock.chiu@paradetech.corp-partner.google.com> wrote: > > How does T4/T5 impact the real case? We talked previously the T4/T5 > shouldn't cause user impact. > Do we have testing data from ODM? > Please leave comments below the previous comment's headline. I'm confused. The reason I upstreamed this patch is because this is in your application note and you asked us to help upstream it. Now do you mean that we don't need T4 and T5? > Rock Chiu > > Hsin-Yi Wang <hsinyi@chromium.org> 於 2022年8月18日 週四 上午10:43寫道： > > > > On Thu, Aug 18, 2022 at 6:54 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > Hi, > > > > > > On Mon, Aug 15, 2022 at 2:39 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > > > > > > > The double reset power-on sequence is a workaround for the hardware > > > > flaw in some chip that SPI Clock output glitch and cause internal MPU > > > > unable to read firmware correctly. The sequence is suggested in ps8640 > > > > application note. > > > > > > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > > > > --- > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > > > index 49107a6cdac18..d7483c13c569b 100644 > > > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > > > @@ -375,6 +375,11 @@ static int __maybe_unused ps8640_resume(struct device *dev) > > > > gpiod_set_value(ps_bridge->gpio_reset, 1); > > > > usleep_range(2000, 2500); > > > > gpiod_set_value(ps_bridge->gpio_reset, 0); > > > > + /* Double reset for T4 and T5 */ > > > > + msleep(50); > > > > + gpiod_set_value(ps_bridge->gpio_reset, 1); > > > > + msleep(50); > > > > + gpiod_set_value(ps_bridge->gpio_reset, 0); > > > > > > We really need another 100 ms here? ps8640 is already quite slow at > > > powering itself up and that has a real user impact. Why was it only > > > 2.5 ms for the first reset and 50 ms for the second? > > > > > > > The T4 and T5 are required by Parade. I'm wondering if they can > > shorten the 200ms below: > > > > /* > > * Mystery 200 ms delay for the "MCU to be ready". It's unclear if > > * this is truly necessary since the MCU will already signal that > > * things are "good to go" by signaling HPD on "gpio 9". See > > * _ps8640_wait_hpd_asserted(). For now we'll keep this mystery delay > > * just in case. > > */ > > msleep(200); > > > > Does this have to wait 200ms? Can it shorten to 100 due to the > > additional 100ms from T4 and T5? > > > > > -Doug
Hi, On Wed, Aug 17, 2022 at 8:22 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > On Thu, Aug 18, 2022 at 11:19 AM Rock Chiu > <rock.chiu@paradetech.corp-partner.google.com> wrote: > > > > How does T4/T5 impact the real case? We talked previously the T4/T5 > > shouldn't cause user impact. > > Do we have testing data from ODM? > > > Please leave comments below the previous comment's headline. > > I'm confused. The reason I upstreamed this patch is because this is in > your application note and you asked us to help upstream it. Now do you > mean that we don't need T4 and T5? I think Rock is asking what problems the extra delay is causing. In other words: why do we care about keeping these delays short? The answer is that it affects boot speed and also resume speed of devices. Adding these two delays here means that there's an extra 100 ms before the user can see something on the screen. That may not seem like a lot, but those kinds of delays add up quickly. At least on Chromebooks, booting quickly is always a big goal. -Doug
Hi, On Thu, Aug 18, 2022 at 8:03 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Wed, Aug 17, 2022 at 8:22 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > > > On Thu, Aug 18, 2022 at 11:19 AM Rock Chiu > > <rock.chiu@paradetech.corp-partner.google.com> wrote: > > > > > > How does T4/T5 impact the real case? We talked previously the T4/T5 > > > shouldn't cause user impact. > > > Do we have testing data from ODM? > > > > > Please leave comments below the previous comment's headline. > > > > I'm confused. The reason I upstreamed this patch is because this is in > > your application note and you asked us to help upstream it. Now do you > > mean that we don't need T4 and T5? > > I think Rock is asking what problems the extra delay is causing. In > other words: why do we care about keeping these delays short? > > The answer is that it affects boot speed and also resume speed of > devices. Adding these two delays here means that there's an extra 100 > ms before the user can see something on the screen. That may not seem > like a lot, but those kinds of delays add up quickly. At least on > Chromebooks, booting quickly is always a big goal. While I'm not very happy with this change and I don't really understand why these delays need to be so long, if folks are really certain that we need them and can't make them shorter then I guess we can land it. I'll wait a few more days in case anyone wants to chime in with their thoughts. -Doug
Hi, On Mon, Aug 22, 2022 at 9:33 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, Aug 18, 2022 at 8:03 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Wed, Aug 17, 2022 at 8:22 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > > > > > On Thu, Aug 18, 2022 at 11:19 AM Rock Chiu > > > <rock.chiu@paradetech.corp-partner.google.com> wrote: > > > > > > > > How does T4/T5 impact the real case? We talked previously the T4/T5 > > > > shouldn't cause user impact. > > > > Do we have testing data from ODM? > > > > > > > Please leave comments below the previous comment's headline. > > > > > > I'm confused. The reason I upstreamed this patch is because this is in > > > your application note and you asked us to help upstream it. Now do you > > > mean that we don't need T4 and T5? > > > > I think Rock is asking what problems the extra delay is causing. In > > other words: why do we care about keeping these delays short? > > > > The answer is that it affects boot speed and also resume speed of > > devices. Adding these two delays here means that there's an extra 100 > > ms before the user can see something on the screen. That may not seem > > like a lot, but those kinds of delays add up quickly. At least on > > Chromebooks, booting quickly is always a big goal. > > While I'm not very happy with this change and I don't really > understand why these delays need to be so long, if folks are really > certain that we need them and can't make them shorter then I guess we > can land it. I'll wait a few more days in case anyone wants to chime > in with their thoughts. I'll continue to grumble, but I did push it. 55453c0914d9 drm/bridge: ps8640: Add double reset T4 and T5 to power-on sequence I pushed to "drm-misc-next" and not "drm-misc-fixes". It doesn't feel massively urgent since apparently we've been without the "double-reset" for years and having the extra bake time feels like the better way to lean. -Doug
+ Jason. -Rock Doug Anderson <dianders@chromium.org> 於 2022年8月30日 週二 清晨5:49寫道： > > Hi, > > On Mon, Aug 22, 2022 at 9:33 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Thu, Aug 18, 2022 at 8:03 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > Hi, > > > > > > On Wed, Aug 17, 2022 at 8:22 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > > > > > > > On Thu, Aug 18, 2022 at 11:19 AM Rock Chiu > > > > <rock.chiu@paradetech.corp-partner.google.com> wrote: > > > > > > > > > > How does T4/T5 impact the real case? We talked previously the T4/T5 > > > > > shouldn't cause user impact. > > > > > Do we have testing data from ODM? > > > > > > > > > Please leave comments below the previous comment's headline. > > > > > > > > I'm confused. The reason I upstreamed this patch is because this is in > > > > your application note and you asked us to help upstream it. Now do you > > > > mean that we don't need T4 and T5? > > > > > > I think Rock is asking what problems the extra delay is causing. In > > > other words: why do we care about keeping these delays short? > > > > > > The answer is that it affects boot speed and also resume speed of > > > devices. Adding these two delays here means that there's an extra 100 > > > ms before the user can see something on the screen. That may not seem > > > like a lot, but those kinds of delays add up quickly. At least on > > > Chromebooks, booting quickly is always a big goal. > > > > While I'm not very happy with this change and I don't really > > understand why these delays need to be so long, if folks are really > > certain that we need them and can't make them shorter then I guess we > > can land it. I'll wait a few more days in case anyone wants to chime > > in with their thoughts. > > I'll continue to grumble, but I did push it. > > 55453c0914d9 drm/bridge: ps8640: Add double reset T4 and T5 to power-on sequence > > I pushed to "drm-misc-next" and not "drm-misc-fixes". It doesn't feel > massively urgent since apparently we've been without the > "double-reset" for years and having the extra bake time feels like the > better way to lean. > > -Doug
The chip is functional ready 200ms after the chip reset. The 200ms is mainly for loading firmware from spi rom. -Jason On Thu, Sep 8, 2022 at 8:32 AM Rock Chiu <rock.chiu@paradetech.corp-partner.google.com> wrote: > > + Jason. > > -Rock > > Doug Anderson <dianders@chromium.org> 於 2022年8月30日 週二 清晨5:49寫道： > > > > Hi, > > > > On Mon, Aug 22, 2022 at 9:33 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > Hi, > > > > > > On Thu, Aug 18, 2022 at 8:03 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > > > Hi, > > > > > > > > On Wed, Aug 17, 2022 at 8:22 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > > > > > > > > > On Thu, Aug 18, 2022 at 11:19 AM Rock Chiu > > > > > <rock.chiu@paradetech.corp-partner.google.com> wrote: > > > > > > > > > > > > How does T4/T5 impact the real case? We talked previously the T4/T5 > > > > > > shouldn't cause user impact. > > > > > > Do we have testing data from ODM? > > > > > > > > > > > Please leave comments below the previous comment's headline. > > > > > > > > > > I'm confused. The reason I upstreamed this patch is because this is in > > > > > your application note and you asked us to help upstream it. Now do you > > > > > mean that we don't need T4 and T5? > > > > > > > > I think Rock is asking what problems the extra delay is causing. In > > > > other words: why do we care about keeping these delays short? > > > > > > > > The answer is that it affects boot speed and also resume speed of > > > > devices. Adding these two delays here means that there's an extra 100 > > > > ms before the user can see something on the screen. That may not seem > > > > like a lot, but those kinds of delays add up quickly. At least on > > > > Chromebooks, booting quickly is always a big goal. > > > > > > While I'm not very happy with this change and I don't really > > > understand why these delays need to be so long, if folks are really > > > certain that we need them and can't make them shorter then I guess we > > > can land it. I'll wait a few more days in case anyone wants to chime > > > in with their thoughts. > > > > I'll continue to grumble, but I did push it. > > > > 55453c0914d9 drm/bridge: ps8640: Add double reset T4 and T5 to power-on sequence > > > > I pushed to "drm-misc-next" and not "drm-misc-fixes". It doesn't feel > > massively urgent since apparently we've been without the > > "double-reset" for years and having the extra bake time feels like the > > better way to lean. > > > > -Doug
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 49107a6cdac18..d7483c13c569b 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -375,6 +375,11 @@ static int __maybe_unused ps8640_resume(struct device *dev) gpiod_set_value(ps_bridge->gpio_reset, 1); usleep_range(2000, 2500); gpiod_set_value(ps_bridge->gpio_reset, 0); + /* Double reset for T4 and T5 */ + msleep(50); + gpiod_set_value(ps_bridge->gpio_reset, 1); + msleep(50); + gpiod_set_value(ps_bridge->gpio_reset, 0); /* * Mystery 200 ms delay for the "MCU to be ready". It's unclear if
The double reset power-on sequence is a workaround for the hardware flaw in some chip that SPI Clock output glitch and cause internal MPU unable to read firmware correctly. The sequence is suggested in ps8640 application note. Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> --- drivers/gpu/drm/bridge/parade-ps8640.c | 5 +++++ 1 file changed, 5 insertions(+)