On Thu, Nov 12, 2020 at 04:35:32PM +0100, Christian Eggers wrote:
> Interrupts are required for TX time stamping. Probably they could also
> be used for PHY connection status.
Do the KSZ switches have an internal PHY? And there's a single interrupt
line, shared between the PTP timestamping engine, and the internal PHY
that is driver by phylib?
> This patch only adds the basic infrastructure for interrupts, no
> interrupts are actually enabled nor handled.
>
> ksz9477_reset_switch() must be called before requesting the IRQ (in
> ksz9477_init() instead of ksz9477_setup()).
A patch can never be "too simple". Maybe you could factor out that code
movement into a separate patch.
> Signed-off-by: Christian Eggers <cegg...@arri.de>
> ---
> drivers/net/dsa/microchip/ksz9477_i2c.c | 2 +
> drivers/net/dsa/microchip/ksz9477_main.c | 103 +++++++++++++++++++++--
> drivers/net/dsa/microchip/ksz9477_spi.c | 2 +
> include/linux/dsa/ksz_common.h | 1 +
> 4 files changed, 100 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c
> b/drivers/net/dsa/microchip/ksz9477_i2c.c
> index 4e053a25d077..4ed1f503044a 100644
> --- a/drivers/net/dsa/microchip/ksz9477_i2c.c
> +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
> @@ -41,6 +41,8 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c,
> if (i2c->dev.platform_data)
> dev->pdata = i2c->dev.platform_data;
>
> + dev->irq = i2c->irq;
> +
> ret = ksz9477_switch_register(dev);
>
> /* Main DSA driver may not be started yet. */
> diff --git a/drivers/net/dsa/microchip/ksz9477_main.c
> b/drivers/net/dsa/microchip/ksz9477_main.c
> index abfd3802bb51..6b5a981fb21f 100644
> --- a/drivers/net/dsa/microchip/ksz9477_main.c
> +++ b/drivers/net/dsa/microchip/ksz9477_main.c
> @@ -7,7 +7,9 @@
>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/interrupt.h>
> #include <linux/iopoll.h>
> +#include <linux/irq.h>
> #include <linux/platform_data/microchip-ksz.h>
> #include <linux/phy.h>
> #include <linux/if_bridge.h>
> @@ -1345,19 +1347,12 @@ static void ksz9477_config_cpu_port(struct dsa_switch
> *ds)
> static int ksz9477_setup(struct dsa_switch *ds)
> {
> struct ksz_device *dev = ds->priv;
> - int ret = 0;
>
> dev->vlan_cache = devm_kcalloc(dev->dev, sizeof(struct vlan_table),
> dev->num_vlans, GFP_KERNEL);
> if (!dev->vlan_cache)
> return -ENOMEM;
>
> - ret = ksz9477_reset_switch(dev);
> - if (ret) {
> - dev_err(ds->dev, "failed to reset switch\n");
> - return ret;
> - }
> -
> /* Required for port partitioning. */
> ksz9477_cfg32(dev, REG_SW_QM_CTRL__4, UNICAST_VLAN_BOUNDARY,
> true);
> @@ -1535,12 +1530,84 @@ static const struct ksz_chip_data
> ksz9477_switch_chips[] = {
> },
> };
>
> +static irqreturn_t ksz9477_switch_irq_thread(int irq, void *dev_id)
> +{
> + struct ksz_device *dev = dev_id;
> + u32 data;
> + int port;
> + int ret;
> + irqreturn_t result = IRQ_NONE;
Please keep local variable declaration sorted in the reverse order of
line length. But....
> +
> + /* Read global port interrupt status register */
> + ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data);
> + if (ret)
> + return result;
...Is there any point at all in keeping the "result" variable?
> +
> + for (port = 0; port < dev->port_cnt; port++) {
> + if (data & BIT(port)) {
You can reduce the indentation level by 1 here using:
if (!(data & BIT(port)))
continue;
> + u8 data8;
> +
> + /* Read port interrupt status register */
> + ret = ksz_read8(dev, PORT_CTRL_ADDR(port,
> REG_PORT_INT_STATUS),
> + &data8);
> + if (ret)
> + return result;
> +
> + /* ToDo: Add specific handling of port interrupts */
Buggy? Please return IRQ_HANDLED, otherwise the system, when bisected to
this commit exactly, will emit interrupts and complain that nobody cared.
> + }
> + }
> +
> + return result;
> +}
> +
> +static int ksz9477_enable_port_interrupts(struct ksz_device *dev)
> +{
> + u32 data;
> + int ret;
> +
> + ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
> + if (ret)
> + return ret;
> +
> + /* Enable port interrupts (0 means enabled) */
> + data &= ~((1 << dev->port_cnt) - 1);
And what's the " - 1" for?
> + ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
> + if (ret)
> + return ret;
> +
> + return 0;
return ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
> +}
> +
> +static int ksz9477_disable_port_interrupts(struct ksz_device *dev)
> +{
> + u32 data;
> + int ret;
> +
> + ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
> + if (ret)
> + return ret;
> +
> + /* Disable port interrupts (1 means disabled) */
> + data |= ((1 << dev->port_cnt) - 1);
> + ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
> + if (ret)
> + return ret;
> +
> + return 0;
same comments as above.
Also, it's almost as if you want to implement these in the same
function, with a "bool enable"?
> +}
> +
> static int ksz9477_switch_init(struct ksz_device *dev)
> {
> - int i;
> + int i, ret;
>
> dev->ds->ops = &ksz9477_switch_ops;
>
> + ret = ksz9477_reset_switch(dev);
> + if (ret) {
> + dev_err(dev->dev, "failed to reset switch\n");
> + return ret;
> + }
> +
> for (i = 0; i < ARRAY_SIZE(ksz9477_switch_chips); i++) {
> const struct ksz_chip_data *chip = &ksz9477_switch_chips[i];
>
> @@ -1584,12 +1651,32 @@ static int ksz9477_switch_init(struct ksz_device *dev)
>
> /* set the real number of ports */
> dev->ds->num_ports = dev->port_cnt;
> + if (dev->irq > 0) {
> + unsigned long irqflags =
> irqd_get_trigger_type(irq_get_irq_data(dev->irq));
What is irqd_get_trigger_type and what does it have to do with the
"irqflags" argument of request_threaded_irq? Where else have you even
seen this?
> +
> + irqflags |= IRQF_ONESHOT;
And shared maybe?
> + ret = devm_request_threaded_irq(dev->dev, dev->irq, NULL,
> + ksz9477_switch_irq_thread,
> + irqflags,
> + dev_name(dev->dev),
> + dev);
> + if (ret) {
> + dev_err(dev->dev, "failed to request IRQ.\n");
> + return ret;
> + }
> +
> + ret = ksz9477_enable_port_interrupts(dev);
> + if (ret)
> + return ret;
Could you also clear pending interrupts before enabling the line?
> + }
>
> return 0;
> }
>
> static void ksz9477_switch_exit(struct ksz_device *dev)
> {
> + if (dev->irq > 0)
> + ksz9477_disable_port_interrupts(dev);
I think it'd look a bit nicer if you moved this condition into
ksz9477_disable_port_interrupts:
if (!dev->irq)
return;
> ksz9477_reset_switch(dev);
> }
>
