| Message ID | 20201230042208.8997-1-dqfext@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Registering IRQ for MT7530 internal PHYs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: linux-mediatek@lists.infradead.org linux-arm-kernel@lists.infradead.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | fail | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 0 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Alignment should match open parenthesis WARNING: line length of 108 exceeds 80 columns |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 0 this patch: 1 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On 30.12.2020 05:22, DENG Qingfang wrote: > Hi, > > I added MT7530 IRQ support and registered its internal PHYs to IRQ. > It works but my patch used two hacks. > > 1. Removed phy_drv_supports_irq check, because config_intr and > handle_interrupt are not set for Generic PHY. > I don't think that's the best option. First, did you check which consequences this has? The check is there for a reason. You may want to add a PHY driver for your chip. Supposedly it supports at least PHY suspend/resume. You can use the RTL8366RB PHY driver as template. > 2. Allocated ds->slave_mii_bus before calling ds->ops->setup, because > we cannot call dsa_slave_mii_bus_init which is private. > > Any better ideas? > > Regards, > Qingfang > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index a67cac15a724..d59a8c50ede3 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -10,6 +10,7 @@ > #include <linux/mfd/syscon.h> > #include <linux/module.h> > #include <linux/netdevice.h> > +#include <linux/of_irq.h> > #include <linux/of_mdio.h> > #include <linux/of_net.h> > #include <linux/of_platform.h> > @@ -1639,6 +1640,125 @@ mtk_get_tag_protocol(struct dsa_switch *ds, int port, > } > } > > +static irqreturn_t > +mt7530_irq(int irq, void *data) > +{ > + struct mt7530_priv *priv = data; > + bool handled = false; > + int phy; > + u32 val; > + > + val = mt7530_read(priv, MT7530_SYS_INT_STS); > + mt7530_write(priv, MT7530_SYS_INT_STS, val); > + > + dev_info_ratelimited(priv->dev, "interrupt status: 0x%08x\n", val); > + dev_info_ratelimited(priv->dev, "interrupt enable: 0x%08x\n", mt7530_read(priv, MT7530_SYS_INT_EN)); > + This is debug code to be removed in the final version? > + for (phy = 0; phy < MT7530_NUM_PHYS; phy++) { > + if (val & BIT(phy)) { > + unsigned int child_irq; > + > + child_irq = irq_find_mapping(priv->irq_domain, phy); > + handle_nested_irq(child_irq); > + handled = true; > + } > + } > + > + return handled ? IRQ_HANDLED : IRQ_NONE; IRQ_RETVAL() could be used here. > +} > + > +static void mt7530_irq_mask(struct irq_data *d) > +{ > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); > + > + priv->irq_enable &= ~BIT(d->hwirq); Here you don't actually do something. HW doesn't support masking interrupt generation for a port? > +} > + > +static void mt7530_irq_unmask(struct irq_data *d) > +{ > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); > + > + priv->irq_enable |= BIT(d->hwirq); > +} > + > +static void mt7530_irq_bus_lock(struct irq_data *d) > +{ > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); > + > + mutex_lock(&priv->reg_mutex); > +} > + > +static void mt7530_irq_bus_sync_unlock(struct irq_data *d) > +{ > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); > + > + mt7530_write(priv, MT7530_SYS_INT_EN, priv->irq_enable); > + mutex_unlock(&priv->reg_mutex); > +} > + > +static struct irq_chip mt7530_irq_chip = { > + .name = MT7530_NAME, > + .irq_mask = mt7530_irq_mask, > + .irq_unmask = mt7530_irq_unmask, > + .irq_bus_lock = mt7530_irq_bus_lock, > + .irq_bus_sync_unlock = mt7530_irq_bus_sync_unlock, > +}; > + > +static int > +mt7530_irq_map(struct irq_domain *domain, unsigned int irq, > + irq_hw_number_t hwirq) > +{ > + irq_set_chip_data(irq, domain->host_data); > + irq_set_chip_and_handler(irq, &mt7530_irq_chip, handle_simple_irq); > + irq_set_noprobe(irq); > + > + return 0; > +} > + > +static const struct irq_domain_ops mt7530_irq_domain_ops = { > + .map = mt7530_irq_map, > + .xlate = irq_domain_xlate_onecell, > +}; > + > +static void > +mt7530_irq_init(struct mt7530_priv *priv) > +{ > + struct mii_bus *bus = priv->ds->slave_mii_bus; > + struct device *dev = priv->dev; > + struct device_node *np = dev->of_node; > + int parent_irq; > + int phy, ret; > + > + parent_irq = of_irq_get(np, 0); > + if (parent_irq <= 0) { > + dev_err(dev, "failed to get parent IRQ: %d\n", parent_irq); > + return; > + } > + > + mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL); > + ret = devm_request_threaded_irq(dev, parent_irq, NULL, mt7530_irq, > + IRQF_ONESHOT, MT7530_NAME, priv); > + if (ret) { > + dev_err(dev, "failed to request IRQ: %d\n", ret); > + return; > + } > + > + priv->irq_domain = irq_domain_add_linear(np, MT7530_NUM_PHYS, > + &mt7530_irq_domain_ops, priv); > + if (!priv->irq_domain) { > + dev_err(dev, "failed to create IRQ domain\n"); > + return; > + } > + > + /* IRQ for internal PHYs */ > + for (phy = 0; phy < MT7530_NUM_PHYS; phy++) { > + unsigned int irq = irq_create_mapping(priv->irq_domain, phy); > + > + irq_set_parent(irq, parent_irq); > + bus->irq[phy] = irq; > + } > +} > + > static int > mt7530_setup(struct dsa_switch *ds) > { > @@ -2578,8 +2698,13 @@ static int > mt753x_setup(struct dsa_switch *ds) > { > struct mt7530_priv *priv = ds->priv; > + int ret = priv->info->sw_setup(ds); > > - return priv->info->sw_setup(ds); > + /* Setup interrupt */ > + if (!ret) > + mt7530_irq_init(priv); > + > + return ret; > } > > static int > @@ -2780,6 +2905,9 @@ mt7530_remove(struct mdio_device *mdiodev) > dev_err(priv->dev, "Failed to disable io pwr: %d\n", > ret); > > + if (priv->irq_domain) > + irq_domain_remove(priv->irq_domain); > + > dsa_unregister_switch(priv->ds); > mutex_destroy(&priv->reg_mutex); > } > @@ -2788,7 +2916,7 @@ static struct mdio_driver mt7530_mdio_driver = { > .probe = mt7530_probe, > .remove = mt7530_remove, > .mdiodrv.driver = { > - .name = "mt7530", > + .name = MT7530_NAME, > .of_match_table = mt7530_of_match, > }, > }; > diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h > index 32d8969b3ace..b1988d8085bb 100644 > --- a/drivers/net/dsa/mt7530.h > +++ b/drivers/net/dsa/mt7530.h > @@ -6,7 +6,10 @@ > #ifndef __MT7530_H > #define __MT7530_H > > +#define MT7530_NAME "mt7530" > + > #define MT7530_NUM_PORTS 7 > +#define MT7530_NUM_PHYS 5 > #define MT7530_CPU_PORT 6 > #define MT7530_NUM_FDB_RECORDS 2048 > #define MT7530_ALL_MEMBERS 0xff > @@ -380,6 +383,12 @@ enum mt7531_sgmii_force_duplex { > #define SYS_CTRL_SW_RST BIT(1) > #define SYS_CTRL_REG_RST BIT(0) > > +/* Register for system interrupt */ > +#define MT7530_SYS_INT_EN 0x7008 > + > +/* Register for system interrupt status */ > +#define MT7530_SYS_INT_STS 0x700c > + > /* Register for PHY Indirect Access Control */ > #define MT7531_PHY_IAC 0x701C > #define MT7531_PHY_ACS_ST BIT(31) > @@ -761,6 +770,8 @@ struct mt7530_priv { > struct mt7530_port ports[MT7530_NUM_PORTS]; > /* protect among processes for registers access*/ > struct mutex reg_mutex; > + struct irq_domain *irq_domain; > + u32 irq_enable; > }; > > struct mt7530_hw_vlan_entry { > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 80c2e646c093..cdddc27e2df7 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -2846,12 +2846,6 @@ static int phy_probe(struct device *dev) > > phydev->drv = phydrv; > > - /* Disable the interrupt if the PHY doesn't support it > - * but the interrupt is still a valid one > - */ > - if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev)) > - phydev->irq = PHY_POLL; > - > if (phydrv->flags & PHY_IS_INTERNAL) > phydev->is_internal = true; > > diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c > index 183003e45762..ef5db1106581 100644 > --- a/net/dsa/dsa2.c > +++ b/net/dsa/dsa2.c > @@ -448,19 +448,19 @@ static int dsa_switch_setup(struct dsa_switch *ds) > if (err) > goto unregister_devlink_ports; > > + ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev); > + if (!ds->slave_mii_bus) { > + err = -ENOMEM; > + goto unregister_notifier; > + } > + > err = ds->ops->setup(ds); > if (err < 0) > goto unregister_notifier; > > devlink_params_publish(ds->devlink); > > - if (!ds->slave_mii_bus && ds->ops->phy_read) { > - ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev); > - if (!ds->slave_mii_bus) { > - err = -ENOMEM; > - goto unregister_notifier; > - } > - > + if (ds->ops->phy_read) { > dsa_slave_mii_bus_init(ds); > > err = mdiobus_register(ds->slave_mii_bus); >
Hi Heiner, Thanks for your reply. On Wed, Dec 30, 2020 at 3:39 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > I don't think that's the best option. I'm well aware of that. > You may want to add a PHY driver for your chip. Supposedly it > supports at least PHY suspend/resume. You can use the RTL8366RB > PHY driver as template. There's no MediaTek PHY driver yet. Do we really need a new one just for the interrupts? > > + dev_info_ratelimited(priv->dev, "interrupt status: 0x%08x\n", val); > > + dev_info_ratelimited(priv->dev, "interrupt enable: 0x%08x\n", mt7530_read(priv, MT7530_SYS_INT_EN)); > > + > This is debug code to be removed in the final version? Yes. > > + for (phy = 0; phy < MT7530_NUM_PHYS; phy++) { > > + if (val & BIT(phy)) { > > + unsigned int child_irq; > > + > > + child_irq = irq_find_mapping(priv->irq_domain, phy); > > + handle_nested_irq(child_irq); > > + handled = true; > > + } > > + } > > + > > + return handled ? IRQ_HANDLED : IRQ_NONE; > > IRQ_RETVAL() could be used here. Good to know :) > > > +} > > + > > +static void mt7530_irq_mask(struct irq_data *d) > > +{ > > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); > > + > > + priv->irq_enable &= ~BIT(d->hwirq); > > Here you don't actually do something. HW doesn't support masking > interrupt generation for a port? priv->irq_enable will be written to MT7530_SYS_INT_EN in mt7530_irq_bus_sync_unlock. You can think of it as an inverted mask.
On 30.12.2020 10:07, DENG Qingfang wrote: > Hi Heiner, > Thanks for your reply. > > On Wed, Dec 30, 2020 at 3:39 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: >> I don't think that's the best option. > > I'm well aware of that. > >> You may want to add a PHY driver for your chip. Supposedly it >> supports at least PHY suspend/resume. You can use the RTL8366RB >> PHY driver as template. > > There's no MediaTek PHY driver yet. Do we really need a new one just > for the interrupts? > Not only for the interrupts. The genphy driver e.g. doesn't support PHY suspend/resume. And the PHY driver needs basically no code, just set the proper callbacks. >>> + dev_info_ratelimited(priv->dev, "interrupt status: 0x%08x\n", val); >>> + dev_info_ratelimited(priv->dev, "interrupt enable: 0x%08x\n", mt7530_read(priv, MT7530_SYS_INT_EN)); >>> + >> This is debug code to be removed in the final version? > > Yes. > >>> + for (phy = 0; phy < MT7530_NUM_PHYS; phy++) { >>> + if (val & BIT(phy)) { >>> + unsigned int child_irq; >>> + >>> + child_irq = irq_find_mapping(priv->irq_domain, phy); >>> + handle_nested_irq(child_irq); >>> + handled = true; >>> + } >>> + } >>> + >>> + return handled ? IRQ_HANDLED : IRQ_NONE; >> >> IRQ_RETVAL() could be used here. > > Good to know :) > >> >>> +} >>> + >>> +static void mt7530_irq_mask(struct irq_data *d) >>> +{ >>> + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); >>> + >>> + priv->irq_enable &= ~BIT(d->hwirq); >> >> Here you don't actually do something. HW doesn't support masking >> interrupt generation for a port? > > priv->irq_enable will be written to MT7530_SYS_INT_EN in > mt7530_irq_bus_sync_unlock. You can think of it as an inverted mask. >
Hi Qingfang, On 2020-12-30 04:22, DENG Qingfang wrote: > Hi, > > I added MT7530 IRQ support and registered its internal PHYs to IRQ. > It works but my patch used two hacks. > > 1. Removed phy_drv_supports_irq check, because config_intr and > handle_interrupt are not set for Generic PHY. > > 2. Allocated ds->slave_mii_bus before calling ds->ops->setup, because > we cannot call dsa_slave_mii_bus_init which is private. > > Any better ideas? > > Regards, > Qingfang > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index a67cac15a724..d59a8c50ede3 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -10,6 +10,7 @@ > #include <linux/mfd/syscon.h> > #include <linux/module.h> > #include <linux/netdevice.h> > +#include <linux/of_irq.h> > #include <linux/of_mdio.h> > #include <linux/of_net.h> > #include <linux/of_platform.h> > @@ -1639,6 +1640,125 @@ mtk_get_tag_protocol(struct dsa_switch *ds, int > port, > } > } > > +static irqreturn_t > +mt7530_irq(int irq, void *data) > +{ > + struct mt7530_priv *priv = data; > + bool handled = false; > + int phy; > + u32 val; > + > + val = mt7530_read(priv, MT7530_SYS_INT_STS); > + mt7530_write(priv, MT7530_SYS_INT_STS, val); If that is an ack operation, it should be dealt with as such in an irqchip callback instead of being open-coded here. > + > + dev_info_ratelimited(priv->dev, "interrupt status: 0x%08x\n", val); > + dev_info_ratelimited(priv->dev, "interrupt enable: 0x%08x\n", > mt7530_read(priv, MT7530_SYS_INT_EN)); > + I don't think printing these from an interrupt handler is a good idea. Use the debug primitives if you really want these messages. > + for (phy = 0; phy < MT7530_NUM_PHYS; phy++) { > + if (val & BIT(phy)) { > + unsigned int child_irq; > + > + child_irq = irq_find_mapping(priv->irq_domain, phy); > + handle_nested_irq(child_irq); > + handled = true; > + } > + } > + > + return handled ? IRQ_HANDLED : IRQ_NONE; > +} What is the reason for not implementing this as a chained interrupt flow? > + > +static void mt7530_irq_mask(struct irq_data *d) > +{ > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); > + > + priv->irq_enable &= ~BIT(d->hwirq); > +} > + > +static void mt7530_irq_unmask(struct irq_data *d) > +{ > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); > + > + priv->irq_enable |= BIT(d->hwirq); > +} > + > +static void mt7530_irq_bus_lock(struct irq_data *d) > +{ > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); > + > + mutex_lock(&priv->reg_mutex); Are you always guaranteed to be in a thread context? I guess that is the case, given that you request a threaded interrupt, but it would be worth documenting. > +} > + > +static void mt7530_irq_bus_sync_unlock(struct irq_data *d) > +{ > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); > + > + mt7530_write(priv, MT7530_SYS_INT_EN, priv->irq_enable); > + mutex_unlock(&priv->reg_mutex); > +} > + > +static struct irq_chip mt7530_irq_chip = { > + .name = MT7530_NAME, > + .irq_mask = mt7530_irq_mask, > + .irq_unmask = mt7530_irq_unmask, > + .irq_bus_lock = mt7530_irq_bus_lock, > + .irq_bus_sync_unlock = mt7530_irq_bus_sync_unlock, > +}; > + > +static int > +mt7530_irq_map(struct irq_domain *domain, unsigned int irq, > + irq_hw_number_t hwirq) > +{ > + irq_set_chip_data(irq, domain->host_data); > + irq_set_chip_and_handler(irq, &mt7530_irq_chip, handle_simple_irq); > + irq_set_noprobe(irq); > + > + return 0; > +} > + > +static const struct irq_domain_ops mt7530_irq_domain_ops = { > + .map = mt7530_irq_map, > + .xlate = irq_domain_xlate_onecell, > +}; > + > +static void > +mt7530_irq_init(struct mt7530_priv *priv) > +{ > + struct mii_bus *bus = priv->ds->slave_mii_bus; > + struct device *dev = priv->dev; > + struct device_node *np = dev->of_node; > + int parent_irq; > + int phy, ret; > + > + parent_irq = of_irq_get(np, 0); > + if (parent_irq <= 0) { > + dev_err(dev, "failed to get parent IRQ: %d\n", parent_irq); > + return; It seems odd not to propagate the error, since I assume the device will not be functional. > + } > + > + mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL); > + ret = devm_request_threaded_irq(dev, parent_irq, NULL, mt7530_irq, > + IRQF_ONESHOT, MT7530_NAME, priv); > + if (ret) { > + dev_err(dev, "failed to request IRQ: %d\n", ret); > + return; > + } > + > + priv->irq_domain = irq_domain_add_linear(np, MT7530_NUM_PHYS, > + &mt7530_irq_domain_ops, priv); The creation order is... interesting. You have a handler ready to fire, nothing seems to initialise the HW state, and the priv data structure is not fully populated. If any interrupt is pending at this stage, you have a panic in your hands. > + if (!priv->irq_domain) { > + dev_err(dev, "failed to create IRQ domain\n"); > + return; > + } > + > + /* IRQ for internal PHYs */ > + for (phy = 0; phy < MT7530_NUM_PHYS; phy++) { > + unsigned int irq = irq_create_mapping(priv->irq_domain, phy); Why are you eagerly creating all the interrupt mappings? They should be created on demand as the endpoint drivers come up. > + > + irq_set_parent(irq, parent_irq); > + bus->irq[phy] = irq; > + } > +} > + > static int > mt7530_setup(struct dsa_switch *ds) > { > @@ -2578,8 +2698,13 @@ static int > mt753x_setup(struct dsa_switch *ds) > { > struct mt7530_priv *priv = ds->priv; > + int ret = priv->info->sw_setup(ds); > > - return priv->info->sw_setup(ds); > + /* Setup interrupt */ > + if (!ret) > + mt7530_irq_init(priv); > + > + return ret; > } > > static int > @@ -2780,6 +2905,9 @@ mt7530_remove(struct mdio_device *mdiodev) > dev_err(priv->dev, "Failed to disable io pwr: %d\n", > ret); > > + if (priv->irq_domain) > + irq_domain_remove(priv->irq_domain); See the comment in front of irq_domain_remove() about the need to discard the mappings prior to removing a domain. Thanks, M.
> > +static void mt7530_irq_bus_lock(struct irq_data *d) > > +{ > > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); > > + > > + mutex_lock(&priv->reg_mutex); > > Are you always guaranteed to be in a thread context? I guess that > is the case, given that you request a threaded interrupt, but > it would be worth documenting. Hi Marc These Ethernet switches are often connected by MDIO, SPI or I2C busses to the SoC. So in order to access switch registers over these busses, sleeping is required. So yes, threaded interrupts are required. Andrew
> 2. Allocated ds->slave_mii_bus before calling ds->ops->setup, because > we cannot call dsa_slave_mii_bus_init which is private. > > Any better ideas? Do what mv88e6xxx does, allocate the MDIO bus inside the switch driver. Andrew
On Wed, Dec 30, 2020 at 09:42:09AM +0000, Marc Zyngier wrote: > > +static irqreturn_t > > +mt7530_irq(int irq, void *data) > > +{ > > + struct mt7530_priv *priv = data; > > + bool handled = false; > > + int phy; > > + u32 val; > > + > > + val = mt7530_read(priv, MT7530_SYS_INT_STS); > > + mt7530_write(priv, MT7530_SYS_INT_STS, val); > > If that is an ack operation, it should be dealt with as such in > an irqchip callback instead of being open-coded here. Hi Qingfang Does the PHY itself have interrupt control and status registers? My experience with the Marvell Switch and its embedded PHYs is that the PHYs are just the same as the discrete PHYs. There are bits to enable different interrupts, and there are status bits indicating what event caused the interrupt. Clearing the interrupt in the PHY clears the interrupt in the switch interrupt controller. So in the mv88e6xxx interrupt code, you see i do a read of the switch interrupt controller status register, but i don't write to it as you have done. Andrew
On 12/30/2020 1:12 AM, Heiner Kallweit wrote: > On 30.12.2020 10:07, DENG Qingfang wrote: >> Hi Heiner, >> Thanks for your reply. >> >> On Wed, Dec 30, 2020 at 3:39 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: >>> I don't think that's the best option. >> >> I'm well aware of that. >> >>> You may want to add a PHY driver for your chip. Supposedly it >>> supports at least PHY suspend/resume. You can use the RTL8366RB >>> PHY driver as template. >> >> There's no MediaTek PHY driver yet. Do we really need a new one just >> for the interrupts? >> > Not only for the interrupts. The genphy driver e.g. doesn't support > PHY suspend/resume. And the PHY driver needs basically no code, > just set the proper callbacks. That statement about not supporting suspend/resume is not exactly true, the generic "1g" PHY driver only implements suspend/resume through the use of the standard BMCR power down bit, but not anything more complicated than that. Interrupt handling within the PHY itself is not defined by the existing standard registers and will typically not reside in a standard register space either, so just for that reason you do need a custom PHY driver. There are other advantages if you need to expose additional PHY features down the road like PHY counters, energy detection, automatic power down etc. I don't believe we will see discrete/standalone Mediatek PHY chips, but if that happens, then you would already have a framework for supporting them.
On 12/30/2020 7:16 AM, Andrew Lunn wrote: >> 2. Allocated ds->slave_mii_bus before calling ds->ops->setup, because >> we cannot call dsa_slave_mii_bus_init which is private. >> >> Any better ideas? > > Do what mv88e6xxx does, allocate the MDIO bus inside the switch > driver. Yes, exactly, or you could add additional hooks to allow intercepting the initialization and de-initialization of the ds->slave_mii_bus, something like this: https://github.com/ffainelli/linux/commit/758da087a819cd1a284de074ea7d8eae9f875f0b which was part of a larger series adding threaded IRQ support to the b53 driver: https://github.com/ffainelli/linux/commits/b53-irq
On 30.12.2020 17:15, Florian Fainelli wrote: > > > On 12/30/2020 1:12 AM, Heiner Kallweit wrote: >> On 30.12.2020 10:07, DENG Qingfang wrote: >>> Hi Heiner, >>> Thanks for your reply. >>> >>> On Wed, Dec 30, 2020 at 3:39 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: >>>> I don't think that's the best option. >>> >>> I'm well aware of that. >>> >>>> You may want to add a PHY driver for your chip. Supposedly it >>>> supports at least PHY suspend/resume. You can use the RTL8366RB >>>> PHY driver as template. >>> >>> There's no MediaTek PHY driver yet. Do we really need a new one just >>> for the interrupts? >>> >> Not only for the interrupts. The genphy driver e.g. doesn't support >> PHY suspend/resume. And the PHY driver needs basically no code, >> just set the proper callbacks. > > That statement about not supporting suspend/resume is not exactly true, > the generic "1g" PHY driver only implements suspend/resume through the > use of the standard BMCR power down bit, but not anything more > complicated than that. > Oh, right. Somehow I had in the back of my mind that the genphy driver has no suspend/resume callbacks set. > Interrupt handling within the PHY itself is not defined by the existing > standard registers and will typically not reside in a standard register > space either, so just for that reason you do need a custom PHY driver. > There are other advantages if you need to expose additional PHY features > down the road like PHY counters, energy detection, automatic power down etc. > > I don't believe we will see discrete/standalone Mediatek PHY chips, but > if that happens, then you would already have a framework for supporting > them. >
Hi Andrew, On Wed, Dec 30, 2020 at 11:23 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Wed, Dec 30, 2020 at 09:42:09AM +0000, Marc Zyngier wrote: > > > +static irqreturn_t > > > +mt7530_irq(int irq, void *data) > > > +{ > > > + struct mt7530_priv *priv = data; > > > + bool handled = false; > > > + int phy; > > > + u32 val; > > > + > > > + val = mt7530_read(priv, MT7530_SYS_INT_STS); > > > + mt7530_write(priv, MT7530_SYS_INT_STS, val); > > > > If that is an ack operation, it should be dealt with as such in > > an irqchip callback instead of being open-coded here. > > Hi Qingfang > > Does the PHY itself have interrupt control and status registers? MT7531's internal PHY has an interrupt status register, but I don't know if the same applies to MT7530. > > My experience with the Marvell Switch and its embedded PHYs is that > the PHYs are just the same as the discrete PHYs. There are bits to > enable different interrupts, and there are status bits indicating what > event caused the interrupt. Clearing the interrupt in the PHY clears > the interrupt in the switch interrupt controller. So in the mv88e6xxx > interrupt code, you see i do a read of the switch interrupt controller > status register, but i don't write to it as you have done. > > Andrew
Hi Qingfang, On Wed, 2021-01-06 at 16:54 +0800, DENG Qingfang wrote: > Hi Andrew, > > On Wed, Dec 30, 2020 at 11:23 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > On Wed, Dec 30, 2020 at 09:42:09AM +0000, Marc Zyngier wrote: > > > > +static irqreturn_t > > > > +mt7530_irq(int irq, void *data) > > > > +{ > > > > + struct mt7530_priv *priv = data; > > > > + bool handled = false; > > > > + int phy; > > > > + u32 val; > > > > + > > > > + val = mt7530_read(priv, MT7530_SYS_INT_STS); > > > > + mt7530_write(priv, MT7530_SYS_INT_STS, val); > > > > > > If that is an ack operation, it should be dealt with as such in > > > an irqchip callback instead of being open-coded here. > > > > Hi Qingfang > > > > Does the PHY itself have interrupt control and status registers? > > MT7531's internal PHY has an interrupt status register, but I don't > know if the same applies to MT7530. Interrupt status/mask registers of MT7530 internal PHY is the same as MT7531. The switch interrupt status register MT7530_SYS_INT_STS[14:8] reflects internal PHY interrupt status. MT7530_SYS_INT_STS[6:0] we used before does not related to internal PHY "interrupt". However, base on MT753x hardware behavior, after read-clear interrupt status of internal phy, we still need to write-clear MT7530_SYS_INT_STS[14:8] to clear switch interrupt. Landen > > > > > My experience with the Marvell Switch and its embedded PHYs is that > > the PHYs are just the same as the discrete PHYs. There are bits to > > enable different interrupts, and there are status bits indicating what > > event caused the interrupt. Clearing the interrupt in the PHY clears > > the interrupt in the switch interrupt controller. So in the mv88e6xxx > > interrupt code, you see i do a read of the switch interrupt controller > > status register, but i don't write to it as you have done. > > > > Andrew
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index a67cac15a724..d59a8c50ede3 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -10,6 +10,7 @@ #include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/netdevice.h> +#include <linux/of_irq.h> #include <linux/of_mdio.h> #include <linux/of_net.h> #include <linux/of_platform.h> @@ -1639,6 +1640,125 @@ mtk_get_tag_protocol(struct dsa_switch *ds, int port, } } +static irqreturn_t +mt7530_irq(int irq, void *data) +{ + struct mt7530_priv *priv = data; + bool handled = false; + int phy; + u32 val; + + val = mt7530_read(priv, MT7530_SYS_INT_STS); + mt7530_write(priv, MT7530_SYS_INT_STS, val); + + dev_info_ratelimited(priv->dev, "interrupt status: 0x%08x\n", val); + dev_info_ratelimited(priv->dev, "interrupt enable: 0x%08x\n", mt7530_read(priv, MT7530_SYS_INT_EN)); + + for (phy = 0; phy < MT7530_NUM_PHYS; phy++) { + if (val & BIT(phy)) { + unsigned int child_irq; + + child_irq = irq_find_mapping(priv->irq_domain, phy); + handle_nested_irq(child_irq); + handled = true; + } + } + + return handled ? IRQ_HANDLED : IRQ_NONE; +} + +static void mt7530_irq_mask(struct irq_data *d) +{ + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); + + priv->irq_enable &= ~BIT(d->hwirq); +} + +static void mt7530_irq_unmask(struct irq_data *d) +{ + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); + + priv->irq_enable |= BIT(d->hwirq); +} + +static void mt7530_irq_bus_lock(struct irq_data *d) +{ + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); + + mutex_lock(&priv->reg_mutex); +} + +static void mt7530_irq_bus_sync_unlock(struct irq_data *d) +{ + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); + + mt7530_write(priv, MT7530_SYS_INT_EN, priv->irq_enable); + mutex_unlock(&priv->reg_mutex); +} + +static struct irq_chip mt7530_irq_chip = { + .name = MT7530_NAME, + .irq_mask = mt7530_irq_mask, + .irq_unmask = mt7530_irq_unmask, + .irq_bus_lock = mt7530_irq_bus_lock, + .irq_bus_sync_unlock = mt7530_irq_bus_sync_unlock, +}; + +static int +mt7530_irq_map(struct irq_domain *domain, unsigned int irq, + irq_hw_number_t hwirq) +{ + irq_set_chip_data(irq, domain->host_data); + irq_set_chip_and_handler(irq, &mt7530_irq_chip, handle_simple_irq); + irq_set_noprobe(irq); + + return 0; +} + +static const struct irq_domain_ops mt7530_irq_domain_ops = { + .map = mt7530_irq_map, + .xlate = irq_domain_xlate_onecell, +}; + +static void +mt7530_irq_init(struct mt7530_priv *priv) +{ + struct mii_bus *bus = priv->ds->slave_mii_bus; + struct device *dev = priv->dev; + struct device_node *np = dev->of_node; + int parent_irq; + int phy, ret; + + parent_irq = of_irq_get(np, 0); + if (parent_irq <= 0) { + dev_err(dev, "failed to get parent IRQ: %d\n", parent_irq); + return; + } + + mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL); + ret = devm_request_threaded_irq(dev, parent_irq, NULL, mt7530_irq, + IRQF_ONESHOT, MT7530_NAME, priv); + if (ret) { + dev_err(dev, "failed to request IRQ: %d\n", ret); + return; + } + + priv->irq_domain = irq_domain_add_linear(np, MT7530_NUM_PHYS, + &mt7530_irq_domain_ops, priv); + if (!priv->irq_domain) { + dev_err(dev, "failed to create IRQ domain\n"); + return; + } + + /* IRQ for internal PHYs */ + for (phy = 0; phy < MT7530_NUM_PHYS; phy++) { + unsigned int irq = irq_create_mapping(priv->irq_domain, phy); + + irq_set_parent(irq, parent_irq); + bus->irq[phy] = irq; + } +} + static int mt7530_setup(struct dsa_switch *ds) { @@ -2578,8 +2698,13 @@ static int mt753x_setup(struct dsa_switch *ds) { struct mt7530_priv *priv = ds->priv; + int ret = priv->info->sw_setup(ds); - return priv->info->sw_setup(ds); + /* Setup interrupt */ + if (!ret) + mt7530_irq_init(priv); + + return ret; } static int @@ -2780,6 +2905,9 @@ mt7530_remove(struct mdio_device *mdiodev) dev_err(priv->dev, "Failed to disable io pwr: %d\n", ret); + if (priv->irq_domain) + irq_domain_remove(priv->irq_domain); + dsa_unregister_switch(priv->ds); mutex_destroy(&priv->reg_mutex); } @@ -2788,7 +2916,7 @@ static struct mdio_driver mt7530_mdio_driver = { .probe = mt7530_probe, .remove = mt7530_remove, .mdiodrv.driver = { - .name = "mt7530", + .name = MT7530_NAME, .of_match_table = mt7530_of_match, }, }; diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h index 32d8969b3ace..b1988d8085bb 100644 --- a/drivers/net/dsa/mt7530.h +++ b/drivers/net/dsa/mt7530.h @@ -6,7 +6,10 @@ #ifndef __MT7530_H #define __MT7530_H +#define MT7530_NAME "mt7530" + #define MT7530_NUM_PORTS 7 +#define MT7530_NUM_PHYS 5 #define MT7530_CPU_PORT 6 #define MT7530_NUM_FDB_RECORDS 2048 #define MT7530_ALL_MEMBERS 0xff @@ -380,6 +383,12 @@ enum mt7531_sgmii_force_duplex { #define SYS_CTRL_SW_RST BIT(1) #define SYS_CTRL_REG_RST BIT(0) +/* Register for system interrupt */ +#define MT7530_SYS_INT_EN 0x7008 + +/* Register for system interrupt status */ +#define MT7530_SYS_INT_STS 0x700c + /* Register for PHY Indirect Access Control */ #define MT7531_PHY_IAC 0x701C #define MT7531_PHY_ACS_ST BIT(31) @@ -761,6 +770,8 @@ struct mt7530_priv { struct mt7530_port ports[MT7530_NUM_PORTS]; /* protect among processes for registers access*/ struct mutex reg_mutex; + struct irq_domain *irq_domain; + u32 irq_enable; }; struct mt7530_hw_vlan_entry { diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 80c2e646c093..cdddc27e2df7 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -2846,12 +2846,6 @@ static int phy_probe(struct device *dev) phydev->drv = phydrv; - /* Disable the interrupt if the PHY doesn't support it - * but the interrupt is still a valid one - */ - if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev)) - phydev->irq = PHY_POLL; - if (phydrv->flags & PHY_IS_INTERNAL) phydev->is_internal = true; diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index 183003e45762..ef5db1106581 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -448,19 +448,19 @@ static int dsa_switch_setup(struct dsa_switch *ds) if (err) goto unregister_devlink_ports; + ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev); + if (!ds->slave_mii_bus) { + err = -ENOMEM; + goto unregister_notifier; + } + err = ds->ops->setup(ds); if (err < 0) goto unregister_notifier; devlink_params_publish(ds->devlink); - if (!ds->slave_mii_bus && ds->ops->phy_read) { - ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev); - if (!ds->slave_mii_bus) { - err = -ENOMEM; - goto unregister_notifier; - } - + if (ds->ops->phy_read) { dsa_slave_mii_bus_init(ds); err = mdiobus_register(ds->slave_mii_bus);