by Maxime Ripard

[permalink] [raw]

Subject: Re: [RESEND PATCH 12/19] dt-bindings: Add bindings for USB phy on Allwinner A100

On Sat, Nov 28, 2020 at 08:18:16PM +0000, Andr? Przywara wrote:
> On 11/11/2020 22:50, Rob Herring wrote:
>
> Hi,
>
> > On Tue, Nov 10, 2020 at 02:39:42PM +0800, Frank Lee wrote:
> >> From: Yangtao Li <[email protected]>
> >>
> >> Add a device tree binding for the A100's USB PHY.
>
> Not your fault, Yangto, but why do we actually have a separate binding
> document per SoC, when the differences between the PHYs are so minimal
> that we get away with some flags in the compatible match, in one driver
> file?

They are similar, but there's massive differences between them still
(like which regulators are supposed to be there, or the register names).
So putting them all in the same file just ended up in an unmaintainable
mess.

> For a start this file is basically identical to the A64 one (apart from
> the example), so can you just add the A100 compatible string to that
> one, instead?

But yeah, if two are identical they should be merged

Maxime

Attachments:

(No filename) (992.00 B)
signature.asc (235.00 B)
Download all attachments

2020-12-07 01:23:40

by Andre Przywara

[permalink] [raw]

Subject: Re: [RESEND PATCH 13/19] phy: sun4i-usb: add support for A100 USB PHY

On 10/11/2020 06:40, Frank Lee wrote:

Hi,

> From: Yangtao Li <[email protected]>
>
> Add support for a100's usb phy, which with 2 PHYs.
>
> Signed-off-by: Yangtao Li <[email protected]>
> ---
> drivers/phy/allwinner/phy-sun4i-usb.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
> index a6900495baa5..1a0e403131e7 100644
> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> @@ -107,6 +107,7 @@ enum sun4i_usb_phy_type {
> sun8i_r40_phy,
> sun8i_v3s_phy,
> sun50i_a64_phy,
> + sun50i_a100_phy,
> sun50i_h6_phy,
> };
>
> @@ -289,7 +290,13 @@ static int sun4i_usb_phy_init(struct phy *_phy)
> }
>
> if (data->cfg->type == sun8i_a83t_phy ||
> + data->cfg->type == sun50i_a100_phy ||
> data->cfg->type == sun50i_h6_phy) {
> + if (phy->pmu && data->cfg->enable_pmu_unk1) {
> + val = readl(phy->pmu + REG_PMU_UNK1);
> + writel(val & ~BIT(3), phy->pmu + REG_PMU_UNK1);
> + }
> +

So having a closer look, this does not look right. We should not use
this very same variable (enable_pmu_unk1) for a different bit.

So what about changing "bool enable_pmu_unk1;" into "u32
pmu_phy_tune_mask;", and using this to mask bits in this PMU register,
regardless of the PHY type (above this "if" statement)? We just check it
for being 0 and possibly skip the R/M/W sequence.
Then the newer SoCs get .pmu_phy_tune_mask = BIT(1), in their config
below, and the A100 gets BIT(3). Older PHYs just omit this line at all,
are initialised to 0, and are skipped.

That would look more cleaner and might even be a bit future proof.

Cheers,
Andre

> if (phy->index == 0) {
> val = readl(data->base + data->cfg->phyctl_offset);
> val |= PHY_CTL_VBUSVLDEXT;
> @@ -339,6 +346,7 @@ static int sun4i_usb_phy_exit(struct phy *_phy)
>
> if (phy->index == 0) {
> if (data->cfg->type == sun8i_a83t_phy ||
> + data->cfg->type == sun50i_a100_phy ||
> data->cfg->type == sun50i_h6_phy) {
> void __iomem *phyctl = data->base +
> data->cfg->phyctl_offset;
> @@ -960,6 +968,16 @@ static const struct sun4i_usb_phy_cfg sun50i_a64_cfg = {
> .phy0_dual_route = true,
> };
>
> +static const struct sun4i_usb_phy_cfg sun50i_a100_cfg = {
> + .num_phys = 2,
> + .type = sun50i_a100_phy,
> + .disc_thresh = 3,
> + .phyctl_offset = REG_PHYCTL_A33,
> + .dedicated_clocks = true,
> + .enable_pmu_unk1 = true,
> + .phy0_dual_route = true,
> +};
> +
> static const struct sun4i_usb_phy_cfg sun50i_h6_cfg = {
> .num_phys = 4,
> .type = sun50i_h6_phy,
> @@ -983,6 +1001,7 @@ static const struct of_device_id sun4i_usb_phy_of_match[] = {
> { .compatible = "allwinner,sun8i-v3s-usb-phy", .data = &sun8i_v3s_cfg },
> { .compatible = "allwinner,sun50i-a64-usb-phy",
> .data = &sun50i_a64_cfg},
> + { .compatible = "allwinner,sun50i-a100-usb-phy", .data = &sun50i_a100_cfg },
> { .compatible = "allwinner,sun50i-h6-usb-phy", .data = &sun50i_h6_cfg },
> { },
> };
>