| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 13 Jan 2020 16:19:02 +0100
From: guillaume La Roque <glaroque@...libre.com>
To: Hans Verkuil <hverkuil-cisco@...all.nl>, narmstrong@...libre.com,
mchehab@...nel.org, khilman@...libre.com,
devicetree@...r.kernel.org
Cc: linux-media@...r.kernel.org, linux-amlogic@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] media: platform: meson-ao-cec-g12a: add wakeup
support
On 1/13/20 2:30 PM, Hans Verkuil wrote:
> On 1/10/20 4:06 PM, guillaume La Roque wrote:
>> Hi Hans,
>>
>> On 1/7/20 3:36 PM, Hans Verkuil wrote:
>>> Hi Guillaume,
>>>
>>> On 12/13/19 2:29 PM, Guillaume La Roque wrote:
>>>> add register configuration to activate wakeup feature in bl301
>>>>
>>>> Tested-by: Kevin Hilman <khilman@...libre.com>
>>>> Signed-off-by: Guillaume La Roque <glaroque@...libre.com>
>>>> ---
>>>> drivers/media/platform/meson/ao-cec-g12a.c | 33 ++++++++++++++++++++++
>>>> 1 file changed, 33 insertions(+)
>>>>
>>>> diff --git a/drivers/media/platform/meson/ao-cec-g12a.c b/drivers/media/platform/meson/ao-cec-g12a.c
>>>> index 891533060d49..85850b974126 100644
>>>> --- a/drivers/media/platform/meson/ao-cec-g12a.c
>>>> +++ b/drivers/media/platform/meson/ao-cec-g12a.c
>>>> @@ -25,6 +25,7 @@
>>>> #include <media/cec.h>
>>>> #include <media/cec-notifier.h>
>>>> #include <linux/clk-provider.h>
>>>> +#include <linux/mfd/syscon.h>
>>>>
>>>> /* CEC Registers */
>>>>
>>>> @@ -168,6 +169,18 @@
>>>>
>>>> #define CECB_WAKEUPCTRL 0x31
>>>>
>>>> +#define CECB_FUNC_CFG_REG 0xA0
>>>> +#define CECB_FUNC_CFG_MASK GENMASK(6, 0)
>>>> +#define CECB_FUNC_CFG_CEC_ON 0x01
>>>> +#define CECB_FUNC_CFG_OTP_ON 0x02
>>>> +#define CECB_FUNC_CFG_AUTO_STANDBY 0x04
>>>> +#define CECB_FUNC_CFG_AUTO_POWER_ON 0x08
>>>> +#define CECB_FUNC_CFG_ALL 0x2f
>>>> +#define CECB_FUNC_CFG_NONE 0x0
>>>> +
>>>> +#define CECB_LOG_ADDR_REG 0xA4
>>>> +#define CECB_LOG_ADDR_MASK GENMASK(22, 16)
>>>> +
>>>> struct meson_ao_cec_g12a_data {
>>>> /* Setup the internal CECB_CTRL2 register */
>>>> bool ctrl2_setup;
>>>> @@ -177,6 +190,7 @@ struct meson_ao_cec_g12a_device {
>>>> struct platform_device *pdev;
>>>> struct regmap *regmap;
>>>> struct regmap *regmap_cec;
>>>> + struct regmap *regmap_ao_sysctrl;
>>>> spinlock_t cec_reg_lock;
>>>> struct cec_notifier *notify;
>>>> struct cec_adapter *adap;
>>>> @@ -518,6 +532,13 @@ meson_ao_cec_g12a_set_log_addr(struct cec_adapter *adap, u8 logical_addr)
>>>> BIT(logical_addr - 8));
>>>> }
>>>>
>>>> + if (ao_cec->regmap_ao_sysctrl)
>>>> + ret |= regmap_update_bits(ao_cec->regmap_ao_sysctrl,
>>>> + CECB_LOG_ADDR_REG,
>>>> + CECB_LOG_ADDR_MASK,
>>>> + FIELD_PREP(CECB_LOG_ADDR_MASK,
>>>> + logical_addr));
>>>> +
>>>> /* Always set Broadcast/Unregistered 15 address */
>>>> ret |= regmap_update_bits(ao_cec->regmap_cec, CECB_LADD_HIGH,
>>>> BIT(CEC_LOG_ADDR_UNREGISTERED - 8),
>>>> @@ -618,6 +639,13 @@ static int meson_ao_cec_g12a_adap_enable(struct cec_adapter *adap, bool enable)
>>>> regmap_write(ao_cec->regmap_cec, CECB_CTRL2,
>>>> FIELD_PREP(CECB_CTRL2_RISE_DEL_MAX, 2));
>>>>
>>>> + if (ao_cec->regmap_ao_sysctrl) {
>>>> + regmap_update_bits(ao_cec->regmap_ao_sysctrl,
>>>> + CECB_FUNC_CFG_REG,
>>>> + CECB_FUNC_CFG_MASK,
>>>> + CECB_FUNC_CFG_ALL);
>>> What exactly is enabled here? Looking at CECB_FUNC_CFG_ALL it seems to
>>> enable automatic standby (I presume when the STANDBY message is received?)
>>> and power on (I presume when SET_STREAM_PATH is received?).
>> this register and flags are used by bl301 part.
>>
>> amlogic implemented a task to check cec event/message.
>>
>> for power on in bl301 it's not only on SET_STREAM_PATH but also on :
>>
>> USER_CONTROL_PRESSED
>> TEXT_VIEW_ON
> Not IMAGE_VIEW_ON?
sorry i forgot it , yes on IMAGE_VIEW_ON too
>
>> ACTIVE_SOURCE
>> ROUTING_CHANGE
>>
>> when in CECB_FUNC_CFG_REG register we put CECB_FUNC_CFG_CEC_ON and CECB_FUNC_CFG_AUTO_POWER_ON
>>
>> it's not possible to change this
> Too bad since ACTIVE_SOURCE and ROUTING_CHANGE should not power on a CEC device.
>
> See section 11.5.2 in the HDMI 2.0 Specification for more details, if you have
> that spec.
i will return your comment to amlogic but not sure they do change in bl301.
>
>>> Do you really want to automatically handle STANDBY that way? What does this
>>> do on the hardware level anyway? Isn't this something that should be
>>> controlled in userspace?
>> in fact i do a new check in bl301 code amlogic do nothing on STANDBY so i will clean code
>>
>> and activate real option supported by bl301
>>
>>> Similar questions for power on: you may not always want to enable this feature
>>> since it depends very much on the precise use-case.
>>>
>>> And which messages it reacts to in order to do a power-on needs to be
>>> documented since this differs depending on whether the CEC adapter is
>>> used for a TV or for a playback device. This feature may be hardwired for
>>> a playback device only, in which case it should probably be disabled if
>>> the CEC adapter is configured as a TV.
>>>
>>> In any case I would like to see some more details about how this works,
>>> especially since this is the first implementation of such a feature.
>>>
>>> I suspect that some userspace API might be needed to get the right level
>>> of control of such a feature.
>> i will send v3 next week with some comments and fix ( disable are missing for example)
>>
>> actual usercase is for android TV.
>>
>> when cec was enable android TV want to be wakeup by cec event.
>>
>>
>>> Regards,
>>>
>>> Hans
>>>
>> thanks for your review
> No problem!
>
> Hans
>
>> Regards
>>
>> Guillaume
>>
>>>> + }
>>>> +
>>>> meson_ao_cec_g12a_irq_setup(ao_cec, true);
>>>>
>>>> return 0;
>>>> @@ -685,6 +713,11 @@ static int meson_ao_cec_g12a_probe(struct platform_device *pdev)
>>>> goto out_probe_adapter;
>>>> }
>>>>
>>>> + ao_cec->regmap_ao_sysctrl = syscon_regmap_lookup_by_phandle
>>>> + (pdev->dev.of_node, "amlogic,ao-sysctrl");
>>>> + if (IS_ERR(ao_cec->regmap_ao_sysctrl))
>>>> + dev_warn(&pdev->dev, "ao-sysctrl syscon regmap lookup failed.\n");
>>>> +
>>>> irq = platform_get_irq(pdev, 0);
>>>> ret = devm_request_threaded_irq(&pdev->dev, irq,
>>>> meson_ao_cec_g12a_irq,
>>>>
Regards
Guillaume
Powered by blists - more mailing lists