From: Yang Xiwen <[email protected]>
Direct MMIO resgiter access is used by Hi3798MV200. For other models,
of_iomap() returns NULL due to insufficient length. So they are
unaffected.
Also Hi3798MV200 INNO PHY has an extra reset required to be deasserted,
switch to reset_control_array_*() APIs for that.
Signed-off-by: Yang Xiwen <[email protected]>
---
drivers/phy/hisilicon/phy-hisi-inno-usb2.c | 66 ++++++++++++++++++------------
1 file changed, 40 insertions(+), 26 deletions(-)
diff --git a/drivers/phy/hisilicon/phy-hisi-inno-usb2.c b/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
index b7e740eb4752..df154cd99ed8 100644
--- a/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
+++ b/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
@@ -10,6 +10,7 @@
#include <linux/io.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/of_address.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
#include <linux/reset.h>
@@ -24,6 +25,7 @@
#define PHY_TYPE_0 0
#define PHY_TYPE_1 1
+#define PHY_TYPE_MMIO 2
#define PHY_TEST_DATA GENMASK(7, 0)
#define PHY_TEST_ADDR_OFFSET 8
@@ -43,6 +45,7 @@
#define PHY_CLK_ENABLE BIT(2)
struct hisi_inno_phy_port {
+ void __iomem *base;
struct reset_control *utmi_rst;
struct hisi_inno_phy_priv *priv;
};
@@ -50,7 +53,7 @@ struct hisi_inno_phy_port {
struct hisi_inno_phy_priv {
void __iomem *mmio;
struct clk *ref_clk;
- struct reset_control *por_rst;
+ struct reset_control *rsts;
unsigned int type;
struct hisi_inno_phy_port ports[INNO_PHY_PORT_NUM];
};
@@ -62,26 +65,31 @@ static void hisi_inno_phy_write_reg(struct hisi_inno_phy_priv *priv,
u32 val;
u32 value;
- if (priv->type == PHY_TYPE_0)
- val = (data & PHY_TEST_DATA) |
- ((addr << PHY_TEST_ADDR_OFFSET) & PHY0_TEST_ADDR) |
- ((port << PHY0_TEST_PORT_OFFSET) & PHY0_TEST_PORT) |
- PHY0_TEST_WREN | PHY0_TEST_RST;
- else
- val = (data & PHY_TEST_DATA) |
- ((addr << PHY_TEST_ADDR_OFFSET) & PHY1_TEST_ADDR) |
- ((port << PHY1_TEST_PORT_OFFSET) & PHY1_TEST_PORT) |
- PHY1_TEST_WREN | PHY1_TEST_RST;
- writel(val, reg);
-
- value = val;
- if (priv->type == PHY_TYPE_0)
- value |= PHY0_TEST_CLK;
- else
- value |= PHY1_TEST_CLK;
- writel(value, reg);
-
- writel(val, reg);
+ if (priv->ports[port].base)
+ /* FIXME: fill stride in priv */
+ writel(data, (u32 *)priv->ports[port].base + addr);
+ else {
+ if (priv->type == PHY_TYPE_0)
+ val = (data & PHY_TEST_DATA) |
+ ((addr << PHY_TEST_ADDR_OFFSET) & PHY0_TEST_ADDR) |
+ ((port << PHY0_TEST_PORT_OFFSET) & PHY0_TEST_PORT) |
+ PHY0_TEST_WREN | PHY0_TEST_RST;
+ else
+ val = (data & PHY_TEST_DATA) |
+ ((addr << PHY_TEST_ADDR_OFFSET) & PHY1_TEST_ADDR) |
+ ((port << PHY1_TEST_PORT_OFFSET) & PHY1_TEST_PORT) |
+ PHY1_TEST_WREN | PHY1_TEST_RST;
+ writel(val, reg);
+
+ value = val;
+ if (priv->type == PHY_TYPE_0)
+ value |= PHY0_TEST_CLK;
+ else
+ value |= PHY1_TEST_CLK;
+ writel(value, reg);
+
+ writel(val, reg);
+ }
}
static void hisi_inno_phy_setup(struct hisi_inno_phy_priv *priv)
@@ -104,7 +112,7 @@ static int hisi_inno_phy_init(struct phy *phy)
return ret;
udelay(REF_CLK_STABLE_TIME);
- reset_control_deassert(priv->por_rst);
+ reset_control_deassert(priv->rsts);
udelay(POR_RST_COMPLETE_TIME);
/* Set up phy registers */
@@ -122,7 +130,7 @@ static int hisi_inno_phy_exit(struct phy *phy)
struct hisi_inno_phy_priv *priv = port->priv;
reset_control_assert(port->utmi_rst);
- reset_control_assert(priv->por_rst);
+ reset_control_assert(priv->rsts);
clk_disable_unprepare(priv->ref_clk);
return 0;
@@ -158,15 +166,16 @@ static int hisi_inno_phy_probe(struct platform_device *pdev)
if (IS_ERR(priv->ref_clk))
return PTR_ERR(priv->ref_clk);
- priv->por_rst = devm_reset_control_get_exclusive(dev, NULL);
- if (IS_ERR(priv->por_rst))
- return PTR_ERR(priv->por_rst);
+ priv->rsts = devm_reset_control_array_get_exclusive(dev);
+ if (IS_ERR(priv->rsts))
+ return PTR_ERR(priv->rsts);
priv->type = (uintptr_t) of_device_get_match_data(dev);
for_each_child_of_node(np, child) {
struct reset_control *rst;
struct phy *phy;
+ void __iomem *base;
rst = of_reset_control_get_exclusive(child, NULL);
if (IS_ERR(rst)) {
@@ -174,7 +183,10 @@ static int hisi_inno_phy_probe(struct platform_device *pdev)
return PTR_ERR(rst);
}
+ base = of_iomap(child, 0);
+
priv->ports[i].utmi_rst = rst;
+ priv->ports[i].base = base;
priv->ports[i].priv = priv;
phy = devm_phy_create(dev, child, &hisi_inno_phy_ops);
@@ -205,6 +217,8 @@ static const struct of_device_id hisi_inno_phy_of_match[] = {
.data = (void *) PHY_TYPE_0 },
{ .compatible = "hisilicon,hi3798mv100-usb2-phy",
.data = (void *) PHY_TYPE_1 },
+ { .compatible = "hisilicon,hi3798mv200-usb2-phy",
+ .data = (void *) PHY_TYPE_MMIO },
{ },
};
MODULE_DEVICE_TABLE(of, hisi_inno_phy_of_match);
--
2.43.0
On 05-03-24, 21:32, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <[email protected]>
That is quite an email id!
>
> Direct MMIO resgiter access is used by Hi3798MV200. For other models,
> of_iomap() returns NULL due to insufficient length. So they are
so how is that fixed... Pls describe the change...
> unaffected.
>
> Also Hi3798MV200 INNO PHY has an extra reset required to be deasserted,
> switch to reset_control_array_*() APIs for that.
That probably should be a different patch
>
> Signed-off-by: Yang Xiwen <[email protected]>
> ---
> drivers/phy/hisilicon/phy-hisi-inno-usb2.c | 66 ++++++++++++++++++------------
> 1 file changed, 40 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/phy/hisilicon/phy-hisi-inno-usb2.c b/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
> index b7e740eb4752..df154cd99ed8 100644
> --- a/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
> +++ b/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
> @@ -10,6 +10,7 @@
> #include <linux/io.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/of_address.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> #include <linux/reset.h>
> @@ -24,6 +25,7 @@
>
> #define PHY_TYPE_0 0
> #define PHY_TYPE_1 1
> +#define PHY_TYPE_MMIO 2
>
> #define PHY_TEST_DATA GENMASK(7, 0)
> #define PHY_TEST_ADDR_OFFSET 8
> @@ -43,6 +45,7 @@
> #define PHY_CLK_ENABLE BIT(2)
>
> struct hisi_inno_phy_port {
> + void __iomem *base;
> struct reset_control *utmi_rst;
> struct hisi_inno_phy_priv *priv;
> };
> @@ -50,7 +53,7 @@ struct hisi_inno_phy_port {
> struct hisi_inno_phy_priv {
> void __iomem *mmio;
> struct clk *ref_clk;
> - struct reset_control *por_rst;
> + struct reset_control *rsts;
> unsigned int type;
> struct hisi_inno_phy_port ports[INNO_PHY_PORT_NUM];
> };
> @@ -62,26 +65,31 @@ static void hisi_inno_phy_write_reg(struct hisi_inno_phy_priv *priv,
> u32 val;
> u32 value;
>
> - if (priv->type == PHY_TYPE_0)
> - val = (data & PHY_TEST_DATA) |
> - ((addr << PHY_TEST_ADDR_OFFSET) & PHY0_TEST_ADDR) |
> - ((port << PHY0_TEST_PORT_OFFSET) & PHY0_TEST_PORT) |
> - PHY0_TEST_WREN | PHY0_TEST_RST;
> - else
> - val = (data & PHY_TEST_DATA) |
> - ((addr << PHY_TEST_ADDR_OFFSET) & PHY1_TEST_ADDR) |
> - ((port << PHY1_TEST_PORT_OFFSET) & PHY1_TEST_PORT) |
> - PHY1_TEST_WREN | PHY1_TEST_RST;
> - writel(val, reg);
> -
> - value = val;
> - if (priv->type == PHY_TYPE_0)
> - value |= PHY0_TEST_CLK;
> - else
> - value |= PHY1_TEST_CLK;
> - writel(value, reg);
> -
> - writel(val, reg);
> + if (priv->ports[port].base)
> + /* FIXME: fill stride in priv */
when?
> + writel(data, (u32 *)priv->ports[port].base + addr);
> + else {
> + if (priv->type == PHY_TYPE_0)
> + val = (data & PHY_TEST_DATA) |
> + ((addr << PHY_TEST_ADDR_OFFSET) & PHY0_TEST_ADDR) |
> + ((port << PHY0_TEST_PORT_OFFSET) & PHY0_TEST_PORT) |
> + PHY0_TEST_WREN | PHY0_TEST_RST;
> + else
> + val = (data & PHY_TEST_DATA) |
> + ((addr << PHY_TEST_ADDR_OFFSET) & PHY1_TEST_ADDR) |
> + ((port << PHY1_TEST_PORT_OFFSET) & PHY1_TEST_PORT) |
> + PHY1_TEST_WREN | PHY1_TEST_RST;
> + writel(val, reg);
> +
> + value = val;
> + if (priv->type == PHY_TYPE_0)
> + value |= PHY0_TEST_CLK;
> + else
> + value |= PHY1_TEST_CLK;
> + writel(value, reg);
> +
> + writel(val, reg);
val and value are very helpful variables, do consider naming them
better!
> + }
> }
>
> static void hisi_inno_phy_setup(struct hisi_inno_phy_priv *priv)
> @@ -104,7 +112,7 @@ static int hisi_inno_phy_init(struct phy *phy)
> return ret;
> udelay(REF_CLK_STABLE_TIME);
>
> - reset_control_deassert(priv->por_rst);
> + reset_control_deassert(priv->rsts);
> udelay(POR_RST_COMPLETE_TIME);
>
> /* Set up phy registers */
> @@ -122,7 +130,7 @@ static int hisi_inno_phy_exit(struct phy *phy)
> struct hisi_inno_phy_priv *priv = port->priv;
>
> reset_control_assert(port->utmi_rst);
> - reset_control_assert(priv->por_rst);
> + reset_control_assert(priv->rsts);
> clk_disable_unprepare(priv->ref_clk);
>
> return 0;
> @@ -158,15 +166,16 @@ static int hisi_inno_phy_probe(struct platform_device *pdev)
> if (IS_ERR(priv->ref_clk))
> return PTR_ERR(priv->ref_clk);
>
> - priv->por_rst = devm_reset_control_get_exclusive(dev, NULL);
> - if (IS_ERR(priv->por_rst))
> - return PTR_ERR(priv->por_rst);
> + priv->rsts = devm_reset_control_array_get_exclusive(dev);
> + if (IS_ERR(priv->rsts))
> + return PTR_ERR(priv->rsts);
>
> priv->type = (uintptr_t) of_device_get_match_data(dev);
>
> for_each_child_of_node(np, child) {
> struct reset_control *rst;
> struct phy *phy;
> + void __iomem *base;
>
> rst = of_reset_control_get_exclusive(child, NULL);
> if (IS_ERR(rst)) {
> @@ -174,7 +183,10 @@ static int hisi_inno_phy_probe(struct platform_device *pdev)
> return PTR_ERR(rst);
> }
>
> + base = of_iomap(child, 0);
> +
> priv->ports[i].utmi_rst = rst;
> + priv->ports[i].base = base;
> priv->ports[i].priv = priv;
>
> phy = devm_phy_create(dev, child, &hisi_inno_phy_ops);
> @@ -205,6 +217,8 @@ static const struct of_device_id hisi_inno_phy_of_match[] = {
> .data = (void *) PHY_TYPE_0 },
> { .compatible = "hisilicon,hi3798mv100-usb2-phy",
> .data = (void *) PHY_TYPE_1 },
> + { .compatible = "hisilicon,hi3798mv200-usb2-phy",
> + .data = (void *) PHY_TYPE_MMIO },
> { },
> };
> MODULE_DEVICE_TABLE(of, hisi_inno_phy_of_match);
>
> --
> 2.43.0
--
~Vinod
On 4/6/2024 12:52 AM, Vinod Koul wrote:
> On 05-03-24, 21:32, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <[email protected]>
> That is quite an email id!
>
>> Direct MMIO resgiter access is used by Hi3798MV200. For other models,
>> of_iomap() returns NULL due to insufficient length. So they are
> so how is that fixed... Pls describe the change...
The commit log will be rewritten in next revision. I'll try to emphasize
the PHY and its configuration interface briefly. Though i don't have
access to the datasheets and TRM so most things can not be verified.
For CV200 and MV100 INNO PHY, the configuration interface is attached to
PERICTRL(Peripheral Control Block). So we just use a register called
PERI_USB3 to configure the PHY. The bus reset, clock are all controlled
in PERI_USB3 register. To read/write to a register of the PHY, a special
sequence of register writes and reads are needed, which was implemented
in this driver.
But for MV200 INNO PHY, the configuration interface is attached directly
to system bus(MMIO). The bus clocks and resets are controlled via Clock
Reset Generator(CRG). Now we have to control them with the help of linux
clk and reset framework because they are provided by other modules.
>> unaffected.
>>
>> Also Hi3798MV200 INNO PHY has an extra reset required to be deasserted,
>> switch to reset_control_array_*() APIs for that.
The commit msg is misleading here. There is no extra reset actually. The
reset also exist for existing users. The initial author just decided to
manage it in the hisi_inno_phy_write_reg() routine(without using
reset_control_* APIs) and omit it in the binding.
> That probably should be a different patch
I guess so. From my point of view, the whole patch is to introduce the
support for Hi3798MV200 variant of the INNO PHY. So i've decided to
squash the two changes into one single commit.
>
>> Signed-off-by: Yang Xiwen <[email protected]>
>> ---
>> drivers/phy/hisilicon/phy-hisi-inno-usb2.c | 66 ++++++++++++++++++------------
>> 1 file changed, 40 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/phy/hisilicon/phy-hisi-inno-usb2.c b/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
>> index b7e740eb4752..df154cd99ed8 100644
>> --- a/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
>> +++ b/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
>> @@ -10,6 +10,7 @@
>> #include <linux/io.h>
>> #include <linux/module.h>
>> #include <linux/of.h>
>> +#include <linux/of_address.h>
>> #include <linux/phy/phy.h>
>> #include <linux/platform_device.h>
>> #include <linux/reset.h>
>> @@ -24,6 +25,7 @@
>>
>> #define PHY_TYPE_0 0
>> #define PHY_TYPE_1 1
>> +#define PHY_TYPE_MMIO 2
>>
>> #define PHY_TEST_DATA GENMASK(7, 0)
>> #define PHY_TEST_ADDR_OFFSET 8
>> @@ -43,6 +45,7 @@
>> #define PHY_CLK_ENABLE BIT(2)
>>
>> struct hisi_inno_phy_port {
>> + void __iomem *base;
>> struct reset_control *utmi_rst;
>> struct hisi_inno_phy_priv *priv;
>> };
>> @@ -50,7 +53,7 @@ struct hisi_inno_phy_port {
>> struct hisi_inno_phy_priv {
>> void __iomem *mmio;
>> struct clk *ref_clk;
>> - struct reset_control *por_rst;
>> + struct reset_control *rsts;
>> unsigned int type;
>> struct hisi_inno_phy_port ports[INNO_PHY_PORT_NUM];
>> };
>> @@ -62,26 +65,31 @@ static void hisi_inno_phy_write_reg(struct hisi_inno_phy_priv *priv,
>> u32 val;
>> u32 value;
>>
>> - if (priv->type == PHY_TYPE_0)
>> - val = (data & PHY_TEST_DATA) |
>> - ((addr << PHY_TEST_ADDR_OFFSET) & PHY0_TEST_ADDR) |
>> - ((port << PHY0_TEST_PORT_OFFSET) & PHY0_TEST_PORT) |
>> - PHY0_TEST_WREN | PHY0_TEST_RST;
>> - else
>> - val = (data & PHY_TEST_DATA) |
>> - ((addr << PHY_TEST_ADDR_OFFSET) & PHY1_TEST_ADDR) |
>> - ((port << PHY1_TEST_PORT_OFFSET) & PHY1_TEST_PORT) |
>> - PHY1_TEST_WREN | PHY1_TEST_RST;
>> - writel(val, reg);
>> -
>> - value = val;
>> - if (priv->type == PHY_TYPE_0)
>> - value |= PHY0_TEST_CLK;
>> - else
>> - value |= PHY1_TEST_CLK;
>> - writel(value, reg);
>> -
>> - writel(val, reg);
>> + if (priv->ports[port].base)
>> + /* FIXME: fill stride in priv */
> when?
I'm not sure. Maybe until some other users with stride other than 3? I
don't have much knowledge about other SoCs.
Maybe replace the FIXME here with some additional information.
>
>> + writel(data, (u32 *)priv->ports[port].base + addr);
>> + else {
>> + if (priv->type == PHY_TYPE_0)
>> + val = (data & PHY_TEST_DATA) |
>> + ((addr << PHY_TEST_ADDR_OFFSET) & PHY0_TEST_ADDR) |
>> + ((port << PHY0_TEST_PORT_OFFSET) & PHY0_TEST_PORT) |
>> + PHY0_TEST_WREN | PHY0_TEST_RST;
>> + else
>> + val = (data & PHY_TEST_DATA) |
>> + ((addr << PHY_TEST_ADDR_OFFSET) & PHY1_TEST_ADDR) |
>> + ((port << PHY1_TEST_PORT_OFFSET) & PHY1_TEST_PORT) |
>> + PHY1_TEST_WREN | PHY1_TEST_RST;
>> + writel(val, reg);
>> +
>> + value = val;
>> + if (priv->type == PHY_TYPE_0)
>> + value |= PHY0_TEST_CLK;
>> + else
>> + value |= PHY1_TEST_CLK;
>> + writel(value, reg);
>> +
>> + writel(val, reg);
> val and value are very helpful variables, do consider naming them
> better!
I'll consider renaming them in the next revision. Maybe val and val2?
They are just some temp vars to store register values.
>
>> + }
>> }
>>
>> static void hisi_inno_phy_setup(struct hisi_inno_phy_priv *priv)
>> @@ -104,7 +112,7 @@ static int hisi_inno_phy_init(struct phy *phy)
>> return ret;
>> udelay(REF_CLK_STABLE_TIME);
>>
>> - reset_control_deassert(priv->por_rst);
>> + reset_control_deassert(priv->rsts);
>> udelay(POR_RST_COMPLETE_TIME);
>>
>> /* Set up phy registers */
>> @@ -122,7 +130,7 @@ static int hisi_inno_phy_exit(struct phy *phy)
>> struct hisi_inno_phy_priv *priv = port->priv;
>>
>> reset_control_assert(port->utmi_rst);
>> - reset_control_assert(priv->por_rst);
>> + reset_control_assert(priv->rsts);
>> clk_disable_unprepare(priv->ref_clk);
>>
>> return 0;
>> @@ -158,15 +166,16 @@ static int hisi_inno_phy_probe(struct platform_device *pdev)
>> if (IS_ERR(priv->ref_clk))
>> return PTR_ERR(priv->ref_clk);
>>
>> - priv->por_rst = devm_reset_control_get_exclusive(dev, NULL);
>> - if (IS_ERR(priv->por_rst))
>> - return PTR_ERR(priv->por_rst);
>> + priv->rsts = devm_reset_control_array_get_exclusive(dev);
>> + if (IS_ERR(priv->rsts))
>> + return PTR_ERR(priv->rsts);
>>
>> priv->type = (uintptr_t) of_device_get_match_data(dev);
>>
>> for_each_child_of_node(np, child) {
>> struct reset_control *rst;
>> struct phy *phy;
>> + void __iomem *base;
>>
>> rst = of_reset_control_get_exclusive(child, NULL);
>> if (IS_ERR(rst)) {
>> @@ -174,7 +183,10 @@ static int hisi_inno_phy_probe(struct platform_device *pdev)
>> return PTR_ERR(rst);
>> }
>>
>> + base = of_iomap(child, 0);
>> +
>> priv->ports[i].utmi_rst = rst;
>> + priv->ports[i].base = base;
>> priv->ports[i].priv = priv;
>>
>> phy = devm_phy_create(dev, child, &hisi_inno_phy_ops);
>> @@ -205,6 +217,8 @@ static const struct of_device_id hisi_inno_phy_of_match[] = {
>> .data = (void *) PHY_TYPE_0 },
>> { .compatible = "hisilicon,hi3798mv100-usb2-phy",
>> .data = (void *) PHY_TYPE_1 },
>> + { .compatible = "hisilicon,hi3798mv200-usb2-phy",
>> + .data = (void *) PHY_TYPE_MMIO },
>> { },
>> };
>> MODULE_DEVICE_TABLE(of, hisi_inno_phy_of_match);
>>
>> --
>> 2.43.0
--
Regards,
Yang Xiwen
On 06-04-24, 01:53, Yang Xiwen wrote:
> On 4/6/2024 12:52 AM, Vinod Koul wrote:
> > On 05-03-24, 21:32, Yang Xiwen via B4 Relay wrote:
> > > From: Yang Xiwen <[email protected]>
> > That is quite an email id!
> >
> > > Direct MMIO resgiter access is used by Hi3798MV200. For other models,
> > > of_iomap() returns NULL due to insufficient length. So they are
> > so how is that fixed... Pls describe the change...
>
>
> The commit log will be rewritten in next revision. I'll try to emphasize the
> PHY and its configuration interface briefly. Though i don't have access to
> the datasheets and TRM so most things can not be verified.
>
>
> For CV200 and MV100 INNO PHY, the configuration interface is attached to
> PERICTRL(Peripheral Control Block). So we just use a register called
> PERI_USB3 to configure the PHY. The bus reset, clock are all controlled in
> PERI_USB3 register. To read/write to a register of the PHY, a special
> sequence of register writes and reads are needed, which was implemented in
> this driver.
>
>
> But for MV200 INNO PHY, the configuration interface is attached directly to
> system bus(MMIO). The bus clocks and resets are controlled via Clock Reset
> Generator(CRG). Now we have to control them with the help of linux clk and
> reset framework because they are provided by other modules.
Okay better log is welcome
>
>
> > > unaffected.
> > >
> > > Also Hi3798MV200 INNO PHY has an extra reset required to be deasserted,
> > > switch to reset_control_array_*() APIs for that.
>
>
> The commit msg is misleading here. There is no extra reset actually. The
> reset also exist for existing users. The initial author just decided to
> manage it in the hisi_inno_phy_write_reg() routine(without using
> reset_control_* APIs) and omit it in the binding.
>
>
> > That probably should be a different patch
>
>
> I guess so. From my point of view, the whole patch is to introduce the
> support for Hi3798MV200 variant of the INNO PHY. So i've decided to squash
> the two changes into one single commit.
Not really you can build smaller reviewable changes leading up to adding
the Hi3798MV200 support
>
>
> >
> > > Signed-off-by: Yang Xiwen <[email protected]>
> > > ---
> > > drivers/phy/hisilicon/phy-hisi-inno-usb2.c | 66 ++++++++++++++++++------------
> > > 1 file changed, 40 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/phy/hisilicon/phy-hisi-inno-usb2.c b/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
> > > index b7e740eb4752..df154cd99ed8 100644
> > > --- a/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
> > > +++ b/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
> > > @@ -10,6 +10,7 @@
> > > #include <linux/io.h>
> > > #include <linux/module.h>
> > > #include <linux/of.h>
> > > +#include <linux/of_address.h>
> > > #include <linux/phy/phy.h>
> > > #include <linux/platform_device.h>
> > > #include <linux/reset.h>
> > > @@ -24,6 +25,7 @@
> > > #define PHY_TYPE_0 0
> > > #define PHY_TYPE_1 1
> > > +#define PHY_TYPE_MMIO 2
> > > #define PHY_TEST_DATA GENMASK(7, 0)
> > > #define PHY_TEST_ADDR_OFFSET 8
> > > @@ -43,6 +45,7 @@
> > > #define PHY_CLK_ENABLE BIT(2)
> > > struct hisi_inno_phy_port {
> > > + void __iomem *base;
> > > struct reset_control *utmi_rst;
> > > struct hisi_inno_phy_priv *priv;
> > > };
> > > @@ -50,7 +53,7 @@ struct hisi_inno_phy_port {
> > > struct hisi_inno_phy_priv {
> > > void __iomem *mmio;
> > > struct clk *ref_clk;
> > > - struct reset_control *por_rst;
> > > + struct reset_control *rsts;
> > > unsigned int type;
> > > struct hisi_inno_phy_port ports[INNO_PHY_PORT_NUM];
> > > };
> > > @@ -62,26 +65,31 @@ static void hisi_inno_phy_write_reg(struct hisi_inno_phy_priv *priv,
> > > u32 val;
> > > u32 value;
> > > - if (priv->type == PHY_TYPE_0)
> > > - val = (data & PHY_TEST_DATA) |
> > > - ((addr << PHY_TEST_ADDR_OFFSET) & PHY0_TEST_ADDR) |
> > > - ((port << PHY0_TEST_PORT_OFFSET) & PHY0_TEST_PORT) |
> > > - PHY0_TEST_WREN | PHY0_TEST_RST;
> > > - else
> > > - val = (data & PHY_TEST_DATA) |
> > > - ((addr << PHY_TEST_ADDR_OFFSET) & PHY1_TEST_ADDR) |
> > > - ((port << PHY1_TEST_PORT_OFFSET) & PHY1_TEST_PORT) |
> > > - PHY1_TEST_WREN | PHY1_TEST_RST;
> > > - writel(val, reg);
> > > -
> > > - value = val;
> > > - if (priv->type == PHY_TYPE_0)
> > > - value |= PHY0_TEST_CLK;
> > > - else
> > > - value |= PHY1_TEST_CLK;
> > > - writel(value, reg);
> > > -
> > > - writel(val, reg);
> > > + if (priv->ports[port].base)
> > > + /* FIXME: fill stride in priv */
> > when?
>
>
> I'm not sure. Maybe until some other users with stride other than 3? I don't
> have much knowledge about other SoCs.
>
>
> Maybe replace the FIXME here with some additional information.
Better
>
>
> >
> > > + writel(data, (u32 *)priv->ports[port].base + addr);
> > > + else {
> > > + if (priv->type == PHY_TYPE_0)
> > > + val = (data & PHY_TEST_DATA) |
> > > + ((addr << PHY_TEST_ADDR_OFFSET) & PHY0_TEST_ADDR) |
> > > + ((port << PHY0_TEST_PORT_OFFSET) & PHY0_TEST_PORT) |
> > > + PHY0_TEST_WREN | PHY0_TEST_RST;
> > > + else
> > > + val = (data & PHY_TEST_DATA) |
> > > + ((addr << PHY_TEST_ADDR_OFFSET) & PHY1_TEST_ADDR) |
> > > + ((port << PHY1_TEST_PORT_OFFSET) & PHY1_TEST_PORT) |
> > > + PHY1_TEST_WREN | PHY1_TEST_RST;
> > > + writel(val, reg);
> > > +
> > > + value = val;
> > > + if (priv->type == PHY_TYPE_0)
> > > + value |= PHY0_TEST_CLK;
> > > + else
> > > + value |= PHY1_TEST_CLK;
> > > + writel(value, reg);
> > > +
> > > + writel(val, reg);
> > val and value are very helpful variables, do consider naming them
> > better!
>
>
> I'll consider renaming them in the next revision. Maybe val and val2? They
> are just some temp vars to store register values.
Yeah, that might be better
>
>
> >
> > > + }
> > > }
> > > static void hisi_inno_phy_setup(struct hisi_inno_phy_priv *priv)
> > > @@ -104,7 +112,7 @@ static int hisi_inno_phy_init(struct phy *phy)
> > > return ret;
> > > udelay(REF_CLK_STABLE_TIME);
> > > - reset_control_deassert(priv->por_rst);
> > > + reset_control_deassert(priv->rsts);
> > > udelay(POR_RST_COMPLETE_TIME);
> > > /* Set up phy registers */
> > > @@ -122,7 +130,7 @@ static int hisi_inno_phy_exit(struct phy *phy)
> > > struct hisi_inno_phy_priv *priv = port->priv;
> > > reset_control_assert(port->utmi_rst);
> > > - reset_control_assert(priv->por_rst);
> > > + reset_control_assert(priv->rsts);
> > > clk_disable_unprepare(priv->ref_clk);
> > > return 0;
> > > @@ -158,15 +166,16 @@ static int hisi_inno_phy_probe(struct platform_device *pdev)
> > > if (IS_ERR(priv->ref_clk))
> > > return PTR_ERR(priv->ref_clk);
> > > - priv->por_rst = devm_reset_control_get_exclusive(dev, NULL);
> > > - if (IS_ERR(priv->por_rst))
> > > - return PTR_ERR(priv->por_rst);
> > > + priv->rsts = devm_reset_control_array_get_exclusive(dev);
> > > + if (IS_ERR(priv->rsts))
> > > + return PTR_ERR(priv->rsts);
> > > priv->type = (uintptr_t) of_device_get_match_data(dev);
> > > for_each_child_of_node(np, child) {
> > > struct reset_control *rst;
> > > struct phy *phy;
> > > + void __iomem *base;
> > > rst = of_reset_control_get_exclusive(child, NULL);
> > > if (IS_ERR(rst)) {
> > > @@ -174,7 +183,10 @@ static int hisi_inno_phy_probe(struct platform_device *pdev)
> > > return PTR_ERR(rst);
> > > }
> > > + base = of_iomap(child, 0);
> > > +
> > > priv->ports[i].utmi_rst = rst;
> > > + priv->ports[i].base = base;
> > > priv->ports[i].priv = priv;
> > > phy = devm_phy_create(dev, child, &hisi_inno_phy_ops);
> > > @@ -205,6 +217,8 @@ static const struct of_device_id hisi_inno_phy_of_match[] = {
> > > .data = (void *) PHY_TYPE_0 },
> > > { .compatible = "hisilicon,hi3798mv100-usb2-phy",
> > > .data = (void *) PHY_TYPE_1 },
> > > + { .compatible = "hisilicon,hi3798mv200-usb2-phy",
> > > + .data = (void *) PHY_TYPE_MMIO },
> > > { },
> > > };
> > > MODULE_DEVICE_TABLE(of, hisi_inno_phy_of_match);
> > >
> > > --
> > > 2.43.0
>
>
> --
> Regards,
> Yang Xiwen
--
~Vinod