RE: [PATCH v3 11/12] PCI: exynos: Add support for Tesla FSD SoC

["Shradha Todi" <shradha.t@samsung.com>]

> > > > +static irqreturn_t fsd_pcie_irq_handler(int irq, void *arg)
> > > > +{
> > > > +	u32 val;
> > > > +	struct exynos_pcie *ep = arg;
> > > > +	struct dw_pcie *pci = &ep->pci;
> > > > +	struct dw_pcie_rp *pp = &pci->pp;
> > > > +
> > > > +	val = readl(ep->elbi_base + FSD_IRQ2_STS);
> > > > +	if ((val & FSD_IRQ_MSI_ENABLE) == FSD_IRQ_MSI_ENABLE) {
> > > > +		val &= FSD_IRQ_MSI_ENABLE;
> > > > +		writel(val, ep->elbi_base + FSD_IRQ2_STS);
> > >
> > > This looks weird because FSD_IRQ_MSI_ENABLE sounds like an *enable*
> > > bit, but here you're treating it as a *status* bit.
> > >
> > > As far as I can tell, you set FSD_IRQ_MSI_ENABLE once at probe-time in
> > > fsd_pcie_msi_init(), then you clear it here in an IRQ handler, and it
> > > will never be set again.  That seems wrong; am I missing something?
> >
> > Actually the status IRQ and enable IRQ registers are different offsets
> > but the bit position for MSI remains same in both cases so I just reused
> > the macro.
> 
> Ah, that's what I missed, thanks!  At probe-time, fsd_pcie_msi_init()
> enables it in FSD_IRQ2_EN.  Here you clear it in FSD_IRQ2_STS.
> 
> > But I understand that it's confusing so I will add another
> > macro for FSD_IRQ_MSI_STATUS or just rename the macro to
> > FSD_IRQ_MSI to re-use.
> 
> Using the same name just because a similar bit happens to be at the
> same position in two different registers is definitely confusing.  I
> think it will be better to have two macros, one for FSD_IRQ2_STS and
> another for FSD_IRQ2_EN, e.g.,
> 
>   #define FSD_IRQ2_STS                         0x008
>   #define   FSD_IRQ2_STS_MSI                   BIT(17)
>   #define FSD_IRQ2_EN                          0x018
>   #define   FSD_IRQ2_EN_MSI                    BIT(17)
> 
> Another question about the test:
> 
>   if ((val & FSD_IRQ_MSI_ENABLE) == FSD_IRQ_MSI_ENABLE) {
> 
> This assumes there are no other bits in FSD_IRQ2_STS that could be
> set.  I would have expected a test like this:
> 
>   if (val & FSD_IRQ_MSI_ENABLE) {
> 

Thanks for pointing this out. FSD_IRQ_MSI_ENABLE is a single-bit, so there
is no functional difference in the two statements. I didn't have a specific
reason for using "== FSD_IRQ_MSI_ENABLE".
But I see that "val & FSD_IRQ_MSI_ENABLE" would have been the more
standard way to write this. I will update this for clarity.

> Is there a reason to restrict it to the case when *only*
> FSD_IRQ_MSI_ENABLE is set?
> 
> Bjorn