Re: [PATCH v2 2/3] mmc: Add APM X-Gene SoC SDHC controller support to Arasan SDHCI driver

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

 



Hi,

On 06/05/2014 01:15 AM, Loc Ho wrote:
> Hi,
> 
>>>  #define SDHCI_ARASAN_CLK_CTRL_OFFSET 0x2c
>>> @@ -34,6 +36,19 @@
>>>   */
>>>  struct sdhci_arasan_data {
>>>       struct clk      *clk_ahb;
>>> +     struct platform_device *pdev;
>>> +     void __iomem    *ahb_aim_csr;
>>> +     const struct sdhci_arasan_ahb_ops *ahb_ops;
>>> +};
>>> +
>>> +/**
>>> + * struct sdhci_arasan_ahb_ops
>>> + * @init_ahb Initialize translation bus
>>> + * @xlat_addr        Set up an 64-bit addressing translation
>>
>> This definitely breaking kernel-doc format. Please fix.
> 
> I will add the missing ":".
> 
>>> +                     sdhci_arasan->ahb_ops->xlat_addr(sdhci_arasan,
>>> +                             sg_dma_address(host->data->sg));
>>> +     }
>>> +     writel(val, host->ioaddr + reg);
>>> +}
>>
>> The same code was submitted in v1 and Arnd commented it but you are still keeping
>> it here. Why?
> 
> I responded back to Arnd. If I move this code to IO MMU, what do I do
> when we enable the actual IO MMU? The structure for the bus type only
> has one IO MMU pointer. We would need to IO MMU pointer and not sure
> how the IO MMU framework would handle this.

Then good time to start investigating this.

>>
>>> +
>>>  static struct sdhci_ops sdhci_arasan_ops = {
>>> +     .write_l = sdhci_arasan_writel,
>>>       .get_max_clock = sdhci_pltfm_clk_get_max_clock,
>>>       .get_timeout_clock = sdhci_arasan_get_timeout_clock,
>>>  };
>>> @@ -121,13 +150,83 @@ static int sdhci_arasan_resume(struct device *dev)
>>>  static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend,
>>>                        sdhci_arasan_resume);
>>>
>>> +static int sdhci_arasan_xgene_init_ahb(struct sdhci_arasan_data *data)
>>> +{
>>> +     #define AIM_SIZE_CTL_OFFSET     0x00000004
>>> +     #define  AIM_EN_N_WR(src)       (((u32) (src) << 31) & 0x80000000)
>>> +     #define  ARSB_WR(src)           (((u32) (src) << 24) & 0x0f000000)
>>> +     #define  AWSB_WR(src)           (((u32) (src) << 20) & 0x00f00000)
>>> +     #define  AIM_MASK_N_WR(src)     (((u32) (src)) & 0x000fffff)
>>
>> Remove that one more space between #define and name.
>> I am not fan of having these defines just here - move them to the top.
>>
>> Also these macros are used just here at one location.
>> Isn't it better just to define that BITS you want to setup instead of
>> these macros which are hardly to read?
> 
> The only field that is single bit is AIM_EN_N_WR. All others are
> multiple bit fields. Either I write them in the code or use these
> defines. Would it be better if I just write them in the code?

>From my point of view will be the best to compose one macro like
#define AIM_EN_N_WR	BIT(31)
...
#define AIM...INIT_VAL	(AIM_EN_N_WR |... )



>>> +static void sdhci_arasn_xgene_xlat_addr(struct sdhci_arasan_data *data,
>>> +                                    u64 dma_addr)
>>> +{
>>> +     #define AIM_AXI_HI_OFFSET       0x0000000c
>>> +     #define  AIM_AXI_ADDRESS_HI_N_WR(src) \
>>> +                                     (((u32) (src) << 20) & 0xfff00000)
>>
>> ditto with indentation.
>> 20 should be shift
>> 0xfff00000 is mask.
>>
>> Also you should take this opportunity and add function description
>> in kernel doc to be exactly clear what this function is doing.
> 
> The function is pretty small, simply and don't believe it needs
> function description.

That documentation is for everybody not just for you who write the code.
Simple description will be useful.


>>> +
>>> +     if (!data->ahb_aim_csr)
>>> +             return;
>>> +
>>> +     writel(AIM_AXI_ADDRESS_HI_N_WR(dma_addr >> 32),
>>> +             data->ahb_aim_csr + AIM_AXI_HI_OFFSET);
>>> +}
>>> +
>>> +static const struct sdhci_arasan_ahb_ops xgene_ahb_ops = {
>>> +     .init_ahb = sdhci_arasan_xgene_init_ahb,
>>> +     .xlat_addr = sdhci_arasn_xgene_xlat_addr,
>>> +};
>>> +
>>> +static const struct of_device_id sdhci_arasan_of_match[] = {
>>> +     { .compatible = "arasan,sdhci-8.9a" },
>>> +     { .compatible = "apm,arasan,sdhci-8.9a", .data = &xgene_ahb_ops },
>>> +     { }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
>>> +
>>>  static int sdhci_arasan_probe(struct platform_device *pdev)
>>>  {
>>>       int ret;
>>> -     struct clk *clk_xin;
>>> +     struct clk *clk_xin = NULL;
>>>       struct sdhci_host *host;
>>>       struct sdhci_pltfm_host *pltfm_host;
>>>       struct sdhci_arasan_data *sdhci_arasan;
>>> +     const struct of_device_id *of_id =
>>> +                     of_match_device(sdhci_arasan_of_match, &pdev->dev);
>>> +     struct resource *res;
>>>
>>>       sdhci_arasan = devm_kzalloc(&pdev->dev, sizeof(*sdhci_arasan),
>>>                       GFP_KERNEL);
>>> @@ -136,8 +235,9 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>>>
>>>       sdhci_arasan->clk_ahb = devm_clk_get(&pdev->dev, "clk_ahb");
>>>       if (IS_ERR(sdhci_arasan->clk_ahb)) {
>>> -             dev_err(&pdev->dev, "clk_ahb clock not found.\n");
>>> -             return PTR_ERR(sdhci_arasan->clk_ahb);
>>> +             /* Clock is optional */
>>> +             sdhci_arasan->clk_ahb = NULL;
>>> +             goto skip_clk;
>>
>> Clocks are optional for your SoC but they are necessary for Zynq.
>> You can't just skip clocks for zynq because if DT is wrong clocks won't be enabled
>> and even checked.
>> Does it mean that there are no clocks for this IP?
> 
> The clock for X-Gene is enabled by the time Linux boot. As the clock
> in programmed in the SDHC register, it knows the clock frequency.
> 
>> Or do you miss clock driver?
> 
> The clock will be configured by the FW and left out intentionally as
> this will also support ACPI boot.

Not a problem if this is what you like but you can't just break Zynq by this change
which is what you are doing right now.

Thanks,
Michal


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux