On 24/01/2019 13:41, Timo Alho wrote:
> Hi Jon,
>
> Thanks for reviewing :)
>
> On 24.1.2019 14.16, Jon Hunter wrote:
>
> ...
>
>>> +static int tegra210_bpmp_ring_doorbell(struct tegra_bpmp *bpmp)
>>> +{
>>> + struct tegra210_bpmp *priv = bpmp->priv;
>>> + struct irq_data *irq_data;
>>> +
>>> + /* Tegra Legacy Interrupt Controller (LIC) is used to notify
>>> + * BPMP of available messages
>>> + */
>>> + irq_data = irq_get_irq_data(priv->txirq);
>>> + if (!irq_data)
>>> + return -EINVAL;
>>
>> Why not check this in probe?
>>
>
> Indeed I can move irq_get_irq_data() to probe and store the return value
> directly to priv->txirq instead. I'll just do that then.
>
>>> +
>>> + return irq_data->chip->irq_retrigger(irq_data);
>>
>> We should check that the irq_retrigger is populated as well.
>
> I'll add a check.
>
>> In general, I am not sure if there is a better way to do this, but I
>> don't see an alternative.
>>
>
> I'd be also happy to hear if someone has good alternative.
>
> ...
>
>>> +static int tegra210_bpmp_init(struct tegra_bpmp *bpmp)
>>> +{
>>> + struct platform_device *pdev = to_platform_device(bpmp->dev);
>>> + struct tegra210_bpmp *priv;
>>> + struct resource *res;
>>> + unsigned int i;
>>> + int err;
>>> +
>>> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>> + if (!priv)
>>> + return -ENOMEM;
>>> +
>>> + bpmp->priv = priv;
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + priv->atomics = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(priv->atomics))
>>> + return PTR_ERR(priv->atomics);
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>> + priv->arb_sema = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(priv->arb_sema))
>>> + return PTR_ERR(priv->arb_sema);
>>> +
>>> + err = tegra210_bpmp_channel_init(bpmp->tx_channel, bpmp,
>>> + bpmp->soc->channels.cpu_tx.offset);
>>> + if (err < 0)
>>> + return err;
>>> +
>>> + err = tegra210_bpmp_channel_init(bpmp->rx_channel, bpmp,
>>> + bpmp->soc->channels.cpu_rx.offset);
>>> + if (err < 0)
>>> + return err;
>>
>> Don't we need to unmap any iomem on error that we mapped in
>> tegra210_bpmp_channel_init()?
>
> Good point. I'll just replace ioremap() with devm_ioremap(). I think
> that should do it.
>
> ...
>
>>> diff --git a/drivers/firmware/tegra/bpmp.c
>>> b/drivers/firmware/tegra/bpmp.c
>>> index c6716ec..22c91ab 100644
>>> --- a/drivers/firmware/tegra/bpmp.c
>>> +++ b/drivers/firmware/tegra/bpmp.c
>>> @@ -765,17 +765,23 @@ static int tegra_bpmp_probe(struct
>>> platform_device *pdev)
>>> if (err < 0)
>>> goto free_mrq;
>>> - err = tegra_bpmp_init_clocks(bpmp);
>>> - if (err < 0)
>>> - goto free_mrq;
>>> + if (of_find_property(pdev->dev.of_node, "#clock-cells", NULL)) {
>>> + err = tegra_bpmp_init_clocks(bpmp);
>>> + if (err < 0)
>>> + goto free_mrq;
>>> + }
>>> - err = tegra_bpmp_init_resets(bpmp);
>>> - if (err < 0)
>>> - goto free_mrq;
>>> + if (of_find_property(pdev->dev.of_node, "#reset-cells", NULL)) {
>>> + err = tegra_bpmp_init_resets(bpmp);
>>> + if (err < 0)
>>> + goto free_mrq;
>>> + }
>>> - err = tegra_bpmp_init_powergates(bpmp);
>>> - if (err < 0)
>>> - goto free_mrq;
>>> + if (of_find_property(pdev->dev.of_node, "#power-domain-cells",
>>> NULL)) {
>>> + err = tegra_bpmp_init_powergates(bpmp);
>>> + if (err < 0)
>>> + goto free_mrq;
>>> + }
>>
>> Should we use soc_data for these rather than relying on the nodes to be
>> populated correctly in DT?
>
> There are some t210 systems where BPMP functions as clocks provider (for
> EMC), and some where it does not. So controlling this via device tree
> can be handier.
Is it the same T210 device or could the compatibility string be used
here for this?
Cheers
Jon
--
nvpublic
