Re: [PATCH 2/5] soundwire: qcom: add support for mmio soundwire devices

[Jonathan Marek <jonathan@xxxxxxxx>]

On 6/9/20 5:19 AM, Srinivas Kandagatla wrote:
> On 08/06/2020 21:43, Jonathan Marek wrote:
> > Adds support for qcom soundwire devices with memory mapped IO registers.
> >
> > Signed-off-by: Jonathan Marek <jonathan@xxxxxxxx>
> > ---
>
> In general patch itself looks pretty trivial, but I would like to see 
> what 1.5.1 controller provides in terms of error reporting of SoundWire 
> slave register reads/writes. On WCD based controller we did not have a 
> mechanism to report things like if the read is ignored or not. I was 
> hoping that this version of controller would be able to report that.
>
> I will be nice to those patches if that is something which is supported 
> in this version.
>
> --srini

It does seem to support additional error reporting (it gets an error 
during enumeration after finding the 2 WSA slaves). However the 
downstream driver seems to disable this by setting BIT(31) in 
FIFO_CFG_ADDR (the comment says "For SWR master version 1.5.1, continue 
execute on command ignore"). Outside of the initial enumeration, it 
doesn't seem to produce any extra errors (still relying on the timeout 
mechanism to know if read/write is ignored).

> >   drivers/soundwire/qcom.c | 25 +++++++++++++++++++++++--
> >   1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> > index f38d1fd3679f..628747df1c75 100644
> > --- a/drivers/soundwire/qcom.c
> > +++ b/drivers/soundwire/qcom.c
> > @@ -90,6 +90,7 @@ struct qcom_swrm_ctrl {
> >       struct sdw_bus bus;
> >       struct device *dev;
> >       struct regmap *regmap;
> > +    void __iomem *mmio;
> >       struct completion *comp;
> >       struct work_struct slave_work;
> >       /* read/write lock */
> > @@ -154,6 +155,20 @@ static int qcom_swrm_ahb_reg_write(struct 
> > qcom_swrm_ctrl *ctrl,
> >       return SDW_CMD_OK;
> >   }
> > +static int qcom_swrm_cpu_reg_read(struct qcom_swrm_ctrl *ctrl, int reg,
> > +                  u32 *val)
> > +{
> > +    *val = readl(ctrl->mmio + reg);
> > +    return SDW_CMD_OK;
> > +}
> > +
> > +static int qcom_swrm_cpu_reg_write(struct qcom_swrm_ctrl *ctrl, int reg,
> > +                   int val)
> > +{
> > +    writel(val, ctrl->mmio + reg);
> > +    return SDW_CMD_OK;
> > +}
> > +
> >   static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 
> > cmd_data,
> >                        u8 dev_addr, u16 reg_addr)
> >   {
> > @@ -746,6 +761,7 @@ static int qcom_swrm_probe(struct platform_device 
> > *pdev)
> >       struct sdw_master_prop *prop;
> >       struct sdw_bus_params *params;
> >       struct qcom_swrm_ctrl *ctrl;
> > +    struct resource *res;
> >       int ret;
> >       u32 val;
> > @@ -760,8 +776,13 @@ static int qcom_swrm_probe(struct platform_device 
> > *pdev)
> >           if (!ctrl->regmap)
> >               return -EINVAL;
> >       } else {
> > -        /* Only WCD based SoundWire controller is supported */
> > -        return -ENOTSUPP;
> > +        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +
> > +        ctrl->reg_read = qcom_swrm_cpu_reg_read;
> > +        ctrl->reg_write = qcom_swrm_cpu_reg_write;
> > +        ctrl->mmio = devm_ioremap_resource(dev, res);
> > +        if (IS_ERR(ctrl->mmio))
> > +            return PTR_ERR(ctrl->mmio);
> >       }
> >       ctrl->irq = of_irq_get(dev->of_node, 0);