Re: [PATCH 3/4] nvmem: add a driver for the Amlogic Meson6/Meson8/Meson8b SoCs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Mon, Oct 2, 2017 at 5:28 PM, Srinivas Kandagatla
<srinivas.kandagatla@xxxxxxxxxx> wrote:
> Minor comments!!
that was a VERY quick code review (and by this I mean the time between
sending the patch and getting valuable feedback) - many thanks for
this!

>
> On 01/10/17 13:56, Martin Blumenstingl wrote:
>>
>> This adds a driver to access the efuse on Amlogic Meson6, Meson8 and
>> Meson8b SoCs.
>> These SoCs are accessing the efuse IP block directly through the
>> registers in the "secbus" region. This makes it different from the Meson
>> GX efuse driver which uses the "secure monitor" firmware to access the
>> efuse.
>>
>> The efuse on Meson6 can only read one byte at a time, while the efuse on
>> Meson8 and Meson8b always reads 4 bytes at a time. The new driver
>> supports both, but due to lack of hardware Meson6 support was not tested.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
>> ---
>>   drivers/nvmem/Kconfig          |  10 ++
>>   drivers/nvmem/Makefile         |   2 +
>>   drivers/nvmem/meson-mx-efuse.c | 272
>> +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 284 insertions(+)
>>   create mode 100644 drivers/nvmem/meson-mx-efuse.c
>>
>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>> +
>> +
>> +static void meson_mx_efuse_mask_bits(struct meson_mx_efuse *efuse, u32
>> reg,
>> +                                    u32 mask, u32 set)
>> +{
>> +       u32 data;
>> +
>> +       data = readl(efuse->base + reg);
>> +       data &= ~mask;
>> +       data |= (set & mask);
>> +
>> +       writel(data, efuse->base + reg);
>> +}
>> +
>> +static int meson_mx_efuse_hw_enable(struct meson_mx_efuse *efuse)
>> +{
>> +       int err;
>> +
>> +       err = clk_prepare_enable(efuse->core_clk);
>> +       if (err)
>> +               return err;
>> +
>> +       /* power up the efuse */
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_PD_ENABLE, 0);
>> +
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL4,
>> +                                MESON_MX_EFUSE_CNTL4_ENCRYPT_ENABLE, 0);
>> +
>> +       return 0;
>> +}
>> +
>> +static void meson_mx_efuse_hw_disable(struct meson_mx_efuse *efuse)
>> +{
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_PD_ENABLE,
>> +                                MESON_MX_EFUSE_CNTL1_PD_ENABLE);
>> +
>> +       clk_disable_unprepare(efuse->core_clk);
>> +}
>> +
>> +static int meson_mx_efuse_read_addr(struct meson_mx_efuse *efuse,
>> +                                   unsigned int addr, u32 *value)
>> +{
>> +       int err;
>> +       u32 regval;
>> +
>> +       /* write the address to read */
>> +       regval = FIELD_PREP(MESON_MX_EFUSE_CNTL1_BYTE_ADDR_MASK, addr);
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_BYTE_ADDR_MASK,
>> regval);
>> +
>> +       /* inform the hardware that we changed the address */
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_BYTE_ADDR_SET,
>> +                                MESON_MX_EFUSE_CNTL1_BYTE_ADDR_SET);
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_BYTE_ADDR_SET, 0);
>> +
>> +       /* start the read process */
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_AUTO_RD_START,
>> +                                MESON_MX_EFUSE_CNTL1_AUTO_RD_START);
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_AUTO_RD_START, 0);
>> +
>> +       /*
>> +        * perform a dummy read to ensure that the HW has the RD_BUSY bit
>> set
>> +        * when polling for the status below.
>> +        */
>> +       readl(efuse->base + MESON_MX_EFUSE_CNTL1);
>> +
>> +       err = readl_poll_timeout_atomic(efuse->base +
>> MESON_MX_EFUSE_CNTL1,
>> +                       regval,
>> +                       (!(regval & MESON_MX_EFUSE_CNTL1_AUTO_RD_BUSY)),
>> +                       1, 1000);
>> +       if (err) {
>> +               dev_err(efuse->config.dev,
>> +                       "Timeout while reading efuse address %u\n", addr);
>> +               return err;
>> +       }
>> +
>> +       *value = readl(efuse->base + MESON_MX_EFUSE_CNTL2);
>> +
>> +       return 0;
>> +}
>> +
>> +static int meson_mx_efuse_read(void *context, unsigned int offset,
>> +                              void *buf, size_t bytes)
>> +{
>> +       struct meson_mx_efuse *efuse = context;
>> +       u32 tmp;
>> +       int err, i, addr;
>> +
>> +       err = meson_mx_efuse_hw_enable(efuse);
>> +       if (err)
>> +               return err;
>> +
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE,
>> +                                MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE);
>> +
>> +       for (i = offset; i < offset + bytes; i += efuse->config.word_size)
>> {
>> +               addr = i / efuse->config.word_size;
>> +
>> +               err = meson_mx_efuse_read_addr(efuse, addr, &tmp);
>> +               if (err)
>> +                       break;
>> +
>> +               memcpy(buf + i, &tmp, efuse->config.word_size);
>> +       }
>> +
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE, 0);
>> +
>> +       meson_mx_efuse_hw_disable(efuse);
>> +
>> +       return err;
>> +}
>
> ...
>>
>> +
>> +static int meson_mx_efuse_probe(struct platform_device *pdev)
>> +{
>> +       const struct meson_mx_efuse_platform_data *drvdata;
>> +       struct meson_mx_efuse *efuse;
>> +       struct resource *res;
>> +
>
>
> ...
>
>> +
>> +       efuse->core_clk = devm_clk_get(&pdev->dev, "core");
>> +       if (IS_ERR(efuse->core_clk)) {
>> +               dev_err(&pdev->dev, "Failed to get core clock\n");
>> +               return PTR_ERR(efuse->core_clk);
>> +       }
>> +
>> +       efuse->nvmem = nvmem_register(&efuse->config);
>> +       if (IS_ERR(efuse->nvmem)) {
>> +               clk_unprepare(efuse->core_clk);
>
>
> Do you need this???
oh, this is a left-over from an earlier version where I only enabled
the clock in .probe (and disabled it again in .remove)
however, this could break during suspend and it keeps the clock
unnecessarily enabled when it's not needed

>
>> +               return PTR_ERR(efuse->nvmem);
>> +       }
>> +
>> +       platform_set_drvdata(pdev, efuse);
>> +
>> +       return 0;
>> +}
>> +
>> +static int meson_mx_efuse_remove(struct platform_device *pdev)
>> +{
>> +       struct meson_mx_efuse *efuse = platform_get_drvdata(pdev);
>> +       int err;
>> +
>> +       err = nvmem_unregister(efuse->nvmem);
>> +
>> +       clk_unprepare(efuse->core_clk);
>
>
> Do you need this disable here? AFAIU, this driver would never leave the clk
> prepared unless we are in the middle of read.
yes, good catch here as well - this clk_unprepare call has to be removed

>
>> +
>> +       return err;
>> +}
>> +
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux