| Message ID | b323d8c7ea4eb6bc325f6a6465cb2547cc6be757.1604988979.git.frank@allwinnertech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Second step support for A100 | expand |
On 10/11/2020 06:40, Frank Lee wrote: Hi, > From: Yangtao Li <frank@allwinnertech.com> > > Add support for a100's usb phy, which with 2 PHYs. > > Signed-off-by: Yangtao Li <frank@allwinnertech.com> > --- > 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, But with that patch fixing the H6 support you don't need a new name, do you? Because below you just add the sun50i_a100_phy name next to every place with a sun50i_h6_phy check. > 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); > + } > + > 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, So you could just use the sun50i_h6_phy type here. Cheers, Andre > + .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 }, > { }, > }; >
On 10/11/2020 06:40, Frank Lee wrote: Hi, > From: Yangtao Li <frank@allwinnertech.com> > > Add support for a100's usb phy, which with 2 PHYs. > > Signed-off-by: Yangtao Li <frank@allwinnertech.com> > --- > 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 }, > { }, > }; >
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); + } + 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 }, { }, };