| Message ID | 20170903224100.17893-6-stefan.bruens@rwth-aachen.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, Sep 04, 2017 at 12:40:56AM +0200, Stefan Brüns wrote: > Preparatory patch: If the same compatible is used for different SoCs which > have a common register layout, but different number of channels, the > channel count can no longer be stored in the config. Store it in the > device structure instead. > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> As stated already, we already are going to have a different compatible, and this is not something that will change from one instance to the other. Having code is therefore: A) Making the code more complex B) For no particular reason. Maxime
On Montag, 4. September 2017 09:43:55 CEST Maxime Ripard wrote: > On Mon, Sep 04, 2017 at 12:40:56AM +0200, Stefan Brüns wrote: > > Preparatory patch: If the same compatible is used for different SoCs which > > have a common register layout, but different number of channels, the > > channel count can no longer be stored in the config. Store it in the > > device structure instead. > > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > As stated already, we already are going to have a different > compatible, and this is not something that will change from one > instance to the other. Having code is therefore: > A) Making the code more complex > B) For no particular reason. If the dma channel count (which is a standard dma binding, likely for a reason) goes into the devicetree, it has to be moved out of the config. The R40 (which has register manuals available) has the same register layout as the A64, but *does* have a different channel count. So you think it is a good idea to introduce a new compatible again? If you had been half as picky when merging the H3 and A83T support, we would not have this mess now. There is also the H6, where there is no register manual available yet, but I bet it has the H3 (and A64, H5, R40) register layout, but unlikely the same number of DMA channels *and* the same number of ports. Regards, Stefan -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 04, 2017 at 02:30:59PM +0000, Brüns, Stefan wrote: > On Montag, 4. September 2017 09:43:55 CEST Maxime Ripard wrote: > > On Mon, Sep 04, 2017 at 12:40:56AM +0200, Stefan Brüns wrote: > > > Preparatory patch: If the same compatible is used for different SoCs which > > > have a common register layout, but different number of channels, the > > > channel count can no longer be stored in the config. Store it in the > > > device structure instead. > > > > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > > > As stated already, we already are going to have a different > > compatible, and this is not something that will change from one > > instance to the other. Having code is therefore: > > A) Making the code more complex > > B) For no particular reason. > > If the dma channel count (which is a standard dma binding, likely for a > reason) goes into the devicetree, it has to be moved out of the config. > > The R40 (which has register manuals available) has the same register layout as > the A64, but *does* have a different channel count. So you think it is a good > idea to introduce a new compatible again? > > If you had been half as picky when merging the H3 and A83T support, we would > not have this mess now. > > There is also the H6, where there is no register manual available yet, but I > bet it has the H3 (and A64, H5, R40) register layout, but unlikely the same > number of DMA channels *and* the same number of ports. The thing is that this kind of things usually grow organically until you can't just add a simple if case any more. I'm sorry you were at the tipping point, but I'm sure you also understand that adding more to the mess until the next one shows up isn't viable either. That being said, thinking a bit more on that one, if you add to the binding that we need to have both the current SoC compatible (to workaround any variation / bugs we might encounter in the future) and the "generation" one (to avoid adding one for each new IP), plus the mandatory dma-channels / dma-requests properties, that would work for me. Maxime
diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c index 335a8ec88b0b..c69dadb853d2 100644 --- a/drivers/dma/sun6i-dma.c +++ b/drivers/dma/sun6i-dma.c @@ -187,6 +187,9 @@ struct sun6i_dma_dev { const struct sun6i_dma_config *cfg; u32 src_burst_lengths; u32 dst_burst_lengths; + u32 num_pchans; + u32 num_vchans; + u32 max_request; }; static struct device *chan2dev(struct dma_chan *chan) @@ -411,7 +414,6 @@ static int sun6i_dma_start_desc(struct sun6i_vchan *vchan) static void sun6i_dma_tasklet(unsigned long data) { struct sun6i_dma_dev *sdev = (struct sun6i_dma_dev *)data; - const struct sun6i_dma_config *cfg = sdev->cfg; struct sun6i_vchan *vchan; struct sun6i_pchan *pchan; unsigned int pchan_alloc = 0; @@ -439,7 +441,7 @@ static void sun6i_dma_tasklet(unsigned long data) } spin_lock_irq(&sdev->lock); - for (pchan_idx = 0; pchan_idx < cfg->nr_max_channels; pchan_idx++) { + for (pchan_idx = 0; pchan_idx < sdev->num_pchans; pchan_idx++) { pchan = &sdev->pchans[pchan_idx]; if (pchan->vchan || list_empty(&sdev->pending)) @@ -460,7 +462,7 @@ static void sun6i_dma_tasklet(unsigned long data) } spin_unlock_irq(&sdev->lock); - for (pchan_idx = 0; pchan_idx < cfg->nr_max_channels; pchan_idx++) { + for (pchan_idx = 0; pchan_idx < sdev->num_pchans; pchan_idx++) { if (!(pchan_alloc & BIT(pchan_idx))) continue; @@ -482,7 +484,7 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id) int i, j, ret = IRQ_NONE; u32 status; - for (i = 0; i < sdev->cfg->nr_max_channels / DMA_IRQ_CHAN_NR; i++) { + for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) { status = readl(sdev->base + DMA_IRQ_STAT(i)); if (!status) continue; @@ -974,7 +976,7 @@ static struct dma_chan *sun6i_dma_of_xlate(struct of_phandle_args *dma_spec, struct dma_chan *chan; u8 port = dma_spec->args[0]; - if (port > sdev->cfg->nr_max_requests) + if (port > sdev->max_request) return NULL; chan = dma_get_any_slave_channel(&sdev->slave); @@ -1007,7 +1009,7 @@ static inline void sun6i_dma_free(struct sun6i_dma_dev *sdev) { int i; - for (i = 0; i < sdev->cfg->nr_max_vchans; i++) { + for (i = 0; i < sdev->num_vchans; i++) { struct sun6i_vchan *vchan = &sdev->vchans[i]; list_del(&vchan->vc.chan.device_node); @@ -1167,26 +1169,30 @@ static int sun6i_dma_probe(struct platform_device *pdev) sdc->slave.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST; sdc->slave.dev = &pdev->dev; - sdc->pchans = devm_kcalloc(&pdev->dev, sdc->cfg->nr_max_channels, + sdc->num_pchans = sdc->cfg->nr_max_channels; + sdc->num_vchans = sdc->cfg->nr_max_vchans; + sdc->max_request = sdc->cfg->nr_max_requests; + + sdc->pchans = devm_kcalloc(&pdev->dev, sdc->num_pchans, sizeof(struct sun6i_pchan), GFP_KERNEL); if (!sdc->pchans) return -ENOMEM; - sdc->vchans = devm_kcalloc(&pdev->dev, sdc->cfg->nr_max_vchans, + sdc->vchans = devm_kcalloc(&pdev->dev, sdc->num_vchans, sizeof(struct sun6i_vchan), GFP_KERNEL); if (!sdc->vchans) return -ENOMEM; tasklet_init(&sdc->task, sun6i_dma_tasklet, (unsigned long)sdc); - for (i = 0; i < sdc->cfg->nr_max_channels; i++) { + for (i = 0; i < sdc->num_pchans; i++) { struct sun6i_pchan *pchan = &sdc->pchans[i]; pchan->idx = i; pchan->base = sdc->base + 0x100 + i * 0x40; } - for (i = 0; i < sdc->cfg->nr_max_vchans; i++) { + for (i = 0; i < sdc->num_vchans; i++) { struct sun6i_vchan *vchan = &sdc->vchans[i]; INIT_LIST_HEAD(&vchan->node);
Preparatory patch: If the same compatible is used for different SoCs which have a common register layout, but different number of channels, the channel count can no longer be stored in the config. Store it in the device structure instead. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- drivers/dma/sun6i-dma.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)