linux-kernel - Re: [PATCH v2 1/6] phy: rockchip-typec: deprecate some DT properties for various register fields.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2052597.3vIQ3VQ44p@phil>
Date:   Thu, 15 Feb 2018 17:06:03 +0100
From:   Heiko Stuebner <heiko@...ech.de>
To:     Enric Balletbo i Serra <enric.balletbo@...labora.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        Brian Norris <briannorris@...omium.org>, dianders@...omium.org,
        Chris Zhong <zyw@...k-chips.com>,
        William wu <wulf@...k-chips.com>, hl@...k-chips.com,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
        kernel@...labora.com
Subject: Re: [PATCH v2 1/6] phy: rockchip-typec: deprecate some DT properties for various register fields.

Hi Enric,

Am Mittwoch, 14. Februar 2018, 17:54:42 CET schrieb Enric Balletbo i Serra:
> Adding properties for various register fields in the DT doesn't scale and
> this information should be in the driver instead.
> 
> Before this patch these registers (description below) were specified in
> the DT, every register node contained 3 sections: offset, enable bit,
> write mask bit.
> 
>  - rockchip,typec-conn-dir : the register of type-c connector direction,
>    for type-c phy0, it must be <0xe580 0 16>;
>    for type-c phy1, it must be <0xe58c 0 16>;
>  - rockchip,usb3tousb2-en : the register of type-c force usb3 to usb2 enable
>    control.
>    for type-c phy0, it must be <0xe580 3 19>;
>    for type-c phy1, it must be <0xe58c 3 19>;
>  - rockchip,external-psm : the register of type-c phy external psm clock
>    selection.
>    for type-c phy0, it must be <0xe588 14 30>;
>    for type-c phy1, it must be <0xe594 14 30>;
>  - rockchip,pipe-status : the register of type-c phy pipe status.
>    for type-c phy0, it must be <0xe5c0 0 0>;
>    for type-c phy1, it must be <0xe5c0 16 16>;
> 
> After this patch these register definitions are in the driver. So can be
> removed from the DT. Note that there are 2 type-c phys for RK3399 with
> different offsets, the driver checks the phy base address of the running
> instance and applies the right offsets.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>

Generally I really support moving that stuff back into the driver and
judging by the recent mail conversations with Rob and Brian, this also
seems to be a consensus of sorts.

The one but I see here is, that the newly added things represent
rk3399-specific values and should be prefixed accordingly and also
already be flexible enough for the next soc using it.

Examples below.


> @@ -349,6 +349,9 @@
>  #define MODE_DFP_USB			BIT(1)
>  #define MODE_DFP_DP			BIT(2)
>  
> +#define TYPEC_PHY0_BASE_ADDRESS		0xff7c0000
> +#define TYPEC_PHY1_BASE_ADDRESS		0xff800000

RK3399_TYPEC_PHY0_ADDRESS etc ?

> @@ -362,6 +365,20 @@ struct rockchip_usb3phy_port_cfg {
>  	struct usb3phy_reg pipe_status;
>  };
>  
> +static const struct rockchip_usb3phy_port_cfg tcphy0_port_cfg = {
> +	.typec_conn_dir	= { 0xe580, 0, 16 },
> +	.usb3tousb2_en	= { 0xe580, 3, 19 },
> +	.external_psm	= { 0xe588, 14, 30 },
> +	.pipe_status	= { 0xe5c0, 0, 0 },
> +};
> +
> +static const struct rockchip_usb3phy_port_cfg tcphy1_port_cfg = {
> +	.typec_conn_dir	= { 0xe58c, 0, 16 },
> +	.usb3tousb2_en	= { 0xe58c, 3, 19 },
> +	.external_psm	= { 0xe594, 14, 30 },
> +	.pipe_status	= { 0xe5c0, 16, 16 },
> +};

rk3399_tcphy1_port_cfg. Especially important as these are GRF-registers
(general register files = a dumping ground for random settings bits)
and nothing ever stays the same in the GRF between chips.


> @@ -1103,6 +1078,13 @@ static int rockchip_typec_phy_probe(struct platform_device *pdev)
>  	if (IS_ERR(tcphy->base))
>  		return PTR_ERR(tcphy->base);
>  
> +	if (res->start == TYPEC_PHY0_BASE_ADDRESS)
> +		tcphy->port_cfgs = &tcphy0_port_cfg;
> +	else if (res->start == TYPEC_PHY1_BASE_ADDRESS)
> +		tcphy->port_cfgs = &tcphy1_port_cfg;
> +	else
> +		return -EINVAL;

should be selected according to the compatible.
Like just create a struct similar to things like the inno-usb2-phy.
That way you can also just write down the address itself in a reg
property and do not need specific constants like the ones at the top.


Heiko